Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance in the WP_Theme_JSON class by simplifying calls to array functions #3555

Closed
wants to merge 25 commits into from

Conversation

aristath
Copy link
Member

@aristath aristath commented Nov 3, 2022

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

This PR simplifies expensive array-functions calls where appropriate.

Attaching some profiling results to showcase the impact.

Before (trunk):
Screenshot 2022-11-03 at 9 43 25 AM

After (with this PR):
Screenshot 2022-11-03 at 9 43 15 AM

All numbers in the screenshots above use milliseconds, and the impact of these seemingly simple changes is bigger than expected, shaving off more than 200ms. Of course those numbers are a bit inflated due to the fact that profiling slows down a page significantly, but the overall improvement on the page-load is more than 3%.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

props @jrfnl 🙌

@aristath aristath force-pushed the feature/pp-ari-wp-theme-json branch 2 times, most recently from 3ddcf7e to d0bc5f2 Compare November 3, 2022 08:40
@aristath aristath marked this pull request as ready for review November 3, 2022 09:05
@aristath aristath force-pushed the feature/pp-ari-wp-theme-json branch from 52ae187 to 213e94b Compare November 8, 2022 07:19
@spacedmonkey
Copy link
Member

While doing some profiling, I noticed there are still calls to _wp_array_get. This calls array_key_exists, which is expensive. I wonder if we could avoid these somehow.

@@ -2317,6 +2343,9 @@ public function merge( $incoming ) {
) {
_wp_array_set( $this->theme_json, $path, $content );
} else {
$slugs_node = static::get_default_slugs( $this->theme_json, $node['path'] );
$slugs = array_merge_recursive( $slugs_global, $slugs_node );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't added, it was just moved here. 😉
A few lines above we were calling these 2 on the previous foreach loop (used to be line 2281), so they were always running. Instead of always calling them, we moved them here where they are actually used. This way these rather expensive calls run fewer times, only when necessary.

@jrfnl
Copy link
Member

jrfnl commented Nov 8, 2022

While doing some profiling, I noticed there are still calls to _wp_array_get. This calls array_key_exists, which is expensive. I wonder if we could avoid these somehow.

I wouldn't worry too much about (or overestimate) the impact of array_key_exists().

Note: isset() and array_key_exists() also have a functional difference in how they treat a null value, so for array_key_exists() to be equivalent to isset(), it MUST be accompanied by a && $var !== null.

@aristath
Copy link
Member Author

aristath commented Nov 9, 2022

Note: isset() and array_key_exists() also have a functional difference in how they treat a null value, so for array_key_exists() to be equivalent to isset(), it MUST be accompanied by a && $var !== null.

Yeah, that's an important distinction!
In the comments I added for future reference to replace array_key_exists() with isset() (// Note: array_key_exists() should be replaced with an isset() check once WordPress drops support for PHP 5.6.), I checked the values in each case and only added that comment in cases where the value can not be null.
In one case the value can be null, and I added a comment in that one that // We need to use array_key_exists() instead of isset() because the value can be null. 👍

@spacedmonkey
Copy link
Member

In trunk.
Screenshot 2022-11-09 at 21 54 06

In this brunch.
Screenshot 2022-11-09 at 21 54 15

@felixarntz
Copy link
Member

felixarntz commented Nov 10, 2022

@jrfnl @aristath Following up on array_key_exists: Makes sense not to overthink this in terms of performance impact compared to isset, however I had one follow up thought I wanted to at least raise here:

  • Most of the usage of array_key_exists here could be replaced with isset as the additional null consideration is irrelevant for most (if not all) of the code where it's used here.
  • Given that, there would be the alternative option of assigning the result to a variable and then calling isset on that variable.

For example, instead of:

if ( array_key_exists( $element, static::__EXPERIMENTAL_ELEMENT_CLASS_NAMES ) ) {
}

Do:

$element_class_names = static::__EXPERIMENTAL_ELEMENT_CLASS_NAMES;
if ( isset( $element_class_names[ $element ] ) {
}

Again, not saying this is necessarily worth doing (and certainly not a blocker), but I'd be curious if the first or the last is generally faster. :)

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes here look solid to me!

I of course trust the benchmarking data provided in https://core.trac.wordpress.org/ticket/56974#comment:1, but it may additionally be helpful to quantify the performance gain here in milliseconds as well.

Given that parsing theme.json affects the wp_head action quite a bit (see also #3536), I thought I'd do a performance comparison there first. Based on 9 test runs, the median impact there is small but consistent:

  • With Twenty Twenty-Three (i.e. block theme): 26.63ms vs 27.78ms (~4% faster)
  • With Twenty Twenty-One (i.e. classic theme): 15.30ms vs 16.05ms (~5% faster)

See this spreadsheet for the full data.

So also in that regard it looks like a small but worthwhile win. Potentially I can do the same for more concretely the parsing of theme.json itself as that would be even closer to the changes here and allow us to quantify impact in that way too.

@jrfnl
Copy link
Member

jrfnl commented Nov 10, 2022

There's bound to be more room for improvement, but yes, let's take this step by step and I do believe this is a valuable step in that.

Re: assign+isset vs array_key_exists: as most array_key_exists() are only in place for PHP 5.6, might be more effective to re-open the discussion on dropping support for PHP 5.6 ... 😇

On that note - PHP 8.3 will bring another very useful improvement json_validate() (validate a JSON input before decoding without running the risk of the JSON tying up all the memory), but that is no doubt still a bridge too far ;-)

Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few nits.

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
@felixarntz felixarntz requested a review from desrosj November 10, 2022 22:09
Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Commented with a few small questions/suggestions, but none of them are really to be considered blockers.

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Show resolved Hide resolved
@felixarntz
Copy link
Member

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

@@ -2035,7 +2056,9 @@ public function get_styles_for_block( $block_metadata ) {
// the feature selector. This may occur when multiple block
// support features use the same custom selector.
if ( isset( $feature_declarations[ $feature_selector ] ) ) {
$feature_declarations[ $feature_selector ] = array_merge( $feature_declarations[ $feature_selector ], $new_feature_declarations );
foreach ( $new_feature_declarations as $new_feature_declaration ) {
$feature_declarations[ $feature_selector ][] = $feature_declaration;
Copy link
Member

@oandregal oandregal Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aristath @desrosj @felixarntz @spacedmonkey @jrfnl @hellofromtonya

Shouldn't the variable assigned be $new_feature_declaration instead of $feature_declaration? This has been reported as problematic by the Gutenberg lint jobs (see conversation).

I haven't looked at what regression this introduced but it'd be good to create a follow-up PR fixing it and adding a test case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oandregal I concur. This is a bug which should be fixed.

Suggested change
$feature_declarations[ $feature_selector ][] = $feature_declaration;
$feature_declarations[ $feature_selector ][] = $new_feature_declaration;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 yeah, looks like this was a typo.. Good catch 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happens to the best of us ;-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this code path, so asked folks to provide details for testing at WordPress/gutenberg#46579 (comment) so we can add a new unit test and gauge how severe this is.

Copy link
Member

@jrfnl jrfnl Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS, this should go in the next 6.1 patch release.

Edit to clarify: I'm basing this assessment on the following:

  • It's very clearly a bug with an obvious fix.
  • It's likely a regression compared to previous behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants