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
Introduce LazyLoad Backend #3
Conversation
5454e9b
to
b6e1c13
Compare
@@ -0,0 +1,61 @@ | |||
require 'test_helper' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a temporary test that's used to setup the benchmarks. It'll be presented upstream, but I don't intend on merging this.
Please review @Shopify/rails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work, Paarth! 👏 🚀
As discussed IRL: #available_locales
doesn't work right now, because it will only look at loaded locales, which is likely to be incomplete. I think we'll need to do something similar to what @shioyama did in Core and use our selection heuristics to grab available locales from the load path.
A couple of points we talked about IRL that are not blockers for this PR, but that we might want to think about in terms of delivering this as a feature upstream. I've writing them down here in case anyone else has thoughts / feedback on them, and to remind myself what was discussed 😄
- We might want to ensure that this can work out of the box with Rails. There are default translations in Rails that don't conform to the path-matching regex we've specified. Maybe this means additional support in https://github.com/svenfuchs/rails-i18n to ensure that these translations are always loaded by default, similar to what we did in Core.
- Should we have some sort of sanity check that apps can use to verify their translations are set up correctly? Might be nicer than failing silently if a translation has an unusual file path and doesn't get picked up properly. Possibly a rake task?
I don't have sufficient context on the failing JRuby tests -- maybe someone else from the team can step in there.
One more question: are we intending that folks use this in production? Obviously the use case we have in Core is strictly for our test environment. Prod is eager loaded so this is a no-go 🤦♀️
I think our next step is to try to adopt this in Core, and make sure all the tests continue to pass as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 👍 Just some initial comments, mainly about the format conventions for locale filenames.
I believe you should make it clear in that section that this backend is for development and test environment, and shouldn't be used in production environments. |
8a801a9
to
74091d5
Compare
Thanks for the reviews, I incorporated all stylistic changes, nits, and minor feedback.
I agree, I've updated documentation to make this more clear, and I've changed the behaviour of |
The crux of this problem, now, seems to be which naming format the backend assumes and how we can generate available locales from this. I'd like to collect your feedback on some approaches for how to proceed. Here's a small set of criteria I've used to evaluate the approaches:
With this criteria in mind, here are some approaches Adrianna and I discussed Approach 1: Assume all files abide by
|
While it's the most accurate, I don't think the mapping solution has good ergonomics. I think we should either enforce a naming convention and / or have a list of regexp or other type of callbacks to extract a locale from a path. This means that we should have some kind of strict mode if the assumptions end up wrong, and they should be tested via loading all |
I'd lean towards approach 3. As Jean said, option 4 guarantees accuracy but is a bit tedious and involved for users to set up. I think that especially because this is a test environment optimization, it should be as simple as possible for users to set up / opt into. Going with 3 allows us to start very simple and assume that 95% of apps can comply with those expectations out of the box (I think we should start with the assumption that "all files abide by
I'd actually argue that this is not so involved. For most applications, going with the default format should be enough. We can be pretty explicit about we we expect from the "path resolve" feature -- as Jean suggested, it could be as simple as providing a regex / list of regexes that extracts the locale from the filepath. Users wouldn't necessarily need to write TL;DR - I think we can keep things simple for now and be rigid about the naming configuration we expect, and figure out a way to offer configuration as a follow-up. |
After syncing with Jean and Adrianna, an approach to get a first iteration and possibly integrate with Core in the future:
If the PR is well-received, the plan for Core / Rails can be as simple as:
The amount of work remaining to propose this upstream as defined above is limited. The calendar time for reviews may be longer, but if the PR does make it upstream, the Core work can also be done at any time in the future. It should take a few days at most, mostly scripting and testing. |
1a8c5ce
to
0709ec7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor stuff, but looks like it's on the right track, and it really isn't that much code.
5841e5b
to
996965f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, Paarth! I really appreciate the amount of detail you've put into test cases and documentation ❤️ I have a bunch of minor comments (feel free to contest any suggestions I've made that don't make sense 😆 ), and I'd like to give it a tophat on a simple Rails app, but this looks pretty much ready to be proposed upstream IMO 🚀
lib/i18n/backend/lazy_loadable.rb
Outdated
def file_named_correctly?(path, translations) | ||
locales = translations.keys.map(&:to_sym) | ||
return false unless locales.one? | ||
|
||
LocaleExtractor.locale_from_path(path) == locales.first | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than return true/false
this method should raise a final error. This way we can include in the error message which assumption was broken, and in which file, making it much easier to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was on the fence about which way to go. I agree that knowing which assumption was broken will help, and this is something I'll add with the current approach.
The reason why I decided to raise with all the offending files at once is mainly a DX concern. If someone's onboarding using this strategy and would like to acquire a complete list of offending files, it would be handy to raise this in a single error.
My worry is that people will fight with repeated errors being raised and only able to fix them on a one-by-one basis.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, collecting all the offenses and raising once is even better. But I think you can do both, it just can't be expressed by simple booleans since you have multiple types of offense (e.g. path not matching at all, and content not respecting what the path claims).
So maybe you can collect a list of error message in an array and then raise if it's not empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We reached the same conclusion when pairing on this yesterday! 😄 I believe Paarth has submitted the changes upstream now: https://github.com/ruby-i18n/i18n/pull/612/files#diff-8e52202d93e7810223b33ab64de65635e73f04ffa8d7d38b14b5e656039d3ea1R152-R162
50765ec
to
05605be
Compare
05605be
to
8a633b8
Compare
Closing in favour of ruby-i18n#612 |
Note: I'm using this PR to collect feedback internally. I'll close it once we're aligned and propose this upstream.
What's in this PR
This PR introduces a new
LazyLoad
backend following this discussion in ruby-i18n#592.What does the
LazyLoad
Backend offer?The
Simple
backend can't infer which files belong to which locale, so it loads all files in the load path and resolves the locale by inspecting the translations that are loaded. This is a fool proof strategy, but it comes at the expense of needing to load all files for all locales, for any arbitrary locale.This backend avoids the cost of loading unnecessary translation files by carefully selecting only those files which are needed for the current locale. It lazily initializes translations on a per locale basis.
How does the
LazyLoad
Backend work?This backend trades off the expensive cost of
I/O
with the cost of perform string matching on files in the load path. It makes assumptions about which files belong to a locale and selectively loads only these files.How does the
LazyLoad
Backend know which files belong to which locale?It makes assumptions about how files are named. Clients must abide by this naming system if they decide to use this backend.
The heuristic used to bind a file to its locale can be defined as follows:
When should someone use this backend?
Workloads that operate in the context of a single locale at a time and have many translations files for many locales. For instance, a large Rails workload would benefit from this backend.
It's designed for test environments, not environments where eager loading is preferred.
Benchmarks: Comparing the
Simple
backend to theLazyLoad
backendA benchmark setup was used to compare the performance of these two backends.
Table 1: Setup with 10 files per locale, 100 keys in each file:
Table 2: Setup with 100 files per locale, 1000 keys in each file:
Exploring the results
The
Simple
backend works for the same amount of time in the case when it needs to load translations for a single locale, and when it loads translations for all locales. This makes sense as the backend loads all translations irrespective of the current locale.The
LazyLoad
backend reduces working time as it avoids loading unnecessary files. In the case when loading for a single locale, we see that theLazyLoad
backend outperformsSimple
, 0.005 vs 0.013 in Table 1 and 0.4899 vs 1.363 in Table 2.The
LazyLoad
backend performs roughly on-par with theSimple
backend when it needs to load all translations. There is additional overhead of string matching which brings down the performance in small workloads. It's negligible in any significant workloads compared to the time spent inI/O
.Remarks
This backend is designed to bring performance improvements to workloads with a large volume of locales, translation files, and translation keys.
Performance isn't guaranteed for all applications, which is why the backend is designed to be opt-in.
At Shopify, we've patched
ruby-i18n
locally to implement a similar strategy. We've observed close to 10x speed ups locally in specific tests and roughly 20% speeds across the suite.