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.

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

Persistently cache theme.json files via WP Cache API #56095

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

Conversation

felixarntz
Copy link
Member

@felixarntz felixarntz commented Nov 14, 2023

What?

This ports the WP core PR WordPress/wordpress-develop#5267 to Gutenberg, as the relevant change should be implemented here first. Related Trac ticket: https://core.trac.wordpress.org/ticket/57789

The PR adds persistent object caching (where supported) via the WP Cache API around the theme.json files (and the related i18n schema. This improves performance notably for sites using a persistent object cache.

Why?

Getting the file contents and JSON-decoding them is a costly operation that happens on every page load. By caching the results we can cut down WordPress's server response time.

Benchmarks using https://github.com/swissspidy/compare-wp-performance confirm the positive impact on performance. Since our normal performance benchmarking workflows don't come with a persistent object cache enabled and thus would be useless to assess the impact of this change, I used that project, with a custom-built WordPress 6.4.1 ZIP that is exactly the same as the real WordPress 6.4.1, except that it has the changes from this PR appied.

Here are the benchmarking results for the most relevant metric, Server-Timing wp-total (which corresponds to overall WordPress response time):

The control runs https://github.com/felixarntz/compare-wp-performance/actions/runs/6857231774 for Twenty Twenty-Three and https://github.com/felixarntz/compare-wp-performance/actions/runs/6857233957 for Twenty Twenty-One do not show the improvement, as expected, as these runs were made without a persistent object cache enabled.

How?

The PR uses the WP Cache API to avoid getting and decoding these JSON file contents, using a new cache group theme_json_files. All cache keys include a relevant version string (e.g. the current core version or theme version), as a means to automatically invalidate the cache when a relevant update is made.

As part of that change, it also fixes a bug: As of today, the theme's theme.json data may be attempted to be translated with the incorrect text domain, in case a child theme is used which doesn't have its own theme.json. In that case, the parent theme's theme.json content would be translated with the child theme text domain, which would effectively lead to untranslated content for the entire theme.json file.

As a side effect from that enhancement, the internal class properties $i18n_schema and $theme_json_file_cache are no longer needed, as they are effectively replaced by the WP Cache API usage.

Testing Instructions

There is no relevant testing for this, except using a site with a persistent object cache enabled (e.g. Redis or Memcached) and checking that there is no breakage.

Regarding the benchmarks shared above, you can replicate them yourself by using the same workflow, comparing https://wordpress.org/wordpress-6.4.1.zip with https://github.com/felixarntz/compare-wp-performance/raw/custom-wp-builds/wordpress-6.4.1-theme-json-resolver-cache.zip, with the checkbox for Memcached checked. Note that the numbers won't be exactly the same since there is always a bit of variance.

@felixarntz felixarntz added [Type] Bug An existing feature does not function as intended [Type] Performance Related to performance efforts [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Needs PHP backport Needs PHP backport to Core labels Nov 14, 2023
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/class-wp-theme-json-resolver-gutenberg.php

@felixarntz felixarntz removed the [Type] Bug An existing feature does not function as intended label Nov 14, 2023
Comment on lines -80 to -96
/**
* Container to keep loaded i18n schema for `theme.json`.
*
* @since 5.8.0 As `$theme_json_i18n`.
* @since 5.9.0 Renamed from `$theme_json_i18n` to `$i18n_schema`.
* @var array
*/
protected static $i18n_schema = null;

/**
* `theme.json` file cache.
*
* @since 6.1.0
* @var array
*/
protected static $theme_json_file_cache = array();

Copy link
Member

Choose a reason for hiding this comment

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

We might need to keep them if someone is extending the class and expects these to exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that matters here as the class is marked private. It must not be overwritten for this purpose, and if anyone does so it's at their own risk (similar to e.g. using "experimental" APIs in Gutenberg).

@swissspidy
Copy link
Member

As part of that change, it also fixes a bug: As of today, the theme's theme.json data may be attempted to be translated with the incorrect text domain

Nice find! I always appreciate a bug fix :) Are there any tests for this?

Getting the file contents and JSON-decoding them is a costly operation that happens on every page load. By caching the results we can cut down WordPress's server response time.

Regarding performance tests, just a note that it might be worth looking into phpbench for more lower-level benchmarks to better compare the performance difference.

@felixarntz
Copy link
Member Author

As part of that change, it also fixes a bug: As of today, the theme's theme.json data may be attempted to be translated with the incorrect text domain

Nice find! I always appreciate a bug fix :) Are there any tests for this?

There aren't at this point. I think it's worth adding a test in core once we get there, but the Gutenberg test setup will require additional hoops, where I don't think it's worth spending time on, given that we would then have to rewrite the test for core eventually. It's a pretty basic bug fix that you can tell from the code has been fixed.


if ( false === $i18n_schema ) {
$i18n_schema = wp_json_file_decode( __DIR__ . '/theme-i18n.json' );
$i18n_schema = null === $i18n_schema ? array() : $i18n_schema;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use $i18n_schema = ( null === $i18n_schema ) ? array() : $i18n_schema;? Wrap condition in the brackets. So it can be a good readable format.

Copy link
Member Author

Choose a reason for hiding this comment

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

@krupal-panchal I don't think it's needed for a single condition like this? I'm all for readability, but I personally find parentheses like this only helpful if there's more than one conditions in a ternary operator.

Also worth noting this line is already in WordPress core in exactly the same format, so I'm not sure we should change it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, not an issue. LGTM also.

if ( $file_path ) {
if ( array_key_exists( $file_path, static::$theme_json_file_cache ) ) {
return static::$theme_json_file_cache[ $file_path ];
$cache_group = 'theme_json_files';
Copy link
Member Author

@felixarntz felixarntz Jan 23, 2024

Choose a reason for hiding this comment

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

@joemcgill Related to https://core.trac.wordpress.org/ticket/59719#comment:18, what is your thought on this?

  • The PR currently uses theme_json_files, yet another cache group name than what we've talked about in that ticket so far.
  • Out of the 3 names we've been discussing there, this fits best into theme_files, as it's global data and the cache keys already include the theme slug. It's also exclusively about data from the filesystem (i.e. unlike the theme_json cache group), plus it needs to be persistent to be beneficial (i.e. unlike the themes cache group).
  • One caveat is that currently this cache is also used for theme.json files that are not from a theme. Hence the theme_json_files cache group name makes sense here, whereas using a broader theme_files cache group name would be misleading in that sense, as core's theme.json is not really a theme file.

I think we have 3 options here:

  1. Change this to become part of the theme_files cache group, and acknowledge that that cache group has a special case for WP core's own "fallback" theme files (specifically in this case, the theme.json file). In that case, the only other change needed here would be to adjust the cache key names so that they include a mention of theme_json, to clarify that these are cache keys for a theme.json file, rather than another file from a theme.
  2. Keep this as is. Probably best from a separation of concerns and understandability perspective, but yet another cache group related to theme data may seem excessive.
  3. Combine this with what we've so far considered under the theme_files cache group, but use another name that is broadly about any files. In that case, the cache keys would not only contain the theme slug, but also some prefix like theme_{themeSlug}, so that it could also cover files that are in core, or in theory even plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Needs PHP backport Needs PHP backport to Core [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants