Global styles: cache post-processed style variations

[oandregal]

Trac ticket https://core.trac.wordpress.org/ticket/61451
Follow-up to #6843
Backports WordPress/gutenberg#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.

Test

How to test

1. Verify the theme style variations are working as expected.

Go to "Site Editor > Styles" and apply one of them. Verify the changes are reflected in the front end. Do the same from the global styles sidebar in the site editor.

2. Verify the block style variations defined via theme.json are working as expected.

Create a partial.json file within the styles/ folder with the following contents:

{
        "$schema": "https://schemas.wp.org/trunk/theme.json",
        "version": 2,
        "title": "Partial",
        "blockTypes": [ "core/group" ],
        "styles": {
                "color": {
                        "background": "aliceblue"
                }
        }
}

Go to any editor, add a group block, and verify there is a "Partial" style variation (Block Settings > Styles). Apply the variation and save the changes. Verify the contents are the expected (background color is aliceblue) — also in the front-end.

Commit

Proposal for commit message, to make it easier for committers:

Global styles: cache post-processed style variations.

Props aaronrobertshaw, ramonopoly, oandregal.
See #61451.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props oandregal, joemcgill, aaronrobertshaw.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[oandregal]

@joemcgill what are your thoughts on doing this?

I was looking at the performance results for #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).

[joemcgill]

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.

[oandregal]

This seems like a likely bug.

As far as I tested, the feature is working correctly in trunk (instructions, though I did more random things as well). I also tested 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.

[joemcgill]

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?

[aaronrobertshaw]

@oandregal and @joemcgill what are the next steps that can be taken here?

There were also some changes recently around the theme.json resolver's functions retrieving style variations (see WordPress/gutenberg#63318). Would it be easier to get those backported sooner or can we also approach this from the Gutenberg side?

[joemcgill]

@aaronrobertshaw I think that this got abandoned last time because we were not able to validate that this cache actually had a measurable improvement in WP::trunk and possibly even would have led to performance. Before adding this or a similar cache, it would be good to verify that it will result in a measurable performance improvement.

One of the challenges is that the code paths relative to WP_Theme_JSON_Resolver are inconsistent between the GB plugin and when the same code is ported to WP Core, due to the way these systems currently get integrated via filters – See WordPress/gutenberg#62594 for some context.

[aaronrobertshaw]

Thanks for extra info @joemcgill 👍

I was following up as I'd been advised that TwentyTwentyFour was seeing a noticeable increase in TTFB when running WP6.6.1 without any plugins.

Some initial digging there apparently flagged the wp_render_block_style_variation_support_styles filter as the likely culprit. This makes some sense as that filter calls WP_Theme_JSON_Resolver::get_merged_data() which when retrieving the theme data, calls get_style_variations that has to read and parse theme.json partials.

For the TT4 home page, the wp_render_block_style_variation_support_styles filter is run more than a dozen times. Given there's only a simple static variable cache on the read JSON files at present, it seems like there's an opportunity for further optimization, such as caching the translated theme json data as proposed in this PR etc.

I'm not well-versed in debugging performance issues. Is there a recommended approach to profiling locally to establish a benchmark before tinkering with different caching strategies and other optimizations?

[oandregal]

I'm not well-versed in debugging performance issues. Is there a recommended approach to profiling locally to establish a benchmark before tinkering with different caching strategies and other optimizations?

For measuring the "real" impact any change may have, one thing you can do is preparing a pull request — this is great if the change you want to test is trivial.

Local setup

For a local setup, in the past, I prepared a production site and timed curl requests to the homepage. This is the TLDR of my process back then:

1. Clone the wordpress-develop repository and switch to the corresponding version to benchmark.
2. Set it as production: WP_DEBUG, WP_DEBUG_LOG, WP_DEBUG_DISPLAY, SCRIPT_DEBUG to false in .env.
3. Start the environment: npm install && npm run build:dev && npm run env:start && npm run env:install.
4. Time a request to the homepage. I used this command: seq 1000 | xargs -Iz curl -o /dev/null -H 'Cache-Control: no-cache' -s -w "%{time_starttransfer}\n" http://localhost:8889 | xclip -selection clipboard. Instead of dispatching thousand requests you can use 20/100/250, any number of requests that provides consistent results or a mean that's representative enough.
5. Paste the results in a spreadsheet to analyze further.
6. Switch to a different scenario (theme, with/without a plugin, etc.). Go to step 4.

