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

Performance regression from v1 to v2 #182

Closed
bep opened this issue Jun 3, 2019 · 7 comments · Fixed by #189
Closed

Performance regression from v1 to v2 #182

bep opened this issue Jun 3, 2019 · 7 comments · Fixed by #189

Comments

@bep
Copy link
Contributor

bep commented Jun 3, 2019

The relevant Hugo benchmarks comparing v1 with v2:

benchmark                                                    old ns/op     new ns/op     delta
BenchmarkI18nTranslate/all-present-4                         775           796           +2.71%
BenchmarkI18nTranslate/present-in-default-4                  1372          49923         +3538.70%
BenchmarkI18nTranslate/present-in-current-4                  777           797           +2.57%
BenchmarkI18nTranslate/missing-4                             1106          47469         +4191.95%
BenchmarkI18nTranslate/file-missing-4                        3729          2109          -43.44%
BenchmarkI18nTranslate/context-provided-4                    2087          1258          -39.72%
BenchmarkI18nTranslate/same-id-and-translation-4             806           792           -1.74%
BenchmarkI18nTranslate/same-id-and-translation-default-4     1413          49749         +3420.81%
BenchmarkI18nTranslate/unknown-language-code-4               1821          2073          +13.84%

benchmark                                                    old allocs     new allocs     delta
BenchmarkI18nTranslate/all-present-4                         6              4              -33.33%
BenchmarkI18nTranslate/present-in-default-4                  10             201            +1910.00%
BenchmarkI18nTranslate/present-in-current-4                  6              4              -33.33%
BenchmarkI18nTranslate/missing-4                             8              192            +2300.00%
BenchmarkI18nTranslate/file-missing-4                        21             11             -47.62%
BenchmarkI18nTranslate/context-provided-4                    15             5              -66.67%
BenchmarkI18nTranslate/same-id-and-translation-4             6              4              -33.33%
BenchmarkI18nTranslate/same-id-and-translation-default-4     10             201            +1910.00%
BenchmarkI18nTranslate/unknown-language-code-4               13             12             -7.69%

benchmark                                                    old bytes     new bytes     delta
BenchmarkI18nTranslate/all-present-4                         152           176           +15.79%
BenchmarkI18nTranslate/present-in-default-4                  216           9315          +4212.50%
BenchmarkI18nTranslate/present-in-current-4                  152           176           +15.79%
BenchmarkI18nTranslate/missing-4                             152           8924          +5771.05%
BenchmarkI18nTranslate/file-missing-4                        600           256           -57.33%
BenchmarkI18nTranslate/context-provided-4                    704           200           -71.59%
BenchmarkI18nTranslate/same-id-and-translation-4             152           165           +8.55%
BenchmarkI18nTranslate/same-id-and-translation-default-4     216           9304          +4207.41%
BenchmarkI18nTranslate/unknown-language-code-4               696           640           -8.05%

I notice some patterns in the above in that it finds a slow path if the string isn't found in the given language, but these tests are very small bundles with like 2 languages.

@nicksnyder
Copy link
Owner

v2.0.1 includes a fix for a fast path that wasn't getting taken when it should have. Can you please re-run your benchmarks with v2.0.1? I suspect some cases involving fallback will get slower due to the more complicated fallback logic, but I can investigate to be sure.

@bep
Copy link
Contributor Author

bep commented Jun 6, 2019

So, v2.0.0 vs v2.0.1:

benchmark                                                    old ns/op     new ns/op     delta
BenchmarkI18nTranslate/all-present-4                         788           424           -46.19%
BenchmarkI18nTranslate/present-in-default-4                  49753         49105         -1.30%
BenchmarkI18nTranslate/present-in-current-4                  793           427           -46.15%
BenchmarkI18nTranslate/missing-4                             47109         46857         -0.53%
BenchmarkI18nTranslate/file-missing-4                        2108          2024          -3.98%
BenchmarkI18nTranslate/context-provided-4                    1237          1211          -2.10%
BenchmarkI18nTranslate/same-id-and-translation-4             784           418           -46.68%
BenchmarkI18nTranslate/same-id-and-translation-default-4     49628         49124         -1.02%
BenchmarkI18nTranslate/unknown-language-code-4               2048          2050          +0.10%

benchmark                                                    old allocs     new allocs     delta
BenchmarkI18nTranslate/all-present-4                         4              0              -100.00%
BenchmarkI18nTranslate/present-in-default-4                  201            197            -1.99%
BenchmarkI18nTranslate/present-in-current-4                  4              0              -100.00%
BenchmarkI18nTranslate/missing-4                             192            192            +0.00%
BenchmarkI18nTranslate/file-missing-4                        11             11             +0.00%
BenchmarkI18nTranslate/context-provided-4                    5              5              +0.00%
BenchmarkI18nTranslate/same-id-and-translation-4             4              0              -100.00%
BenchmarkI18nTranslate/same-id-and-translation-default-4     201            197            -1.99%
BenchmarkI18nTranslate/unknown-language-code-4               12             12             +0.00%

