[5.6 Beta] Performance regression with i10n changes when translations files are not present

[vedjain]

In WooCommerce's internal automated performance testing, we are seeing performance degradations in all paths where any plugin is loaded whose translation file is not present. Root cause seems to in changeset 49236, where we added some changes and introduced WP_Textdomain_Registry to store text domains and their language directory paths.

Looks like, if translation file is not present for a domain, we hit the is_readable on every l10n call. While cached internally, it seems like this call is still slow on many hosts and it will be good to add additional caching around it.

Adding line $wp_textdomain_registry->set( $domain, false ); inside if ( ! is_readable( $mofile ) ) block restores the performance on our test sites, and I'd recommend that we commit this change, because:

1. It's unlikely that the result of is_readable( $mofile ) will ever change during the context of a request, so it should be fine to cache this result in wp_textdomain_registry.

2. Setting it to false here will cause an early return inside _load_textdomain_just_in_time, thereby reducing calls to is_readaable eventually.

3. As far as I can see, this still preserves the original fix intended in [49236].

4. We were in fact caching the output of this call previous to [49236].

[vedjain]

Apologies, the link to the original changeset is not correct, its should be [49236] (instead of the ticket link). Can an admin please edit the description and change the link from ticket to changeset?

[desrosj]

Thanks @vedjain! I am moving this into the milestone for visibility so it can be investigated.

[hellofromTonya]

@ocean90 can you take a look to validate this reported regression for changeset https://core.trac.wordpress.org/changeset/49236?

[swissspidy]

He's on vacation until next week.

But I think it makes sense

[SergeyBiryukov]

Replying to vedjain:

Apologies, the link to the original changeset is not correct, its should be [49236] (instead of the ticket link). Can an admin please edit the description and change the link from ticket to changeset?

Sure, done :)

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

[hellofromTonya]

The proposal is to add the set() in load_textdomain(), which is applied in ​PR 677:

<?php
        if ( ! is_readable( $mofile ) ) {
                $wp_textdomain_registry->set( $domain, false );
                return false;
        }

This change prevents switching the locale during the same request when the initial locale does not have a .mo file. This can be seen in the failing Tests_Locale_Switcher::test_switch_reloads_plugin_translations_outside_wp_lang_dir test.

@vedjain notes:

It's unlikely that the result of is_readable( $mofile ) will ever change during the context of a request

Marking for 2nd-opinion from @dd32, @ocean90, @swissspidy What do you think about disabling the ability to switch locale in the same request when the initial locale does not have a .mo file?

[swissspidy]

What do you think about disabling the ability to switch locale in the same request when the initial locale does not have a .mo file?

So if there‘s no es_ES mo file I cannot switch to de_DE? Sounds odd.

[hellofromTonya]

Replying to swissspidy:

So if there‘s no es_ES mo file I cannot switch to de_DE? Sounds odd.

I agree. Though it may seem "unlikely" to switch locale in the same request, it does seem plausible that there are sites where this occurs. For those sites, this would be a breaking change. Hence, seeking 2nd opinion on the original proposal.

Is there a way to retain the ability to switch locales while also addressing the performance concern raised by @vedjain?

[ocean90]

Though it may seem "unlikely" to switch locale in the same request, it does seem plausible that there are sites where this occurs.

That's definitely not unlikely and actually the only purpose of this function, to switch the locale multiple times during a request.

It seems like there's need for another registry which caches the full path per locale.

[swissspidy]

Replying to ocean90:

It seems like there's need for another registry which caches the full path per locale.

The current registry could be extended accordingly, using a multi-dimensional array? Sounds reasonable to have a per-locale cache

[hellofromTonya]

​Adding notes from Slack from Rodrigo Primo:

The performance regression that is fixed in this ticket has a big impact in WooCommerce core. without this fix some of the REST API requests that are performed to load most of our admin pages take over ten seconds to complete making those pages almost unusable. please let me know if there is anything that the WooCommerce team can do to help move this ticket forward.

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

This is first POC at making WP_Textdomain_Registry locale-aware.

Because of how load_textdomain gets called from multiple places with different locale sources (theme, plugin, core, sth else potentially?), I _think_ the early exit within is_readable cannot use determine_locale, but has to guess it from the mo file. Therefore, I added another function, get_locale_from_mo_file().

I'm setting up the test suite now to check if any tests need to be updated as well.

Hi @peterfabian! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request, so here are a few things to be aware of that may help you out.

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this one mirrors. But please feel free to use pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in ​this blog post.

Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the ​Automated Testing page in the handbook.

If you have not had a chance, please review the ​Contribute with Code page in the ​WordPress Core Handbook.

The ​Developer Hub also documents the various ​coding standards that are followed:

• ​PHP Coding Standards
• ​CSS Coding Standards
• ​HTML Coding Standards
• ​JavaScript Coding Standards
• ​Accessibility Coding Standards
• ​Inline Documentation Standards

Please remember that the WordPress project is largely maintained by volunteers

