Make theme.json related caches persistent

[flixos90]

The theme_json cache group is currently non-persistent, which means that, even when a site uses a persistent object cache, the data in this cache group will only be cached within a single request. In other words, there is only a benefit to this caching when a function is called several times, at least more than once, in a single request.

Needless to say, we have noticed in the past that parsing theme.json and running related logic can be expensive, and the preferable outcome would be to make those caches persistent so that they also can be cached beyond requests.

The biggest challenge to this is to implement proper cache invalidation. Originally, within Gutenberg, the cache group was persistent, but it led to problems due to:

• cache invalidation hard to figure out (what are all the events can lead to the cached values becoming stale?)
• caching logic, rather than database values, is not yet a common practice in WordPress, and one of the quirks is that e.g. action/filter hooks may no longer be run when the logic that contains them is cached, which can lead to a clunky developer experience

See related comment https://core.trac.wordpress.org/ticket/57648#comment:17

[flixos90]

This tracking ticket will likely involve several issues and PRs in the Gutenberg repository to identify the best approach for the various usages of caching related to theme.json and the theme_json cache group.

I'm hopeful we can at least make some notable progress here for WordPress 6.3, so I'll milestone it for that release for now, acknowledging though that most of the work will need to happen in the Gutenberg repository, so it'll probably be a while until we get a PR here.

cc @oandregal @spacedmonkey

[iCaleb]