benchmark                                                    old bytes     new bytes     delta
BenchmarkI18nTranslate/all-present-4                         176           0             -100.00%
BenchmarkI18nTranslate/present-in-default-4                  9315          9138          -1.90%
BenchmarkI18nTranslate/present-in-current-4                  176           0             -100.00%
BenchmarkI18nTranslate/missing-4                             8925          8925          +0.00%
BenchmarkI18nTranslate/file-missing-4                        256           256           +0.00%
BenchmarkI18nTranslate/context-provided-4                    200           200           +0.00%
BenchmarkI18nTranslate/same-id-and-translation-4             165           0             -100.00%
BenchmarkI18nTranslate/same-id-and-translation-default-4     9306          9139          -1.79%
BenchmarkI18nTranslate/unknown-language-code-4               640           640           +0.00%

Compared to v1:

benchmark                                                    old ns/op     new ns/op     delta
BenchmarkI18nTranslate/all-present-4                         768           425           -44.66%
BenchmarkI18nTranslate/present-in-default-4                  1360          49039         +3505.81%
BenchmarkI18nTranslate/present-in-current-4                  769           423           -44.99%
BenchmarkI18nTranslate/missing-4                             1091          46897         +4198.53%
BenchmarkI18nTranslate/file-missing-4                        3653          2029          -44.46%
BenchmarkI18nTranslate/context-provided-4                    2040          1202          -41.08%
BenchmarkI18nTranslate/same-id-and-translation-4             793           416           -47.54%
BenchmarkI18nTranslate/same-id-and-translation-default-4     1401          49009         +3398.14%
BenchmarkI18nTranslate/unknown-language-code-4               1795          2006          +11.75%

benchmark                                                    old allocs     new allocs     delta
BenchmarkI18nTranslate/all-present-4                         6              0              -100.00%
BenchmarkI18nTranslate/present-in-default-4                  10             197            +1870.00%
BenchmarkI18nTranslate/present-in-current-4                  6              0              -100.00%
BenchmarkI18nTranslate/missing-4                             8              192            +2300.00%
BenchmarkI18nTranslate/file-missing-4                        21             11             -47.62%
BenchmarkI18nTranslate/context-provided-4                    15             5              -66.67%
BenchmarkI18nTranslate/same-id-and-translation-4             6              0              -100.00%
BenchmarkI18nTranslate/same-id-and-translation-default-4     10             197            +1870.00%
BenchmarkI18nTranslate/unknown-language-code-4               13             12             -7.69%

benchmark                                                    old bytes     new bytes     delta
BenchmarkI18nTranslate/all-present-4                         152           0             -100.00%
BenchmarkI18nTranslate/present-in-default-4                  216           9138          +4130.56%
BenchmarkI18nTranslate/present-in-current-4                  152           0             -100.00%
BenchmarkI18nTranslate/missing-4                             152           8926          +5772.37%
BenchmarkI18nTranslate/file-missing-4                        600           256           -57.33%
BenchmarkI18nTranslate/context-provided-4                    704           200           -71.59%
BenchmarkI18nTranslate/same-id-and-translation-4             152           0             -100.00%
BenchmarkI18nTranslate/same-id-and-translation-default-4     216           9140          +4131.48%
BenchmarkI18nTranslate/unknown-language-code-4               696           640           -8.05%

The main path is now 2x faster than v1, which is great. But the fall back the other language (the tests above has at most 2 languages in a bundle) is in my head common enough, and 0.05 ms per lookup is on the long side for my use. I may look into the API to see if there is a way to do this manually (if not found, do a lookup for en).

@nicksnyder
Copy link
Owner

nicksnyder commented Jun 13, 2019

The remaining performance regression is caused by the instantiating a NewMatcher in the slow path: https://github.com/nicksnyder/go-i18n/blob/master/v2/i18n/localizer.go#L185

In the particular case that you are benchmarking, a new matcher is overkill because there is only one language to fallback to (I could optimize this), but in the general case there would still be what you are seeing here.

If there are >2 languages and the translation is not in the preferred language, what should happen? What currently happens is a new matcher is instantiated to return the “best” one of the remaining. Alternatively, we could (1) always fallback to the default message if available (2) always return an error, or (3) return both the default message and an error so callers can choose the desired behavior. What do you think?

@bep
Copy link
Contributor Author

bep commented Jun 13, 2019

I'm not sure what you regard as "the best alternative", if not found. I assume that means I get the Swedish or Danish version if no Norwegian version is found.

If the above ruleset is solid and works for all languages, it could be worth it. But then you should consider adding a simple cache. The problem with the above benchmark is mainly that if you call translate("this-key-does-not-exist-in-this-language") a million times with the same key (a common use case), it would take 50 seconds.

@nicksnyder
Copy link
Owner

I have a draft PR which simplifies the fallback logic in an appropriate way I think and resolved the performance regression. There are some more tests that I want to add before merging.

@nicksnyder
Copy link
Owner

This significant perf improvement is released in v2.0.4

@bep
Copy link
Contributor Author

bep commented Sep 29, 2020

Thanks, much appreciated.

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

Successfully merging a pull request may close this issue.

2 participants