Thank you,
The WordPress Project

[peterfabian1000]

Hi folks. As Rodrigo and Vedanshu mentioned in Slack and in this ticket, this has rather large impact on WooCommerce (and potentially anything that uses multiple requests to build a page).

I did a coarse benchmark and compared 5.5.3, 5.6-beta3 and I can see quite large difference, as you can see in the screenshot, two bottom rectangles: ​https://cloudup.com/cnW7jCJgzxN. The slowest request in 5.5.3 took about 1.87s, while in 5.6-beta3 it's 11.66s.

Part of it could be our fault and how we use translations strings, but I think more plugins will probably face the same issue.

I've tried to create a PR with a POC of WP_Textdomain_Registry caching also on locale-level, ​https://github.com/WordPress/wordpress-develop/pull/694.

I've also tested how it performs and you can see the results in the same screenshot, first rectangle shows load times with the early exit within is_readable, the second one is the same POC code, but without the early exit.

To have a more exact number, I also counted the calls to load_textdomain() and I got the following numbers on loading WC home:

• without early exit: 112992 calls
• with early exit: 83 calls

Hope it helps in pushing this problem to resolution. Thanks!

For context see ​the caching in 5.5.3.

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

[vedjain]

To add on more info, this will impact any plugin/theme which does not have .mo file present for the current locale.

I used this small benchmarking code to assess:

<?php
        load_plugin_textdomain( 'domain_xyz' );

        $time_start = microtime( true );
        for( $i = 0; $i < 5000; $i++ ) {
                __( 'Any text ' . $i, 'domain_xyz' );
        }
        $end_time = microtime( true ) - $time_start;
        echo $end_time;

In trunk, the output was 0.22249007225037 whereas in branch 5.5, output was 0.011736869812012 which implies every translation call getting about 20x slower. (I went on vacation immediately after opening this ticket, so that's why the delay of response).

After applying patches separately by @ocean90 and @peterfabian1000, these figures are improved, but it's still at 0.12674498558044 which is still about 10x slower than in 5.5, so perhaps something is still missing in that patch, although I have not figured it out yet.

[vedjain]

Possible patch for performance regression issue

[vedjain]

Added another possible solution in https://core.trac.wordpress.org/attachment/ticket/51678/patch_51678.patch which is basically the implemention of original suggestion but also clears the registry cache whenever locale is switched to prevent issue with switching locales in same request. This also restores the original performance as per 5.5 in my internal testing.

Note that this can be improved a bit more by not clearing cached_mo_files, which I can make if the overall approach makes sense.

[ocean90]

@vedjain Can you please submit your patches as a PR? This should also show you that it will break a few tests. It's still not clear what exactly you're testing and how it's related to the is_readable() line which isn't new.

​PR707 restores some static caching which we had before to reduce some load_textdomain() calls. Initially it's still called once. If we don't want this either we could also restore _get_path_to_translation_from_lang_dir().

Testing for CI

Hi @vedanshujain! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request, so here are a few things to be aware of that may help you out.

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this one mirrors. But please feel free to use pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in ​this blog post.

Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the ​Automated Testing page in the handbook.

If you have not had a chance, please review the ​Contribute with Code page in the ​WordPress Core Handbook.

The ​Developer Hub also documents the various ​coding standards that are followed:

• ​PHP Coding Standards
• ​CSS Coding Standards
• ​HTML Coding Standards
• ​JavaScript Coding Standards
• ​Accessibility Coding Standards
• ​Inline Documentation Standards

Please remember that the WordPress project is largely maintained by volunteers

Thank you,
The WordPress Project

[vedjain]

@ocean90 you are right, looks like I was not applying the patch correctly. Looks like we are good to merge then?

Could you perhaps expand a bit on this change? Why another static cache on before the registry?

@swissspidy See the linked Trac ticket and the caching we had before. Without the cache we now repeatedly call load_textdomain() (and thus increased file system reads) for domains without any files.

Right. I was more thinking about calling $wp_textdomain_registry->set() in _load_textdomain_just_in_time() so that $wp_textdomain_registry acts as the sole cache here. (as suggested in https://core.trac.wordpress.org/ticket/51678#comment:14). But perhaps that's a bit too complex?

@swissspidy This would only work if we make the registry locale aware, right? ​https://github.com/WordPress/wordpress-develop/pull/694 would be related.

yes indeed

In this case I tend to revert the change for 5.6 so we'd have enough time for testing other implementations.

Could also use this approach here and extend the class in 5.7, no strong opinion either way

[hellofromTonya]

@swissspidy @ocean90 Beta 4 is today. Is it possible for this fix to be included?

[ocean90]

In 49566:

I18N: Revert [49236] for now to investigate alternative implementations.

See #39210, #51678, #26511.

[ocean90]

Duplicate of #39210.

[rodrigosprimo]

Thanks for [49566], @ocean90. I just tested WP 5.6-beta4 and could confirm that the performance regression that was impacting WooCommerce is fixed.