Increase in function calls to get_locale() because of _load_textdomain_just_in_time()

[sharkomatic]

When I updated a site to WordPress 4.6, I noticed a dramatic increase in function calls to get_locale() in New Relic, which we use to track our server and site performance. I believe I have traced the issue to the new function _load_textdomain_just_in_time(). In the function description, it says that when it first encounters a new textdomain, it will try to load translation files from 'wp-content/languages.' The problem is that if no translation files exist for that textdomain, _load_textdomain_just_in_time() goes through the entire process each time it is called because the domain is never being added to the $l10n_unloaded array. I added the following line just before line 857 in l10n.php as a temporary fix on our site:

$l10n_unloaded[ $domain ] = true;

I'm attaching a screenshot of the graph in New Relic that shows the increase in function calls and execution time for get_locale() with the current _load_textdomain_just_in_time() function code in place. Since adding the domain to $l10_unloaded, I have not noticed any performance decreases in the site.

[sharkomatic]

Increase in get_locale() function calls in WP 4.6+

[sharkomatic]

The two vertical lines on the attached graph are:

1) When I first deployed the 4.6.1 WordPress update
2) When I deployed the pre-4.6-version of l10n.php, without the _load_textdomain_just_in_time() function.

I have since deployed the newest version of l10n.php with the line of code I mentioned previously.

Replying to sharkomatic:

When I updated a site to WordPress 4.6, I noticed a dramatic increase in function calls to get_locale() in New Relic, which we use to track our server and site performance. I believe I have traced the issue to the new function _load_textdomain_just_in_time(). In the function description, it says that when it first encounters a new textdomain, it will try to load translation files from 'wp-content/languages.' The problem is that if no translation files exist for that textdomain, _load_textdomain_just_in_time() goes through the entire process each time it is called because the domain is never being added to the $l10n_unloaded array. I added the following line just before line 857 in l10n.php as a temporary fix on our site:

$l10n_unloaded[ $domain ] = true;

I'm attaching a screenshot of the graph in New Relic that shows the increase in function calls and execution time for get_locale() with the current _load_textdomain_just_in_time() function code in place. Since adding the domain to $l10_unloaded, I have not noticed any performance decreases in the site.

[johnbillion]

Thanks for the report, @sharkomatic. I've not tested this but your report does look valid.

I'm going to place this into the 4.6.2 milestone for visibility, then we can decide whether this is worth fixing in a minor release, or whether it can wait until 4.7.

[sharkomatic]

Thanks, John. In our case, it can't wait until 4.7 because of the noticeable performance hit on our site. I will keep an eye on the changelogs in upcoming releases and re-apply my fix as needed.

Replying to johnbillion:

Thanks for the report, @sharkomatic. I've not tested this but your report does look valid.

I'm going to place this into the 4.6.2 milestone for visibility, then we can decide whether this is worth fixing in a minor release, or whether it can wait until 4.7.

[swissspidy]

The problem is that if no translation files exist for that textdomain, _load_textdomain_just_in_time() goes through the entire process each time it is called because the domain is never being added to the $l10n_unloaded array

That's not how this array is being used. It is only filled when explicitly calling unload_textdomain().

We should rather change get_translations_for_domain() so that it returns $noop_translations after the first failed attempt.

[Chouby]

Replying to sharkomatic:

I added the following line just before line 857 in l10n.php as a temporary fix on our site:

$l10n_unloaded[ $domain ] = true;

You don't need to hack the l10n.php, but can add this line in a plugin. I did the same in Polylang as I use a child class of MO which does not store the strings in a .mo file.

[sharkomatic]

Thanks for your reply. I obviously don't have as much knowledge of how the code is supposed to function and didn't investigate very thoroughly. And you're correct, obviously. unload_textdomain() is the only place I see the $l10n_unloaded array being filled, and that's only if $plugin_override is true or if unload_textdomain() is called on a domain that is in the $l10n array.

If you instead change get_translations_for_domain() as you suggested to return $noop_translations upon first fail, wouldn't it still go through the _load_textdomain_just_in_time() process each time anyway (within the if statement on line 875)?

Couldn't you just unload the textdomain where I explicitly added the domain to the $l10n_unloaded array in _load_textdomain_just_in_time()? That would also require a change to unload_textdomain() to allow the domain to be added to the $l10n_unloaded array regardless of whether $plugin_override is true or if the domain exists within the $l10n array (which in this case would not).

