Refactor determine_locale() for performance

[Cybr]

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.

[Cybr]

Refactorization of determine_locale

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

[SergeyBiryukov]

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.

[swissspidy]

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.

[Cybr]

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.

[spacedmonkey]

Patches need a rebase after [55760].

[swissspidy]

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

[Cybr]

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.

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

[swissspidy]

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

[swissspidy]

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.

Committed in https://core.trac.wordpress.org/changeset/55862

https://core.trac.wordpress.org/changeset/55862