Make WordPress Core

Opened 11 months ago

Closed 11 months ago

Last modified 9 months ago

#58321 closed defect (bug) (fixed)

`_load_textdomain_just_in_time()` firing too often for en_US sites

Reported by: swissspidy's profile swissspidy Owned by: swissspidy's profile swissspidy
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.2
Component: I18N Keywords: has-patch has-unit-tests needs-dev-note
Focuses: performance Cc:

Description

_load_textdomain_just_in_time is supposed to run once on the first translation call to see if translations need to be loaded.

This works great on a localized site. However, I found out now that this doesn't work on a bare en_US site.

At first glance, things to improve:

  1. In WP_Textdomain_Registry, change this ! empty() check to an isset() check, to distinguish between absent (null) and retrieved values but missing translation files (false): https://github.com/WordPress/wordpress-develop/blob/f375b68447d392ca7c189ebfadb6b908931d3812/src/wp-includes/class-wp-textdomain-registry.php#L100 This way, _load_textdomain_just_in_time() bails earlier
  2. In get_translations_for_domain() (or maybe elsewhere), set $l10n[ $domain ] = &$noop_translations; so that _load_textdomain_just_in_time is only called once. Needs some testing with locale switching to see if that actually works.

Not sure if this is a regression from 6.2, 6.1, or even from 4.6.

Change History (10)

#1 @swissspidy
11 months ago

Probably also happens for a localized site where the translation files are missing

This ticket was mentioned in PR #4463 on WordPress/wordpress-develop by @swissspidy.


11 months ago
#2

  • Keywords has-patch has-unit-tests added; needs-patch removed

Fix reported issue by setting Noop_Translations instances on the $l10n global.

There is one failing test right now, test_unload_textdomain_non_existent_file, but it passes in isolation. So probably some other test is not cleaning up.

Help with that test appreciated.

To-do:

  • [ ] Fix tests
  • [ ] Add new tests covering the new desired behavior

Trac ticket: https://core.trac.wordpress.org/ticket/58321

#3 @mukesh27
11 months ago

  • Priority changed from normal to high

As a member of the Performance team, I am going to set the priority to high as the major website is in en_US, and these changes will help improve performance for all of them.

#4 @swissspidy
11 months ago

  • Milestone changed from Future Release to 6.3

#5 @swissspidy
11 months ago

  • Priority changed from high to normal

#6 @swissspidy
11 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55865:

I18N: Improve _load_textdomain_just_in_time() logic when there are no translation files.

Fixes a performance issue where the JIT logic is invoked for every translation call if the there are no translations in the current locale. With this change, the information is cached by adding Noop_Translations instances to the global $l10n array. This way, get_translations_for_domain() returns earlier, thus avoiding subsequent _load_textdomain_just_in_time() calls.

Props swissspidy, johnbillion, ocean90.
Fixes #58321.

#8 @swissspidy
11 months ago

Tested this change using npm run research benchmark-server-timing -- -n 100 -c 2 -u http://localhost:8889/ using en_US locale.

Before this change:

wp-total (median): 397.24

After this change:

wp-total (median): 387.65

So that's a ~2.5% improvement

#9 @flixos90
11 months ago

For reference, in my test runs with TT3 and en_US locale, WP_DEBUG and other debugging constants all set to false (npm run research -- benchmark-server-timing -u http://localhost:8889 -n 100 -p), I got:

  • 157.24ms with this change, 157.89ms without (~1% faster)
  • 157.06ms with this change, 158.02ms without (~1.6% faster)
  • 159.08ms with this change, 161.14ms without (~2.3% faster)

While the difference oveerall is small enough to potentially be due to variance between test runs, I consistently see better results with the commit applied. It's a relatively small win, but I think it's safe to say it is a win. 👍

#10 @swissspidy
9 months ago

  • Keywords needs-dev-note added
Note: See TracTickets for help on using tickets.