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

Add WP_Object_Cache to the gutenberg_get_global_settings method #45372

Merged
merged 1 commit into from
Nov 30, 2022

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Oct 28, 2022

Part of #45171
Depends on:

What?

This PR uses WP_Object_Cache to cache the results of querying settings from the theme.json.

Why?

This PR reduces the total time of a request from 521.59ms to 339.62ms, a ~35% performance increase.

How?

Adds a cache to the high-level public method that consumers use to query the settings coming from theme.json APIs (WordPress, theme, and user data).

How performance was measured

To measure performance, I have loaded the "Hello World!" post as a logged-out user under the following scenarios:

  • Gutenberg: this PR vs trunk at ee90c2b1df7cb59e1f6ea3e2aaeb3b15386396ac
  • WordPress: 6.2-alpha-54852
  • Theme: TwentyTwentyThree

For each Gutenberg branch, I have conducted the test 9 times and then calculated the median to get the time for the request. Download the data for:

How to reproduce the performance measures

You don't need to, as I've made available the cachegrind data (see section above). Though you may still want to run this yourself:

  1. Configure WP_DEBUG and SCRIPT_DEBUG to false by creating a .wp-env.override.json file with the following contents:
{
  "config": {
    "WP_DEBUG": false,
    "SCRIPT_DEBUG": false
  }
}
  1. Start the wp-env with xdebug enabled by running npm run wp-env start -- --xdebug=develop,debug,profile
  2. Load the "Hello World!" post by visiting http://localhost:8888/?p=1
  3. Take the cachegrind data generated by xdebug out of docker:
# List and find the latest timestamp of cachegrind.out.* files
docker exec -it `docker ps -f name=54_wordpress_1 -q` ls -lat --time-style=+%H:%M:%S /tmp/
# Copy from docker to manipulate with any tool in your computer
docker cp `docker ps -f name=54_wordpress_1 -q`:'/tmp/cachegrind.out.<pid-with-latest-timestamp>' ~/Desktop/
  • Inspect the time it takes with any tool of your choice. I use kcachegrind, but there are alternatives such as qcachegrind, webgrind, etc. kcachegrind shows times aggregated by 10ns, so the total included time of the request in the following screenshot would be 350 442 020ns, which amounts to 350ms.

Captura de ecrã de 2022-10-28 13-13-26

How to test

  • Make sure automated test pass.
  • Test block theme (TwentyTwentyThree)
    • Create new colors as user from the GS sidebar and verify they are shown in the color components in the block sidebar (site editor, post editor).
    • Modify theme.json settings of the theme
  • Test classic theme (TwentyTwenty)
    • Verify colors defined by the theme via add_theme_support( 'editor-color-palette', $palette ) work as expected.
    • Disable custom colors by adding add_theme_support( 'disable-custom-colors' ); to the functions.php of the theme. Verify that users cannot add custom colors.

@codesandbox
Copy link

codesandbox bot commented Oct 28, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@oandregal oandregal changed the title Add cache the gutenberg_get_global_settings method Add cache to the gutenberg_get_global_settings method Oct 28, 2022
@oandregal oandregal self-assigned this Oct 28, 2022
@oandregal oandregal added [Status] In Progress Tracking issues with work in progress [Type] Performance Related to performance efforts labels Oct 28, 2022
@oandregal
Copy link
Member Author

@ramonjd @andrewserong @scruffian @tellthemachines @noisysocks I'm working on this PR and the only test failures are related to WP_Block_Supports_Typography_Test, introduced #44764 and #44762.

I need to look at them deeply, but thought you may be able to offer some quick thoughts.

@ramonjd
Copy link
Member

ramonjd commented Oct 28, 2022

@ramonjd @andrewserong @scruffian @tellthemachines @noisysocks I'm working on this PR and the only test failures are related to WP_Block_Supports_Typography_Test, introduced #44764 and #44762.

I can look more closely next week, but does this PR changes the way we resolve the theme.json for the test theme we're using in the typography tests?

https://github.com/WordPress/gutenberg/blob/trunk/phpunit/data/themedir1/block-theme-child-with-fluid-typography/theme.json

