Improve performance of WP_Theme_JSON::compute_style_properties

[spacedmonkey]

When profiling WP 6.4 ( other releases ), it was not that the method WP_Theme_JSON::compute_style_properties can result in around 6-11% of server time spent to generate a page. Research should be conducted to see if this method and child function / method calls, could be improved, to improve performance of this method.

[pereirinha]

Request overview before

[pereirinha]

Function details before

[pereirinha]

Request overview after

[pereirinha]

Function details after

[pereirinha]

I tested using WordPress 6.5-alpha-56966-src with the TTF theme while visiting an archive page with 10 posts. I can replicate similar results with other versions and themes and am happy to share the results if found relevant. For profiling, I used ​XHProf after an unsuccessful attempt to use Blackfire.

These are the results of the tests I made and the findings.

Though the compute_style_properties method is static, I didn't find a case where the arguments would be the same to benefit the runtime.

In my testing, the number of calls for this method averaged 85 calls per request.

The description of the method in its docblock states that:

Given a styles array, it extracts the style properties and adds them to the $declarations array…

In a single request, the cumulative size of its arguments is:

• Styles: 23.725 KB,
• Settings: 803.350 KB,
• Properties: 202.456 KB,
• Theme Json: 1.748 MB,
• Selector: 2.634 KB
• Iterations: 85

Responsible for about 50% of the wall time for this method is the child function str_starts_with, followed by WP_Theme_JSON::get_property_value with about 33.5%.

Most of the time is spent with PHP's natural functions, which pull data out of large datasets, so it can't be optimized much across the board. However, I gave it a go with object caching, and the benefits are considerable for those who can leverage this feature.

Attached are the print screens from the tests conducted. The before is as core, and the after runs a warm cache.

Add object caching to improve the performance of WP_Theme_JSON::compute_style_properties when supported.

Trac ticket: 59595

[mukesh27]

Assign to @pereirinha as he is actively working on it.

@pereirinha I did some benchmarking for TT4 theme home page using XHProf but i can't i can't find any improvement. Did i miss anything?

cc. @felixarntz @joemcgill

@pereirinha I did some benchmarking for TT4 theme home page using XHProf but i can't i can't find any improvement. Did i miss anything?

cc. @felixarntz @joemcgill

Thanks, @mukeshpanchal27, for picking it up. Can the reason why you don't see improvements might be that you aren't using ​Memcached?

Thanks, @mukeshpanchal27, for picking it up. Can the reason why you don't see improvements might be that you aren't using ​Memcached?

I just enabled the Memcached through ENV and now i got the improvement after the changes.

When and how is this cache invalidated? Adding caching is great, but if not correctly cache invalidated when file is change, this will make more problems than it solves.

@spacedmonkey, the cache is created based on the requested arguments. A single page load will use several different cache entries, and we are using these arguments to create a md5 hash. Now we will also include the wp_get_global_settings(), so that any changes the users make will force a new cache entry.

The cleanup will happen daily so that unused data gets purged. The caveat is that, even if there are no changes, the cache will have to be regenerated once a day.

@felixarntz @mukeshpanchal27 this is ready for you to circle back.

I appreciate your patience here.

Thanks for the ping! I can't see any obvious issues with this approach and changing global styles still works well in my local dev environment. Perhaps @andrewserong or @ramonjd might have additional thoughts as iirc they worked on previous caching-related bugs.

Thanks for the ping! I can't see any obvious issues with it, either, but I'm a little cautious about caching in general for these classes just because it can be really hard to predict what sorts of things might go wrong. For example, I see there's no guard against development mode. Will that make it hard for theme and or core developers working locally who are experimenting with making changes in the processing logic (outside of just changes within their theme)?

I agree with this comment about reducing the cache expiry time down to 1 hour: ​https://github.com/WordPress/wordpress-develop/pull/5567#discussion_r1445408359 that feels safer to me in case there are any issues for folks during a WP version upgrade.

Actually, that'd probably be my main question: would this cache get automatically flushed when a site is upgraded? It's likely that processing logic will change between releases, for example the fluid typography rules can be tweaked, or there could be additions to this function in the future.

Very cool that adding caching here can improve performance! At first I was wondering if it might introduce a performance hit with the extra calls to wp_get_global_settings() and wp_json_encode... interesting that it's cheaper to do both of them to generate a key than to process compute_style_properties with the provided values 🤔

Also thanks for the ping!

Love caching stuff when we can, especially huge buckets of style gloop.

I'd lean on @oandregal's experience and opinion here. I believe he worked on caching theme.json output before. E.g., ​https://github.com/WordPress/gutenberg/pull/45543

