Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize Backend::Simple#available_locales #406

Merged
merged 1 commit into from Feb 9, 2018

Conversation

@jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Feb 6, 2018

Previously available_locales was a little bit slow. For each locale in translations it would:

  • build a new array of all the top-level keys in that locale
  • build a second array of those keys except :i18n
  • add that locale to the list if the second array was not empty

For locales with many translations this can build somewhat sizeable arrays.

Instead we can perform the same operation, rejecting locales with either no keys or only :i18n without allocating any new objects. We reject based on the condition:

data.size <= 1 && (data.empty? || data.has_key?(:i18n))

This ends up being about 4x faster (though this of course depends on the exact locales being used):

Benchmark.ips do |x|
  x.report("I18n.available_locales") do
    I18n.available_locales
  end
end

Before:

11.447k (± 2.9%) i/s -     57.869k in   5.059738s

After:

47.810k (± 2.8%) i/s -    242.060k in   5.067332s
Previously available_locales was a little bit slow. For each locale in
translations it would:
* build a new array of all the top-level keys in that locale
* build a second array of those keys except :i18n
* add that locale to the list if the second array was not empty

For locales with many translations this can build somewhat sizeable
arrays.

Instead we can perform the same operation, rejecting locales with either
no keys or only :i18n without allocating any new objects. We reject
based on the condition:

    data.size <= 1 && (data.empty? || data.has_key?(:i18n))

This ends up being about 4x faster (though this of course depends on
the exact locales being used):

    Benchmark.ips do |x|
      x.report("I18n.available_locales") do
        I18n.available_locales
      end
    end

Before:

    11.447k (± 2.9%) i/s -     57.869k in   5.059738s

After:

    47.810k (± 2.8%) i/s -    242.060k in   5.067332s
@@ -44,7 +44,7 @@ def store_translations(locale, data, options = {})
def available_locales
init_translations unless initialized?
translations.inject([]) do |locales, (locale, data)|
locales << locale unless (data.keys - [:i18n]).empty?
locales << locale unless data.size <= 1 && (data.empty? || data.has_key?(:i18n))

This comment has been minimized.

@dmitry

dmitry Feb 6, 2018

Please replace with if condition to make it more readable: if data.size > 1 || !(data.empty? || data.has_key?(:i18n))

This comment has been minimized.

@jhawthorn

jhawthorn Feb 8, 2018
Author Contributor

I'd be happy to change it, but I'm not sure that improves readability. My mental model of this block is a reject we are implementing with inject (so that we get easy locale, data pairs) and that maps best to unless.

Does an i18n maintainer have a preference?

This comment has been minimized.

@radar

radar Feb 9, 2018
Collaborator

Hi :) I am an i18n maintainer with an opinion on this topic.

I think unless is fine. I think the intention is clear here.

@radar
radar approved these changes Feb 9, 2018
@radar radar merged commit 9ddc9f5 into ruby-i18n:master Feb 9, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@radar
Copy link
Collaborator

@radar radar commented Feb 9, 2018

Many thanks @jhawthorn :) Glad to see you're still alive and kicking!

@radar
Copy link
Collaborator

@radar radar commented Feb 9, 2018

I should also mention: this change will go out in 0.9.4. I am waiting on #407 to be confirmed before doing that release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants