Conversation
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
joemcgill
left a comment
There was a problem hiding this comment.
I like the idea here, but I worry about the memory consumption since we end up duplicating the theme.json data in the object cache (when pulled from the transient) and also in static methods of this object.
I think this could be cleaner by making the existing static properties an implementation detail of a generic cache getter and replacing the checks for the static properties with checks for a static value. Here's a pseudocode example:
// In get_core_data().
public static function get_core_data() {
$cached = get_cached_data( 'core' );
if ( null !== $cached && static::has_same_registerd_blocks( 'core' ) ) {
return $cached;
}
// Do non-cached logic here.
}
// Getting cached data.
private static function get_cached_data( $key ) {
$can_use_cached = ! wp_is_development_mode( 'theme' );
// Return cache from the transient if possible.
if ( $can_use_cached ) {
$cached = get_transient( 'wp_theme_json_resolver_cache' );
return $cached[ $key ] ?? false;
}
// Return false for an empty value.
return self::${ $key } ?: false;
}| $cache = array(); | ||
| } | ||
| $cache[ $cache_key ] = $value; | ||
| return set_site_transient( 'wp_theme_json_resolver_cache', $cache, 10 * MINUTE_IN_SECONDS ); |
There was a problem hiding this comment.
Since this transient is storing a combination of core data, theme data, and registered bocks, I think the will need to be site specific and use set|get_transient() rather than network wide (e.g., set|get_site_transient()) functions.
We should resolve the open questions about transients usage in this comment before using a similar approach here.
🔢 Applies throughout.
| $cache_value = static::get_cache_data( $cache_key ); | ||
| if ( $cache_value ) { | ||
| // Defined in this function below. | ||
| $theme_json = apply_filters( 'wp_theme_json_data_default', $cache_value ); |
There was a problem hiding this comment.
Any time we're passing theme.json data through a filter, or pulling from cache, I think we need to make sure it's been validated using the current WP_Theme_JSON schema expected by the application, similar to what we did in WordPress/wordpress-develop@6ae489c, which hasn't been backported to this class yet.
In fact, it would probably be safer to cache the raw data (e.g. WP_Theme_JSON_Data_Gutenberg::get_data() )to ensure that the first time we're repopulating the data from cache, that we're passing it through the WP_Theme_JSON_Data_Gutenberg() constructor. That might negate the performance benefits by avoiding the initial WP_Theme_JSON_Gutenberg construction, but would good to check.
There was a problem hiding this comment.
We can try caching get_data, but it will negate all our effort as most of the performance regression came from WP_Theme_JSON_Data_Gutenberg constructor call.
For the other part, should we wait for the backporting, or should I copy past that here? I was avoiding it here because merging might become complicated. I have handled the merge for core PR.
There was a problem hiding this comment.
most of the performance regression came from WP_Theme_JSON_Data_Gutenberg constructor call
I thought that might be the case.
Let's handle backporting WordPress/wordpress-develop@6ae489c separately. I'll get a PR open.
https://core.trac.wordpress.org/ticket/59600
What?
Caching persistently inside
get_theme_dataandget_core_datais estimated to provide a performance benefit of at least ~2%. Block data fromthemeandcoreare relatively static, so they can be persistently cached for a shorter period.Why?
For faster load time.
How?
By caching and skipping constructor calls to
WP_Theme_JSON | WP_Theme_JSON_Gutenberg, which is costly.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast