Remove building wp_template_parts variation call for frontend #5718

kt-12 · 2023-11-29T14:48:32Z

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

Gutenberg PR - WordPress/gutenberg#56952

For TT4, running 500 rounds twice and taking the mean.

Title	Mean (Trunk)	Mean (PR)
Response Time (median)	403.305	398.08
wp-load-alloptions-query (median)	3.19	3.225
wp-before-template (median)	171.795	154.855
wp-before-template-db-queries (median)	0	0

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.

joemcgill

Left a few questions, but this would be a simple updated if it doesn't cause any unforeseen side effects.

src/wp-includes/blocks/template-part.php

joemcgill · 2023-12-01T16:48:50Z

Thanks for the latest update @kt-12. Going to break out of the inline thread and respond to your last update here.

This approach is a bit better, because it allows variations to only be loaded in get_block_editor_server_block_settings(), but it still seems like a workaround and not a true API, since this will cause the registered block properties to be different based on whether that filter happens to be called, which also makes registered blocks mutable and thus non-cacheable (or at least more challenging). I do think that ensuring variations show up in the API responses would be a requirement, even if we did temporarily move variant registration for this block to only get_block_editor_server_block_settings() and the API, though.

What would you think about updating the block registration schema so that you could pass a callable as the variations property, which would be called only when variations are needed? @Mamaduka suggested something similar in the original Gutenberg thread.

Additionally, I think for consistency, we should apply whatever changes we're making for template part variations to other core blocks that register variations dynamically on the server, e.g. Navigation Link, and Post Terms.

BTW, this change does seem to have a measurable impact on server response time on the front end, in my local benchmarking, trunk is loading in 290.79 ms (p50) while this PR is 242.19 ms (p50), which is a sizable improvement.

src/wp-includes/class-wp-block-type.php

joemcgill

This looks pretty straightforward, @kt-12. I assume we also need to do something similar in the other places where WordPress expects to return a block definition that includes the full set of registered variations, like in WP_REST_Block_Types_Controller or additionally implement as a getter method in WP_Block_Type as @Mamaduka was suggesting here.

src/wp-includes/class-wp-block-type.php

mukeshpanchal27 · 2023-12-06T15:31:54Z

@kt-12 Some unit tests going fails, The wp-includes/blocks/template-part.php needs to update in GB first then the changes needs to backport.

kt-12 · 2023-12-07T13:39:28Z

@joemcgill Implemented the same for Navigation-Link and Post-Term and then did some benchmarking.

The following command was run thrice for both Trunk and PR and then Mean (Trunk) and Mean (PR) was obtained

npm run research -- benchmark-server-timing --url http://localhost:8889/ -n 500 -c 2

Performance Improvement for TT4 (Tested locally on default homepage)

Metric	Mean (Trunk)	Mean (PR)	Diff
Response Time (median)	397.66 ms	362.16 ms	-35.5 ms
wp-load-alloptions-query (median)	3.23 ms	2.96 ms	-0.27 ms
wp-before-template (median)	173.53 ms	143.51 ms	-30.02 ms

@Mamaduka This PR is purely based on our findings-based interpretation that the dynamically registered variations were only used inside get_block_editor_server_block_settings and block-types rest API. We want someone core to Gutenberg to check and confirm if these changes aren't breaking anywhere else.

Furthermore, I have some improvements found on the editor side that could lead to similar performance upgrades on the editor side. However, I don't have the expertise to confirm my findings.

Remove inline block schema and replace it with prefetch

We have used the following code at site-editor, widgets-form-blocks, edit-form, wp-customize-widgets
It's a 5-year-old code mostly introduced to handle some edge cases related to third-party blocks and dynamic variations.

wp_add_inline_script(
	'wp-blocks',
	'wp.blocks.unstable__bootstrapServerSideBlockDefinitions(' . wp_json_encode( get_block_editor_server_block_settings() ) . ');'
);

The above code preloads the schema for all block types. In the default setup, I found it to add 92 block types with it's variations.
I could confirm this by running wp.blocks.getBlockTypes() in the browser console for the editor. However, if I comment the above code (I tried on edit-form), reload the editor and then run wp.blocks.getBlockTypes() it still loads all the block types, except that some blocks have dynamic information missing in this case. This means that information in inlining is being used but most of the data is just a repeat of what is already available at the editor.

