Store merged Theme JSON data in memory. #54742

joemcgill · 2023-09-22T16:33:17Z

What?

This adds an additional static cache to WP_Theme_JSON_Resolver_Gutenberg that stores the merged data for each origin in memory to avoid recreating unnecessary WP_Theme_JSON_Gutenberg objects each time a function needs to get Theme JSON data.

Why?

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

How?

Testing Instructions

Running a profiler, like XHProf on a pageload before and after applying this change should demonstrate a reduction in calls to each of the getter methods for each origin, along with a reduction in merges. See example from the core repo: https://core.trac.wordpress.org/ticket/57789#comment:23

Testing Instructions for Keyboard

n/a

Screenshots or screencast

This adds an additional static cache to `WP_Theme_JSON_Resolver_Gutenberg` that stores the merged data for each origin in memory to avoid recrating unnecessary `WP_Theme_JSON_Gutenberg` objects each time a function needs to get Theme JSON data. See https://core.trac.wordpress.org/ticket/57789

joemcgill · 2023-09-22T16:35:53Z

These changes were originally made in WordPress/wordpress-develop#5024. This applies the latest PR feedback and makes changes to the Gutenberg repo so we could test this in the plugin prior to a core merge.

felixarntz

Per my previous feedback on WordPress/wordpress-develop#5024, LGTM!

oandregal · 2023-10-03T08:30:30Z

lib/class-wp-theme-json-resolver-gutenberg.php