It seems like it would be even better to check if a textdomain has been unloaded before get_translations_for_domain() is called to avoid all those extra function calls if we already know there are no translation files for a particular domain (translate()/translate_with_gettext_context()/_n()/_nx() call get_translations_for_domain() which calls _load_textdomain_just_in_time() which should be unloading the textdomain). This way, for each domain without translations (and which aren't explicitly overriding the WordPress translations, that entire function process is only executed once).

As for @Chouby's suggestion, I have no interest in keeping up with each plugin and theme's textdomain and whether they have available translations or should be overridden in my own code.

Again, thanks for your reply. I am leaving my code change in place for now, rather than having to change two functions within l10n.php.

• Steph

Replying to swissspidy:

The problem is that if no translation files exist for that textdomain, _load_textdomain_just_in_time() goes through the entire process each time it is called because the domain is never being added to the $l10n_unloaded array

That's not how this array is being used. It is only filled when explicitly calling unload_textdomain().

We should rather change get_translations_for_domain() so that it returns $noop_translations after the first failed attempt.

[sharkomatic]

I'm attaching the l10n file with the modifications I just implemented on our site. I'm already noticing a considerable performance increase in New Relic. Server response time has gone down ~120 ms, throughput is higher. I made changes to unload_textdomain(), _load_textdomain_just_in_time(), get_translations_for_domain(), and added a new function called is_textdomain_unloaded().

Feel free to offer any input on my changes.

Thanks,
Steph

[sharkomatic]

l10n.php with performance improvements

[swissspidy]

If you instead change get_translations_for_domain() as you suggested to return $noop_translations upon first fail, wouldn't it still go through the _load_textdomain_just_in_time() process each time anyway (within the if statement on line 875)?

Not if the function was rewritten a bit. I should've explained it a bit further I guess.

Couldn't you just unload the textdomain where I explicitly added the domain to the $l10n_unloaded array in _load_textdomain_just_in_time()? That would also require a change to unload_textdomain() to allow the domain to be added to the $l10n_unloaded array regardless of whether $plugin_override is true or if the domain exists within the $l10n array (which in this case would not).

Thinking more about it, it really makes more sense to go this route. Also, a is_textdomain_unloaded() function is indeed handy.

In 37997.diff​ I turned your adjustments to the l10n.php file into a proper patch. Makes it easier to discuss any changes.

[sharkomatic]

Thanks @swissspidy.

[swissspidy]

@ocean90 What do you think of the latest patch?

[ocean90]

Replying to swissspidy:

@ocean90 What do you think of the latest patch?

Looks hackish. unload_textdomain() should only be used to unload existing text domains IMO. 🤔

[swissspidy]

Replying to ocean90:

Replying to swissspidy:

@ocean90 What do you think of the latest patch?

Looks hackish. unload_textdomain() should only be used to unload existing text domains IMO.

Yeah my first suggestion was to solve this another way as well. 37997.2.diff​ resolves this by modifying get_translations_for_domain(). It also adds an actual test to ensure get_locale() is only called exactly once, even if there are 5 calls to __() or the lilke.

[swissspidy]

@ocean90 mentioned that 37997.2.diff​ would basically revert [36538]

[swissspidy]

Moving out of the 4.6.2 milestone as it's not that critical to need to go into a minor release.

Note: This would need to account for get_user_locale(). Still not sure about the best approach. @ocean90 might have an idea here.

[ocean90]

Yeah, I've to dig into the previous tickets. Maybe @jrf has some thoughts too.

[jrf]

Thanks for the ping @ocean90 .

I've just read through the thread, the slack logs and the patches.

First (rough) idea which comes to my mind would be - what about adding a new is_translation_available( $domain ) function/method which would keep a record in a static ?
Something along the lines of:

static $translation_located = array(
   'domainA' => false,
   'domainB' => 'path_to_translation_file_located',
);

Some advantages of that:

• Single responsibility principle for is_textdomain_loaded() which was implemented in [36538] does not get undone.
• The new function again would have a single responsibility.
• It would lower the complexity of the _load_textdomain_just_in_time() function.

I'd need to look at all the relevant code in more detail to fine-tune the idea (and create a patch), but as a general solution direction, what do you think ?

[ocean90]

I'm not really a fan of static variables but it seems like an idea which we should explore. @jrf Can you work on a patch? The function needs probably some sort of a $reset argument to make it testable and compatible with the new locale switching functions.

[jrf]

@ocean90

I'm not really a fan of static variables

Well, the alternatives would be a global variable (Yikes!) or a class property, but that would require refactoring the text domain loading to a class...

The refactoring to a class might actually be a much better way forward in the long run, but is not really a feasibly option for now if this is to make it into 4.7.

The function needs probably some sort of a $reset argument to make it testable and compatible with the new locale switching functions.

I see your point. In that case, I'd suggest splitting it into two functions, one (internal use) which does the actually checking and one which keeps track of the static and can reset it.

Can you work on a patch?

I can work on a patch, but I have limited time at the moment. What timeframe are you looking for ?

[jrf]

Just out of curiosity and as it hasn't been asked before.

As the chart explicitly states the issue is with the locale hook:
@sharkomatic Could you check which functions are hooked into the locale hook on your install ?

You can check it using the ​Debug Bar plugin combined with the ​Debug Bar Actions and Filters Addon.

It be great to see the list of hooked in functions for both front-end as well as back-end to verify.

On a default install, AFAICS, nothing is hooked in, so the hook itself should not cause such a performance hit.

I'm just wondering if we're not trying to solve an issue caused by a plugin instead of by WP core here....

[jrf]

Proof of concept for caching of paths for just in time language loading

[jrf]

Just uploaded a proof of concept/potential patch. (doesn't contain any unit tests for the new methods yet).

Did some quick tests on a relatively standard WP install with only some debug plugins active.
For a front-end page load, I found the following:

• Without the patch, the locale filter was called 2200+ times.
• With the patch, the locale filter was only called 72 times.

So independently of whether or not the actual slow-down is caused by plugins, I do believe the patch is worth considering as the difference is significant.

[ocean90]

Thanks @jrf! trac-37997-improve-just-in-time-performance.patch​ looks good on a first glance. I think we just need to find some better names for the functions. :) We could probably merge _get_path_to_glotpress_translation() into _get_path_to_translation() but I get the single responsibility idea.

[jrf]

@ocean90

I think we just need to find some better names for the functions. :)

