[wp-trac] [WordPress Trac] #51678: [5.6 Beta] Performance regression with i10n changes when translations files are not present

[WordPress Trac]

#51678: [5.6 Beta] Performance regression with i10n changes when translations files
are not present
---------------------------------------+---------------------
 Reporter:  vedjain                    |       Owner:  (none)
     Type:  defect (bug)               |      Status:  new
 Priority:  high                       |   Milestone:  5.6
Component:  I18N                       |     Version:  trunk
 Severity:  normal                     |  Resolution:
 Keywords:  needs-testing needs-patch  |     Focuses:
---------------------------------------+---------------------

Old description:

> 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
> [https://core.trac.wordpress.org/changeset/49236 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
> [https://core.trac.wordpress.org/browser/trunk/src/wp-
> includes/l10n.php?rev=49236#L742 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
> #49326.
>
> 4. We were in fact caching the output of this call previous to #49326.

New description:

 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
 [https://core.trac.wordpress.org/changeset/49236 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
 [https://core.trac.wordpress.org/browser/trunk/src/wp-
 includes/l10n.php?rev=49236#L742 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].

--

Comment (by SergeyBiryukov):

 Replying to [comment:1 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 :)

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/51678#comment:6>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform