Object caching `WP_Theme_JSON::compute_style_properties` #6392

kt-12 · 2024-04-15T12:25:29Z

Trac ticket: 59595

Incorporating feedback from - #5567, I did the first version #5567

`theme_json` based caching based on initial feeback ( commit )

The commit included using, theme_json for caching. However theme_json being non-persistent had no benefit. Instead, the cache key generation added additional overhead. This is because compute_style_properties( is called 85 times on the home page out of which 83 calls had a unique signature, so it was as good as having no cache at all + cache key generation overhead. The same -ve effect was found for the non-cached site. Overall the code resulted in a 6% degrade.

No cache - All score below in (ms)

Trunk Response Time (median) - 399.62
After PR - 424.81 -- Note this cost is mainly due to cache key creation code run separately 85 times

Meme cache with `theme_json` non persistent cache

Trunk Response Time (median - 375.47
After PR with cache - 402.58

Switching to transient-based caching ( this commit )

The way around this was to use transient function-based caching. The idea was transient would override the -ve effect of overhead due to cache key creation on non-persistent sites and it would give the benefit of a persistent cache for other sites.

Testing the above concept this commit -

No cache

Trunk - 400
PR (transient cache) - 471.61 ( mainly because get_transient_site is costly) + cache_key code.

Meme cache

Trunk - 374
PR - 376 (benefit negated by wp_json_encode and md5 )

Improved transient caching through static caching (current state)

Meme cache

Trunk - 374
PR - 356 (~ 6% improvement )

No cache

Trunk - 400
PR (transient cache) - 384 ( ~4% improvement )

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

github-actions · 2024-04-15T12:43:09Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

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

github-actions · 2024-04-16T14:28:19Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props thekt12, joemcgill, spacedmonkey.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

joemcgill

Thanks @kt-12. I can confirm that this makes a big improvement in my profiling, so it would be great to keep some focus on this problem. The biggest issue with the current approach is the use of wp_add_global_styles_for_blocks() inside the WP_Theme_JSON class. We need to find an approach to caching this data that does't require the use of this global function or rethink the approach a bit.

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

cs fixes Co-authored-by: Joe McGill <[email protected]>

joemcgill

I like the idea here. I've left a few suggestions for how I think this could be improved.

src/wp-includes/global-styles-and-settings.php

joemcgill · 2024-05-20T22:17:19Z

src/wp-includes/global-styles-and-settings.php

+			$style_cache_key = $settings_hash . '_' . $metadata_hash;
+			if ( isset( $cached[ $style_cache_key ] ) ) {
+				$block_css = $cached[ $style_cache_key ];
+			} else {
+				$block_css                  = $tree->get_styles_for_block( $metadata );
+				$cached[ $style_cache_key ] = $block_css;
+			}


If the settings hash is part of the transient key, it doesn't need to be used for each block node key:

Suggested change

$style_cache_key = $settings_hash . '_' . $metadata_hash;

if ( isset( $cached[ $style_cache_key ] ) ) {

$block_css = $cached[ $style_cache_key ];

} else {

$block_css = $tree->get_styles_for_block( $metadata );

$cached[ $style_cache_key ] = $block_css;

}

if ( isset( $cached[ $metadata_hash ] ) ) {

$block_css = $cached[ $metadata_hash ];

} else {

$block_css = $tree->get_styles_for_block( $metadata );

$cached[ $metadata_hash ] = $block_css;

}

src/wp-includes/global-styles-and-settings.php

…style-properties

…erties

src/wp-includes/global-styles-and-settings.php

joemcgill · 2024-05-29T01:07:11Z

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.

kt-12 · 2024-05-29T20:42:18Z

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

@@ -1000,6 +1000,14 @@ protected static function get_blocks_metadata() {
 		$registry = WP_Block_Type_Registry::get_instance();
 		$blocks   = $registry->get_all_registered();

+		// Unset old blocks from static variable.


We haven't been clearing unregistered blocks till now. This caused inconsistent outcomes, for WP_Theme_JSON::get_styles_block_nodes()

Good find. I'm really unsure if having stale metadata stored in the WP_Theme_JSON class is really a problem, as it seems like most of the places that make use of this data are doing so for validation purposes when comparing against the active WP_Theme_JSON data.

I am curious if you still see the behavior that you noticed if the changes from #6696 are applied.

If not removing this metadata leads to a bug, it would be best to document the specifics of the bug with steps to reproduce and propose this change outside the scope of this PR.

tests/phpunit/tests/theme/wpThemeJsonResolver.php

tests/phpunit/tests/theme/wpAddGlobalStylesForBlocks.php

joemcgill · 2024-05-31T17:16:12Z

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

@@ -1000,6 +1000,14 @@ protected static function get_blocks_metadata() {
 		$registry = WP_Block_Type_Registry::get_instance();
 		$blocks   = $registry->get_all_registered();

+		// Unset old blocks from static variable.


Good find. I'm really unsure if having stale metadata stored in the WP_Theme_JSON class is really a problem, as it seems like most of the places that make use of this data are doing so for validation purposes when comparing against the active WP_Theme_JSON data.

I am curious if you still see the behavior that you noticed if the changes from #6696 are applied.

If not removing this metadata leads to a bug, it would be best to document the specifics of the bug with steps to reproduce and propose this change outside the scope of this PR.

tests/phpunit/tests/theme/wpAddGlobalStylesForBlocks.php

This reverts commit 48a310c.

src/wp-includes/global-styles-and-settings.php

…erties

… update/59595-fix-tests

tests/phpunit/tests/theme/base.php

This reinstates the code block that handled proper inlining of styles that are added through theme.json files and updates the way block nodes are being cached so that we use the block name as the cache key when available, or a hash of nodes that don't have a block name because they've been added through theme.json files.

joemcgill · 2024-06-03T20:25:54Z

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

No longer applicable

joemcgill · 2024-06-03T22:58:31Z

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.

joemcgill · 2024-06-04T14:51:41Z

Merged in 58334.

kt-12 added 2 commits April 15, 2024 11:14

add caching

b7ba582

transient implementation

3bba97f

Improve caching key computation

97806ea

kt-12 commented Apr 16, 2024

View reviewed changes

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

kt-12 commented Apr 16, 2024

View reviewed changes

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

kt-12 added 3 commits April 16, 2024 02:53

improving performace for non persistance cache.

341e218

cache related call only for non development mode

9c92f65

cache clear code

cce7c9a

kt-12 commented Apr 16, 2024

View reviewed changes

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

kt-12 changed the title ~~Enhancement object cache compute style properties~~ Object caching WP_Theme_JSON::compute_style_properties Apr 16, 2024

kt-12 added 3 commits April 16, 2024 12:44

change cache invalidating logic

7c7bcec

remove get compute style from constructor

b55aa6f

remove uncessary check

2732050

kt-12 commented Apr 16, 2024

View reviewed changes

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

kt-12 marked this pull request as ready for review April 16, 2024 14:28

attach clear_compute_style_properties_cache directly to hook

2afb765

joemcgill requested changes Apr 19, 2024

View reviewed changes

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

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

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

kt-12 and others added 9 commits April 23, 2024 12:55

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

974a292

cs fixes Co-authored-by: Joe McGill <[email protected]>

global style caching

99ac114

update old test

fc6c235

remove additional function

d1bd38a

update function name

2957ae4

cs fixes

ffa4771

cache is set check

cbc7be5

cache is set check

a4fc0b5

assert if it's not empty after

23270b2

joemcgill previously requested changes May 20, 2024

View reviewed changes

Merge branch 'WordPress:trunk' into enhancement/object-cache-compute-…

672525f

…style-properties

kt-12 added 5 commits May 27, 2024 17:48

remove repetition

5899f63

Merge branch 'trunk' into enhancement/object-cache-compute-style-prop…

23c518e

…erties

update style block caching logic

2c26302

update test cases.

4c8dfe2

revert test changes

b08183e

kt-12 commented May 28, 2024

View reviewed changes

src/wp-includes/global-styles-and-settings.php Outdated Show resolved Hide resolved

unregster bug fix

48a310c

kt-12 commented May 29, 2024

View reviewed changes

joemcgill reviewed May 31, 2024

View reviewed changes

joemcgill added 4 commits May 31, 2024 13:16

Revert "unregster bug fix"

8b16eb5

This reverts commit 48a310c.

Clear theme json data between theme unit tests

76435fc

Merge branch 'trunk' into update/59595-fix-tests

a8c7453

Don't cache nodes without names

21c2922

joemcgill reviewed May 31, 2024

View reviewed changes

src/wp-includes/global-styles-and-settings.php Outdated Show resolved Hide resolved

joemcgill mentioned this pull request May 31, 2024

Updates for PR 6392 #6701

Closed

joemcgill and others added 3 commits June 3, 2024 12:46

Merge branch 'trunk' into enhancement/object-cache-compute-style-prop…

c938aa5

…erties

Merge branch 'enhancement/object-cache-compute-style-properties' into…

6becf8d

… update/59595-fix-tests

remove test case

accffb6

joemcgill reviewed Jun 3, 2024

View reviewed changes

tests/phpunit/tests/theme/base.php Outdated Show resolved Hide resolved

kt-12 and others added 3 commits June 3, 2024 20:02

cache node with no name

b6de61f

test cases fixes

0947a38

joemcgill mentioned this pull request Jun 4, 2024

Improve theme file caching in class-wp-theme-json.php #6707

Closed

Inline docs improvements

561ee41

joemcgill closed this Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Object caching `WP_Theme_JSON::compute_style_properties` #6392

Object caching `WP_Theme_JSON::compute_style_properties` #6392

kt-12 commented Apr 15, 2024 •

edited

Loading

github-actions bot commented Apr 15, 2024

github-actions bot commented Apr 16, 2024 •

edited

Loading

joemcgill left a comment

joemcgill left a comment

joemcgill May 20, 2024

joemcgill commented May 29, 2024

kt-12 May 29, 2024 •

edited

Loading

joemcgill May 31, 2024

joemcgill May 31, 2024

joemcgill commented Jun 3, 2024

joemcgill commented Jun 3, 2024

joemcgill commented Jun 4, 2024

Object caching WP_Theme_JSON::compute_style_properties #6392

Object caching WP_Theme_JSON::compute_style_properties #6392

Conversation

kt-12 commented Apr 15, 2024 • edited Loading

theme_json based caching based on initial feeback ( commit )

No cache - All score below in (ms)

Meme cache with theme_json non persistent cache

Switching to transient-based caching ( this commit )

No cache

Meme cache

Improved transient caching through static caching (current state)

Meme cache

No cache

github-actions bot commented Apr 15, 2024

Test using WordPress Playground

Some things to be aware of

github-actions bot commented Apr 16, 2024 • edited Loading

joemcgill left a comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

joemcgill May 20, 2024

Choose a reason for hiding this comment

joemcgill commented May 29, 2024

kt-12 May 29, 2024 • edited Loading

Choose a reason for hiding this comment

joemcgill May 31, 2024

Choose a reason for hiding this comment

joemcgill May 31, 2024

Choose a reason for hiding this comment

joemcgill commented Jun 3, 2024

joemcgill commented Jun 3, 2024

joemcgill commented Jun 4, 2024

Object caching `WP_Theme_JSON::compute_style_properties` #6392

Object caching `WP_Theme_JSON::compute_style_properties` #6392

kt-12 commented Apr 15, 2024 •

edited

Loading

`theme_json` based caching based on initial feeback ( commit )

Meme cache with `theme_json` non persistent cache

github-actions bot commented Apr 16, 2024 •

edited

Loading

kt-12 May 29, 2024 •

edited

Loading