Fix `wp_head` performance regression for classic themes #3536

felixarntz · 2022-10-27T20:29:56Z

A performance analysis for WordPress 6.1 has shown that there is a performance regression in wp_head due to additional logic focused on FSE, which particularly for classic themes appears to be unnecessary.
See relevant Trac ticket comments discussing this so far:
The original implementation of this PR improves median wp_head execution time from 16.45ms to 11.92ms (~28% faster) for classic themes (concretely tested with Twenty Twenty-One), while apparently not missing any actual functionality needed.
However, based on Fix wp_head performance regression for classic themes #3536 (comment), a more complex part of this implementation was broken out into a separate Gutenberg PR Create parsed PHP version of core's theme.json at build time rather than during runtime gutenberg#45616, so that the rest of this PR can realistically make it into the WordPress 6.1.1 release.
The latest implementation of this PR still improves median wp_head execution time from 16.45ms to 14.78ms (~10% faster) for the same test environment. This is quite a bit less of an improvement from the original, but it makes sense as with the latest implementation still core's theme.json and theme-i18n.json files are being parsed. That said, this still brings a worthwhile performance improvement and sets the stage for the follow up work in Create parsed PHP version of core's theme.json at build time rather than during runtime gutenberg#45616 to address the remaining part of the performance issues.
See this spreadsheet for the test results.

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

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.

…sic theme without support.

…upport.

src/wp-includes/class-wp-theme-json-resolver.php

spacedmonkey · 2022-11-01T17:05:03Z

cc @oandregal

oandregal · 2022-11-02T14:57:13Z

👋 I've seen the changes and this is what I can share:

get_core_data: this data needs to be available both for classic and block themes. It loads the default editor configuration (for example, the default color palettes)
get_block_data: this data is necessary for both classic and block themes. It loads style information from blocks.
get_theme_data: this data is required for both classic and block themes. For classic, it takes it from the theme supports and returns it as if it was a theme.json. There may be some opportunities to micro-optimize here, such as do not load the i18n schema if the theme and its parent do not have a theme.json (we'd need to measure how impactful that single change is, that´d be a nice single PR).
get_user_data: this is safe to skip for classic themes. There are edge cases, such as a theme that has theme.json and then it removes it, though I'm inclined to think user styles are safe to ignore in that case. Coincidentally, I've done the same thing in a prototype I'm working on. It can be extracted as a small, focused PR.

Coincidentally, for the area of "theme.json APIs", I'm compiling a list of tasks I find most pressing at WordPress/gutenberg#45171 I'm very optimistic about how those will impact the performance and code quality. For example, I've just published a prototype that reduces the total time of a request from 514.44ms to 343.71ms, a 33% performance increase.

Another thing that it'd be useful is to prepare these PRs directly in Gutenberg. That way, they can be tested widely and bugs will be raised earlier than the WordPress RC cycles, which is always a lot less stressful and give people time to figure out long-term solutions rather than patches for the RC.

oandregal · 2022-11-02T15:00:54Z

This PR improves median wp_head execution time from 16.45ms to 11.92ms (~28% faster) for classic themes (concretely tested with Twenty Twenty-One

Felix, would you share the steps you did to test this, so it can be replicated in a variety of environments?

As a side note, it'd be great to track some statistics across WordPress versions and watch its evolution, like we do for the editor (see live at https://codehealth.vercel.app/).

felixarntz · 2022-11-02T21:12:53Z

@oandregal Thank you for providing additional context on why (parts of) these methods are needed for classic themes as well. Based on what you're saying, I still think there are a number of performance optimizations to make here to avoid running FSE-only logic for classic themes. Here are some of the high-level ideas I'm thinking:

In get_core_data(), parsing WP's own theme.json is a rather wasteful implementation in itself, more so with the additional translation parsing. Since this file is shipped by core (and thus never changes between core updates), we should find a more efficient solution. For example, how about we generate a fully PHP-ready parsed version including i18n strings of WP's theme.json at build time? I see how theme.json is certainly more portable than the same data in PHP array format, but we could create an autogenerated version of that file, like a theme-json.php file that can then simply be required by PHP, no JSON parsing and i18n schema parsing necessary at runtime.
In get_block_data(), everything looks fine as is to me.
In get_theme_data(), we should simply not parse the i18n schema and try to parse the theme's theme.json if it doesn't exist, like you're saying and like this PR already does.
get_user_data() can bail early if in a classic theme, also like you're saying and like this PR already does.

I'll make some updates to the proposed code here accordingly, the main thing though that requires some more work would be to generate some version of a theme-json.php file as part of the build process. If we build something like that, we could even share it with e.g. theme developers so they can run it on their theme.json files too.

Felix, would you share the steps you did to test this, so it can be replicated in a variety of environments?

It was a fairly basic process, here's what I did:

Modify WP core's wp_head() function directly, to measure microtime( true ) before the do_action( 'wp_head' ) call and after it, then get the difference to know how long the action took.
I did this with trunk and with this PR branch, using a site with Twenty Twenty-One on just the regular basic "Hello world" singular post URL.
I ran both tests 9 times each (to account for variance) and calculated the median.

If we wanted to dive in more deeply, it would also be interesting to see what the difference would be on a more complex piece of content, e.g. the Gutenberg demo page.

… pre-built PHP-parsed version of WP core theme.json if present.

…xport()).

felixarntz · 2022-11-02T21:51:25Z

@oandregal I've pushed some updates here as mentioned above, please have a look. I have included what the theme-json.php file could look like in principle, although of course this would need to be auto-generated by a build process eventually.

One additional concern/question I have: Why is the condition around whether to reuse the previously "cached" theme data only present around parsing the (child) theme's theme.json (see https://github.com/WordPress/wordpress-develop/pull/3536/files#diff-d6b86476eed058e7cf8b6e57fa52c4fd75b1f0907e1a9ccb0149528a24f7578bR254)? Are there any problems if we change it to apply to the entire method, similar to how it is for the other get_*_data() methods? Otherwise some of that logic is still run on every single method call over and over again. cc @hellofromtonya

spacedmonkey · 2022-11-03T10:57:58Z

This PR is heavily related - #3556.

src/wp-includes/class-wp-theme-json-resolver.php

src/wp-includes/class-wp-theme-json.php

spacedmonkey · 2022-11-08T19:00:48Z

This is hard to code review the code to generate theme-php.php, fix the unit test and coding standards.

felixarntz · 2022-11-08T19:04:41Z

@spacedmonkey

This is hard to code review the code to generate theme-php.php, fix the unit test and coding standards.

No point to review the theme-json.php file, as eventually it will be auto-generated. There's also no point in fixing the WPCS etc violations in that file - see the TODO comment and the issue comments above outlining that. :)

I'll work on further polishing here today.

@draganescu @hellofromtonya Any chance I could get your thoughts on the changes and proposed approach from this draft PR? Please make sure to review my above issue comments for context.

spacedmonkey · 2022-11-08T19:06:42Z

No point to review the theme-json.php file, as eventually it will be auto-generated. There's also no point in fixing the WPCS etc violations in that file - see the TODO comment and the issue comments above outlining that. :)

I don't think we should merge this without the automatic generation code, in JS grunt.

I think we should consider making this out to another PR, where we have more time to review / test.

felixarntz · 2022-11-08T23:38:07Z

@spacedmonkey

I don't think we should merge this without the automatic generation code, in JS grunt.

That's exactly what I'm saying.

I think we should consider making this out to another PR, where we have more time to review / test.

Sounds good, I've opened WordPress/gutenberg#45616 for that.

Will remove the theme-json.php part from here and get this PR ready for 6.1.1.

felixarntz · 2022-11-09T02:54:28Z

@spacedmonkey @draganescu @hellofromtonya This PR is now ready for a full review, including test coverage. Would be great to get your feedback so that we can aim to get this into 6.1.1.

oandregal · 2022-11-09T04:50:34Z

src/wp-includes/class-wp-theme-json-resolver.php

@@ -582,8 +593,8 @@ public static function get_user_global_styles_post_id() {
 	public static function theme_has_support() {
 		if ( ! isset( static::$theme_has_support ) ) {
 			static::$theme_has_support = (
-				is_readable( static::get_file_path_from_theme( 'theme.json' ) ) ||
-				is_readable( static::get_file_path_from_theme( 'theme.json', true ) )
+				static::get_file_path_from_theme( 'theme.json' ) !== '' ||


This is nice, get_file_path_from_theme already calls is_readable.

peterwilsoncc

Added a fwe notes inline.

tests/phpunit/tests/theme/wpThemeJsonResolver.php

peterwilsoncc · 2022-11-09T04:51:31Z

src/wp-includes/class-wp-theme-json-resolver.php

+				static::get_file_path_from_theme( 'theme.json' ) !== '' ||
+				static::get_file_path_from_theme( 'theme.json', true ) !== ''


I don't think this is needed here. Yoda conditions are only required around variable to value comparisons, not with functions (since setting a function to another value would result in an error anyway).

src/wp-includes/class-wp-theme-json-resolver.php

tests/phpunit/tests/theme/wpThemeJsonResolver.php

oandregal · 2022-11-09T05:30:12Z

src/wp-includes/class-wp-theme-json-resolver.php

@@ -400,6 +408,9 @@ private static function remove_json_comments( $array ) {
 	 * @return array Custom Post Type for the user's origin config.
 	 */
 	public static function get_user_data_from_wp_global_styles( $theme, $create_post = false, $post_status_filter = array( 'publish' ) ) {
+		if ( ! static::theme_has_support() ) {


The only edge case I can think of is a theme that uses theme.json and removes it in a later version. In that case, this changes the behavior: the code will now ignore user data while it was used before.

YES! Love this change.

That's a fair point. However, I would consider this indeed an edge-case which I don't think is worth addressing here. If a theme removed its theme.json it would come with a whole lot of other behavioral changes anyway - I don't think realistically any theme author is going to change a block theme to become a classic theme.

oandregal · 2022-11-09T06:18:56Z

👋 I've run some local profiling tests using TwentyTwentyOne. My test was loading the "hello world" post as a logged-out user. These are the numbers (median of 9 tries): it takes 393.98ms with this request and 411.04ms without these changes. This improves 17.06ms, a 4.15% improvement for that theme.

src/wp-includes/class-wp-theme-json-resolver.php

oandregal · 2022-11-09T07:02:13Z

This improves 17.06ms, a 4.15% improvement for that theme.

For reference, block themes also improve in 12.47ms.

This PR does a few changes:

1. remove is_readable for theme.json check (both themes with and without theme.json)
2. avoid looking up the parent if the theme data is already set (both themes with and withouth theme.json)
3. remove one query for themes without theme.json
4. do not load the translation for themes with no theme.json

I've done my testing with TwentyTwentyOne and TwentyTwentyThree (none is a child theme). Given the changes and the numbers are so close (-12ms vs -17ms for themes with and without theme.json), it sounds like the most impactful change was actually 1 (the only code that both themes have in common in my testing). Removing the query for classic themes is a 5ms improvement at best. I'm time-bounded at the moment and could not dig deeper. My impression is that the results are not lined up with the initial expectations (we are doing more work for classic themes). Would that be a fair statement?

I'm a bit torn about whether this is worth doing. It'd be great if we could see if only applying the changes at 1 and 2 brings the same amount of improvement. If that's the case I'd remove all the other changes, as they make the code harder to read and would not bring a gain worth pursuing.

As I said, I won't have much time to look into this in the following days, though it looks like a safe change from a code point of view. Hope this is helpful.

oandregal · 2022-11-09T07:16:04Z

I also want to share the two things that I consider more impactful at the moment to focus on:

Review and cache the public API methods. An initial investigation I did resulted in this PR, which provides a 200ms reduction, a 33% improvement. Every ms counts, so squeezing all of them at the maximum is always helpful. Though there are so many things to work on that I think it'd be helpful to look at performance strategically: intervene for the bigger wins first, then look at the smaller bits. I'll continue to look into this more when I'm back at full speed.
Having an automated way to look at numbers and define what's important for us. At the moment, everyone is tracking things manually, and it takes a lot of time from the PR author and from the reviewers. It's also a bit inconsistent: should we measure the full time-to-request as logged-out users or is it best tracking particular actions such as wp_head? Having a common understanding of what to improve and tracking automatically instead of manually is a huge win that would help us to move forward faster. Unfortunately, I don't think I'll be able to help in this area much. It'd be lovely if people with more experience and background in setting these things up would take the lead.

spacedmonkey · 2022-11-09T10:54:14Z

In trunk

In this branch.

spacedmonkey

PR works. It doesn't solve all the issues in wp_head, but it a great start

felixarntz · 2022-11-09T23:39:41Z

@oandregal Thank you for running additional performance benchmarks on the latest code here! This is super helpful.

I've also run the latest iteration of the PR through the same methodology that I originally used, and posted results in an updated PR description. Of course the benefit is much less than originally, now that I've broken out fixing the core theme.json parsing piece into WordPress/gutenberg#45616. Nevertheless, at a ~10% wp_head performance improvement I think this is a worthwhile change especially as they look straightforward from a code perspective - baby steps so to speak, but together they'll add up eventually, and doing the work here is particularly relevant to allow for the above Gutenberg issue to actually have the intended impact.

src/wp-includes/class-wp-theme-json-resolver.php

peterwilsoncc

LGTM with a nit pick inline

src/wp-includes/class-wp-theme-json-resolver.php

Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>

src/wp-includes/class-wp-theme-json-resolver.php

…lixarntz/wordpress-develop into fix/classic-themes-wp_head-regression

felixarntz · 2022-11-10T22:16:12Z

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

felixarntz added 2 commits October 27, 2022 13:21

Avoid parsing core's theme.json and theme-i18n.json when using a clas…

787b2c2

…sic theme without support.

Avoid DB query for global styles when using a classic theme without s…

aff6347

…upport.

felixarntz requested review from draganescu, audrasjb, dream-encode, hellofromtonya and desrosj October 27, 2022 20:29

eugene-manuilov reviewed Oct 27, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json-resolver.php Outdated Show resolved Hide resolved

spacedmonkey self-requested a review November 1, 2022 16:32

felixarntz added 3 commits November 2, 2022 14:13

Merge branch 'trunk' into fix/classic-themes-wp_head-regression

196c215

Further optimize WP_Theme_JSON_Resolver logic for classic themes, use…

0c768d3

… pre-built PHP-parsed version of WP core theme.json if present.

Add demo version of theme-json.php from WP core theme.json (via var_e…

b9ff774

…xport()).

felixarntz mentioned this pull request Nov 3, 2022

Code quality check for theme.json related APIs WordPress/gutenberg#45171

Open

spacedmonkey reviewed Nov 8, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json-resolver.php Outdated Show resolved Hide resolved

spacedmonkey reviewed Nov 8, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json.php Show resolved Hide resolved

felixarntz mentioned this pull request Nov 8, 2022

Create parsed PHP version of core's theme.json at build time rather than during runtime WordPress/gutenberg#45616

Open

felixarntz added 4 commits November 8, 2022 15:42

Merge branch 'trunk' into fix/classic-themes-wp_head-regression

f34fe53

Remove temporary theme.json in favor of WordPress/gutenberg#45616.

6a835a4

Include parsing parent theme.json logic in cached logic block.

1ab511c

Merge branch 'trunk' into fix/classic-themes-wp_head-regression

bf4e711

Add test coverage and fix relevant tests to require a block theme.

4966109

felixarntz marked this pull request as ready for review November 9, 2022 01:04

felixarntz requested review from spacedmonkey and eugene-manuilov November 9, 2022 02:54

oandregal reviewed Nov 9, 2022

View reviewed changes

peterwilsoncc reviewed Nov 9, 2022

View reviewed changes

oandregal reviewed Nov 9, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json-resolver.php Show resolved Hide resolved

oandregal reviewed Nov 9, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json-resolver.php Show resolved Hide resolved

oandregal reviewed Nov 9, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json-resolver.php Show resolved Hide resolved

Merge branch 'trunk' into fix/classic-themes-wp_head-regression

3a24f75

spacedmonkey approved these changes Nov 9, 2022

View reviewed changes

Address code review feedback.

34c1957

felixarntz requested review from peterwilsoncc and removed request for eugene-manuilov November 9, 2022 23:40

spacedmonkey reviewed Nov 9, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json-resolver.php Show resolved Hide resolved

peterwilsoncc reviewed Nov 9, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json-resolver.php Outdated Show resolved Hide resolved

Limit comment to 80 characters per line.

392391a

Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>

spacedmonkey reviewed Nov 10, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json-resolver.php Show resolved Hide resolved

felixarntz added 2 commits November 10, 2022 11:45

Merge branch 'trunk' into fix/classic-themes-wp_head-regression

782c49d

Merge branch 'fix/classic-themes-wp_head-regression' of github.com:fe…

3ab53bf

…lixarntz/wordpress-develop into fix/classic-themes-wp_head-regression

felixarntz closed this Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `wp_head` performance regression for classic themes #3536

Fix `wp_head` performance regression for classic themes #3536

felixarntz commented Oct 27, 2022 •

edited

Loading

spacedmonkey commented Nov 1, 2022

oandregal commented Nov 2, 2022 •

edited

Loading

oandregal commented Nov 2, 2022

felixarntz commented Nov 2, 2022

felixarntz commented Nov 2, 2022

spacedmonkey commented Nov 3, 2022

spacedmonkey commented Nov 8, 2022

felixarntz commented Nov 8, 2022

spacedmonkey commented Nov 8, 2022

felixarntz commented Nov 8, 2022

felixarntz commented Nov 9, 2022

oandregal Nov 9, 2022

peterwilsoncc left a comment

peterwilsoncc Nov 9, 2022

felixarntz Nov 9, 2022

oandregal Nov 9, 2022

spacedmonkey Nov 9, 2022

felixarntz Nov 9, 2022

oandregal commented Nov 9, 2022

oandregal commented Nov 9, 2022 •

edited

Loading

oandregal commented Nov 9, 2022 •

edited

Loading

spacedmonkey commented Nov 9, 2022

spacedmonkey left a comment

felixarntz commented Nov 9, 2022

peterwilsoncc left a comment

felixarntz commented Nov 10, 2022

		static::get_file_path_from_theme( 'theme.json' ) !== '' \|\|
		static::get_file_path_from_theme( 'theme.json', true ) !== ''

Fix wp_head performance regression for classic themes #3536

Fix wp_head performance regression for classic themes #3536

Conversation

felixarntz commented Oct 27, 2022 • edited Loading

spacedmonkey commented Nov 1, 2022

oandregal commented Nov 2, 2022 • edited Loading

oandregal commented Nov 2, 2022

felixarntz commented Nov 2, 2022

felixarntz commented Nov 2, 2022

spacedmonkey commented Nov 3, 2022

spacedmonkey commented Nov 8, 2022

felixarntz commented Nov 8, 2022

spacedmonkey commented Nov 8, 2022

felixarntz commented Nov 8, 2022

felixarntz commented Nov 9, 2022

oandregal Nov 9, 2022

Choose a reason for hiding this comment

peterwilsoncc left a comment

Choose a reason for hiding this comment

peterwilsoncc Nov 9, 2022

Choose a reason for hiding this comment

felixarntz Nov 9, 2022

Choose a reason for hiding this comment

oandregal Nov 9, 2022

Choose a reason for hiding this comment

spacedmonkey Nov 9, 2022

Choose a reason for hiding this comment

felixarntz Nov 9, 2022

Choose a reason for hiding this comment

oandregal commented Nov 9, 2022

oandregal commented Nov 9, 2022 • edited Loading

oandregal commented Nov 9, 2022 • edited Loading

spacedmonkey commented Nov 9, 2022

spacedmonkey left a comment

Choose a reason for hiding this comment

felixarntz commented Nov 9, 2022

peterwilsoncc left a comment

Choose a reason for hiding this comment

felixarntz commented Nov 10, 2022

Fix `wp_head` performance regression for classic themes #3536

Fix `wp_head` performance regression for classic themes #3536

felixarntz commented Oct 27, 2022 •

edited

Loading

oandregal commented Nov 2, 2022 •

edited

Loading

oandregal commented Nov 9, 2022 •

edited

Loading

oandregal commented Nov 9, 2022 •

edited

Loading