Caching inside `get_theme_data` and `get_core_data` #6781

kt-12 · 2024-06-11T14:57:40Z

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

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-06-11T14:58:28Z

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, stellastopfer.

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

github-actions · 2024-06-11T15:11:02Z

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.

joemcgill

Thanks @kt-12. I think loading the initial theme data from a transient could be a nice improvement, but at present, the potential gains are offset by the fact that transients with an expiration time do not get autoloaded, which results in this PR adding additional DB queries to each request.

Given that we really only need to invalidate the transient if the theme's theme.json data changes, I wonder if we can include a hash of the data just before it's passed to the wp_theme_json_data_theme in the cached data to use for invalidation, instead of the functional args and avoid the transient timeout alltogether?

Aside, this change should happen first in the Gutenberg repo and then get synced back to WP core.

joemcgill · 2024-06-11T21:00:28Z

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


+		$can_use_cached = ! wp_is_development_mode( 'theme' );
+		if ( $can_use_cached ) {
+			$cache_key = 'get_theme_data_cache_' . md5( wp_json_encode( func_get_args() ) );


I don't think using func_get_args here is necessary, because the theme data get's cached prior to when the with_supports option is calculated.

I have used $options instead in the new update as we don't quite need other parameters. At the moment set_transient is getting called on every run which is the reason why we don't see any benefit. Comparison of two static variables is not working quite well also other interesting find (off topic) I am able to serialize those variables but not convert into JSON

@joemcgill You were correct we don't need $options too.

…57789

…ub.com/10up/wordpress-develop into enhancement/cache_get_theme_data_57789

joemcgill

@kt-12 I've left a bunch of comments inline, about the specifics of this PR, but overall I have a few concerns:

First, the scope of this PR has now grown well beyond the original purpose, which was to make Theme JSON data from the current theme persistent, and seems to extend it to core data and block data as well. I'm not opposed to trying to add persistence to all three origins, but it does make cache invalidation a bit trickier, which is why we were originally talking about trying to handle these one case at a time—starting with just core and theme data. If there is value in handling all of these cases in the same PR, can you please update the PR title and provide some more context explaining why the scope of the PR has changed?

Second, I think that you can probably simplify a lot of the implementation and instead convert many of the static properties in this class to transients rather than duplicating cached data as a private static property and also as a transient. The easiest way to go about that might be to convert the *_from_persistent_cache() methods to general use get|set_cached_data() methods that populate the static values from persistent cache when $can_use_cached() is true.

Finally, one of the original reasons that these caches were made non-persistent is because it would cause the wp_theme_json_data_default and wp_theme_json_data_theme filters to get skipped. It seems like this approach will still suffer from that problem. I would love to see what the performance benefit would be of storing the theme.json config after it has been processed, but before they are passed to the relevant filters.

joemcgill · 2024-06-13T18:45:12Z

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

-		if ( null === static::$theme || ! static::has_same_registered_blocks( 'theme' ) ) {
+		$can_use_cached = ! wp_is_development_mode( 'theme' );
+		if ( $can_use_cached ) {
+			$cache_key = 'get_theme_data_cache_' . md5( wp_json_encode( $options ) );


I still don't think you need to include the options in the cache key here. $options are only used in this function to check for whether theme data should be returned with_supports, meaning whether classic theme theme supports should be merged into the final theme data. However, we're only caching the data from theme.json before it gets merged with theme supports data, so this options shouldn't affect our cache.

joemcgill · 2024-06-13T18:49:04Z

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

+		}
+
+		$has_theme_changed          = false;
+		$has_same_registered_blocks = $can_use_cached ? static::has_same_registered_blocks( 'theme', true ) : static::has_same_registered_blocks( 'theme' );


This could be simpler

Suggested change

$has_same_registered_blocks = $can_use_cached ? static::has_same_registered_blocks( 'theme', true ) : static::has_same_registered_blocks( 'theme' );

$has_same_registered_blocks = static::has_same_registered_blocks( 'theme', $can_use_cached );

joemcgill · 2024-06-13T19:07:26Z

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

+	 * @return mixed The cache value.
+	 */
+	private static function get_from_persitent_cache( $cache_key ) {
+		if ( null === static::$persistent_all_cache ) {


The need to back transients with a static cache should be unnecessary because options get saved to the site-options cache after being fetched from the DB so calling get_site_transient() for the same key multiple times during the same pageload should only hit the DB once.

joemcgill · 2024-06-13T19:14:20Z

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

+	}
+
+	/**
+	 * Retrieves persistent cache data of this class for given key.


Update docblock description.

joemcgill · 2024-06-13T19:24:31Z

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

 	 * @return bool True on success, false otherwise.
 	 */
-	protected static function has_same_registered_blocks( $origin ) {
+	protected static function has_same_registered_blocks( $origin, $persistant_cache = false ) {


Juggling two different caches for registered blocks seems overly complex to me. What if we just converted the static $blocks_cache to an autoloaded transient (no expiration) that only gets used if $can_use_cached is true? We're already validating the cache at the point where this is called by checking the current WP_Block_Type_Registry for all registered blocks.

@joemcgill It was a bit of a design decision. I wanted theme data to be checked against the persistent list as we assume the blocks from the theme will not change before the expiry, whereas blocks from user can change dynamically at any instance.

$blocks_cache is just a static cache, and it contains blank arrays of origins in the first call.
If we use the original $blocks_cache the first time has_same_registered_blocks is called, it will always return false as initially $blocks_cache is set to blank arrays, and has_same_registered_blocks sees all blocks as new blocks.

Original has_same_registered_blocks can't be used with persistent cache inside get_theme_data as it will assume the block has changed and will update static::theme, thereby also calling set_site_transient().

Now I can't update $blocks_cache with precipitant value either, w.r.t value of $can_use_cached; as updating
$blocks_cache with persistent value will affect other origins, too, like user or block, which depend on change based on has_same_registered_blocks or not.

Let me check again to see if there is a scope for using just one variable.

joemcgill · 2024-06-13T19:27:26Z

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

 		static::$user                     = null;
 		static::$user_custom_post_type_id = null;
 		static::$i18n_schema              = null;
+		delete_persitent_cache();


Suggested change

delete_persitent_cache();

static::delete_persitent_cache();

joemcgill · 2024-06-13T19:29:14Z

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

 			'theme'  => array(),
 			'user'   => array(),
 		);
+		static::$persistent_blocks_cache  = null;


If we don't backup the persistent cache in a static property of this class, then we can remove this as well.

kt-12 · 2024-06-14T06:04:42Z

@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, giving no room to include filters before the cache. It's important to note that values are statically cached only 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.

kt-12 · 2024-06-18T14:38:05Z

@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 a common expiry of 10 minutes. My main aim was to have a single database call for all the cache rather than multiple database calls with different expiries.
Another point is we can't have no expiration for has_same_registered_blocks as it only takes new blocks into consideration. Since we are updating $block_cache itself, we are at the moment caching blocks from all origin. This means if a block that is present is removed at some point and then added back (very rare ) chances are we won't record that change.

kt-12 · 2024-06-24T14:20:41Z

@joemcgill GB issue for this task - WordPress/gutenberg#62794

sstopfer · 2024-07-08T15:41:17Z

@joemcgill Putting this on hold.

caching with get_theme_data

e3ed551

kt-12 changed the title ~~Caching with get_theme_data~~ Caching inside get_theme_data Jun 11, 2024

kt-12 marked this pull request as ready for review June 11, 2024 14:58

joemcgill assigned joemcgill and kt-12 Jun 11, 2024

kt-12 added 2 commits June 11, 2024 17:21

update cache key

7341269

change func_get_args with

8904be9

joemcgill requested changes Jun 11, 2024

View reviewed changes

kt-12 and others added 8 commits June 13, 2024 08:42

update cache logic

58b6d9b

keep singe cache for resolver class

06c95ee

core caching

b695581

Merge branch 'WordPress:trunk' into enhancement/cache_get_theme_data_…

03b1b98

…57789

Merge branch 'trunk' into enhancement/cache_get_theme_data_57789

1384fbe

fix key issue

ae79a08

Merge branch 'enhancement/cache_get_theme_data_57789' of https://gith…

b1ed4d2

…ub.com/10up/wordpress-develop into enhancement/cache_get_theme_data_57789

delete persistant cache

99b8c88

joemcgill requested changes Jun 13, 2024

View reviewed changes

kt-12 changed the title ~~Caching inside get_theme_data~~ Caching inside get_theme_data and get_core_data Jun 14, 2024

kt-12 added 9 commits June 18, 2024 08:16

remove caching from core_data

c50e423

cache key change

6022cc0

update cache logic

43e629e

fix static call

070aafb

remove cache

12f555e

remove static variable for cache

130a560

fix cache logic

800607e

change function names

04a5338

cache get core data

9e3cf64

Merge branch 'trunk' into enhancement/cache_get_theme_data_57789

46e8899

sstopfer mentioned this pull request Jul 8, 2024

Trac 57789: Make theme.json related caches persistent WordPress/performance#847

Open

	$has_same_registered_blocks = $can_use_cached ? static::has_same_registered_blocks( 'theme', true ) : static::has_same_registered_blocks( 'theme' );
	$has_same_registered_blocks = static::has_same_registered_blocks( 'theme', $can_use_cached );

Caching inside get_theme_data and get_core_data #6781

Are you sure you want to change the base?

Caching inside get_theme_data and get_core_data #6781

Conversation

kt-12 commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 11, 2024

Test using WordPress Playground

Some things to be aware of

Uh oh!

joemcgill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joemcgill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kt-12 Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kt-12 commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kt-12 commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kt-12 commented Jun 24, 2024

Uh oh!

sstopfer commented Jul 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Caching inside `get_theme_data` and `get_core_data` #6781

Caching inside `get_theme_data` and `get_core_data` #6781

kt-12 commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading

kt-12 Jun 14, 2024 •

edited

Loading

kt-12 commented Jun 14, 2024 •

edited

Loading

kt-12 commented Jun 18, 2024 •

edited

Loading