One additional step that can help spot issues is preparing the site with a variety of data. You can do so by importing the theme-test-data. Some old recipe I have:

npm run env:cli -- plugin install wordpress-importer --activate --path=/var/www/build
curl -O https://raw.githubusercontent.com/WordPress/theme-test-data/master/themeunittestdata.wordpress.xml
npm run env:cli -- import themeunittestdata.wordpress.xml --authors=create --path=/var/www/build

Testing locally may end up being very fast and the numbers won't really correspond to any real production environment as it doesn't account for network requests, servers are not usually M1/M2 machines, etc. However, the important part is whether results are consistent. If you are able to have them within a threshold, then pushing to a PR that runs the core benchmarks should display the same the difference you saw locally — even if the numbers are different. That's my experience at least :)

Drill down into the full request

Sometimes, it may be useful to analyze how specific parts of the request react to some code changes — specially if the impact may be masked by the setup. For example, a change that scales with the number of plugins you have. The impact may seem minimal if you have X plugins active, but will be bigger if you have X+Y.

For this, I use server-timing headers. I have this old recipe (hope it still works 🤞) inspired by some gists shared by Felix:

ob_start();
$count = 0;
foreach( [ 'plugins_loaded', 'after_setup_theme', 'init', 'wp_loaded', 'template_redirect' ] as $hook_name ) {
	add_action(
		$hook_name,
		function() use ( $hook_name ) {
			global $server_timing_values, $timestart, $count;
			if ( ! is_array( $server_timing_values ) ) {
				$server_timing_values = array();
			}

			$count++;
			$server_timing_values[ $count . '-' . $hook_name ] = microtime( true ) - $timestart;
		},
		-9999
	);
}
foreach( [ 'template_include' ] as $hook_name ) {
	add_filter(
		$hook_name,
		function( $passthrough ) use ( $hook_name ) {
			global $server_timing_values, $timestart, $count;
			if ( ! is_array( $server_timing_values ) ) {
				$server_timing_values = array();
			}

			$count++;
			$server_timing_values[ $count . '-' . $hook_name ] = microtime( true ) - $timestart;

			return $passthrough;
		},
		-9999
	);
}

add_action(
	'shutdown',
	function() {
		global $server_timing_values, $timestart, $count;
		if ( ! is_array( $server_timing_values ) ) {
			$server_timing_values = array();
		}

		$count++;
		$server_timing_values[ $count . '-shutdown'] = microtime( true ) - $timestart;

		$output = ob_get_clean();

		$header_values = array();
		foreach ( $server_timing_values as $slug => $value ) {
			if ( is_float( $value ) ) {
				$value = round( $value * 1000.0, 2 );
			}
			$header_values[] = sprintf( 'wp-%1$s;dur=%2$s', $slug, $value );
		}
		header( 'Server-Timing: ' . implode( ', ', $header_values ) );

		echo $output;
	},
	-9999
);

In the browser's console, you'd see something like:

Though I usually was interested in pasting this into a spreadsheet. So, similar to the above, I'd do something like this to copy the data and paste it into the sheet:

seq 100 | xargs -Iz curl -H 'Cache-Control: no-cache' -sD - localhost:8889|grep Server-Timing|sed 's/Server-Timing: //'|sed 's/wp-1-plugins_loaded;dur=//'|sed 's/wp-2-after_setup_theme;dur=//'|sed 's/wp-3-init;dur=//' | sed 's/wp-4-wp_loaded;dur=//'|sed 's/wp-5-template_redirect;dur=//'|sed 's/wp-6-template_include;dur=//'|sed 's/wp-7-shutdown;dur=//' | xclip -selection clipboard

Hope this helps!

[oandregal]

btw, I'm going to close this PR as I won't pursue it, but the conversation can continue :)