Performance regression in WP 6.1 for `theme.json` processing #44772

aristath · 2022-10-07T09:28:23Z

Description

I'm currently trying to improve performance in WP-Core, and the thing that currently stands out as a bottleneck is the WP_Theme_JSON::remove_keys_not_in_schema method.
Posting a screenshot of a webgrind analysis below:

WP_Theme_JSON::remove_keys_not_in_schema runs 38228 times, and consumes 14.36% of resources/server-time.

Step-by-step reproduction instructions

I'll just post instructions on how I setup webgrind on my localhost using wp-env, so hopefully others can also test and debug performance issues:

I use the https://github.com/WordPress/wordpress-develop repo, but I think the same would apply for this one. Edit the .env file. Set LOCAL_PHP_XDEBUG=true, and LOCAL_PHP_XDEBUG_MODE=develop,debug,profile.
Download or clone https://github.com/jokkedk/webgrind locally. I do it in wp-content/plugins/webgrind but all that matters is you put it somewhere you can access it from a URL.
start wp-env normally. Xdebug will now be available and profiling will run. Which also means that page-loads will be significantly slower due to all the profiling happening in the background.
Visit your site, login, then refresh the frontend a few times. Doing multiple runs will allow you to get some more stats and you can ensure that the stats you see later are not a fluke but consistent between all your page-loads.
Visit the URL where you copied webgrind, and examine the results. In my case that's on http://localhost:8889/wp-content/plugins/webgrind/ but it will vary depending on where you copied the files etc.

Screenshots, screen recording, code snippet

No response

Environment info

WordPress trunk
No plugins activated
Happens with and without the Gutenberg plugin active

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

The text was updated successfully, but these errors were encountered:

aristath · 2022-10-07T09:35:43Z

I'm trying to understand why remove_keys_not_in_schema is necessary... 🤔
I understand that it cleans up the parsed JSON, but is there any actual problem if the JSON has some extra things in it?
Do extra/unused items in the json cause any problems? Or do we just use that function because we want to keep things tightly controlled & limited?

cc @getdave, if I'm not mistaken that was added in the sanitize() method in #41786 ?

oandregal · 2022-10-07T11:04:28Z

We merged two recent PRs in wordpress-develop that made the caching only be available for reading the JSON file from the file system, and removed the other processing we did before.

Make Editor and Frontend reflect Block Styles in theme.json wordpress-develop#3408
WP_Theme_JSON: Invalidate blocks metadata cache when needed wordpress-develop#3359

The rationale for this is that the theme.json classes are now used before all the blocks are registered (aka get template data, etc.), hence, the caching wasn't working as expected. See #44434 and #44619

I'd like to have a better understanding of a baseline before/after these PRs to see what's the impact and whether it's something we need to act for 6.1 or can do later.

aristath · 2022-10-07T11:05:19Z

Update: It looks like this commit is the culprit: WordPress/wordpress-develop@a937892
Reverting that commit fixes the issue locally.

Props @ockham and @oandregal for pointing me to that direction 👍

ockham · 2022-10-07T11:13:27Z

Thank you for tracking this down, @aristath. I had left a comment on the originating PR with some suggestions for more caching: WordPress/wordpress-develop#3408 (comment). Maybe applying these could mitigate the issue?

Unfortunately, I'll only have limited availability myself until Tuesday. Pinging @oandregal @andrewserong @ajlende @talldan @glendaviesnz who were involved in the original research for assistance 😅

ockham · 2022-10-07T11:24:13Z

I had left a comment on the originating PR with some suggestions for more caching: WordPress/wordpress-develop#3408 (comment). Maybe applying these could mitigate the issue?