The tests are failing because "fluid": true isn't being read (or doesn't exist)

		"typography": {
			"fluid": true
		}

@oandregal
Copy link
Member Author

Some initial notes to invalidate the cache for settings & styles:

  • Core data changes: when WordPress is updated?
  • Block data changes & block registered:
    • Different blocks may be registered across requests or across different moments in time in the same request
    • What about 3rd parties filtering the block settings?
  • Theme data changes: theme switch, theme preview, theme updated
  • User data changes: wp_global_styles is saved, data is imported
  • Listeners for existing filters change (theme.json data or block registration)
    • Plugins installed/uninstalled/updated.
    • Plugins updated: can modify the listeners
    • Drop-in files: how to detect this if we don't keep a list of listeners?

@oandregal
Copy link
Member Author

I've extracted two small PRs that we can fast-track and land separately:

The goal of this PR should be simply adding object cache to this method.

@oandregal oandregal force-pushed the add/cache-to-wp-get-global-settings branch from b11761c to 178ed79 Compare November 22, 2022 14:46
@oandregal oandregal marked this pull request as ready for review November 22, 2022 14:46
@oandregal
Copy link
Member Author

This is confusing to me. It seem like gutenberg_get_global_stylesheet and gutenberg_get_global_settings are caching the same data.

The piece that needs to be cached is WP_Theme_JSON_Resolver_Gutenberg::get_merged_data();. That is the expensive part that needs caching. Caching twice is wasteful is likely that the cache will get out of date or not correctly be invalidated.

Can be look at adding the caching to WP_Theme_JSON_Resolver_Gutenberg?

We talked about this at length in a recent private conversation, as well as in a few other PRs 🙂

I'm happy to discuss the direction, though at some point we need to move forward with something. If we can't agree on direction, we need to disagree and commit. Being caught in an eternal loop is not fun for anyone.

We already have caching in WP_Theme_JSON_Resolver and it does not work well anymore, given all the datum stored in theme.json have different invalidation characteristics. This was one of the major issues I saw in the WordPress 6.1 and the reason I created this track issue to improve the situation in a strategic way (big wins first, smaller wins later).

@spacedmonkey
Copy link
Member

@oandregal Can you explain why we are caching the same data twice? What is the benefit of this? The reason it keeps coming up, I don’t feel like you have explained yourself in enough detail that I feel like I can drop this. Caching the same data twice, is strange and makes cache invalidation harder, you have two caches to worry about. Not to mention, more memory used in both object cache and in persistent object caches.

As for the fact we discussed this privately, that is part of the reason I am mentioning it again, so we can discuss, with other involved the reasoning behind this code. This conversion is public and future developers will benefit it being documented.

@oandregal
Copy link
Member Author

@oandregal Can you explain why we are caching the same data twice? What is the benefit of this? The reason it keeps coming up, I don’t feel like you have explained yourself in enough detail that I feel like I can drop this. Caching the same data twice, is strange and makes cache invalidation harder, you have two caches to worry about. Not to mention, more memory used in both object cache and in persistent object caches.

As for the fact we discussed this privately, that is part of the reason I am mentioning it again, so we can discuss, with other involved the reasoning behind this code. This conversion is public and future developers will benefit it being documented.

Absolutely. I've been doing for a month now across many issues, so I've got some practice 🙂

We are caching different data: one is a CSS stylesheet, the other is an array of data. While they access the same source of data (theme.json) they do wildly different things with it. We save time if we don't do that calculation more times than necessary.

@oandregal oandregal force-pushed the add/cache-to-wp-get-global-settings branch 2 times, most recently from e00e39f to a8b2676 Compare November 25, 2022 10:25
@spacedmonkey
Copy link
Member

Absolutely. I've been doing for a month now across many issues, so I've got some practice 🙂

I am just catching up on all those issues. There is no one place I feel like you have explained this issue in detail. So I am still trying to understand your thinking.

To zoom out a little, the performance issue is in WP_Theme_JSON_Resolver::get_merged_data. Merge of the data from different sources ( even through they are cached ) results in lots of wasted compute to merge arrays together. See this screenshot from blackfire. 26% of the page load is just this one method alone. This calls theme_json, 500+ times.

