Add object caching for `WP_Theme_JSON::compute_style_properties` #5567

pereirinha · 2023-10-24T22:08:23Z

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

Trac ticket: 59595

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.

mukeshpanchal27

Thanks @pereirinha for the PR. Left one feedback.

mukeshpanchal27 · 2023-10-31T09:14:25Z

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

+		$args      = func_get_args();
+		$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $args ) );
+		$cache     = wp_cache_get( $cache_key, 'wp-styles' );
+
+		if ( $cache ) {
+			return $cache;
+		}


Certainly, you can add the code after line 1944 to avoid creating a cache for empty( $styles ). Please insert the code in the appropriate location to achieve this.

Great catch, @mukeshpanchal27.

mukeshpanchal27 · 2023-10-31T09:25:16Z

@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 · 2023-10-31T10:26:07Z

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

joemcgill

Thanks for digging into this, @pereirinha. Looking at the code path that generates these style properties, I wonder if we would be better off moving caching of this data to wp_add_global_styles_for_blocks(), which is the main caller of this method. One challenge here is that there are a number of external factors that we might need to account for in order to invalidate the cache (e.g., updating the theme, core version bumps, etc.), so limiting the surface area to a specific use case might be safer. Curious what you think?

joemcgill · 2023-10-31T12:54:55Z

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

@@ -1925,6 +1925,14 @@ protected static function flatten_tree( $tree, $prefix = '', $token = '--' ) {
 	 * @return array  Returns the modified $declarations.
 	 */
 	protected static function compute_style_properties( $styles, $settings = array(), $properties = null, $theme_json = null, $selector = null, $use_root_padding = null ) {
+		$args      = func_get_args();
+		$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $args ) );
+		$cache     = wp_cache_get( $cache_key, 'wp-styles' );


You probably need to add a check for development mode before making use of caches, similar to how WP_Theme::get_block_patterns does here.

Question: If we introduce a development mode check, we might end up in a loop of discussions similar to what we had in #59719. Would it be advisable to prioritize and address that ticket first to determine the best way forward, making it easier to adopt the solution in other places? What are your thoughts on this?

Good question—thanks for asking, @mukeshpanchal27. I don't think we need that discussion to block forward progress on this ticket since it's not proposing that we make these caches persistent via transients. In this case, adding a development mode check just ensures that anyone who is developing while using an object cache would still get fresh results.

@mukeshpanchal27 @joemcgill, thanks for these inputs.

I just pushed an update to bail processing empty $styles and ensure we don't leverage cache in development mode.

I'm also adding an expiry time for the cache keys — a day is a reasonable value. This will help the system to clean up old data that it's not used often, in case there's a need for resources.

mukeshpanchal27 · 2023-11-01T06:27:00Z

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.

… empty `$styles`

pereirinha

@mukeshpanchal27 @joemcgill

This is ready for you to circle back, as I pushed an update addressing your feedback. And I do have a few thoughts here.

We might not even need to avoid the cache at all. You'll notice when we are generating the cache key, we use the function arguments to generate it — in a single request, we run this function many times with different arguments. When you change a value in any of the arguments, a new cache key will be generated. We won't use cached data if there's no match in the hash, hence, even if you're using cache, this strategy won't impact the development of a theme.

pereirinha · 2023-10-31T10:25:32Z

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

+		$args      = func_get_args();
+		$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $args ) );
+		$cache     = wp_cache_get( $cache_key, 'wp-styles' );
+
+		if ( $cache ) {
+			return $cache;
+		}


Great catch, @mukeshpanchal27.

pereirinha · 2023-11-03T10:10:31Z

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

@@ -1925,6 +1925,14 @@ protected static function flatten_tree( $tree, $prefix = '', $token = '--' ) {
 	 * @return array  Returns the modified $declarations.
 	 */
 	protected static function compute_style_properties( $styles, $settings = array(), $properties = null, $theme_json = null, $selector = null, $use_root_padding = null ) {
+		$args      = func_get_args();
+		$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $args ) );
+		$cache     = wp_cache_get( $cache_key, 'wp-styles' );


@mukeshpanchal27 @joemcgill, thanks for these inputs.

I just pushed an update to bail processing empty $styles and ensure we don't leverage cache in development mode.

I'm also adding an expiry time for the cache keys — a day is a reasonable value. This will help the system to clean up old data that it's not used often, in case there's a need for resources.

…operties` changes

mukeshpanchal27 · 2023-11-06T11:24:27Z

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

+		$args      = func_get_args(); // phpcs:ignore PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed
+		$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $args ) );
+		$cache     = wp_cache_get( $cache_key, 'wp-styles' );


This code is still being called if the theme is in development mode. Could you please review the approach taken in this section of the code and consider applying a similar approach here?

felixarntz · 2023-12-14T18:09:05Z

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

@@ -1934,6 +1934,16 @@ protected static function compute_style_properties( $styles, $settings = array()
 			return $declarations;
 		}

+		$can_use_cached = ! wp_is_development_mode( 'theme' );


As far as I can tell, we may be able to avoid the wp_is_development_mode() check here entirely. Here's why:

The cache key depends on the given method arguments. This means it'll always be "fresh". For example, if a style changes, a new cache key will be used.

The only exception is the call to wp_get_typography_font_size_value() below, which relies on other data (e.g. wp_get_global_settings()). So maybe we can cache everything except that one? Would that still be beneficial for performance?

That would mean that we cache the result of the foreach loop, except for the font-size CSS property we simply store the $value without calling wp_get_typography_font_size_value().

After that loop, even if we got the result from cache, we can cycle through the loop of the results and call wp_get_typography_font_size_value() for any entry that is using the font-size CSS property.

@felixarntz

I agree that the check for the development mode here doesn't bring any value. For that reason, I removed it.

As for your concern around wp_get_global_settings() for the case when users tweak values, I think that we can get away with making that part of the recipe for the cache key. Here's how.

What do you think?

spacedmonkey

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.

…operties

github-actions · 2024-01-04T00:17:48Z

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.

…ges, it invalidates the cache

pereirinha · 2024-01-04T01:20:16Z

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.

pereirinha · 2024-01-04T01:21:43Z

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

I appreciate your patience here.

joemcgill

Thanks @pereirinha.

This does make a big impact on performance, and I think we're really close. Even after reviewing this part of the WP_Theme_JSON system for a while, I'm not confident that I understand all of the potential side effects that could cause these values to become stale. Would appreciate thoughts from @tellthemachines or someone who worked on the original system in Gutenberg in case there's something we're missing.

I also still wonder if it would be better to move this caching strategy up to WP_Theme_JSON::get_styles_for_block or further up the call stack, but don't think that's a blocker to getting this improvement in for early testing.

I think this data belongs in the theme_json cache group, but that group is specifically marked to be non-persistent because of the man ways this data can become stale. Curious what @spacedmonkey thinks about alternatives for cache groups. here.

joemcgill · 2024-01-08T22:12:46Z

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

+		$args      = array( func_get_args(), wp_get_global_settings() ); // phpcs:ignore PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed
+		$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $args ) );


Suggestion/nitpick: rename $args to $cach_args for readability.

Suggested change

$args = array( func_get_args(), wp_get_global_settings() ); // phpcs:ignore PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed

$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $args ) );

$cache_args = array( func_get_args(), wp_get_global_settings() ); // phpcs:ignore PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed

$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $cache_args ) );

joemcgill · 2024-01-08T22:18:51Z

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

+		// The global settings can include dynamic data related to typography. We need evaluate it so that the cache is invalidated when it changes.
+		$args      = array( func_get_args(), wp_get_global_settings() ); // phpcs:ignore PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed
+		$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $args ) );
+		$cache     = wp_cache_get( $cache_key, 'wp-styles' );


Rather than introducing a new cache group, I think this should be a part of the existing theme_json group. Would need to change it below where this cache value is set, as well.

Suggested change

$cache = wp_cache_get( $cache_key, 'wp-styles' );

$cache = wp_cache_get( $cache_key, 'theme_json' );

joemcgill · 2024-01-08T22:18:54Z

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

@@ -2000,6 +2009,8 @@ protected static function compute_style_properties( $styles, $settings = array()
 			}
 		}

+		wp_cache_set( $cache_key, $declarations, 'wp-styles', DAY_IN_SECONDS );


I'm unsure about the best expiration time here. If there is a side effect that we've not accounted for in the cache key value, showing stale styles for a full day without flushing the cache is pretty long. Maybe limiting this to once an hour for now would be safer?

Suggested change

wp_cache_set( $cache_key, $declarations, 'wp-styles', DAY_IN_SECONDS );

wp_cache_set( $cache_key, $declarations, 'theme_json', HOUR_IN_SECONDS );

Theme json is a none presient cache, so there is no need for an expiry here.

tellthemachines · 2024-01-10T05:37:19Z

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.

andrewserong · 2024-01-10T05:55:39Z

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: #5567 (comment) 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 🤔

ramonjd · 2024-01-11T05:07:31Z

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., WordPress/gutenberg#45543

oandregal · 2024-01-15T08:42:25Z

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

@@ -1934,6 +1934,15 @@ protected static function compute_style_properties( $styles, $settings = array()
 			return $declarations;
 		}

+		// The global settings can include dynamic data related to typography. We need evaluate it so that the cache is invalidated when it changes.
+		$args      = array( func_get_args(), wp_get_global_settings() ); // phpcs:ignore PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed


The way the theme.json code is organized is as follows:

WP_Theme_JSON: private class that deals with a theme.json structure. It doesn't depend on anything else.

WP_Theme_JSON_Resolver: private class that pulls data (theme.json files, theme.json from database) and merges it together appropriately (get_merged_data). It depends on the WP_Theme_JSON class.

Public API: functions such as wp_get_global_settings, wp_get_global_styles, etc. They use and depend on the two previous private classes.

By using a public function such as wp_get_global_settings in the WP_Theme_JSON class, this code is creating a circular dependency. This code is saying: when processing any theme.json structure, read all existing theme.json (core, blocks, theme, user) first.

Thanks for the feedback @oandregal. This makes perfect sense. I think we would either need to add the caching to the public function that relies on WP_Theme_JSON::compute_style_properties or find a different approach to invalidation within WP_Theme_JSON.

Thanks for the feedback @oandregal. I'll have a closer look at this

oandregal

I've left a comment https://github.com/WordPress/wordpress-develop/pull/5567/files#r1452068125 explaining my reasoning.

kamranzafar4343

looks fine to me

pereirinha added 3 commits October 24, 2023 23:05

Add object caching for WP_Theme_JSON::compute_style_properties

85e9170

The values of the args are only useful for the cache key

7450d26

Deal with cache before updating $properties

c9b36ea

mukeshpanchal27 reviewed Oct 31, 2023

View reviewed changes

mukeshpanchal27 requested review from joemcgill and felixarntz October 31, 2023 09:25

joemcgill requested changes Oct 31, 2023

View reviewed changes

Only use cache if not in dev mode for theme. Ensure we do not process…

dd8c83e

… empty `$styles`

pereirinha commented Nov 3, 2023

View reviewed changes

Address PHPCS warning. It really does not matter if the value of `$pr…