Agreed, I wasn't feeling very creative when I coded it ;-) Suggestions welcome.

We could probably merge _get_path_to_glotpress_translation() into _get_path_to_translation() but I get the single responsibility idea.

Having them as two separate functions will also make it a lot easier to unit test this ;-)

[ocean90]

Replying to jrf:

I think we just need to find some better names for the functions. :)

Agreed, I wasn't feeling very creative when I coded it ;-) Suggestions welcome.

How about _get_mo_file_path_for_textdomain() for _get_path_to_translation() and _get_mo_file_path_from_wp_lang_dir_for_textdomain() for _get_path_to_glotpress_translation()?

[jrf]

Refreshed, renamed the path retrieval function, minimal changes in prep for unit tests (incomplete).

[ocean90]

37997.3.diff​ includes trac-37997-v2-improve-just-in-time-performance.patch​ and the test from 37997.2.diff​. In _load_textdomain_just_in_time() I've moved the _get_path_to_translation() call after the $domain and $l10n_unloaded checks.

I also added Tests_Locale_Switcher::test_something() but this one is currently failing, with and without the patch. It looks like WP_Locale_Switcher::load_translations() is loading the .mo file for the previous locale again.

[swissspidy]

37997.4.diff​ fixes the test added in the previous patch. In addition to the filter in the original test overriding any locale switching, WP_Locale_Switcher::load_translations() indeed loaded the previous MO file again after unloading the text domain.

I didn't check why this has been working so far, but in this new patch I greatly simplified the translation loading in WP_Locale_Switcher by just calling unload_textdomain( $domain ) and then get_translations_for_domain( $domain ).

[ocean90]

In 39330:

I18N: Add an additional caching layer for _load_textdomain_just_in_time().

Previously, if no translation files exist for a text domain, _load_textdomain_just_in_time() went through the entire process each time it was called. This results in an increased call to get_locale() and its locale filter.
This change splits the logic into _get_path_to_translation() and _get_path_to_translation_from_lang_dir(). The former, which is used by _load_textdomain_just_in_time(), caches the result of the latter. It also removes some non-working code from WP_Locale_Switcher::load_translations().

Props jrf, swissspidy, sharkomatic, ocean90.
Fixes #37997.