Improve performance of block style variation registration and retrieval

[aaronrobertshaw]

This ticket tracks the backporting of PHP files for the following Gutenberg updates:

• ​https://github.com/WordPress/gutenberg/pull/62529
• ​https://github.com/WordPress/gutenberg/pull/62610

These changes prevent running expensive tasks around parsing theme.json partial files unnecessarily.

This PR backports the PHP updates from:

• ​https://github.com/WordPress/gutenberg/pull/62529
• ​https://github.com/WordPress/gutenberg/pull/62610

The changes avoid repeatedly searching /styles directories, parsing theme.json partials, and translating them. The registration of custom block style variations will now exit early if the theme doesn't have a theme.json or partials directory.

Trac ticket: https://core.trac.wordpress.org/ticket/61451

I've scoped down this backport to only the changes in ​https://github.com/WordPress/gutenberg/pull/62529 The other PR needs more time to clarify questions/address feedback.

[oandregal]

In 58423:

Global Styles: Avoid unnecessary processing of theme.json variation partials.

Props aaronrobertshaw, oandregal, mukesh27.
See #61451.

Committed at https://core.trac.wordpress.org/changeset/58423

Trac ticket https://core.trac.wordpress.org/ticket/61451

## What

Read only the theme.json files stored in the styles/ folder once.

## Why

I've realized that, unlike the theme.json files defined by core & theme, we don't cache these — hence we end up reading them a few times from the filesystem.

## How

By using the existing read_json_file function instead of using wp_json_file_decode directly.

Trac ticket https://core.trac.wordpress.org/ticket/61451

## What

Register the block style variation coming from the different places (theme or user theme.json & theme.json partials) in the WP_Theme_JSON_Resolver.

## Why

The init hook has proven problematic, as it's also fired during installation, see ​slack conversation cc @swissspidy.

## How

The main advantage of this approach is that we register them when the data is read, so we don't have to do any extra data reads:

• theme & custom origins:
• theme.json partials:

The disadvantage is that these block style variations are not registered everywhere, so we have to do that in the specific places that are not yet covered:

• In the global styles endpoint:

## Commit

Do not use init to register block style variations defined via theme.json.

Props oandregal.
See #61451.

I wonder if we can also avoid the potential of processing the same decoded files multiple times as well?

This PR was an initial step given issues encountered in an original attempt to also cache the additional processing of those files in: ​https://github.com/WordPress/gutenberg/pull/62610

I'm not sure I like the idea of adding side effects to the methods that are intended to return the parsed WP_Theme_JSON objects from each origin.

Agreed, this aspect isn't ideal.

I think the previous approach of having a dedicated function that has the responsibility for registering style variations is a much better design

This was the thinking behind the original switch to using the init action to register block style variations. As you noted though it is too broad and ironically has come with a number of its own side effects 😅

I think adding an early return to wp_register_block_style_variations_from_theme() if wp_installing() returns true is probably a better initial improvement.

This could be a good fallback option. One of the problems with leveraging init is that we need to know all the scenarios where we don't want this registration to occur and that is proving difficult. In my view, there's a much smaller set of use cases where the registration needs to happen than not.

Leaving the side-effect issue aside, the approach in this PR suffers the flip side of the above issue. We need to know the scenarios in which block style variations should be registered. This is much more easily reconciled and reasoned about. The happy path for the feature can easily tested.

Can you provide more context about how this variation data gets used?

The block style variations need to be registered to the block styles registry so that their data is not incorrectly stripped during the sanitization of theme.json data. In this context the two are fairly closely linked. This sanitization of variations was deemed important to maintain from a security perspective.

If it's mainly for ensuring styles are enqueued, perhaps we could do this on the enqueue_block_assets hook at priority 9

Unfortunately, there is a little more to it than that. As noted, the registration is also required for the sanitization of theme.json data not just ensuring stylesheets are enqueued. André and I have at different times in the implementation of the feature explored different actions and filters to hook onto but haven't found a workable alternative to init.

---

I'd like to stress that I agree that having registration as a side effect of processing the theme.json data isn't ideal. However, I think for beta 3, we are best served to proceed with the approach in this PR.

This is on a couple of grounds:

• It doesn't introduce any public method/new API/etc.
• It could be considered an implementation detail which leaves the door open to switch back to init if desired once all the edge cases and unknowns can be methodically worked through
• It limits the work done to only where and when we need it

The downside other than the design involving a side effect is that we need to ensure the registration occurs everywhere needed. This can be offset by ensuring the happy path for the feature is fully functional. I believe the testing of the feature prior to the switch to using init provides some confidence here.

Ultimately, this PR's approach helps lower our exposure to unexpected consequences of leveraging init and reduces the potential impact of bugs this late in the release cycle. We are still free to iterate further, even post 6.6, if there are no public functions and APIs added.

On balance, I agree, I think registering block variations as a side effect of retrieving resolved theme JSON data is preferable to using init as the catch all for registration. I also like the idea of it being an implementation detail so it can continue to be refactored if we think it warrants it after the fact.

Ultimately, this PR's approach helps lower our exposure to unexpected consequences of leveraging init and reduces the potential impact of bugs this late in the release cycle. We are still free to iterate further, even post 6.6, if there are no public functions and APIs added.

My reading is that the pros of this PR's approach outnumber the current, demonstrated pros of using the init or any other hook.

Furthermore, the limitations — the introduction of side-effects to these methods — are recognized, and are also isolated to specific methods, which appears to me to be more information than thus far is known about the consequences of registering variations in the 'init' hook.

Therefore it sounds like a viable path to document these limitations and come up with a plan to iterate if folks want this feature to make it for 6.6.

I'd like to stress that I agree that having registration as a side effect of processing the theme.json data isn't ideal.

Just so I understand, in the case of the Global Styles controller, is the processing of theme.json data _required_ to generate an accurate REST transaction?

Just so I understand, in the case of the Global Styles controller, is the processing of theme.json data required to generate an accurate REST transaction?

What's really needed for an accurate REST transaction is for block style variations to be registered at the time the global styles post type is saved as well as prepared for response. Without being registered the block style variation's data will be sanitized and stripped out, creating the issue/inaccuracy.

Without being registered the block style variation's data will be sanitized and stripped out, creating the issue/inaccuracy.

Thanks for the explainer. I was just trying to square it in my brain whether, in the case of the controller, it's really
side-effect given in the input and output values.

Tested and LGTM

Gutenberg sync PR over here:

• ​https://github.com/WordPress/gutenberg/pull/62638

I wonder if we can also avoid the potential of processing the same decoded files multiple times as well?

Very good question! I'll keep that in mind when testing ​https://github.com/WordPress/gutenberg/pull/62638, maybe there's an way to test in the plugin first.

I've created a matching Gutenberg PR for these changes: ​https://github.com/WordPress/gutenberg/pull/62640

Initial smoke tests look good. I'll start giving it a thorough run and review shortly.

Might be worth adding a note about the caching to the @since comment?

I've taken then liberty to push this minor tweak.

Just gave this a test run and switching styles and variations works as expected!

[isabel_brison]

In 58428:

Editor: Read theme.json files stored in styles/ folder only once.

Uses read_json_file to access cached file if it exists instead of using wp_json_file_decode directly.

Props oandregal, aaronrobertshaw, joemcgill, ramonopoly, isabel_brison.
See #61451.

committed in r58428.

After the latest tweaks I've given this another run. It tests well still and the comment tweaks are a nice touch. Thanks!

LGTM 🚢

The different alternatives have pros/cons, none is perfect. Given what people has shared and the current time-constraints, I agree we should go ahead with this approach.

[oandregal]

In 58429:

Do not use init to register block style variations defined via theme.json.

Props oandregal, aaronrobertshaw, joemcgill, ramonopoly, andrewserong, swissspidy.
See #61451.
Fixes #61312.

Committed at https://core.trac.wordpress.org/changeset/58429

[oandregal]

We've done a couple of things for this ticket:

• The feature now only registers block style variations when the theme.json is processed (no extra theme.json parsing, no extra query to retrieve user-data, etc.) + specific paths (user updates the styles of the block style variations). Ref: ​https://github.com/WordPress/wordpress-develop/pull/6844
• Cache the reading of files stored in styles/ as per ​https://github.com/WordPress/wordpress-develop/pull/6843

I'm not sure what's the definition of "done" for this ticket. I believe the two things above could be good enough for the 6.6, though we should keep investigating.

Further opportunities / work mentioned or in current investigation are:

• Cache the partial theme.json files _after_ they are processed (translated). One approach could be ​https://github.com/WordPress/gutenberg/pull/62610
• Create a performance test case that includes block style variations defined in a partial theme.json file, so every PR has that info available. Also including the theme.json file or theme style variations would be good, though it seems more impactful to test the partials (new standalone file, the source of variations that are always present, etc.).
• The styles generation for the block style variations use the wp_theme_json_data_* filters. This requires that we re-create some theme.json structures. We could look at the whether absorbing them as part of the WP_Theme_JSON_Resolver and/or WP_Theme_JSON merge is impactful.

[aaronrobertshaw]

Replying to oandregal:

I'm not sure what's the definition of "done" for this ticket. I believe the two things above could be good enough for the 6.6, though we should keep investigating.

I'm not certain either. I agree this can be considered "done" for 6.6.

I'm happy to create a separate GB issue or ticket to track further investigations and improvements if it helps.

Trac ticket https://core.trac.wordpress.org/ticket/61451
Follow-up to ​https://github.com/WordPress/wordpress-develop/pull/6843
Backports ​https://github.com/WordPress/gutenberg/pull/62610

## What

This PR caches the post-processed style variation files (theme.json within the styles/ folder).

## Why

To improve performance, so we don't have to process them more than once.

## How

Introduces a static $style_variations_cache variable.

@joemcgill what are your thoughts on doing this?

I was looking at the ​performance results for ​https://github.com/WordPress/wordpress-develop/pull/6843 (cache reading the contents from the filesystem), and I'm a bit confused.

I'm not familiar with what we are testing for the "Admin" test, so I don't know if it should have been impacted. However, for the "Front End › Theme: twentytwentyfour" I would have expected to be more impacted. Instead, I see timid or conflicting results:

• wpMemory: increased, which makes sense given we now cache those files.
• wpTotal: it has been reduced minimally and increased (also minimally) in others.
• timeToFirstByte: increased minimally

The results for this PR are similarly confusing:

• wpMemory: increased, as expected.
• wpTotal & timeToFirstByte are both higher (I'd expect the contrary).

This is my current understanding of the new feature:

• In the front-end, we may or may not need to enqueue new styles related to block style variations defined by the theme.
• This requires inspecting all the files within styles/ which is done in the get_style_variations ​method at some point.

Unless the tests are not triggering the file reads from the filesystem for some reason, I'd have expected the numbers to have improved with the cache (previous PR & this one).

In terms of performance, the Front-End tests for TwentyTwenty Four do show a consistent and slight decrease for wpTotal and timeToFirstByte:

In theory, adding a cache here should be more performant. However, it seem that the refactor of WP_Theme_JSON_Resolver::get_style_variations has also added a lot more processing to that method, and the cache (if it's even being used) is not offsetting the additional processing.

Trunk

This PR

It seems odd to me that in trunk, none of the WP_Theme_JSON construction calls were actually running. I've investigated this by setting a breakpoint in this method to see the code execution via XDebug, and it seems like in each of the 4 times this method is called on the TT4 homepage, it's always passing 'block' as the scope, but all of the variations are from the 'theme' scope, so in trunk this is all being skipped and is returning an empty array, whereas now this PR is just checking to see whether there is a scope at all, not just that it matches the scope being passed to the method.

This seems like a likely bug.

[joemcgill]

While spending time looking at possible performance implications of this feature, I noticed that the current implementation results in a large increase in the creation of WP_Theme_JSON objects, which has been known to be a source of performance overhead in the past (see #61112).

Since [58264] the number of calls that method has increased significantly (profiling a request to the homepage of a default site running Twenty Twenty-Four):

• WP_Theme_JSON::__construct increased from 18 to 53

Additionally:

• WP_Theme_JSON_Resolver::get_merged_data increased from 5 to 21
• WP_Theme_JSON_Resolver::get_theme_data increased from 8 to 24

Some of this is probably due to the way this data is being merged into resolved WP_Theme_JSON origins through the use of wp_theme_json_data_theme and wp_theme_json_data_user filters, rather than directly within the WP_Theme_JSON_Resolver class. I wonder if there are ways we can streamline this more to avoid the need to construct so many extra WP_Theme_JSON objects. Given the size of these objects, I also am concerned that just adding caching will increase memory usage, which can also lead to significant performance bottlenecks.

Trac ticket https://core.trac.wordpress.org/ticket/61451
Follow-up work for https://core.trac.wordpress.org/ticket/61312

## What

This PR tries to optimize the data flow for operating with section styles.

## Why

It has proven less optimized than we wanted.

## How

At some point, the code transforms the following:

{
  "styles": {
    "blocks": {
      "variations": {
        "myVariation": {
          "title": "My variation",
          "blockTypes": [ "core/paragraph", "core/group" ],
          "color": {
            "background": "yellow"
          }
        }
      }
    }
  }
}

into:

{
  "styles": {
    "blocks": {
      "core/paragraph": {
        "color": {
          "background": "yellow"
        }
      },
      "core/group": {
        "color": {
          "background": "yellow"
        }
      }
    }
  }
}

The existing code executes that transformation using the wp_theme_json_data_* filters. This is, after any WP_Theme_JSON structure is processed.

The optimization proposed in this PR explores moving that transform into the WP_Theme_JSON constructor directly, following what we do with appearanceTools settings.

## Test

TBD.

This is not finalized, but I wanted to share the approach as soon as possible to gather feedback and gauge the impact on performance.

This seems like a likely bug.

The feature is working correctly in trunk as well as the related one (theme style variations).

I haven't had the time to look at this using xdebug today, as I was investigating a ​proof of concept to optimize the data flow.

I like the idea of trying to move this processing directly into the parsing of a WP_Theme_JSON object, rather than needing to normalize the JSON before passing it to the constructor of a WP_Theme_JSON object. If we are expecting styles.blocks.variations.{variationKey} to be a valid part of the schema, passing that shape to the WP_Theme_JSON constructor should result in a correctly parsed object, IMO.

I ran some initial profiles on this PR compared with the parent commit (c1c8d30398) and can confirm that it currently has very little impact, but that is mostly because the processing being skipped, wp_resolve_block_style_variations_from_primary_theme_json() is currently not doing much in trunk:

Looking further into the profiling data, it looks like when wp_merge_block_style_variations_data is being called, it's being passed an empty array, so no new WP_Theme_JSON_Data is created, nor is anything merged. I'm guessing this is due to the bug I observed in ​https://github.com/WordPress/wordpress-develop/pull/6857#issuecomment-2178985546.

Thanks, I'll do some more testing. I'm just surprised that currently no variations are being processed while running TT4 in trunk, but perhaps it's because those code paths aren't being triggered without adding some variations manually?

​https://github.com/WordPress/wordpress-develop/pull/6873 takes the ideas from this PR and does a few other things.

[oandregal]

In 58466:

Section styles: improve performance and conceptual consistency.

These changes involve:

• Move shared variation definitions from styles.blocks.variations to styles.variations
• Remove blockTypes from styles.variations.
• Do not register shared variations from theme style variation or primary theme.json files.
• Move the merging of theme.json data into the WP_Theme_JSON_Resolver and WP_Theme_JSON classes.

These changes improve performance and are more future-proof API wise.
See conversation at ​https://github.com/WordPress/gutenberg/issues/62686

Props aaronrobertshaw, oandregal, andrewserong, joemcgill, talldanwp, andrewserong, ramonopoly, richtabor, youknowriad.

See #61312, #61451.

[oandregal]

In 58468:

Format: fix spaces for @param.

Props oandregal, mukesh27.

See #61451.

[joemcgill]

Closing this as fixed now that we're at RC1. From this point on, only critical issues found during RC should be fixed and can be done in new tickets to keep the scope of those changes clear.