7d97433

…operties` changes

pereirinha requested review from joemcgill and mukeshpanchal27 November 3, 2023 10:34

mukeshpanchal27 suggested changes Nov 6, 2023

View reviewed changes

felixarntz requested changes Dec 14, 2023

View reviewed changes

spacedmonkey reviewed Jan 3, 2024

View reviewed changes

Merge branch 'WordPress:trunk' into add/object-cache-compute-style-pr…

bd6f08c

…operties

pereirinha added 2 commits January 4, 2024 01:04

Remove unecessary dev mode check for cache

2377348

Add the global settings to the cache key recipe, so that when it chan…

d707a79

…ges, it invalidates the cache

pereirinha requested review from felixarntz and mukeshpanchal27 January 4, 2024 01:20

joemcgill reviewed Jan 8, 2024

View reviewed changes

oandregal reviewed Jan 15, 2024

View reviewed changes

oandregal requested changes Jan 15, 2024

View reviewed changes

kamranzafar4343 reviewed Jan 18, 2024

View reviewed changes

kt-12 mentioned this pull request Apr 15, 2024

Object caching WP_Theme_JSON::compute_style_properties #6392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add object caching for `WP_Theme_JSON::compute_style_properties` #5567

Add object caching for `WP_Theme_JSON::compute_style_properties` #5567

pereirinha commented Oct 24, 2023 •

edited

Loading

mukeshpanchal27 left a comment

mukeshpanchal27 Oct 31, 2023

pereirinha Oct 31, 2023

mukeshpanchal27 commented Oct 31, 2023

pereirinha commented Oct 31, 2023

joemcgill left a comment

joemcgill Oct 31, 2023

mukeshpanchal27 Nov 1, 2023

joemcgill Nov 1, 2023

pereirinha Nov 3, 2023

mukeshpanchal27 commented Nov 1, 2023

pereirinha left a comment

pereirinha Oct 31, 2023

pereirinha Nov 3, 2023

mukeshpanchal27 Nov 6, 2023

felixarntz Dec 14, 2023

pereirinha Jan 4, 2024

spacedmonkey left a comment

github-actions bot commented Jan 4, 2024

pereirinha commented Jan 4, 2024

pereirinha commented Jan 4, 2024

joemcgill left a comment

joemcgill Jan 8, 2024

joemcgill Jan 8, 2024

joemcgill Jan 8, 2024

spacedmonkey Feb 5, 2024

tellthemachines commented Jan 10, 2024

andrewserong commented Jan 10, 2024

ramonjd commented Jan 11, 2024

oandregal Jan 15, 2024 •

edited

Loading

joemcgill Jan 17, 2024

pereirinha Jan 17, 2024

oandregal left a comment •

edited

Loading

kamranzafar4343 left a comment

		$args = array( func_get_args(), wp_get_global_settings() ); // phpcs:ignore PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed
		$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $args ) );

	$cache = wp_cache_get( $cache_key, 'wp-styles' );
	$cache = wp_cache_get( $cache_key, 'theme_json' );

	wp_cache_set( $cache_key, $declarations, 'wp-styles', DAY_IN_SECONDS );
	wp_cache_set( $cache_key, $declarations, 'theme_json', HOUR_IN_SECONDS );

Add object caching for WP_Theme_JSON::compute_style_properties #5567

Are you sure you want to change the base?

Add object caching for WP_Theme_JSON::compute_style_properties #5567

Conversation

pereirinha commented Oct 24, 2023 • edited Loading

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukeshpanchal27 commented Oct 31, 2023

pereirinha commented Oct 31, 2023

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukeshpanchal27 commented Nov 1, 2023

pereirinha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacedmonkey left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 4, 2024

Test using WordPress Playground

Some things to be aware of

pereirinha commented Jan 4, 2024

pereirinha commented Jan 4, 2024

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tellthemachines commented Jan 10, 2024

andrewserong commented Jan 10, 2024

ramonjd commented Jan 11, 2024

oandregal Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal left a comment • edited Loading

Choose a reason for hiding this comment

kamranzafar4343 left a comment

Choose a reason for hiding this comment

Add object caching for `WP_Theme_JSON::compute_style_properties` #5567

Add object caching for `WP_Theme_JSON::compute_style_properties` #5567

pereirinha commented Oct 24, 2023 •

edited

Loading

oandregal Jan 15, 2024 •

edited

Loading

oandregal left a comment •

edited

Loading