Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#51678 closed defect (bug) (duplicate)

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

Reported by: vedjain's profile vedjain Owned by:
Milestone: Priority: high
Severity: normal Version: 5.6
Component: I18N Keywords:
Focuses: performance Cc:

Description (last modified by SergeyBiryukov)

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.
  1. Setting it to false here will cause an early return inside _load_textdomain_just_in_time, thereby reducing calls to is_readaable eventually.
  1. As far as I can see, this still preserves the original fix intended in [49236].
  1. We were in fact caching the output of this call previous to [49236].

Attachments (1)

patch_51678.patch (1.1 KB) - added by vedjain 4 years ago.
Possible patch for performance regression issue

Download all attachments as: .zip

Change History (40)

#1 @vedjain
4 years ago

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?

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#2 @desrosj
4 years ago

  • Component changed from General to I18N
  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 5.6
  • Priority changed from normal to high
  • Version set to trunk

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

#3 @hellofromTonya
4 years ago

  • Description modified (diff)

#4 @hellofromTonya
4 years ago

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

#5 follow-up: @swissspidy
4 years ago

  • Keywords needs-patch added

He's on vacation until next week.

But I think it makes sense

#6 @SergeyBiryukov
4 years ago

  • Description modified (diff)

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 :)

This ticket was mentioned in PR #677 on WordPress/wordpress-develop by hellofromtonya.


4 years ago
#7

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

#8 in reply to: ↑ 5 @hellofromTonya
4 years ago

  • Keywords dev-feedback 2nd-opinion added

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?

#9 follow-up: @swissspidy
4 years ago

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.

#10 in reply to: ↑ 9 @hellofromTonya
4 years ago

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?

This ticket was mentioned in Slack in #core by helen. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by rodrigoprimo. View the logs.


4 years ago

#13 follow-up: @ocean90
4 years ago

  • Keywords needs-patch added; has-patch has-unit-tests 2nd-opinion removed

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.

#14 in reply to: ↑ 13 @swissspidy
4 years ago

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

#15 @hellofromTonya
4 years ago

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.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

#17 @davidbaumwald
4 years ago

  • Focuses performance added

This ticket was mentioned in PR #694 on WordPress/wordpress-develop by peterfabian.


4 years ago
#18

  • Keywords has-patch added; needs-patch removed

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.

github-actions[bot] commented on PR #694:


4 years ago
#19

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:

Please remember that the WordPress project is largely maintained by volunteers

Thank you,
The WordPress Project

#20 @peterfabian1000
4 years ago

  • Keywords needs-patch added; has-patch removed

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!

Last edited 4 years ago by peterfabian1000 (previous) (diff)

This ticket was mentioned in PR #707 on WordPress/wordpress-develop by ocean90.


4 years ago
#21

  • Keywords has-patch added; needs-patch removed

#22 @vedjain
4 years ago

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

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.

Version 0, edited 4 years ago by vedjain (next)

@vedjain
4 years ago

Possible patch for performance regression issue

#23 @vedjain
4 years ago

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.

#24 @ocean90
4 years ago

@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().

This ticket was mentioned in PR #713 on WordPress/wordpress-develop by vedanshujain.


4 years ago
#25

Testing for CI

github-actions[bot] commented on PR #713:


4 years ago
#26

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:

Please remember that the WordPress project is largely maintained by volunteers

Thank you,
The WordPress Project

#27 @vedjain
4 years ago

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

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

swissspidy commented on PR #707:


4 years ago
#29

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

ocean90 commented on PR #707:


4 years ago
#30

@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.

swissspidy commented on PR #707:


4 years ago
#31

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?

ocean90 commented on PR #707:


4 years ago
#32

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

swissspidy commented on PR #707:


4 years ago
#33

yes indeed

ocean90 commented on PR #707:


4 years ago
#34

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

swissspidy commented on PR #707:


4 years ago
#35

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

#36 @hellofromTonya
4 years ago

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

#37 @ocean90
4 years ago

In 49566:

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

See #39210, #51678, #26511.

#38 @ocean90
4 years ago

  • Keywords needs-testing dev-feedback has-patch removed
  • Milestone 5.6 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #39210.

#39 @rodrigosprimo
4 years ago

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

Last edited 4 years ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.