Patterns: Don't inject theme attr on frontend #5455

ockham · 2023-10-10T18:52:56Z

Result of a discussion with @felixarntz, aiming to fix the performance regression re-introduced by https://core.trac.wordpress.org/changeset/56818.

Needs its Gutenberg counterpart PR, which has the Template Part block fall back to the current theme if no theme attribute is present. This should OTOH allow removing theme attr injection on the frontend.

To test locally, patch src/wp-includes/blocks/template-part.php with the changes from https://github.com/WordPress/gutenberg/pull/55217.diff.

cc/ @felixarntz @gziolo

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

felixarntz · 2023-10-10T20:32:37Z

@ockham In order to be able to benchmark the performance impact, it would be great if you could incorporate the full change into this PR as well, i.e. including the Gutenberg changes from WordPress/gutenberg#55217. Obviously this will cause a CI workflow error and shouldn't be merged, but it makes it easier to compare and discuss the changes.

This is a bit unfortunate, but I think for issues that require work in both Gutenberg and Core, it's the easiest way to holistically reason about the solution. Once we agree on a solution, we can:

merge the relevant change in Gutenberg
backport the Gutenberg change to Core
remove the Gutenberg specific part from here, then refresh with latest trunk
merge this PR

ockham · 2023-10-10T20:38:34Z

@ockham In order to be able to benchmark the performance impact, it would be great if you could incorporate the full change into this PR as well, i.e. including the Gutenberg changes from WordPress/gutenberg#55217. Obviously this will cause a CI workflow error and shouldn't be merged, but it makes it easier to compare and discuss the changes.

Makes sense! Done in 62e0be6 😊

gziolo · 2023-10-11T07:40:12Z

One open question is whether the Pattern returned from REST API needs to have the current theme injected as the theme attribute in the Template Part block included in the pattern. If the answer is yes, then we can probably move the logic that injects it to the REST API controller rather than doing it conditionally in the patterns registry. If the answer is no, we should stop injecting it when accessing all registered patterns.

I asked in the original PR WordPress/gutenberg#53423 that introduced this functionality about some recommendations how we can validate that the functionality still works in the editor after the proposed refactoring.

gziolo · 2023-10-11T11:24:37Z

I'm testing locally with the following diff applied to trunk:

diff --git a/src/wp-includes/blocks/template-part.php b/src/wp-includes/blocks/template-part.php
index a7bd4033af..3ad4009069 100644
--- a/src/wp-includes/blocks/template-part.php
+++ b/src/wp-includes/blocks/template-part.php
@@ -18,13 +18,10 @@ function render_block_core_template_part( $attributes ) {
 	$template_part_id = null;
 	$content          = null;
 	$area             = WP_TEMPLATE_PART_AREA_UNCATEGORIZED;
+	$theme            = isset( $attributes['theme'] ) ? $attributes['theme'] : get_stylesheet();
 
-	if (
-		isset( $attributes['slug'] ) &&
-		isset( $attributes['theme'] ) &&
-		get_stylesheet() === $attributes['theme']
-	) {
-		$template_part_id    = $attributes['theme'] . '//' . $attributes['slug'];
+	if ( isset( $attributes['slug'] ) && get_stylesheet() === $theme ) {
+		$template_part_id    = $theme . '//' . $attributes['slug'];
 		$template_part_query = new WP_Query(
 			array(
 				'post_type'           => 'wp_template_part',
@@ -34,7 +31,7 @@ function render_block_core_template_part( $attributes ) {
 					array(
 						'taxonomy' => 'wp_theme',
 						'field'    => 'name',
-						'terms'    => $attributes['theme'],
+						'terms'    => $theme,
 					),
 				),
 				'posts_per_page'      => 1,
diff --git a/src/wp-includes/class-wp-block-patterns-registry.php b/src/wp-includes/class-wp-block-patterns-registry.php
index e516277e42..ddf73446e7 100644
--- a/src/wp-includes/class-wp-block-patterns-registry.php
+++ b/src/wp-includes/class-wp-block-patterns-registry.php
@@ -165,14 +165,16 @@ final class WP_Block_Patterns_Registry {
 	private function prepare_content( $pattern, $hooked_blocks ) {
 		$content = $pattern['content'];
 
-		$before_block_visitor = '_inject_theme_attribute_in_template_part_block';
+		$before_block_visitor = ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ? '_inject_theme_attribute_in_template_part_block' : null;
 		$after_block_visitor  = null;
 		if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) {
 			$before_block_visitor = make_before_block_visitor( $hooked_blocks, $pattern );
 			$after_block_visitor  = make_after_block_visitor( $hooked_blocks, $pattern );
 		}
-		$blocks  = parse_blocks( $content );
-		$content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
+		if ( ! is_null( $before_block_visitor ) || ! is_null( $after_block_visitor ) ) {
+			$blocks  = parse_blocks( $content );
+			$content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
+		}
 
 		return $content;
 	}

I could confirm in my testing that the theme attribute needs to be injected into Template Part blocks in all block patterns when sending to the editor with REST API.

Benchmarking

I used the benchmark-server-timing command, with 100 runs each, and I did that 3 times (i.e. 3 medians, each based on 100 runs). All benchmarks without any plugins active (other than Performance Lab for the Server-Timing headers). Below is a summary of the wp-total metrics my data.

With TT4 home page and this patch:

258.95ms (PR) vs 265.58ms (trunk) --> 2.5% faster 👍🏻
258.62ms (PR) vs 265.36ms (trunk) --> 2.6% faster 👍🏻
258.73ms (PR) vs 265.33ms (trunk) --> 2.5% faster 👍🏻

gziolo · 2023-10-11T11:55:32Z

I also tried applying the same mechanism to patterns, as to template, and template parts:

diff --git a/src/wp-includes/block-template-utils.php b/src/wp-includes/block-template-utils.php
index c5953e1d4c..81f306be93 100644
--- a/src/wp-includes/block-template-utils.php
+++ b/src/wp-includes/block-template-utils.php
@@ -549,15 +549,18 @@ function _build_block_template_result_from_file( $template_file, $template_type
 		$template->area = $template_file['area'];
 	}
 
-	$before_block_visitor = '_inject_theme_attribute_in_template_part_block';
+	$before_block_visitor = ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ? '_inject_theme_attribute_in_template_part_block' : null;
 	$after_block_visitor  = null;
 	$hooked_blocks        = get_hooked_blocks();
 	if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) {
 		$before_block_visitor = make_before_block_visitor( $hooked_blocks, $template );
 		$after_block_visitor  = make_after_block_visitor( $hooked_blocks, $template );
 	}
-	$blocks            = parse_blocks( $template_content );
-	$template->content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
+	if ( ! is_null( $before_block_visitor ) || ! is_null( $after_block_visitor ) ) {
+		$blocks           = parse_blocks( $template_content );
+		$template_content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
+	}
+	$template->content = $template_content;
 
 	return $template;
 }

When combined with the diff included in #5455 (comment) I saw better results.

Benchmarking

I used the benchmark-server-timing command, with 100 runs each, and I did that 3 times (i.e. 3 medians, each based on 100 runs). All benchmarks without any plugins active (other than Performance Lab for the Server-Timing headers). Below is a summary of the wp-total metrics my data.

With TT4 home page and this patch:

256.74ms (PR) vs 265.09ms (trunk) --> 3.1% faster 👍🏻
256.67ms (PR) vs 265.11ms (trunk) --> 3.2% faster 👍🏻
256.99ms (PR) vs 265.78ms (trunk) --> 3.3% faster 👍🏻

felixarntz · 2023-10-11T15:59:38Z

Given @ockham is out today, I have refreshed this PR with the latest updates from WordPress/gutenberg#55217, as well as the changes suggested above from @gziolo: The _inject_theme_attribute_in_template_part_block() should only be called within a REST API request, but not a regular frontend request.

Keep in mind that the changes to the pattern and template-part blocks are only in here for testing and should be removed prior to commit. They need to be backported from the Gutenberg PR first.

felixarntz · 2023-10-11T21:14:50Z

@ockham I have replaced the tests that are no longer applicable now (68bfea6) with new tests ensuring the template part block still works correctly with the theme attribute (8522bc8).

Let me know your thoughts, from my perspective this is good to go pending the backport of WordPress/gutenberg#55217.

dmsnell · 2023-10-12T06:57:32Z

Running some tests and I hope they are helpful.

I want to compare three states here:

[control] before the original refactor in 56805
[broken] with the refactor in 56805 [git sha 9d039e6]
[fixed] with this fix from 56818 [git sha a9bb470]

Obviously it would be unfair to compare all three of these against each other directly, though, since other improvements may have appeared in trunk along the way.

To do this I'm going to create all new commits based on the latest trunk.

[control] is now trunk at a8d5328 with reverts of broken and fixed
- branch control at 76a1aded
[broken] at trunk with revert of fixed
- branch broken at 02db6af7
[fixed] at trunk
- branch fixed at a8d5328
Wiped out the database, reset everything on the site.
Activate Performance Lab and output buffering plugins, disable all Performance Lab options.
curl the home page 500 times in a row for each branch, switch branches, and repeat until enough samples for all three branches have been collected

performance testing is hard - detour!

this initial assessment reports that things are insignificantly faster on my test server after the fix in a9bb470, but interestingly enough there's not that much difference in either of the three branches; plausibly because of the caching that was introduced? I'm guessing that there's a more profound impact when that cache isn't so readily available.

to try and confirm this I reset to the parent commit of 9d039e6 and re-ran the experiment, cherry-picking the fix on that old commit.

[control] is now a55dcf46
[broken] is now 9d039e6c
[fixed] is now 72b829c3

unfortunately the results are almost identical on these old branches, so I think something was wrong with the tests.

restarting every 500 samples when I change branches, I'm now restoring a fresh copy of the database after activating the plugins, from the control branch.

new graphs based on trunk today + the reverts: here are the control, broken, and fixed patches in comparison to each other. these look right.

and here is the same plot if we went back in time to the original parent commit of the breaking patch, then cherry-picked this fix onto the broken patch.

so the fixes have less impact today than they had when they were originally applied because trunk got faster otherwise. there's only one more interesting feature here I noticed in a plot that deserves exploration: my sneaky suspicion. I'm going to run the tests again but wipe out the database between every request and see if the outliers in these plots (not shown because they peak at around 1400 ms) are outliers or represent the true cost of this stuff when caching isn't available.

But what if the caches aren't primed?

given that this testing requires resetting the data on every single request, it's very slow and I have set it to switch branches every five requests instead of every five hundred, and I'm going to look at the data before we have nearly as many samples.

and going back in time…strangely we have two very strong modes that I only noticed after getting about 285 samples of each branch. I don't know why this is. I've separated the graph into a "high" and "low" comparison. maybe this is some wp-cron task running? or something random or time-based? whatever it is, the high/slow mode only appears in about 20% of the samples.

ockham · 2023-10-12T11:48:49Z

src/wp-includes/class-wp-block-patterns-registry.php

@@ -165,14 +165,16 @@ public function unregister( $pattern_name ) {
 	private function prepare_content( $pattern, $hooked_blocks ) {
 		$content = $pattern['content'];

-		$before_block_visitor = '_inject_theme_attribute_in_template_part_block';
+		$before_block_visitor = ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ? '_inject_theme_attribute_in_template_part_block' : null;


We might consider moving the code that injects the theme attr to the patterns controller instead. Probably okay to leave as-is for now, but might be a nice follow-up post-6.4.

(Noting that if we still insert hooked blocks here, we'd be running traverse_and_serialize_blocks twice, which is suboptimal; so we'd also need to move hooked blocks insertion to the controller so we can remove it here; which would then require us to make sure it's run on the frontend. We can probably achieve that through running it in the Pattern block's render_block. This means that we'd need to make quite a few changes, which we most likely don't want to do before 6.4.)

Can you explain why traverse_and_serialize_blocks would get called twice?

Can you explain why traverse_and_serialize_blocks would get called twice?

Assuming that we inject the theme attr in the patterns controller, but keep hooked blocks insertion here in the patterns registry (since it's needed both for the frontend and editor (i.e. REST API)) -- both of these call traverse_and_serialize_blocks 😅

I see what you mean here - If we were to run the theme attribute injection in the REST API controller. Yes, good point that it would get called twice with different processing 👍🏻

ockham · 2023-10-12T12:15:04Z

Thanks a lot folks for pushing this along and for the great performance analysis! With WordPress/gutenberg#55217 merged and ready to be sync'ed, I'll remove the block code from this PR.

ockham · 2023-10-12T12:19:42Z

Thanks a lot folks for pushing this along and for the great performance analysis! With WordPress/gutenberg#55217 merged and ready to be sync'ed, I'll remove the block code from this PR.

Done in 8394dfd.

We now need to wait for #5468 to land before we can merge this one 😊

gziolo

Everything should be ready to go after #5468 lands and this branch gets rebased with trunk.

hellofromtonya · 2023-10-12T14:02:26Z

Heads up: The Gutenberg npm packages are now updated in Core's trunk via https://core.trac.wordpress.org/changeset/56849. This changeset includes the package side fixes from WordPress/gutenberg#55217 which were part of #5468.

hellofromtonya · 2023-10-12T14:03:34Z

With the package updates now in trunk, I'd suggest rebasing this PR to bring in the latest changes. Then another round of testing just to make sure everything works as expected, i.e. before committing.

hellofromtonya · 2023-10-12T14:14:15Z

Noting (you may already know in which case sorry for my redundant note): The PHPUnit tests are failing with this PR:

There was 1 failure:

1) Tests_Blocks_RenderBlockCoreTemplatePart::test_render_block_core_template_part_without_theme_attribute
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<header class="wp-block-template-part">
-<p>Small Header Template Part</p>
-</header>'
+'Template part has been deleted or is unavailable: small-header'

/var/www/tests/phpunit/tests/blocks/renderBlockCoreTemplatePart.php:60
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:106
/var/www/vendor/bin/phpunit:118

…sage.

…ll its usage." This reverts commit 1a410b0.

…API request.

… even without theme attribute.

ockham · 2023-10-12T14:22:21Z

Rebased; this should now include the changes to the Pattern and Template Part blocks from #5468, and thus get the PHPUnit tests to pass 🤞

felixarntz

Thanks @ockham for the updates!

Two super minor non-blocking nit-picks, but good to commit IMO!

src/wp-includes/block-template-utils.php

src/wp-includes/class-wp-block-patterns-registry.php

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

ockham · 2023-10-12T16:40:42Z

Committed to Core in https://core.trac.wordpress.org/changeset/56896.

ockham self-assigned this Oct 10, 2023

gziolo mentioned this pull request Oct 11, 2023

Template Part block: Fall back to current theme if no theme attribute is given. WordPress/gutenberg#55217

Merged

gziolo mentioned this pull request Oct 12, 2023

Tests: Improve code coverage for _build_block_template_result_from_file #5396

Closed

ockham commented Oct 12, 2023

View reviewed changes

ockham marked this pull request as ready for review October 12, 2023 12:18

gziolo approved these changes Oct 12, 2023

View reviewed changes

hellofromtonya mentioned this pull request Oct 12, 2023

Update npm packages to latest versions for 6.4 RC1 #5468

Closed

ockham and others added 8 commits October 12, 2023 16:17

Patterns: Don't inject theme attr on frontend

909ec6b

Remove _inject_theme_attribute_in_template_part_block() and all its u…

8fb2f79

…sage.

Revert "Remove _inject_theme_attribute_in_template_part_block() and a…

9751e1c

…ll its usage." This reverts commit 1a410b0.

Only call _inject_theme_attribute_in_template_part_block() if a REST …

9315aa3

…API request.

Only call _inject_theme_attribute_in_template_part_block() if a REST …

5932fba

…API request.

Avoid duplicate code.

5c27373

Remove tests which are no longer applicable.

44d7eab

Implement test coverage to ensure template part block works correctly…

14f6306

… even without theme attribute.

ockham force-pushed the try/fixing-theme-attr-injection-perf-regression branch from 8394dfd to 14f6306 Compare October 12, 2023 14:18

felixarntz approved these changes Oct 12, 2023

View reviewed changes

src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved

src/wp-includes/class-wp-block-patterns-registry.php Outdated Show resolved Hide resolved

ockham and others added 2 commits October 12, 2023 18:20

Use !== null equality checks

2af8248

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Use !== null equality checks once more

17ca37a

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

ockham closed this Oct 12, 2023

ockham deleted the try/fixing-theme-attr-injection-perf-regression branch October 12, 2023 16:40

This was referenced Oct 15, 2023

Epic: Block Hooks Features for WP 6.5 WordPress/gutenberg#54904

Closed

Patterns, Templates: Try conditional theme attribute injection #5514

Draft

gziolo mentioned this pull request Nov 8, 2023

Template Part: Add fallback to the current theme when not provided WordPress/gutenberg#55965

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patterns: Don't inject theme attr on frontend #5455

Patterns: Don't inject theme attr on frontend #5455

ockham commented Oct 10, 2023 •

edited

Loading

felixarntz commented Oct 10, 2023 •

edited

Loading

ockham commented Oct 10, 2023

gziolo commented Oct 11, 2023 •

edited

Loading

gziolo commented Oct 11, 2023 •

edited

Loading

gziolo commented Oct 11, 2023

felixarntz commented Oct 11, 2023 •

edited

Loading

felixarntz commented Oct 11, 2023

dmsnell commented Oct 12, 2023 •

edited

Loading

ockham Oct 12, 2023

gziolo Oct 12, 2023

ockham Oct 12, 2023

gziolo Oct 12, 2023

ockham commented Oct 12, 2023

ockham commented Oct 12, 2023

gziolo left a comment

hellofromtonya commented Oct 12, 2023

hellofromtonya commented Oct 12, 2023

hellofromtonya commented Oct 12, 2023

ockham commented Oct 12, 2023

felixarntz left a comment

ockham commented Oct 12, 2023

Patterns: Don't inject theme attr on frontend #5455

Patterns: Don't inject theme attr on frontend #5455

Conversation

ockham commented Oct 10, 2023 • edited Loading

felixarntz commented Oct 10, 2023 • edited Loading

ockham commented Oct 10, 2023

gziolo commented Oct 11, 2023 • edited Loading

gziolo commented Oct 11, 2023 • edited Loading

Benchmarking

gziolo commented Oct 11, 2023

Benchmarking

felixarntz commented Oct 11, 2023 • edited Loading

felixarntz commented Oct 11, 2023

dmsnell commented Oct 12, 2023 • edited Loading

ockham Oct 12, 2023

Choose a reason for hiding this comment

gziolo Oct 12, 2023

Choose a reason for hiding this comment

ockham Oct 12, 2023

Choose a reason for hiding this comment

gziolo Oct 12, 2023

Choose a reason for hiding this comment

ockham commented Oct 12, 2023

ockham commented Oct 12, 2023

gziolo left a comment

Choose a reason for hiding this comment

hellofromtonya commented Oct 12, 2023

hellofromtonya commented Oct 12, 2023

hellofromtonya commented Oct 12, 2023

ockham commented Oct 12, 2023

felixarntz left a comment

Choose a reason for hiding this comment

ockham commented Oct 12, 2023

ockham commented Oct 10, 2023 •

edited

Loading

felixarntz commented Oct 10, 2023 •

edited

Loading

gziolo commented Oct 11, 2023 •

edited

Loading

gziolo commented Oct 11, 2023 •

edited

Loading

felixarntz commented Oct 11, 2023 •

edited

Loading

dmsnell commented Oct 12, 2023 •

edited

Loading