Screenshot 2022-11-25 at 10 31 51

Lots of work was done by @aristath to make theme json performance faster. See WordPress/wordpress-develop#3555.

So the core issue of performance is where we should focus our work. This is adding a cache, upstream from the core issue. Like putting a band aid over the issue, instead of treating the larger issue.

The point of the cache, is stop the expensive compute in WP_Theme_JSON_Resolver::get_merged_data. Instead of caching there, we are adding two caches, which will fill up the cache. The fact that both caches are invalidated at the same time, proves that they need to be one cache.

Let's focus on the core issue and not to cover it up with caches.

@oandregal
Copy link
Member Author

I am just catching up on all those issues. There is no one place I feel like you have explained this issue in detail. So I am still trying to understand your thinking.

Hey, I shared as much as possible in all the places people asked me to (issues, PRs, slack, DMs, video, etc.), including in long-form at #45171

If the fact that caching a generated CSS stylesheet and caching an array are two separate things is not obvious, I don't know what else to say to be honest 😞 I may need some time away or hear other voices to learn how to rephrase it and make it more obvious. We all may need to ruminate about this for a few days, so I'm going to refrain to comment on what I've already shared about direction for a bit.

Lots of work was done by @aristath to make theme json performance faster. See WordPress/wordpress-develop#3555.

I have also humbly contributed a little bit (in addition to all recent Gutenberg PRs) 🙂

All those changes are great and I welcome them. They have not been enough so far. I wish they were! None has brought a 35% performance improvement like this PR does.

@spacedmonkey
Copy link
Member

To be clear, I understand what is being cached here. That is never been in question. The question is why, why are we caching? The answer, is the WP_Theme_Json_Resolver class, has performance issues. That is what we need to fix here. Once those performance issues are fixed, then we can call WP_Theme_Json_Resolver without the need for caching. It seems likely that in the future, there will be more functions that will be a consumers of data from WP_Theme_Json_Resolver class. An example of which is wp_get_global_styles_svg_filters. This too will need yet another cache, that will need to be invalidated. And on and on.

I want to fix the root cause of this issue. I think this is going to be really hard to go, within the gutenberg plugin, as it relays deep integrations into the WordPress bootstrap and theming process.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

@oandregal, thank you for all this work that is bringing an incredible improvement of 35% on the time the engine takes to output a page.
When we see improvements of this magnitude, we should do what we can in order to ship them as fast as possible to our users.
It seems an improvement discussed was the invalidation on-site options change which I think makes total sense.
Another aspect was that when we change a file, if there is object cache persistence on the last reload, the file change is still not applied. I guess we could add theme-json to the non-persistent cache groups like the theme is.
Or we could later explore including the last modified timestamp of the theme-json file as part of the cache key.

Provided we include these two enhancements I don't see any other clear blocker.

@jorgefilipecosta
Copy link
Member

I created a complementary PR that should remove a lot of calls to get_merged_data #46074.

@aristath
Copy link
Member

The question is why, why are we caching? The answer, is the WP_Theme_Json_Resolver class, has performance issues. That is what we need to fix here.

Ideally, we'd just rewrite WP_Theme_Json_Resolver from the ground up. When that object was first introduced, I think none of us could fathom how wide its scope would be. However, fixing WP_Theme_Json_Resolver is not easy. Parts of it will probably need to be refactored, and there are a lot of backward-compatibility issues we'll need to address when refactoring that class. It's something that can (and probably should) take a long time to be properly implemented/refactored because we may need to fundamentally change some of the architecture.

I want to fix the root cause of this issue. I think this is going to be really hard to go, within the gutenberg plugin, as it relays deep integrations into the WordPress bootstrap and theming process.

I agree 100%!! This task will take months and can't be implemented before the WP6.2 release - if we even make it for that release and it doesn't go in 6.3. In the meantime, we can use this solution to dramatically improve performance until we come up with a better implementation.

This PR presents a dramatic improvement, it should not be ignored or dismissed.
My suggestion would be this:
We can merge this PR as it's a huge improvement for site visitors, and then work on addressing the root cause of the issue. Merging this cache does not preclude refactoring the WP_Theme_Json_Resolver class - if anything, it gives us more time to make the right decisions for the future of WP_Theme_Json_Resolver.