+		}
+
+		// Map origins to block cache values.
+		$cache_map = array(


What if we update the $blocks_cache to have the proper keys (default instead of core, and custom instead of user) so we don't need this?

@oandregal While I agree this would be ideal, I think this should be subject to another separate issue/PR. There may be backward compatibility implications from changing the cache names - probably not, but that would be a fix not really related to what this PR is aiming to achieve.

If there's a path to making these consistent, I'm all for it, but was out of scope of what I was trying to show here, so I just worked with what existed.

oandregal · 2023-10-03T08:34:14Z

lib/class-wp-theme-json-resolver-gutenberg.php

@@ -597,26 +634,58 @@ public static function get_merged_data( $origin = 'custom' ) {
 			_deprecated_argument( __FUNCTION__, '5.9.0' );
 		}

+		if ( ! in_array( $origin, WP_Theme_JSON_Gutenberg::VALID_ORIGINS, true ) ) {


Do we need this? Doesn't it work the same as it works today?

Technically, I agree it's not needed now more than it was needed before, but it's a good practice to have. We should make sure developers passing invalid values to the function either explicitly fails or "forces" a useful result. Having this here ensures the latter.

This is just a bit of defensive coding to make the rest of the logic more predictable if someone passes an invalid origin value. I added it as a best practice to avoid potential bugs and edge cases.

oandregal · 2023-10-03T08:55:12Z

lib/class-wp-theme-json-resolver-gutenberg.php

+			null !== static::$merged[ $origin ]
+			&& static::has_same_registered_blocks( $cache_map[ $origin ] )
+			// Ensure theme supports data is fresh before returning cached data for theme and custom origins.
+			&& ( ! in_array( $origin, array( 'theme', 'custom' ), true ) || static::get_theme_supports_data() === static::$theme_support_data )


I presume the logic above to convert unknown origins to custom was necessary for this line. Thinking we can do this instead:

// theme and custom origins require access to theme supports,
// so the cached merged data will only be used for those origins if the current supports are equal to the cached ones.
( in_array( $origin, array( 'default', 'blocks' ), true ) || static::get_theme_supports_data() === static::$theme_support_data )

This is one of the places where an invalid $origin value would be problematic, but there could be others. I considered writing this as you suggested, but decided it was likely better to explicitly check for the origin values where the theme supports data check is required, rather than the inverse. Both get the same result though.

oandregal

I left some minor comments – definitely not something that blocks the PR from landing.

In terms of failure scenarios, I cannot come up with any, so this change is good.

I'm conflicted about whether it makes sense to do it, though. Looking at the performance analysis you've posted here and here, the user metrics for velocity don't seem to be impacted. Though, it does double the memory consumed by caching both the single data origin (e.g., theme) and the merged data origin (e.g., up to theme). I feel the memory-velocity trade-off is a bit unbalanced (much memory for no gains in velocity).

At the same time, I reckon that some other scenarios we are not testing may have many more nodes (settings/styles/blocks in theme.json), hence, they could see bigger wins.

I'm on the fence about this one. You have been the most invested in this research, so it's only fair that it's you who decide whether it makes sense or not. In any case, I appreciate the time and commitment to investigate it. Thanks!

felixarntz · 2023-10-03T14:03:52Z

@oandregal I agree this ends up being more of a micro optimization, but I don't see any harm in implementing it. To me it makes sense to have a merged cache alongside the individual caches. Generally, I am more on the side of prioritizing performance (time) over memory (unless of course the tradeoff is large/problematic).

joemcgill

Thanks for the review, @oandregal. I'm similarly on the fence about whether this change is worth making. It does lead to some improvements by avoiding repeated calls to get the data from each origin and merge them together, but most of the time spent in all scenarios is in the initial calculation for each origin rather than the repeated calls.

That said, the most impactful change will be if we can reliably add some persistence to the caching strategy for either the merged data or individual origins and having this in place to start testing can help us discover potential issues with a non-persistent cache prior to introducing any persistence, so it may be worth testing as an experiment to discover scenarios where cache invalidation needs to be considered in the future.

joemcgill · 2023-10-03T21:52:12Z

lib/class-wp-theme-json-resolver-gutenberg.php

@@ -597,26 +634,58 @@ public static function get_merged_data( $origin = 'custom' ) {
 			_deprecated_argument( __FUNCTION__, '5.9.0' );
 		}

+		if ( ! in_array( $origin, WP_Theme_JSON_Gutenberg::VALID_ORIGINS, true ) ) {


This is just a bit of defensive coding to make the rest of the logic more predictable if someone passes an invalid origin value. I added it as a best practice to avoid potential bugs and edge cases.

joemcgill · 2023-10-03T21:56:59Z

lib/class-wp-theme-json-resolver-gutenberg.php

+			null !== static::$merged[ $origin ]
+			&& static::has_same_registered_blocks( $cache_map[ $origin ] )
+			// Ensure theme supports data is fresh before returning cached data for theme and custom origins.
+			&& ( ! in_array( $origin, array( 'theme', 'custom' ), true ) || static::get_theme_supports_data() === static::$theme_support_data )


This is one of the places where an invalid $origin value would be problematic, but there could be others. I considered writing this as you suggested, but decided it was likely better to explicitly check for the origin values where the theme supports data check is required, rather than the inverse. Both get the same result though.

joemcgill · 2024-04-22T20:04:56Z

Closing for now as this didn't seem to have a bit impact on overall performance.

joemcgill requested a review from oandregal September 22, 2023 16:33

joemcgill requested a review from spacedmonkey as a code owner September 22, 2023 16:33

joemcgill added the [Type] Performance Related to performance efforts label Sep 22, 2023

joemcgill added the Needs PHP backport Needs PHP backport to Core label Sep 22, 2023

joemcgill mentioned this pull request Sep 22, 2023

Store merged data in memory WordPress/wordpress-develop#5024

Closed

felixarntz approved these changes Sep 22, 2023

View reviewed changes

oandregal reviewed Oct 3, 2023

View reviewed changes

joemcgill commented Oct 3, 2023

View reviewed changes

joemcgill closed this Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store merged Theme JSON data in memory. #54742

Store merged Theme JSON data in memory. #54742

joemcgill commented Sep 22, 2023 •

edited

Loading

joemcgill commented Sep 22, 2023

felixarntz left a comment

oandregal Oct 3, 2023

felixarntz Oct 3, 2023

joemcgill Oct 3, 2023

oandregal Oct 3, 2023

felixarntz Oct 3, 2023

joemcgill Oct 3, 2023

oandregal Oct 3, 2023

joemcgill Oct 3, 2023

oandregal left a comment

felixarntz commented Oct 3, 2023

joemcgill left a comment

joemcgill Oct 3, 2023

joemcgill Oct 3, 2023

joemcgill commented Apr 22, 2024

Store merged Theme JSON data in memory. #54742

Store merged Theme JSON data in memory. #54742

Conversation

joemcgill commented Sep 22, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

joemcgill commented Sep 22, 2023

felixarntz 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

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

Choose a reason for hiding this comment

felixarntz commented Oct 3, 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

joemcgill commented Apr 22, 2024

joemcgill commented Sep 22, 2023 •

edited

Loading