One approach could be to mainly just utilize the persistent cache only during frontend requests (where it'd get the most performance benefit).

Each logged-in non-ajax admin requests could, at least once, always fetch the theme.json. If there are changes from what is in the persistent cache then it can update. As a backup (in case a theme is updated but nobody logs in), could also allow the same invalidation-checks during cron as well.

Worst case scenario: no cron is running and no admin-side log-ins are occurring. In which case it'd just have to rely on the TTL expiring and a frontend request would have to do the work to populate the cache again.

This gives the benefits of persistent cache, while solving for invalidation in a pretty reasonable way.

[oandregal]

It's worth noting that these caches were implemented as persistent in Gutenberg, and they were removed to unblock this work from landing into core. See ​https://github.com/WordPress/gutenberg/pull/46150 for details, and links to the conversations. The PR can also serve to bring everything back quickly.

The cache invalidation covered all the following scenarios:

• upgrader_process_complete: for WordPress, plugin, and theme updates.
• switch_theme: the active theme is changed.
• start_previewing_theme: previews for customizer and theme directory.
• save_post_wp_global_styles: user updates theme.json data via the site editor.
• activated_plugin and deactivated_plugin: any plugin can hook into the existing theme.json filters, so when they are activated/deactivated the filters may change.

The disagreement was specific to the following two points:

• Consumers that hook into theme.json data using dynamic mechanisms (deciding whether to hook or not based on an option or user meta value). For these cases, the alternatives would be to add more cache invalidation points (option add/update/delete, etc.), or ask the consumers to clear the cache themselves.
• Updates that are not done via the regular WordPress mechanism (FTP updates, updates managed via git repositories, etc.). The alternative is asking consumers to clear the cache themselves.

Note these two cases only if they use a persistent object cache plugin, as core, by default, doesn't persist.

[spacedmonkey]

WordPress core already has the cache group of theme. I wonder if we could use them. This is a special cache, that is an opt-in to persistent. ​See.

This means, for site owner that have control, their can make this group persistent, as they can cache clear when the theme is updated. Otherwise, it is a non persistent groups. This could be a interesting model to follow, as it would allow for production sites, that hosting companies know are not going to change, to persistent otherwise just be an in memory cache.

[flixos90]

@iCaleb Your idea certainly sounds interesting, however there is still the concern about how that affects filter callback logic that runs due to the filters from the logic that generates the data which is then cached. Whenever the cached data is returned, those filters aren't run. Which maybe is fine for some, but is not an expected situation, and with the scale of WordPress I'm sure will lead to breakage.

So the problems here are partially invalidation, partially filters in the logic that is no longer executed when cached (see @oandregal's comment for more details).

@spacedmonkey Regarding the existing theme cache group, I think we should avoid intertwining with that. There are different implications for that cache group, and opting in to making it persistent can be a good idea. Whereas if you now opted in to making the theme-json cache group persistent, you would effectively opt in into broken behavior, which IMO is not something we should offer or encourage.

[spacedmonkey]

This cache could also be global cache group, like the theme group.

[oglekler]

This is an enhancement, and we are in 12 days until Beta 1 after which we will not add new enhancement to the release, so, because there is no patch, I am moving this ticket to 6.4.

[hellofromTonya]

As this ticket will likely involve work in Gutenberg, adding the gutenberg-merge keyword for tracking.

This is a performance optimization of the WP_Theme_JSON_Resolver, which stores the merged WP_Theme_JSON object to a static property to avoid unnecessarily recreating this object by merging from other sources. In my testing, this cuts the execution time of this method in half.

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

[joemcgill]

After spending some time getting a better understanding of this system, and reading through the history of this issue, It seems to me like we should be focusing on a strategy for persistently caching data from WP_Theme_JSON_Resolver.

Currently, this class is responsible for loading theme.json data from four different sources (core, blocks, theme, and editor data) and then merging them together (depending on context).

All of the individual sources only get calculated once and stored to a static property of that class, which helps, but the process of creating a merged WP_Theme_JSON object is still pretty expensive.

I'd like to explore three options:

1. Store the merged data to a static property, similar to the individual sources, to eliminate the cost of merging them.
2. Make the entire merged result cacheable in a persistent way
3. Make the data from each source cacheable in a persistent way, if the entire result cannot be cached persistently

Option 1, seems like a very simple optimization with very little downside. I've opened ​a PR as a proof of concept.

Thanks for looking into this with fresh eyes.

I'd like to provide a use case of how this breaks (but works in trunk), so we can discuss ways forward.

• Apply this patch to your environment:

src/wp-content/themes/twentytwentythree/theme.json

@@ -323 +323 @@
                                }
                        },
                        "core/post-content": {
+
+
+
                                "elements": {
                                        "link": {
                                                "color": {

src/wp-includes/blocks/archives.php

@@ -116 +116 @@
        );
 }
 add_action( 'init', 'register_block_core_archives' );
+
+
+
+
+
+
+
+

• Load the hello world post. The expected result is that the post content uses hotpink as background color, as it does in trunk:

The way I see it, we can make this change for 6.4 as long as we invalidate the get_merged_cached when registered blocks are different, to avoid future issues, like we had in the past.

A longer brain dump:

theme.json data has different lifecycles and sanitization requirements:

• templateParts and customTemplates – they are accessed before block registration (before init), and their sanitization doesn't require blocks.
• settings and styles – they are accessed after block registration, and they require blocks to be sanitized.

Not knowing this has been a source of bugs and performance issues.

WordPress 6.4 is introducing two public methods for consumers to access ​templateParts and ​customTemplates, so it's unlikely that this happens again. However! I still think get_merged_data should not require consumers to know _when_ in the WordPress lifecycle it is safe to call it to avoid cache issues.

Oh! By the way, @joemcgill I'd rather make this change in Gutenberg first, and only port it to core after it has been tested there. This gives us confidence it works and makes synchronizing the two codebases easier for everyone involved.

Thanks for taking a look and giving some feedback, @oandregal. The use case that you explained in your write up is also confirmed by failing unit tests. I can see that there are several different places where theme.json data is recalculated based on the return value of the has_same_registered_blocks() method for the origin being calculated, but trying to use a similar approach for merged data doesn't seem to work (see d1319a1).

Lifecycle issues was one of my concerns with this approach, and this confirms that is indeed a problem. That also means that any function that relies on the WP_Theme_JSON_Resolver::get_merged_data is already unpredictable since its return value can change depending on when in the application lifecycle it's called, so any higher level caching at the utility function level is also likely to cause bugs like this.

I'm not sure if the unit tests failures are due to missing a place where the cache should be invalidated during the test setup, or if there is a more predictable way to know when all of the different sources of data are finalized, at which point it's safe to cache the merged object. Definitely more to dig into.

I'm happy to try to apply an iteration of this change to the Gutenberg version of this class first once we've got a working approach.

I see the failing test is test_should_conditionally_include_font_sizes and it tests 1) switching to a classic theme and 2) updating the "theme supports" it declares. The get_theme_data method does not cache the full result of injecting the "theme supports": if they need to be used, the ​final theme data is recalculated. I don't have much time to help debug this week, I could next week. Hoping this helps anyway?

@joemcgill I like where this is going, however as @oandregal mentioned above, in order to fix the outstanding issues, we would need to somehow cache the "theme data" results to also include their data with_supports.

This was previously attempted in #3624, which however was eventually closed. Looking back at the discussion there though, while there is some complexity to figure out, I don't think there was a clear reason it was closed, I believe we just didn't prioritize it at the time. I do think the idea from that PR is worth picking up, particularly now as it would potentially unlock this greater performance enhancement here.

Maybe we can find a better solution to avoid the problem of having to clean the theme.json caches lots of times (see ​https://github.com/WordPress/wordpress-develop/pull/3624/files#diff-4112a9a0bb18713bc2fc7868ceffe4403d765722f605e42b313ef2cdc218177fR354-R356)? Potentially we can come up with something along the lines of WP_Theme_JSON_Resolver::has_same_registered_blocks(), like a method that checks whether the theme support data has changed from when the data was last calculated.

[joemcgill]

Reassigning to myself to move this forward for now.

@oandregal I've taken another pass at this PR, and it does seem like the main thing causing the types of bugs (and unit test failures) we discussed previously was the need to recalculate the merged data if theme supports data changes. I've modified the approach so that the theme supports data gets saved to a static variable so we can test whether the value has changed, much like we test whether registered blocks have changed.

This results in a big improvement in terms of reducing the number of unnecessary WP_Theme_JSON objects that get created and merged during a page lifecycle, but unfortunately doesn't seem to result in a very large performance benefit from what I can tell, but I want to do some more profiling.

In the two XHProf reports below, you can see the reduction in calls to all the getter methods for various theme.json data origins, as well as a reduction in calls to WP_Theme_JSON::merge, all of which have a positive impact on memory and cpu usage, but only a slight reduction in the overall server time.

Trunk

PR

Do you think something like this is worth pursuing?

While a complexity that has made it complicated to progress https://core.trac.wordpress.org/ticket/57789 has been the additional filters firing around theme.json parsing, this PR explores an approach that is not subject to that complexity: By persistently caching _only_ the raw results of the theme.json files (i.e. none of the following parsing), it already saves a lot of processing time on every page load (for sites that use a persistent object cache).

The impact of this is already quite notable, with wp-total being 8.9% faster for TT1 (classic theme) and 3.2% faster for TT3 (block theme). See also ​https://github.com/WordPress/gutenberg/issues/45616 which is another related idea that focused on the cost of _just_ the JSON decoding of the files while just by itself has a notable negative performance impact.

This PR is currently for testing & benchmarking purposes only. It should preferably be implemented in Gutenberg before being merged into core. If there is consensus on the approach though, I think this would be worth prioritizing still for WordPress 6.4 given the notable performance impact for sites using a persistent object cache.

Keep in mind that this only has an impact when using a persistent object cache. If we wanted to bring these benefits to all WordPress sites, we could consider using transients instead.

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

[flixos90]

I just opened the above PR ​https://github.com/WordPress/wordpress-develop/pull/5267 as another idea I had the other day: While we can't easily cache theme.json data persistently because filters don't fire, we can easily cache the most low-level part of it which is the JSON-decoding of the actual files.

Caching just that part itself already has a notable positive performance impact for sites that use a persistent object cache. Please see the PR description for more context.

If there is consensus on that idea, I'm happy to port this over to a Gutenberg PR. Wanted to implement it in core for simplicity first so I can benchmark it in core itself.

[joemcgill]

Thanks @flixos90. I agree that we should consider both strategies together here:

1. Caching the merged data non-persistently to eliminate the need to recreate the merged dataset multiple times per page load (​PR #5024)
2. Persistently caching each theme data origin (core, blocks, theme, user) based on the different invalidation rules needed by each (​PR 5267)

Could you take a look at the initial PR and provide feedback from your testing? I'll do the same with yours.

@joemcgill In my benchmarking (similar approach that I used for #5267), this PR shows a small but consistent improvement. After rerunning a few times, it is always slightly faster with this PR, so I feel good about making this change.

For reference:

• wp-total with TT3: 86.24ms with the PR, 86.6ms without (0.4% faster)
• wp-total with TT1: 52.1ms with the PR, 52.24ms without (0.3% faster)

It also makes sense to me that the benefit is slightly higher for the block theme given theme.json is more heavily relied upon there, and I assume this in the past resulted in more executions of the costly get_merged_data() logic than it did for the classic theme.

Thanks @felixarntz. I'm going to go ahead and apply these changes to a Gutenberg PR to get feedback from that team and to hopefully get this tested in the plugin prior to a core change.

@felixarntz I've applied your PR feedback and also opened a PR with these changes in the Gutenberg repo, here: ​https://github.com/WordPress/gutenberg/pull/54742

Thanks @joemcgill for the feedback.

In my testing for #5024, I've noticed that the most expensive operations during Theme JSON processing is the core data and the user data, whereas the data for blocks and theme have less of an impact.

That's certainly possible. I only tested overall performance, but didn't profile. Could you share the data for how much the 4 different processes took? Since the profile below is only for get_theme_data()? Another question, did you test with a classic or a block theme? Asking since I'd expect the changes to get_theme_data() to be impactful for block themes only.

I ran some profiles with this PR and I'm not seeing the same degree of impact that you've reported. It also seems like most of the time in the get_theme_data method is spent constructing the WP_Theme_JSON_Data objects, which is unaffected by your changes.

I'm not sure about that. When you say "unaffected", do you mean that in terms of little performance impact? Because the changes here should certainly affect get_theme_data() as it reads a theme.json file.

We could cache the WP_Theme_JSON_Data object before it's passed to the wp_theme_json_data_theme filter, since that should not change unless the theme is updated, which could help, or further improve the performance of that class constructor.

That could work. Have you verified that there are no extension points (e.g. filters) firing within the WP_Theme_JSON_Data constructor logic (which also instantiates a WP_Theme_JSON)? I only wanted to touch the reading JSON file parts because in that one I could be sure there's no extension points.

Could you share the data for how much the 4 different processes took?

Of course, sorry, meant to link to the previous profile that I posted to the ticket, which shows the profile for WP_Theme_JSON_Resolver::get_merged_data(), which shows all of the individual getters as child processes. See: https://core.trac.wordpress.org/ticket/57789#comment:23

WordPress is slower with a profiler running. So the cost of JSON decoding seems lower than it actually is.

I understand your assumption here, but I'm not convinced that it's accurate, given the way XHProf is designed. What's more likely is that there are differences in the time it's taking your local system to do the file reads when compared with mine. As we've seen in other places while profiling, file I/O operations can show greater variability in performance results. It could be that I'm just not reproducing the conditions where those operations are taking a long time because of opcode cache or some other side effect.

When you say "unaffected", do you mean that in terms of little performance impact?

Yes, "unaffected" was a poor word choice. What I meant was that I was seeing little difference in the performance impact of this approach in an A/B test against trunk.

I'll run another round of profiles to see if I can verify the same results.

@joemcgill Right, I certainly don't want our work here to be based on assumptions. Though, can you please clarify how exactly you profiled this (which theme? object cache enabled? etc) and share a before & after comparison? The profile you linked to (https://core.trac.wordpress.org/ticket/57789#comment:23) seems to be for another PR.

[joemcgill]

@flixos90 I ran a round of server timing profiles on each of these PRs and while I'm seeing some improvement with both, I don't know that it's large enough of a change to push hard for at this stage, but would rather see these updates tested via the Gutenberg plugin and applied to a future version of WP. Particularly, since there are Theme JSON data origins that we've not even covered yet. Here's the results from the data I ran today:

## Trunk

### Twenty Twenty-three

╔═════════════════════╤════════════════════════╗
║ URL                 │ http://localhost:8889/ ║
╟─────────────────────┼────────────────────────╢
║ Success Rate        │ 100%                   ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p10) │ 232.64                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p25) │ 236.84                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p50) │ 241.37                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p75) │ 256.21                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p90) │ 260.35                 ║
╚═════════════════════╧════════════════════════╝

### Twenty Twenty-one
╔═════════════════════╤════════════════════════╗
║ URL                 │ http://localhost:8889/ ║
╟─────────────────────┼────────────────────────╢
║ Success Rate        │ 100%                   ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p10) │ 143.93                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p25) │ 145.25                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p50) │ 147.66                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p75) │ 152.7                  ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p90) │ 159.32                 ║
╚═════════════════════╧════════════════════════╝


## PR 5024

### Twenty Twenty-three

╔═════════════════════╤════════════════════════╗
║ URL                 │ http://localhost:8889/ ║
╟─────────────────────┼────────────────────────╢
║ Success Rate        │ 100%                   ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p10) │ 229.26                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p25) │ 230.65                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p50) │ 233.19                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p75) │ 246.22                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p90) │ 262.33                 ║
╚═════════════════════╧════════════════════════╝


### Twenty Twenty-one
╔═════════════════════╤════════════════════════╗
║ URL                 │ http://localhost:8889/ ║
╟─────────────────────┼────────────────────────╢
║ Success Rate        │ 100%                   ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p10) │ 142.86                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p25) │ 143.73                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p50) │ 144.72                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p75) │ 147.78                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p90) │ 156.58                 ║
╚═════════════════════╧════════════════════════╝

## Trunk (w/ memcached)

### Twenty Twenty-three 
╔═════════════════════╤════════════════════════╗
║ URL                 │ http://localhost:8889/ ║
╟─────────────────────┼────────────────────────╢
║ Success Rate        │ 100%                   ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p10) │ 209.38                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p25) │ 210.3                  ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p50) │ 212.58                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p75) │ 221.22                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p90) │ 234.26                 ║
╚═════════════════════╧════════════════════════╝


### Twenty Twenty-one  (with memcached)

╔═════════════════════╤════════════════════════╗
║ URL                 │ http://localhost:8889/ ║
╟─────────────────────┼────────────────────────╢
║ Success Rate        │ 100%                   ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p10) │ 127.66                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p25) │ 128.61                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p50) │ 129.73                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p75) │ 131.87                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p90) │ 138.06                 ║
╚═════════════════════╧════════════════════════╝

## PR 5267 (w memcached)

### Twenty Twenty-three

╔═════════════════════╤════════════════════════╗
║ URL                 │ http://localhost:8889/ ║
╟─────────────────────┼────────────────────────╢
║ Success Rate        │ 100%                   ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p10) │ 206                    ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p25) │ 207.04                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p50) │ 209.3                  ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p75) │ 213.44                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p90) │ 225.13                 ║
╚═════════════════════╧════════════════════════╝


### Twenty Twenty-one (with memcached)

╔═════════════════════╤════════════════════════╗
║ URL                 │ http://localhost:8889/ ║
╟─────────────────────┼────────────────────────╢
║ Success Rate        │ 100%                   ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p10) │ 127.94                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p25) │ 129.12                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p50) │ 132.8                  ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p75) │ 139.86                 ║
╟─────────────────────┼────────────────────────╢
║ Response Time (p90) │ 143.62                 ║
╚═════════════════════╧════════════════════════╝

For each of these, I ran the server timing CLI script from ​https://github.com/GoogleChromeLabs/wpp-research/tree/main/cli with 50 runs to get the results.

While there seems to be improvement from both PRs, the amount of improvement is generally within the range of variance between the p25–p75 amounts, so it's hard to say definitively how much improvement should be expected. I'm going to punt this to the 6.5 milestone for now while we continue working on this via PRs against the Gutenberg repo.

I attempted to do another round of profiling for all of these use cases today, but am seeing some unexpected results, so am planning to run them again when I have a clearer brain.

Also, to clarify the question you asked above:

he profile you linked to (https://core.trac.wordpress.org/ticket/57789#comment:23) seems to be for another PR.

The point I was trying to make is that if you look at the profile of trunk, you can see the relative wall time for each of the methods that get data for the specific origins. Even in my most recent profile runs, I'm consistently seeing core and user (i.e. custom) data taking the most time to generate. Those two seem like the best opportunities to improve via caching strategies.

Continued the conversation in the gutenberg PR ​https://github.com/WordPress/gutenberg/pull/54742#pullrequestreview-1654773700

@oandregal

Reading JSON files already results in cached data, just in static variables within this class.

If we wanted to make them persistent across requests, I presume we should remove the existing caching and use the wp_cache_* instead? As it is, it's adding a _new_ level of caching over preexisting cached data.

Those caches are not the same: The caches that use static variables today cache data that runs WordPress filters and therefore should not be cached persistently, as otherwise the filters won't run.

What I am proposing here is not concerning in that regard as reading JSON files doesn't fire any filters. That's why the caches proposed here can be persistent, while the existing ones can't. If we think that the existing static variable caches don't add much performance benefit on top of what's added here, we could consider removing them, but I don't know whether that's the case, and if so it should be explored in a separate issue.

TL;DR the two means of cache here are not interchangeable.

Another thought I have is: what would be the invalidation scenarios for these caches? For the other theme.json related ​caches we invalidated upon ​theme switching.

Invalidation as in the example you're mentioning isn't needed here, since the caches proposed operate per theme. In other words, it doesn't matter which the current theme is. Updating the theme will naturally rely on a different cache due to a version change. One thing to probably add here is an expiration, to make sure we don't keep stale theme version caches around indefinitely.

@joemcgill Can you please follow up on my question in ​https://github.com/WordPress/wordpress-develop/pull/5267#issuecomment-1733827722 when you get a chance? I would like to understand better what environment and configuration you used for the profiles.

Those caches are not the same: The caches that use static variables today cache data that runs WordPress filters and therefore should not be cached persistently, as otherwise the filters won't run.

🤔 Let me share what I'm seeing:

• We already have caches such as static::$theme_json_file_cache and static::$i18n_schema for the contents of the JSON files (core, theme, and translation schema). These caches are filled before any filter is called. They are implemented as static variables, so they cannot persist across request.
• This PR introduces core_${wp_version}, theme_${stylesheet}_${version}, i18n_schema_${version} holding exactly the same data as the existing ones. Though, they are implemented via wp_cache_* so they can be persisted.

My question is: why don't we remove static::$theme_json_file_cache and static:$i18n_schema if we are introducing caches that can be persisted via wp_cache_*? Does this help?

Invalidation as in the example you're mentioning isn't needed here, since the caches proposed operate per theme.

This is what I see:

• The static:$i18n_schema is indeed cleared via clean_cached_data that is executed upon theme switches (filters switch_theme and start_previewing_theme). If we introduce the same cache via i18n_schema_${version}, why shouldn't it be cleared when its not-persisted version was?

• The static::$theme_json_file_cache is not cleared at any point. However, the corresponding core_${wp_version} and theme_${stylesheet}_${version} caches introduced by this PR are going to be persistent, hence they's more risk of becoming stale. I guess the question we should respond is: is there any case in which the version doesn't change but the underlying theme.json does?

@oandregal Thanks for clarifying, now I get what you mean. Previously I was thinking about the static variables caching WP_Theme_JSON instances (e.g. the $core, $blocks etc variables). But you're right, I agree that those two static variables you mention wouldn't have any additional value if we introduce this persistent cache mechanism, so they could indeed be removed. 👍

[flixos90]

Marking this as a high priority for the 6.5 milestone, partly because of the very positive benchmarks encountered in ​https://github.com/WordPress/wordpress-develop/pull/5267 for sites using a persistent object cache.

We'll need to validate that, but at this point it's a promising approach, and fits well into the general template loading focus we'll continue to pursue in 6.5.

@oandregal In ​https://github.com/WordPress/wordpress-develop/pull/5267/commits/a7d99d78992e370ab130f1cec79475301909d161, I've removed the properties you've mentioned as they are now redundant due to the WP cache API being used.

I've implemented an equivalent Gutenberg pull request ​https://github.com/WordPress/gutenberg/pull/56095, ready for review, including recreatable benchmarks using compare-wp-performance.

[joemcgill]

Replying to joemcgill:

Revisiting the original explorations I proposed for this issue:

I'd like to explore three options:

1. Store the merged data to a static property, similar to the individual sources, to eliminate the cost of merging them.

This idea is explored in ​this Gutenberg PR. It does show some improvement and is something we could merge and test as a micro-optimization.

2. Make the entire merged result cacheable in a persistent way

In order to make the merged data cacheable persistently, invalidation needs to be handled for each of the ways the merged data can be affected. Including updating core versions, switching or modifying the active theme, changes to registered blocks, and changes to user edited settings via the site editor (or similar). Each origin (i.e., core, blocks, theme, and user) can also be filtered, which makes persistent caching challenging, unless we cache the pre-filtered values only rather than the final values that are statically cached currently.

3. Make the data from each source cacheable in a persistent way, if the entire result cannot be cached persistently

Given the complexity of idea 2 above, I think we should focus on this idea.

By far, the most expensive WP_Theme_JSON origin to calculate seems to be WP_Theme_JSON_Resolver::get_theme_data on the surface. So it would make sense to implement caching there, as @flixos90 has done in ​this Gutenberg PR.

However, this is a bit misleading because this method seems to be the first one called during the application lifecycle. That method is called by both wp_get_theme_data_custom_templates and wp_get_theme_data_template_parts prior to WP_Theme_JSON_Resolver::get_merged_data. This means that WP_Theme_JSON_Resolver::get_theme_data ends up including the cost for initially filling out the static properties of the WP_Theme_JSON class, including the initial calls to WP_Theme_JSON::get_blocks_metadata and WP_Theme_JSON::sanitize, which accounts for the bulk of the processing time. So while caching the WP_Theme_JSON data from themes will help, it won't remove the cost of calling the WP_Theme_JSON constructor the first time, it will just move that processing elsewhere.

Even so, I think it's worth continuing to explore optimizations that reduce redundant processing of core and theme data, at minimum. The core data should be able to be cached persistently by WP version, similar to how we've handled caching in register_core_block_style_handles(). For data that is specific to the theme, we should probably follow the same strategy that is being considered in #59719.

As a next step, it would be useful to verify the potential impact of persistently caching the WP_Theme_JSON data provided by core and themes by doing a shallow implementation and running a new set of benchmarks.

[joemcgill]

Rolling this forward for continued discussion/improvement in the next release.

Make theme.json related caches persistent by reusing cache_get and cache_add methods in WP_Theme. This means that caches are only saved for 1800 seconds. Caches for theme json are invalidated for the following reason.

• WP_Theme::cache_delete is called. Which is called when themes are updated, files edited or theme is deleted.
• Global style post is updated.
• Theme is switched or previewed.
• Invalidated after 1800 seconds like other theme caches.

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

[spacedmonkey]

@flixos90 @joemcgill I put together my own ​PR. This is much simplier. It plays nicely with other theme caches and means that theme_json caches would be invalidated when calling WP_Theme::cache_delete.

Would love your thoughts on my PR.

I'm closing this and ​the related Gutenberg PR given that this approach didn't provide a large enough measurable performance impact. In the future, I think this could be useful as reference if we are able to make the merged WP_Theme_JSON data persistent.

[joemcgill]

I'm doing some clean up on this ticket so we can make more progress during the 6.6 release cycle.

@spacedmonkey, I responded to [your PR ​https://github.com/WordPress/wordpress-develop/pull/6289] and in doing so, I realized that clarifying the goal of this ticket could be useful.

The goal of this ticket is not simply to make the data stored to our current theme_json cache group persistent, but instead to address the underlying reasons why we haven't been able to make that data persistent so far.

Generally speaking, all of the places where we are storing data to the theme_json cache group is to avoid expensive operations from the WP_Theme_JSON_Resolver class, including those related to accessing and transforming specific properties from this merged object (settings, styles, custom CSS, etc). The WP_Theme_JSON_Resolver class is responsible for merging together WP_Theme_JSON data derived from 4 sources (i.e. "origins"): core, registered blocks, the theme, and user customizations made in the site editor. Ideally, we could persistently cache the final merged WP_Theme_JSON data and the results of any expensive operations that depend on this merged data. However, handling cache invalidation for each of these origins is complex, which is what has been a blocker in this effort.

In order to move this forward, we should try to handle persistence at the origin level. Data from each origin is already stored to a static property on each request, but that data is not cached in a persistent way. We should be able to persistently cache the data from the "core" origin based on the WordPress version. We can also likely cache data from the "theme" origin based on any changes to the theme's theme.json, the parent theme's theme.json, and any add_theme_support() values. I think we should focus on these two origins for now.

The ​previous PR from @flixos90 already focuses on those two origins but specifically limits caching to just reading and translating the theme.json files. While this is already an improvement, we should validate that the approach still has the same impact that he was reporting and also see if we can increase the impact by caching more than just the translated files. However, I currently have two concerns with that approach:

1. A big part of the cost of reading and building the WP_Theme_JSON objects is spent constructing and sanitizing the WP_Theme_JSON objects themselves.
2. When measuring the impact of any PR, we should be cautious about not over-emphasizing performance gains based on avoiding file operations, since we've already determined that file reads end up getting cached at the OPcache level in most cases.

I've also closed ​my previous PR, given that it didn't result in a measurable impact.

Trac ticket: #57789

Part 1 of 57789 is to see if we could improve performance by caching get_core_data.

Caching JSON before filter only gave a benefit for 0.01%

The major regression at the moment is from WP_Theme_JSON_Data::__construct and WP_Theme_JSON::__construct

[thekt12]

@joemcgill based on your comment ​https://wordpress.slack.com/archives/C02KGN5K076/p1714752083294029 I have investigated WP_Theme_JSON::resolve_custom_css_format and WP_Theme_JSON::remove_keys_not_in_schema.

Both of the functions involve traversing the full tree, and both are doing it recursively.

resolve_custom_css_format is fairly straightforward and there is little to no scope for improvement as all tree nodes must be visited and recursion seems to be the best option.

remove_keys_not_in_schema is a bit more complex implementation but I couldn't find an easy way out. I was checking if some heuristic that can easily remove keys could be built, but so far no luck.

[thekt12]

Adding to the previous comment - I did try to see if caching remove_keys_not_in_schema could improve the situation or not.
The cost of creating a cache key is several times more than the cost involved in recursion. So caching is not a solution here.

Here is my experiment -
​https://github.com/WordPress/wordpress-develop/pull/6583

[oglekler]

We have 3 hours until the commit freeze before Beta 1 release, and it looks like this ticket will not be in Trunk in time. I am rescheduling it to the next milestone. If you are hard at work and about to merge, return it when ready.

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

@joemcgill, the ticket's scope is still within what we initially decided. I am only persistently caching get_theme_data() and get_core_data(). Performance benefit from the core was less than 1%.

My efforts were directed towards maintaining the integrity of wp_theme_json_data_default and wp_theme_json_data_theme. However, the majority of the regression occurred between the static return and the filter call. It's important to note that values are now statically cached after the filter, ensuring they reflect any changes from the filter. Persistent cache should not pose a problem if wp_theme_json_data_default and wp_theme_json_data_theme are used as pure filters and not as actions.

One workaround was to call wp_theme_json_data_default and wp_theme_json_data_theme like actions when we initialize from a persistent cache. This way, both hooks will be available for people who use them as actions. The filter version will be available when the cache expires.

@joemcgill, I have updated this PR with your feedback (I'll shift it to GB shortly). I saw I accidentally pushed 'block' origin cache that I used to get metrics. I have fixed that now.
Now, caching is only added for theme_data and core_data.
In the new update,

• I created a cache just before the filter, so I was able to add the filter even in the cached version.
• Removed all the additional static variables.
• I have kept the common expiry, and it is low at 10 minutes. My main aim was to have a single db call for all the cache rather than multiple db calls with different expiry.

@joemcgill GB issue for this task - ​https://github.com/WordPress/gutenberg/pull/62794

@joemcgill Putting this on hold.