Another aspect was that when we change a file, if there is object cache persistence on the last reload, the file change is still not applied. I guess we could add theme-json to the non-persistent cache groups like the theme is.

I think that the WP_DEBUG condition would probably be enough... Any developer working on their site should always have WP_DEBUG enabled; it's common practice and common knowledge.

If in the future, we see that this cache is no longer needed, we can remove it. But in my opinion, we could/should merge this with a couple of tweaks.

  • I wouldn't want to introduce new public functions like gutenberg_get_global_settings_clean_cache. Perhaps we could prefix it with an underscore? That would make any future refactors of the WP_Theme_Json_Resolver class easier, and if at some point we find that the cache is no longer needed, removing it won't be an issue for backward compatibility.
  • We could consider cleaning the cache upon any option is added/updated/deleted. That would cover any dynamic behavior that depends on them. The performance will still be great given than reading a site is many times more frequent than changing an option.

Other than those 2 small tweaks, this PR looks good to me, and I'd support merging it in a heartbeat.

Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

This is a great improvement and should find it's way into core, ASAP.

Before merging, this here, should need to group the cache group the data is saved into non persistent. Making it non persistent is going to solve a lot of cache invalidation issues, as data is not saved per request. It means we can make quickly follow up with another solution for how we handle dynamically removing / add theme supports. This can be done in another PR, but is a blocker to a core merge, it will make core unit tests fail.

As for adding wrappers for all these cache invalidation, this can also be done in a single PR or on core merge.

Are their trac tickets are all these changes? We need to start working on how we are going to merge this changes into core.

@spacedmonkey
Copy link
Member

I would also like to see #46112 merged first if possible. This shaves around 11-15% of page load. I wonder how much of the performance wins of this change are just covering up this bug.

@oandregal
Copy link
Member Author

Made the theme_json group non-persistent at #46150 as there are other methods that already use it. It can be merged faster and separately. Note that this makes unnecessary some existing cache invalidation hooks. I'll update this PR to take those changes into account.

@oandregal oandregal force-pushed the add/cache-to-wp-get-global-settings branch from d8996f6 to 783e57b Compare November 29, 2022 12:47
@oandregal
Copy link
Member Author

oandregal commented Nov 29, 2022

#46150 is a dependency, so I've merged here as the first commit for easy testing. See f277821

The other commit implements object cache for the settings method 783e57b

@oandregal oandregal force-pushed the add/cache-to-wp-get-global-settings branch from 783e57b to d115157 Compare November 30, 2022 08:17
@oandregal
Copy link
Member Author

oandregal commented Nov 30, 2022

All dependencies have landed and all the feedback has been addressed. This is ready for a final ✔️

(It'd be lovely if it lands this morning to make it part of 14.7: I've potentially marked it for that milestone).

@oandregal oandregal added this to the Gutenberg 14.7 milestone Nov 30, 2022
@oandregal
Copy link
Member Author

I've done a final testing, including (plus some other random actions):

  • Test theme with theme.json (TwentyTwentyThree):
    • Create new colors as a user from the GS sidebar and verify they are shown in the color components in the block sidebar (site editor, post editor).
    • Modify theme.json settings of the theme and disable text and link colors.
  • Test theme without theme.json (TwentyTwenty):
    • Verify colors defined by the theme via add_theme_support( 'editor-color-palette', $palette ) work as expected.
    • Disable custom colors by adding add_theme_support( 'disable-custom-colors' ); to the functions.php of the theme. Verify that users cannot add custom colors.

Everything works as expected.

All feedback in this blocking review has been addressed. Jonny may have limited availability to clear it, so, in the interest of making this PR part of 14.7 RC, I'm going to merge. If there are any bugs I didn't find, we still have a week to fix them (or revert this PR, should it be necessary).

Thanks everyone for the feedback, let's ship this to users.

@oandregal oandregal merged commit 8ef9597 into trunk Nov 30, 2022
@oandregal oandregal deleted the add/cache-to-wp-get-global-settings branch November 30, 2022 10:08
mpkelly pushed a commit to mpkelly/gutenberg that referenced this pull request Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants