#58317 closed enhancement (fixed)
Refactor determine_locale() for performance
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (16)
#1
@
13 days ago
- Component changed from General to I18N
- Milestone changed from Awaiting Review to Future Release
- Severity changed from minor to normal
This ticket was mentioned in PR #4455 on WordPress/wordpress-develop by @spacedmonkey.
13 days ago
#2
Trac ticket: https://core.trac.wordpress.org/ticket/58317
#3
in reply to:
↑ description
@
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
@
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
@
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.
#9
@
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
@
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.
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
@
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.
@swissspidy commented on PR #4510:
44 hours ago
#14
Committed in https://core.trac.wordpress.org/changeset/55862
Refactorization of determine_locale