Ah, that might not cut the mustard 😕 (I had missed these two comments: #44772 (comment) and #44772 (comment).)

aristath · 2022-10-07T11:24:29Z

Posting 2 screenshots with the stats with and without that commit.

With the commit applied:

When reverting the commit:

The times shown there are all in milliseconds, and keep in mind that Xdebug + profiling slow down the site a lot. However, the differences are an indication of the commit's impact.
Reverting the commit reduces the time from 9473ms to 7528.
The WP_Theme_JSON::sanitize() method was called 633 times, and reverting the commit makes that number go down to 251. I believe that is the main issue... The sanitize method is what calls remove_keys_not_in_schema, so all those extra calls just make processing slower.

ockham · 2022-10-07T11:29:39Z

Thank you Ari!

@oandregal Can we cache sanitize based on the 2nd and 3rd argument (list of valid blocks and elements, respectively)? The keys of those arrays (block and element names) should be hopefully enough.

oandregal · 2022-10-07T19:43:45Z

I've started to implement and track some improvements in WordPress/wordpress-develop#3418

oandregal · 2022-10-07T23:00:07Z

The current state of WordPress/wordpress-develop#3418

spacedmonkey · 2022-10-08T00:20:32Z

I did some profiling with blackfire. These are the results.

Can't think of a good reason why the method remove_keys_not_in_schema.

I believe this line is the issue.

https://github.com/WordPress/wordpress-develop/blob/e89286a4a74b75f41cc89a1aa7372992146f8b7f/src/wp-includes/class-wp-theme-json.php#L813

remove_keys_not_in_schema calls itself recurrively and results in a loop somehow. Do we have unit tests for this code?

getdave · 2022-10-11T08:31:32Z

cc @getdave, if I'm not mistaken that was added in the sanitize() method in #41786 ?

Sorry for late reply I've been AFK. Based on my comment here it looks as though the method already existed as I was having to work around it by adding pseudo selectors to the schema so they didn't get stripped by this method.

A significant performance regression was added late in WP 6.1 beta cycle when some of the existing caching for `theme.json` processing was removed. The rationale for removing the caching was this code was now used before all the blocks are registered (aka get template data, etc.) and resulted in stale cache that created issues (see [WordPress/gutenberg#44434 Gutenberg Issue 44434] and [WordPress/gutenberg#44619 Gutenberg Issue 44619]). The changes were limited to only reads from the file system. However, it introduced a big impact in performance. This commit adds caching and checks for blocks with different origins. How? It add caching for the calculated data for core, theme, and user based on the blocks that are registered. If the blocks haven't changed since the last time they were calculated for the origin, the cached data is returned. Otherwise, the data is recalculated and cached. Essentially, this brings back the previous cache, but refreshing it when the blocks change. It partially adds unit tests for these changes. Additional tests will be added. References: * [WordPress/gutenberg#44772 Performance regression in WP 6.1 for theme.json processing] Follow-up to [54251], [54399]. Props aristath, oandregal, bernhard-reiter, spacedmonkey, hellofromTonya. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54493 602fd350-edb4-49c9-b593-d223f7449a82

A significant performance regression was added late in WP 6.1 beta cycle when some of the existing caching for `theme.json` processing was removed. The rationale for removing the caching was this code was now used before all the blocks are registered (aka get template data, etc.) and resulted in stale cache that created issues (see [WordPress/gutenberg#44434 Gutenberg Issue 44434] and [WordPress/gutenberg#44619 Gutenberg Issue 44619]). The changes were limited to only reads from the file system. However, it introduced a big impact in performance. This commit adds caching and checks for blocks with different origins. How? It add caching for the calculated data for core, theme, and user based on the blocks that are registered. If the blocks haven't changed since the last time they were calculated for the origin, the cached data is returned. Otherwise, the data is recalculated and cached. Essentially, this brings back the previous cache, but refreshing it when the blocks change. It partially adds unit tests for these changes. Additional tests will be added. References: * [WordPress/gutenberg#44772 Performance regression in WP 6.1 for theme.json processing] Follow-up to [54251], [54399]. Props aristath, oandregal, bernhard-reiter, spacedmonkey, hellofromTonya. See #56467. Built from https://develop.svn.wordpress.org/trunk@54493 git-svn-id: http://core.svn.wordpress.org/trunk@54052 1a063a9b-81f0-0310-95a4-ce76da25c4cd

A significant performance regression was added late in WP 6.1 beta cycle when some of the existing caching for `theme.json` processing was removed. The rationale for removing the caching was this code was now used before all the blocks are registered (aka get template data, etc.) and resulted in stale cache that created issues (see [WordPress/gutenberg#44434 Gutenberg Issue 44434] and [WordPress/gutenberg#44619 Gutenberg Issue 44619]). The changes were limited to only reads from the file system. However, it introduced a big impact in performance. This commit adds caching and checks for blocks with different origins. How? It add caching for the calculated data for core, theme, and user based on the blocks that are registered. If the blocks haven't changed since the last time they were calculated for the origin, the cached data is returned. Otherwise, the data is recalculated and cached. Essentially, this brings back the previous cache, but refreshing it when the blocks change. It partially adds unit tests for these changes. Additional tests will be added. References: * [WordPress/gutenberg#44772 Performance regression in WP 6.1 for theme.json processing] Follow-up to [54251], [54399]. Props aristath, oandregal, bernhard-reiter, spacedmonkey, hellofromTonya. See #56467. Built from https://develop.svn.wordpress.org/trunk@54493 git-svn-id: https://core.svn.wordpress.org/trunk@54052 1a063a9b-81f0-0310-95a4-ce76da25c4cd

oandregal · 2022-10-11T17:49:41Z

WordPress/wordpress-develop#3418 has landed bringing numbers back to 6.0 baseline.

I'm still investigating this to squeeze more ms and have prepared WordPress/wordpress-develop#3441 as a modest improvement from 6.0 numbers.

aristath · 2022-10-14T08:49:59Z

Since the issue was addressed in Core, I'll go ahead and close this one.
Thank you everyone who worked on this! ❤️

A significant performance regression was added late in WP 6.1 beta cycle when some of the existing caching for `theme.json` processing was removed. The rationale for removing the caching was this code was now used before all the blocks are registered (aka get template data, etc.) and resulted in stale cache that created issues (see [WordPress/gutenberg#44434 Gutenberg Issue 44434] and [WordPress/gutenberg#44619 Gutenberg Issue 44619]). The changes were limited to only reads from the file system. However, it introduced a big impact in performance. This commit adds caching and checks for blocks with different origins. How? It add caching for the calculated data for core, theme, and user based on the blocks that are registered. If the blocks haven't changed since the last time they were calculated for the origin, the cached data is returned. Otherwise, the data is recalculated and cached. Essentially, this brings back the previous cache, but refreshing it when the blocks change. It partially adds unit tests for these changes. Additional tests will be added. References: * [WordPress/gutenberg#44772 Performance regression in WP 6.1 for theme.json processing] Follow-up to [54251], [54399]. Props aristath, oandregal, bernhard-reiter, spacedmonkey, hellofromTonya. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54493 602fd350-edb4-49c9-b593-d223f7449a82

aristath added [Type] Bug An existing feature does not function as intended [Type] Performance Related to performance efforts labels Oct 7, 2022

ockham mentioned this issue Oct 7, 2022

Make Editor and Frontend reflect Block Styles in theme.json WordPress/wordpress-develop#3408

Closed

oandregal mentioned this issue Oct 7, 2022

Improve caching for theme.json processing WordPress/wordpress-develop#3418

Closed

oandregal changed the title ~~Serious PHP performance issue in WP 6.1 with WP_Theme_JSON::remove_keys_not_in_schema~~ Performance regression in WP 6.1 for theme.json processing Oct 7, 2022

oandregal added the [Type] Regression Related to a regression in the latest release label Oct 7, 2022

oandregal mentioned this issue Oct 11, 2022

Improve performance of WP_Theme_JSON::get_merged_data method WordPress/wordpress-develop#3441

Closed

aristath closed this as completed Oct 14, 2022

oandregal mentioned this issue Oct 28, 2022

Code quality check for theme.json related APIs #45171

Open

oandregal mentioned this issue Nov 24, 2022

Add WP_Object_Cache to the gutenberg_get_global_settings method #45372

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance regression in WP 6.1 for `theme.json` processing #44772

Performance regression in WP 6.1 for `theme.json` processing #44772

aristath commented Oct 7, 2022 •

edited

Loading

aristath commented Oct 7, 2022

oandregal commented Oct 7, 2022 •

edited

Loading

aristath commented Oct 7, 2022 •

edited

Loading

ockham commented Oct 7, 2022

ockham commented Oct 7, 2022

aristath commented Oct 7, 2022

ockham commented Oct 7, 2022

oandregal commented Oct 7, 2022

oandregal commented Oct 7, 2022

spacedmonkey commented Oct 8, 2022

getdave commented Oct 11, 2022

oandregal commented Oct 11, 2022

aristath commented Oct 14, 2022

Performance regression in WP 6.1 for theme.json processing #44772

Performance regression in WP 6.1 for theme.json processing #44772

Comments

aristath commented Oct 7, 2022 • edited Loading

Description

Step-by-step reproduction instructions

Screenshots, screen recording, code snippet

Environment info

Please confirm that you have searched existing issues in the repo.

Please confirm that you have tested with all plugins deactivated except Gutenberg.

aristath commented Oct 7, 2022

oandregal commented Oct 7, 2022 • edited Loading

aristath commented Oct 7, 2022 • edited Loading

ockham commented Oct 7, 2022

ockham commented Oct 7, 2022

aristath commented Oct 7, 2022

ockham commented Oct 7, 2022

oandregal commented Oct 7, 2022

oandregal commented Oct 7, 2022

spacedmonkey commented Oct 8, 2022

getdave commented Oct 11, 2022

oandregal commented Oct 11, 2022

aristath commented Oct 14, 2022

Performance regression in WP 6.1 for `theme.json` processing #44772

Performance regression in WP 6.1 for `theme.json` processing #44772

aristath commented Oct 7, 2022 •

edited

Loading

oandregal commented Oct 7, 2022 •

edited

Loading

aristath commented Oct 7, 2022 •

edited

Loading