Make WordPress Core

Opened 13 days ago

Closed 44 hours ago

Last modified 44 hours ago

#58317 closed enhancement (fixed)

Refactor determine_locale() for performance

Reported by: cybr's profile Cybr Owned by: swissspidy's profile swissspidy
Milestone: 6.3 Priority: normal
Severity: normal Version: trunk
Component: I18N Keywords: has-patch has-unit-tests
Focuses: performance Cc:

Description

determine_locale() breaks various performance tropes. Albeit an obscure function, I hope this refactor will be the first of many.

The first defense clause:

<?PHP
if ( ! empty( $determined_locale ) && is_string( $determined_locale ) ) {

Could become:

<?PHP
if ( $determined_locale && is_string( $determined_locale ) ) {

Here, I removed a function call and a NOT operation, resulting in exactly the same code.

Determining the user locale:

<?php
$determined_locale = get_locale();

if ( is_admin() ) {
        $determined_locale = get_user_locale();
}

if ( isset( $_GET['_locale'] ) && 'user' === $_GET['_locale'] && wp_is_json_request() ) {
        $determined_locale = get_user_locale();
}

Could become:

<?php
if ( 
           is_admin() 
        || ( isset( $_GET['_locale'] ) && 'user' === $_GET['_locale'] && wp_is_json_request() )
) {
        $determined_locale = get_user_locale();
} else {
        $determined_locale = get_locale();
}

Here I exchange a jump for an operator (first and second if-statements are combined and exchanged with a logic operator). I also removed writing to the variable twice in admin and insecure JSON requests.

Writing WP lang:

<?php
$wp_lang = '';

if ( ! empty( $_GET['wp_lang'] ) ) {
        $wp_lang = sanitize_text_field( $_GET['wp_lang'] );
} elseif ( ! empty( $_COOKIE['wp_lang'] ) ) {
        $wp_lang = sanitize_text_field( $_COOKIE['wp_lang'] );
}

if ( ! empty( $wp_lang ) && ! empty( $GLOBALS['pagenow'] ) && 'wp-login.php' === $GLOBALS['pagenow'] ) {
        $determined_locale = $wp_lang;
}

Could become:

<?php
if ( isset( $GLOBALS['pagenow'] ) && 'wp-login.php' === $GLOBALS['pagenow'] ) {
        if ( ! empty( $_GET['wp_lang'] ) ) {
                $determined_locale = sanitize_text_field( $_GET['wp_lang'] ) ?: $determined_locale;
        } elseif ( ! empty( $_COOKIE['wp_lang'] ) ) {
                $determined_locale = sanitize_text_field( $_COOKIE['wp_lang'] ) ?: $determined_locale;
        }
}

Here, instead of writing to $wp_lang twice, and then determining whether we actually should use that value if the second attempt didn't happen, we first determine the latter. Using the amazing power of the short ternary, we spare a jump if the value amounts to nothing.

Still, the last two refactorizations could be combined, but I could not think of a way to maintain readability while keeping in mind "DRY" standards.

The refactorization makes the function about 1.5x faster on the front-end, and about 2.2x faster in admin — without changing any functionality.

Attachments (1)

l10n.php.patch (1.6 KB) - added by Cybr 13 days ago.
Refactorization of determine_locale

Download all attachments as: .zip

Change History (16)

@Cybr
13 days ago

Refactorization of determine_locale

#1 @spacedmonkey
13 days ago

  • Component changed from General to I18N
  • Milestone changed from Awaiting Review to Future Release
  • Severity changed from minor to normal

#3 in reply to: ↑ description @SergeyBiryukov
13 days ago

Thanks for the ticket!

Replying to Cybr:

The first defense clause:

<?PHP
if ( ! empty( $determined_locale ) && is_string( $determined_locale ) ) {

Could become:

<?PHP
if ( $determined_locale && is_string( $determined_locale ) ) {

Here, I removed a function call and a NOT operation, resulting in exactly the same code.

I'm curious, could we have a benchmark for this particular change? The PHP manual entry for empty() specifically states that it's a language construct, not a function.

#4 @swissspidy
13 days ago

  • Keywords needs-unit-tests added

Thanks a lot for putting together a patch & detailing the changes after previously mentioning your findings on Slack!

+1 re: the empty() question above. I don't see any change in the numbers when adding that back in. But really just a detail if the behavior is the same.

That said, as long as we have proper test coverage for this function then that doesn't matter too much.

#5 @Cybr
13 days ago

Here I have a benchmark in general for plain/empty/not/not-empty/isset/not-isset: https://3v4l.org/YvKIn.

"Not empty" is consistently the worst performer, then "not isset," then "empty/not," then "not," then (the fastest) "plain." "Not empty" is about twice as slow as a "plain" boolean check.

This is in line with this benchmark, comparing the patch attached to this ticket versus to itself, where one has a "not-empty" ! empty( $determined_locale )-check, and the other a "plain" boolean $determined_locale-check. This alone makes up for a 1.15x performance change.

#6 @swissspidy
12 days ago

  • Milestone changed from Future Release to 6.3

#7 @spacedmonkey
12 days ago

Patches need a rebase after [55760].

#8 @spacedmonkey
5 days ago

  • Owner set to swissspidy
  • Status changed from new to assigned

#9 @swissspidy
4 days ago

@Cybr Curious, how did you end up looking at improving performance for this function? Usually this function is only called a few times per request. Especially after #58321, which fixes some edge case.

Again, the patch looks reasonable to me, but the effect it has might be negligible in the larger context.

#10 @Cybr
4 days ago

I didn't test the total impact of function determine_locale() -- it would likely be negligible in the grand scheme. I created this ticket primarily to educate about various performance pitfalls.

Doing these kinds of optimizations throughout WordPress's most called functions, such as WP_Object_Cache::is_valid_key(), would yield a significant improvement. Repeat this for all of WordPress Core, and I think we can at least halve the load times.

I have some slow PHP functions listed here. For example, we should set autoload: false to all internal class_exist() calls, for it would prevent invoking autoloaders of plugins, especially since WP (still) doesn't use an autoloader --- and once WordPress has an autoloader, we could remove these calls altogether (see #36335, which derailed into handling dependencies via Composer because people still don't know about spl_autoload()).

If you'd like, I could scan for WordPress's most called functions and create a ticket for each one with a greater than 0.1% PHP load impact, explaining what went wrong, how we can solve that, and how to prevent the issue in future updates. I'm afraid the biggest problem with WP now is that wp-settings.php loads over 200 files, many aren't even used at runtime on most sites (like 47 for REST, and another 10 for sitemaps). Autoloading should become of utmost priority, stat.

#50568 should also still need to be resolved for a 30% PHP performance boost; though, IIRC, in WP 6.2, the term cache handler changed.

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

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


3 days ago
#11

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

Refreshed l10n.php.patch patch + additional tests for determine_locale() that pass before and after the patch.

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

#12 @swissspidy
44 hours ago

@Cybr Thanks! I'm sure folks, particularly from the core performance team, would really appreciate such an in-depth analysis. Especially since some people have also spent some time looking at potentially slow functions like file_exists. I would recommend first collecting your findings in a Google doc or so and then sharing in the #core-performance channel. That makes it easier to discuss them and find synergies. Afterwards we can figure out the best path forward when it comes to creating tickets.

#13 @swissspidy
44 hours ago

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

In 55862:

I18N: Refactor determine_locale() for performance and readability.

Refactors the function to avoid unnecessary get_locale() calls and slightly improve performance, while keeping it readable.

Adds tests.

Props Cybr, spacedmonkey, swissspidy.
Fixes #58317.

Note: See TracTickets for help on using tickets.