Editor: Cache merged global styles in the theme_json group - #94
Draft
sirreal wants to merge 3 commits into
Draft
Conversation
This reverts commit caf0fa0. The generation-counter approach did not resolve the underlying problem. `WP_Theme_JSON_Resolver::$cache_generation` was only incremented by `clean_cached_data()`, which is reachable only through `wp_clean_theme_json_cache()` on `switch_theme` and `start_previewing_theme`. The resolver also invalidates itself through `has_same_registered_blocks()`, and that path never touched the counter, so the layout snapshot and the resolver could still disagree: register_block_type( 'test/gap', array( 'supports' => array( 'layout' => true, '__experimentalStyle' => array( 'spacing' => array( 'blockGap' => '77px' ), ), ), ) ); After a layout block had already rendered, `wp_get_global_styles()` returned the fresh `77px` while `wp_render_layout_support_flag()` continued to emit the stale fallback. Two caches with two invalidation rules remained, which is the defect the change set out to remove. It also added `WP_Theme_JSON_Resolver::get_cache_generation()` as permanent public API to work around a private caching detail, and required the regression test to call `clean_cached_data()` directly, which tested the new mechanism rather than the rendered output. The following commit fixes the root cause instead.
`wp_get_global_styles()` is the only `theme_json` accessor without an object cache. Every call builds a fresh `WP_Theme_JSON` and merges four origins through `WP_Theme_JSON_Resolver::get_merged_data()`, at roughly 0.3ms per call. `wp_render_layout_support_flag()` worked around that cost with a function-static snapshot that nothing could invalidate, so a layout rendered after a theme change kept the previous `blockGap` value for the remaining lifetime of the PHP process. That is what forced the block style variation test to run in a separate process. Cache the merged styles in the non-persistent `theme_json` group, keyed by origin and by the `resolve-variables` transform, and clear those keys in `wp_clean_theme_json_cache()`, exactly as `wp_get_global_settings()` already does. The function-static in `wp_render_layout_support_flag()` is then unnecessary and is removed, so `switch_theme` refreshes layout styles and `@runInSeparateProcess` can be dropped from the test. `gallery.php` and `image.php` also call `wp_get_global_styles()` on every render with no cache of their own, and benefit as well. The function-static additionally ignored `wp_is_development_mode( 'theme' )`, so theme developers saw stale block gaps in layout output. The object cache respects that check. Measured on this branch, 1,000 iterations, median of five runs: wp_get_global_styles() before 311.61ms after 0.23ms wp_render_layout_support_flag() before 15.19ms after 15.73ms Trac ticket: https://core.trac.wordpress.org/ticket/65893
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
theme_jsonobject cache group, exactly aswp_get_global_settings()already doeswp_render_layout_support_flag()This PR is intentionally stacked on
agent/test-order-block-supports-isolation, the head branch of WordPress#13192. Merging it into that branch replaces the workaround before WordPress#13192 is merged into WordPress Core.Trac ticket: https://core.trac.wordpress.org/ticket/65893
Note
The generation-counter approach originally proposed here (caf0fa0) has been reverted in 49100aa and replaced by 54293ed. See Why the previous approach was reverted.
Root cause
wp_get_global_styles()is the onlytheme_jsonaccessor with no object cache.wp_get_global_settings(),wp_get_global_stylesheet(),wp_get_global_styles_svg_filters(), andwp_get_global_custom_css()all cache in the non-persistenttheme_jsongroup with keys cleared bywp_clean_theme_json_cache().wp_get_global_styles()instead callsWP_Theme_JSON_Resolver::get_merged_data(), which constructs a freshWP_Theme_JSONand merges four origins on every call, at roughly 0.3ms.wp_render_layout_support_flag()worked around that cost in [61513] with a function-static snapshot:Nothing can invalidate that snapshot.
WP_Theme_JSON_Resolver::clean_cached_data()correctly invalidated the resolver, but not this second cache, so a layout rendered after a theme or registered-style change kept the oldblockGapvalue for the rest of the PHP process. That is what forced@runInSeparateProcesson the regression test.The fix
Give
wp_get_global_styles()the same treatment aswp_get_global_settings(): cache the merged styles in the non-persistenttheme_jsongroup, keyed by origin and by theresolve-variablestransform, and clear those keys inwp_clean_theme_json_cache(). The function-static then has nothing left to do and is removed.The result is one cache with one invalidation rule instead of two that can disagree.
switch_themerefreshes layout styles, and the test needs no isolation annotations.Two incidental benefits:
gallery.phpandimage.phpalso callwp_get_global_styles()on every render with no cache of their own.wp_is_development_mode( 'theme' ), so theme developers saw stale block gaps in layout output. The object cache respects that check.Caching the derived array rather than the
WP_Theme_JSONobject is deliberate:WP_Theme_JSON::resolve_variables()mutates$theme_json->theme_json['styles']in place, so a shared cached instance would be corrupted by its own callers.Why the previous approach was reverted
The generation counter did not resolve the underlying problem.
WP_Theme_JSON_Resolver::$cache_generationwas only incremented byclean_cached_data(), reachable solely throughwp_clean_theme_json_cache()onswitch_themeandstart_previewing_theme. The resolver also invalidates itself throughhas_same_registered_blocks(), and that path never touched the counter.Probing the branch at caf0fa0, after a layout block had already rendered:
wp_get_global_styles()returned the fresh'77px'wp_render_layout_support_flag()still emitted the stale fallback1.2remTwo caches with two invalidation rules still disagreed, which is the defect the change set out to remove. It also introduced
WP_Theme_JSON_Resolver::get_cache_generation()as permanent public API to work around a private caching detail, and required the regression test to callclean_cached_data()inset_up()and mid-test, so the test asserted that the new mechanism worked rather than that the rendered output was correct.Known limitation
This does not close the
has_same_registered_blocks()gap either: a block type registered after the first access still is not reflected, and the probe above reports1.2remunder this branch too. The difference is that there is now a single cache with a single invalidation rule, identical to the onewp_get_global_settings()has used since 6.2, rather than two caches that can diverge.Closing that gap means flushing the
theme_jsongroup when block types are registered, which would fixwp_get_global_settings()as well. That belongs on its own ticket.Performance
1,000 iterations, median of five runs, measured on this branch against its base:
wp_get_global_styles()wp_render_layout_support_flag()The render path pays about 0.5µs per call for the object cache lookup, and every other caller of
wp_get_global_styles()gets three orders of magnitude.Test evidence
1787163001failed because the generated CSS used24pxinstead of99px1787163001after the fix: 400 tests, 532 assertions, passing, with no isolation annotations--group=block-supportspasses, seeds1789000001through1789000050: 0 failures--group=block-supports: 400 tests, 532 assertions, passinggit diff --checkpassedDiff size
Net effect against the base branch is 44 insertions and 12 deletions, of which the test change is two deleted annotations. No changes to
WP_Theme_JSON_Resolver.