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.

https://core.trac.wordpress.org/ticket/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

[Cybr]

Hi @swissspidy, thank you for patching the improvements, but I've noticed that most of the suggestions I've made haven't been fully considered.

Admin: ​https://3v4l.org/suK5D (my proposed patch is 40% faster)
Front: ​https://3v4l.org/JM0l4 (my proposed patch is 5% faster)

I'd like to discuss a few specific points:

1. is_admin() is more often true than the insecure test of wp_is_json_request(); hence I meticulously switched those checks around.
2. else if vs elseif... WordPress has no standard on which to use. I suggest using elseif because one large word is easier to read than two separate words: ​https://nceo.umn.edu/docs/presentations/nceo-lep-iep-ascdhandoutchunking.pdf.
3. Many developers have been advocating for the use of short ternary operators, but after ​4 years of requesting comments, there's absolutely no data given on why short ternary is "not readable." All we got close to receiving were dinosaur excuses and that characters of the same length look alike (how can one see all six?). The patch as-committed adds extra jumps and operations without sufficient justification.
4. ! empty( $_GET['wp_lang'] ) is checked twice now in the same clause.
5. The patch as-committed introduces a bug, whereas my proposal did not: if a bogus Unicode character is sent like ?wp_lang=%E1%A0%8E (add unit tests?), we'll have no language returned at all. To avoid this issue, we should still call get_{user_}locale() even if the query or cookie is set. We could account for that without using short ternary, but that is 5% slower than simply writing the get_{user_}locale()-output in most cases (​https://3v4l.org/CD9MQ).

[swissspidy]

Thanks for letting me know! I'll take a closer look soon, so here's just a quick reply to your points in the meantime.

1. Both are pretty simple functions, so I don't think it makes a big difference in this case.
2. WordPress actually does have a standard (​`elseif`), and I mixed it up. Good catch!
3. I see your point, but I'm not introducing the short ternary operator here because it's against the coding standards.
4. A consequence of not using short ternary operators, no? Let me know if you have a specific change in mind.
5. So your patch inadvertently fixed an edge case bug that was already present in core before this change, right? Because with the added tests I ensured that the existing behavior didn't change. Happy to fix some edge cases with accompanying tests.

All in all it seems like the committed changes have improved the function quite a bit, while keeping it readable at the same time. Given that the function is not called super often, what is the impact of your proposed changes on an actual WP page load?

[Cybr]

1. This ticket is meant to be educational and for me to see whether the WordPress ecosystem is ready to break barriers. If my 1.5~2.2x performance improvements are thrown aside because of obsolete coding standards, I see no reason to continue improving the software at more impactful places.
2. It appears that this standard isn't consistently applied in WordPress. As of writing, 109 instances of else if are present in ​WordPress/wordpress-develop's first-party code.
3. Breaking this barrier to allow short ternary syntax is imperative. WordPress will be held back with this restriction still in place. Short ternary can make the code more concise, readable, and often faster (less writing to variables, fewer jumps). It's a basic syntax that anyone can learn to apply and should be embraced. We can condense class WP_Query by at least a third if we start implementing this syntax.
4. No, that can be written around at the expense of even more legibility if not using short ternary. But this is moot because of 5.
5. No, the bug wasn't present before because the ! empty( $wp_lang ) check prevented that in a separate clause near the end.

[swissspidy]

I see your points regarding the syntax and coding standards. They would be really good arguments for a proposal post on the make/core blog to revisit the topic. After a positive outcome we can tackle all these areas that you mention. I'd be happy to help write/review such a post.

In the meantime I'll look into fixing the other things that I missed in the previous commit.

It appears that this standard isn't consistently applied in WordPress. As of writing, 109 instances of else if are present in WordPress/wordpress-develop's first-party code.

Let's fix that then :) At first glance, only third-party libs and JS code uses else if, which is fine. But if there are valid cases, let's address them in a new ticket.

Also uses proper elseif and checks for is_admin() first

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

FYI @sybrew

[spacedmonkey]

@swissspidy You new PR is ready to commit.

[swissspidy]

I would appreciate a review from @Cybr first

[Cybr]

The fallback no longer considers get_user_locale() when the 'wp_lang' cookie or query is unusable; only get_locale(). I already spoke to @swissspidy about that, and since no harm could be done in practice, we concluded it's fine.

In the real world, is_admin() is a little heavier than simply a return true;, so I upgraded the test to be more akin to the real world.

Comparing to the latest PR (4608), my initially proposed patch is about 1.06x faster on the front end: ​https://3v4l.org/BPjZk.
And about 1.05x faster in the admin area: ​https://3v4l.org/kdEQF.

I consider this difference negligible. Still, it proves that the short ternary operator can be used to speed up code.

Using the same more thorough test on the previous PR (4510), my patch was ​1.01x and ​1.17x faster, respectively. I'm glad to see the changes in 4608 leveled out the impact it had on admin performance.

I don't like that ! empty( $_GET['wp_lang'] ) is tested twice now, but fixing that without having short ternary available only makes things worse.

All in all, the PR 4608 as-is Looks Good To Me!

[swissspidy]

In 56003:

I18N: Ensure determine_locale() does not potentially return an empty string.

Call get_locale() as a last resort in case the sanitized locale is an empty string.

Also swaps conditionals to cater for more typical use case and adds tests.

Follow-up to [55862]

Props Cybr, swissspidy.
Fixes #58317.

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