Persistently cache theme.json files via WP Cache API#56095
Persistently cache theme.json files via WP Cache API#56095felixarntz wants to merge 5 commits intotrunkfrom
theme.json files via WP Cache API#56095Conversation
|
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 |
| /** | ||
| * 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(); | ||
|
|
There was a problem hiding this comment.
We might need to keep them if someone is extending the class and expects these to exist.
There was a problem hiding this comment.
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).
Nice find! I always appreciate a bug fix :) Are there any tests for this?
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. |
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; |
There was a problem hiding this comment.
Can we use $i18n_schema = ( null === $i18n_schema ) ? array() : $i18n_schema;? Wrap condition in the brackets. So it can be a good readable format.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
@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 thetheme_jsoncache group), plus it needs to be persistent to be beneficial (i.e. unlike thethemescache group). - One caveat is that currently this cache is also used for
theme.jsonfiles that are not from a theme. Hence thetheme_json_filescache group name makes sense here, whereas using a broadertheme_filescache group name would be misleading in that sense, as core'stheme.jsonis not really a theme file.
I think we have 3 options here:
- Change this to become part of the
theme_filescache group, and acknowledge that that cache group has a special case for WP core's own "fallback" theme files (specifically in this case, thetheme.jsonfile). In that case, the only other change needed here would be to adjust the cache key names so that they include a mention oftheme_json, to clarify that these are cache keys for atheme.jsonfile, rather than another file from a theme. - 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.
- Combine this with what we've so far considered under the
theme_filescache 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 liketheme_{themeSlug}, so that it could also cover files that are in core, or in theory even plugins.
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.jsonfiles (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.jsondata may be attempted to be translated with the incorrect text domain, in case a child theme is used which doesn't have its owntheme.json. In that case, the parent theme'stheme.jsoncontent would be translated with the child theme text domain, which would effectively lead to untranslated content for the entiretheme.jsonfile.As a side effect from that enhancement, the internal class properties
$i18n_schemaand$theme_json_file_cacheare 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.zipwithhttps://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.