[joemcgill]

@pereirinha it's looking like the current approach won't work, based on @oandregal's ​feedback in the PR.

I think we could either look at setting these caches in wp_add_global_styles_for_blocks(), or as a new property in WP_Theme_JSON_Resolver that produces the WP_Theme_JSON object used in that function via WP_Theme_JSON::get_styles_for_block().

[joemcgill]

This still needs to be updated based on the latest feedback on the PR. Punting to 6.6 ahead of the 6.5 beta 1 date tomorrow.

[joemcgill]

Reassigning to @thekt12, who volunteered to pick this up for the 6.6 milestone. Please make sure to review the feedback on @pereirinha's previous PR.

Trac ticket: 59595

Incorporating feedback from - ​https://github.com/WordPress/wordpress-develop/pull/5567

[thekt12]

I have added a new ​PR#6392 based on feedback from @pereirinha initial ​PR#5567

In the current run, I am not able to record the same improvement that @pereirinha had recorded.

I also see the PR may be improved for sites with persistent cache but the way we are creating cache key it has a -ve effect (almost 6% downgrade) in performance for sites with non-persistant cache.

[joemcgill]

Following up after chatting with @thekt12 about the circular dependency issue noted ​in my PR review. To avoid this, we could consider creating a new method on the WP_Theme_JSON_Resolver class that will get computed styles for the merged JSON object, though a better approach is likely to move caching of this data to the wp_add_global_styles_for_blocks() helper which ends up calling WP_Theme_JSON::compute_style_properties() via WP_Theme_JSON::get_styles_for_block() for each registered block.

In doing so, we may need break up caching so that we are only caching computed style properties for core blocks, so we can invalidate when the WP version changes. Otherwise, we will need to invalidate this cache whenever the registered blocks have changed, which is challenging because we don't have a concept of block versions, so an update to an existing block could introduce changes in the styles that would need to be accounted for.

[thekt12]

@joemcgill I have updated the PR with new changes. I can see 10% improvement with the latest changes.

@kt-12 I spent some time looking over the unit tests and add a couple additional tests that I think could be useful in ​d0b243c. I could not think of a good way of ensuring that the cache was actually used in order to avoid unnecessary calls to get_styles_for_block() since we're not able to modify the WP_Theme_JSON_Resolver class hard coded in wp_get_global_styles_custom_css() nor the WP_Theme_JSON object that is returned by WP_Theme_JSON_Resolver::get_merged_data().

This is looking good. Just doing some testing of the PR and I see a large improvement in the wp_add_global_styles_for_blocks() function. Some of the improvement is offset by the fact that the transient requires another DB query, but in persistent caches, those will also be avoided.

Trunk

This PR

Some of the Unit test failures seem legitimate, so let's figure out those issues and this should be almost ready to go in.

Minor changes have been made to class-wp-theme-json.php to better cache the theme files. This includes adding an 'update_cache' functionality that sets 'str_start_with' and 'str_start_with_cache', and removing redundant checks for empty or non-numerical values. These improvements aim to optimize performance by updating cache when necessary.

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

[spacedmonkey]

@thekt12 @joemcgill @pereirinha I put together my ​PR for this. It add some performance improvements and logic improves. I am seeing some improves. It reduced the calls to str_starts_with from 9k down to 3k.

Can one of you run some profiles and see if this is a real improvement.

[adamsilverstein]

@spacedmonkey is your PR intended as a replacement for the one @thekt12 has been working on?

@kt-12 I went ahead and resolved conflicts and updated this branch with the fixes proposed in ​my other PR so will close that now. Looks like this has fixed the test failures we were seeing on this branch. Let's do some final checks, but I think this is about ready to commit.

With the latest updates this is still profiling really well and showing a large improvement for wp_add_global_styles_for_blocks().

Trunk

PR

That said, those improvements are not currently reflected in the benchmarks performed by ​the Performance Workflow. I suspect it's because the extra query being performed to get the transient is offsetting the savings introduced in the function, but that's just a guess. Given the size of the potential improvement, it still may be worth committing so we can continue to do further testing during the beta period.

Thanks @spacedmonkey. I did some profiling of this change against trunk and it does reduce the number of str_starts_with calls quite drastically.

Trunk

This PR

That said, ​the PR we've been working on has a bigger impact by reducing the number of calls to this method altogether. It may be worth combining both approaches to maximize the benefit, but I'd like to prioritize getting #6392 into trunk first as it has the biggest potential impact.

[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.

[joemcgill]

In 58334:

Editor: Cache global styles for blocks.

This caches the generated CSS from block nodes in merged Theme JSON data to avoid repeated costly operations required to compute style properties for blocks. The generated CSS is saved to a transient that expires every hour.

Props thekt12, spacedmonkey, pereirinha, mukesh27, isabel_brison, oandregal, andrewserong, ramonjd.
Fixes #59595.

Merged in 58334.

[spacedmonkey]

@joemcgill @thekt12 I have created a ​PR for the gutenberg repo. Would love some eyes on this.

@spacedmonkey are ok to close this in favor of ​https://github.com/WordPress/gutenberg/pull/62522?

[joemcgill]

[58334] uses a transient with a 1 hour TTL to cache global styles for blocks to avoid the costly calculation of styles for registered blocks. However, the transient expiration time results in an extra DB query being made on each request since transients with expiration do not get autoloaded, which mostly negates the performance benefit of the cache. I'm reopening to see if we can avoid this extra query.

The approach already uses a hash of the global settings and block nodes as part of the cache key, so it's possible that we could just use that hash for cache invalidation and bust cache whenever the hash misses rather than needing a TTL.

@joemcgill This is the backport PR? We'll need both?

@ellatrix and @spacedmonkey, it's fine to use this PR as the backport from ​https://github.com/WordPress/gutenberg/pull/62522. At present, it's not the same code though, so this will need to be updated once the GB PR is finalized.

This uses a consistent cache key with the cache busting hash saved to the cache value rather than as part of the key. This allows us to remove the expiry time for the cache and only invalidate when the hash changes.

This also swaps the use of a site transient to the use of a regular transient so that the option will be autoloaded since there is no expiration time.

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

[joemcgill]

After spending some time reviewing the caching strategy implemented in [58334], in light of my previous comment, I have worked on a proof of concept PR that tries to avoid the extra DB query introduced here, but I have a few concerns.

To avoid the extra DB query, we can move the hash to inside the transient, but we're currently using the hash key as a way to differentiate different caches on the same network, using site transients. However, site transients never get autoloaded, even if they never expire.

I've moved the hash to a value stored inside the transient value, as a way to validate the cache and rebuild if needed. Even so, the addition of wp_prime_option_caches() to set_transient() (et al.) in [58134] is causing an extra query to prime the timeout value, even if no expiration is present and the value has been autoloaded. That is probably worth a separate ticket altogether, but worth mentioning here.

I'm not seeing a big performance difference with avoiding the extra query, so I don't know that any additional changes are needed here for 6.6, unless additional bugs are reported.

@thekt12, @spacedmonkey, and @peterwilsoncc, I'd appreciate your thoughts here — both on the proposed changes in ​PR #6879 as well as the broader concerns about priming timeouts behavior.

@spacedmonkey, I've approved the related GB PR. Once this PR is updated to reflect the latest changes, I'm happy to give another review.

The method compute_style_properties() in class-wp-theme-json.php has been refactored for more efficient theme style computation. The flow has been improved by reordering conditions and breaking down complex ones into simpler bits. Now the code is easier to read, and the handling of protected properties and root styles are clearer.

Backport from ​https://github.com/WordPress/wordpress-develop/pull/6707

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

Backport from ​https://github.com/WordPress/gutenberg/pull/62522

Started in ​https://github.com/WordPress/wordpress-develop/pull/6707

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

[joemcgill]

This shouldn't be punted to 6.7, because it was reopened to try to improve the original fix. I'm going to close as fixed and we can reopen if there are any reports during RC. Any follow-ups can be in new tickets.

@joemcgill On a side now, get_site_transient only result in one database query now. See

Same with get_transient

[andrewserong]

Hi folks, it looks like the updated caching strategy here has resulted in a regression in WP 6.6 (I'm testing in RC 3), where styles that a user sets in global styles for a block within the site editor become stuck on the site frontend.

To reproduce:

1. Open the site editor on a WP 6.6 RC3 site running Twenty Twenty Four theme
2. Click the editor preview to go into edit mode
3. Open the Global Styles sidebar panel on the right hand side
4. Click Blocks
5. Select the Button block
6. Give the Button block a style, e.g. Background color set to a gradient
7. Save the global styles
8. Load the site frontend — notice that the styling on the Button is applied
9. Go back to global styles and give the Button block another style, e.g. large font size
10. Save the global styles
11. Load the site frontend and notice that the Button block's styling has not changed — it'll be cached with the previous styles that were originally saved

It sounds like the culprit is the caching strategy used in wp_add_global_styles_for_blocks.

Can this be looked at, or reverted for 6.6?

This PR is an initial pass at preventing stale global block styles from being rendered on the frontend in 6.6

#### Problem

Caching was recently added to the global block styles in wp_add_global_styles_for_blocks, this was keyed only on the block name or metadata meaning the cached styles were returned even if the styles had recently changed.

#### Proposed Solution

Cache the global block styles using a hash of the style data as well.

#### Steps to replicate issue on trunk

Copied from #71

1. Open the site editor on a WP 6.6 RC3 site running Twenty Twenty Four theme
2. Click the editor preview to go into edit mode
3. Open the Global Styles sidebar panel on the right hand side
4. Click Blocks
5. Select the Button block
6. Give the Button block a style, e.g. Background color set to a gradient
7. Save the global styles
8. Load the site frontend — notice that the styling on the Button is applied
9. Go back to global styles and give the Button block another style, e.g. large font size
10. Save the global styles
11. Load the site frontend and notice that the Button block's styling has not changed — it'll be cached with the previous styles that were originally saved

#### Test Instructions

1. Checkout this PR and repeat the steps above. This time the styles should update appropriately
2. Confirm that the cache is still used when styles haven't changed.

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

I'll be AFK for a couple of days. So please feel free to push changes, rework, or discard this PR. It's only meant to help find a possible solution for 6.6.

I gave this a quick test run on my local and this does seem to fix the issue!

[joemcgill]

I can also confirm the issue reported by @andrewserong. ​PR #7017 fixes the issue by including a hash of the block styles data with each block node, which will invalidate the cached styles for each block individually if that data changes.

Rather than splitting the responsibility for cache invalidation to both the hash stored as part of the transient name and also a hash saved as part of the transient data, I propose we consider the approach in ​PR #6897, which I've just updated so it's ready for review.

The major changes in my alternate approach are:

1. Uses a hash of the entire raw Theme JSON data for cache invalidation.
2. Saves the hash as part of the saved data rather than as part of the transient name, to make manual clearing of the transient easier (e.g., wp transient delete wp_styles_for_blocks.
3. Changes cache storage from using {get|set}_site_transient() to {get|set}_transient, so that each site on a network has an individual cache, rather than storing all caches at the network level when running as a multisite. This addresses a separate concern that I had noticed while reviewing the performance of the current approach (comment:61), but would be good to fix at the same time.

This reverts https://core.trac.wordpress.org/changeset/58334, to fix this bug which was discovered late in the 6.6 RC cycle.

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

[joemcgill]

​PR #7026 Is a clean revert of [58334] for 6.6, and we can reconsider the performance fix for 6.6.1.

For context, see the ​related conversation in Slack.

CC: @audrasjb, @hellofromtonya, and @spacedmonkey for review of the revert.

[hellofromTonya]

Tested using the steps @andrewserong provided comment:71.

• Can reproduce the issue with 6.6 RC3.
• Reverting [58334] resolves it.

The revert of [58334] is captured in this PR ​https://github.com/WordPress/wordpress-develop/pull/7026.

Marking the revert for commit.

[joemcgill]

In 58710:

Editor: Revert caching of global styles for blocks.

This reverts [58334] to fix a bug where edits to block styles made in the site editor were not showing in the front end.

Props joemcgill, spacedmonkey, andrewserong, hellofromtonya, audrasjb.
See #59595.

[joemcgill]

Leaving this open for backporting to the 6.6 branch.

Merged in https://core.trac.wordpress.org/changeset/58710.

[hellofromTonya]

[58710] LGTM for backport to the 6.6 branch.

Note: This revert was discussed in Making WordPress #core ​slack channel. Consensus and approved by @audrasjb (6.6 Core Tech Lead).

[joemcgill]

In 58711:

Editor: Revert caching of global styles for blocks.

This reverts [58334] to fix a bug where edits to block styles made in the site editor were not showing in the front end.

Reviewed by hellofromtonya.
Merges [58710] to the 6.6 branch.

Props joemcgill, spacedmonkey, andrewserong, hellofromtonya, audrasjb.
See #59595.

[joemcgill]

After reverting [58334], I'm leaving this open, but moving this to the 6.6.1 milestone to reinstate the cache with a fix for the issue reported by @andrewserong in https://core.trac.wordpress.org/ticket/59595#comment:71.

I'd still propose reviewing ​PR #6897 as an updated approach and will update that PR to apply cleanly against trunk following the revert.

Closing in favour of ​https://github.com/WordPress/wordpress-develop/pull/6879

Thanks all, I'll update this PR so it applies cleanly after the revert of the original caching so we can try again for 6.6.1