Also, the information in the inline script is only used when a person searches for a block or some similar option and is not needed when the page loads (just my understanding based on what I discovered - this could defer). So we don't need all the information in the first load.

Proposal

Replace unstable__bootstrapServerSideBlockDefinitions with a call to block type REST API #22812
The information in /wp/v2/block-types is similar to what we are inlining right now. However, it seems that we are not making use of this API endpoint anywhere in the core (not 100% sure). Instead of inlining, we can shift the load to preloading this way we free up the original doc from expensive computation
Only load what is missing instead of looping through all 92 blocks.
Most of the information is getting loaded in the editor even without inlining. So we could avoid looping through all the registered block types and inline it entirely. Instead, only inline those blocks that had some dynamic information. Note: This method would yield lesser improvement than the first.

kt-12 · 2023-12-07T13:42:02Z

@kt-12 Some unit tests going fails, The wp-includes/blocks/template-part.php needs to update in GB first then the changes needs to backport.

Thanks @mukeshpanchal27. Once I have confirmation on the code here I'll raise another PR for GB. At the moment I am confused as to which will part will go in GB and which will stay here, so focusing more on the problem to solve.

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

….com/10up/wordpress-develop into enhancement/remove-variation-fontend

…into enhancement/remove-variation-fontend

src/wp-includes/class-wp-block-type.php

joemcgill

Thanks @kt-12. This is looking promising. I'd love to get feedback from someone like @Mamaduka or @ockham on the general approach.

One important thing to note is that the changes to individual blocks will need to be made in the Gutenberg repo after support for registering variations as a callable is added. I think it makes sense to show the whole change in this PR in the meantime, but we'd likely only commit the changes to get_block_editor_server_block_settings() and the API.

joemcgill · 2023-12-08T20:09:31Z

src/wp-includes/blocks/post-terms.php

 	register_block_type_from_metadata(
 		__DIR__ . '/post-terms',
 		array(
 			'render_callback' => 'render_block_core_post_terms',
-			'variations'      => array_merge( $built_ins, $custom_variations ),
+			'variations'      => 'get_variations_core_post_terms',


It would be nice to use a consistent naming scheme for all of the variation callbacks in core blocks. Right now you have:

get_variations_core_navigation_link

get_variations_core_navigation_link

build_template_part_block_variations

Personally, I'd go with something like get_block_core_{block_name}_variations which is similar to the consistent render callback render_block_core_{block_name}.

I was avoiding build_{block name} as the navigation link already had a similar function with a different purpose ->build_variation_for_navigation_link.

I like the get_block_core_{block_name}_variations naming throughout, but we may have to deprecate the function build_template_part_block_variations which is old and can be in use in ways we don't know.
For now, I have made everything build_{block}_block_variations for consistency.

@joemcgill I have created a draft PR in GB repo - https://github.com/WordPress/gutenberg/pull/56952/files

There are some CS issues which I think someone from GB would be the best person to address.

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

….com/10up/wordpress-develop into enhancement/remove-variation-fontend

spacedmonkey

Changing the type of this property on the block type class feels unwise.

src/wp-includes/class-wp-block-type.php

src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php

Mamaduka · 2023-12-28T07:15:41Z

src/wp-includes/blocks/template-part.php

@@ -276,7 +276,7 @@ function register_block_core_template_part() {
 		__DIR__ . '/template-part',
 		array(
 			'render_callback' => 'render_block_core_template_part',
-			'variations'      => build_template_part_block_variations(),
+			'variations'      => 'build_template_part_block_variations',


I think it's okay to have these changes here for now and remove them before merging into the core. Then, we can update the code in the block-library package in a backward compatible way. The Gutenberg plugin supports multiple WP versions.

spacedmonkey

Mamaduka · 2024-01-16T17:56:00Z

Excellent work, everyone!

joemcgill

This looks good, @kt-12! I left an inline comment about us adding some more unit tests for various use cases, but can commit this as is in the meantime.