Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add object caching for WP_Theme_JSON::compute_style_properties #5567

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from
Open

Add object caching for WP_Theme_JSON::compute_style_properties #5567

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
85e9170
Add object caching for `WP_Theme_JSON::compute_style_properties`
pereirinha Oct 24, 2023
7450d26
The values of the args are only useful for the cache key
pereirinha Oct 24, 2023
c9b36ea
Deal with cache before updating `$properties`
pereirinha Oct 24, 2023
dd8c83e
Only use cache if not in dev mode for theme. Ensure we do not process…
pereirinha Nov 3, 2023
7d97433
Address PHPCS warning. It really does not matter if the value of `$pr…
pereirinha Nov 3, 2023
bd6f08c
Merge branch 'WordPress:trunk' into add/object-cache-compute-style-pr…
pereirinha Jan 4, 2024
2377348
Remove unecessary dev mode check for cache
pereirinha Jan 4, 2024
d707a79
Add the global settings to the cache key recipe, so that when it chan…
pereirinha Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/wp-includes/class-wp-theme-json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1934,6 +1934,16 @@ protected static function compute_style_properties( $styles, $settings = array()
return $declarations;
}

$can_use_cached = ! wp_is_development_mode( 'theme' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


$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' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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' );


if ( $can_use_cached && $cache ) {
return $cache;
}

$root_variable_duplicates = array();

foreach ( $properties as $css_property => $value_path ) {
Expand Down Expand Up @@ -2000,6 +2010,10 @@ protected static function compute_style_properties( $styles, $settings = array()
}
}

if ( $can_use_cached ) {
wp_cache_set( $cache_key, $declarations, 'wp-styles', DAY_IN_SECONDS );
}

return $declarations;
}

Expand Down