Update: Improve performance of gutenberg_render_layout_support_flag. #46074

jorgefilipecosta · 2022-11-25T19:27:29Z

On #45372 (comment), it was possible to see that get_merge_data was being called hundreds of times inside render_layout_support_flag.

render_layout_support_flag is run per block, and inside we were calling gutenberg_get_global_settings three times. gutenberg_get_global_settings calls get_merged_data. render_layout_support_flag is a filter called during the block render. When the blocks start rendering, there is no expectation that the theme.json settings change during the block render so I think the settings and their derived information should all be static information of this function.
This simple change removes 3*NUMBER_OF_BLOCKS calls of get_merged_data to just one call.

We should still add caching to the settings in #45372. But even with caching, there is a cost in retrieving information from the cache, so we should include this PR.

Testing Instructions

Verify the block styles continue to work as expected.

codesandbox · 2022-11-25T19:27:30Z

Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

aristath

Changes here make sense, they improve performance and I see no regressions on my end 👍

oandregal · 2022-11-28T09:44:29Z

Brilliant! Thanks for using the data to drive changes, this is exactly the kind of thinking we need to apply :)

I used the same methodology as in #45372 to run the performance results and this is what I found: this PR improves over trunk at ee90c2b by 22ms, a 4% improvement.

gutenberg_get_global_settings is called 33 times vs 95 before, and WP_Theme_JSON_Resolver::get_merged_data is called 36 times vs 99 before. The full dataset (9 runs) is available for download at: zip

oandregal · 2022-11-28T10:13:45Z

lib/block-supports/layout.php

+	if ( ! $static_information_computed ) {
+		$global_settings               = gutenberg_get_global_settings();
+		$block_gap                     = _wp_array_get( $global_settings, array( 'spacing', 'blockGap' ), $global_settings );
+		$has_block_gap_support         = isset( $block_gap );


My understanding is that block gap has three valid states: null, true, and false. This logic simplification is proper as per how the code is set up ($block_gap cannot be null if it is set), though I wonder what was the intention here. cc @ramonjd @andrewserong @aaronrobertshaw per thoughts as to how to update this code.

I don't have a lot of background with the layout supports and block gap in particular, so can't speak much as to the original intention.

The logic simplification in setting $has_block_gap_support make sense to me; however, I think the default value provided above when retrieving $block_gap is incorrect. According to the docs it should default to null.

@oandregal @aaronrobertshaw Also wondering about that. Unless there's a good answer why this was changed, I'd say we should revert this line to be similar to https://github.com/WordPress/gutenberg/pull/46074/files#diff-324697e41855298e2f2c74b078f174e0cbc9075cef736ce9c1e2c169bf64652eL376 again.

however, I think the default value provided above when retrieving $block_gap is incorrect. According to the docs it should default to null.

Yes, the default of blockGap should be null. It looks like the defaults of the other _wp_array_get calls need to be updated too, I think.

Hi all, I used global settings as the default because that's the default we were using before this change ( the function gutenberg_get_global_settings ends with _wp_array_get( $settings, $path, $settings ); so the default is the settings array if the path is not found). But I followed the feedback and applied the defaults suggested by @andrewserong.

oandregal · 2022-11-28T10:16:33Z

lib/block-supports/layout.php

-		if ( ! $global_layout_settings ) {
-			return $block_content;
-		}
+	static $static_information_computed   = false;


Can we add a comment here to describe why we cache this data at render time, so future readers understand why this is set up in this way?

How is the cache invalidation?

Looking at this code closer, I think we should remove this cache all together. Caching of global settings should be done here - #45372. Cache invalidation should also be done here. That way we can call gutenberg_get_global_settings without worrying parsing again.

The worry here is double caching and cache invalidation.

@spacedmonkey How is cache invalidation relevant for this change? I'm not sure I understand.

Since this PR uses a simple "variable cache" (i.e. in PHP runtime memory), I wouldn't think cache invalidation for this matters (since it's not using WP_Object_Cache and the data here is static).

@felixarntz What if a filter or register theme support changes something in the runtime? How is this runtime cache invalidated?

felixarntz

Mostly looks good to me. Left a few replies to existing comment threads with additional feedback / questions.

andrewserong

Thanks for the ping, and for looking at improving the performance here! (I'll also CC @tellthemachines for visibility since there's been a bit of work on layout.php recently).

I think I agree with the comments that ideally we wouldn't need to add a layer of caching to this function, since it's likely that many other blocks or block supports will need to perform global settings lookups. In my mind, it'd be good if the consuming code didn't need to worry about how expensive that lookup was, and could treat calling gutenberg_get_global_settings much like looking up an array to retrieve a value, rather than considering it an expensive operation.

That said, I don't really see any harm in adding the static variables to avoid the excess calls. The main feedback I have is that it looks like the default values need to be updated.

lib/block-supports/layout.php

andrewserong · 2022-11-29T01:03:22Z

lib/block-supports/layout.php

+	if ( ! $static_information_computed ) {
+		$global_settings               = gutenberg_get_global_settings();
+		$block_gap                     = _wp_array_get( $global_settings, array( 'spacing', 'blockGap' ), $global_settings );
+		$has_block_gap_support         = isset( $block_gap );


however, I think the default value provided above when retrieving $block_gap is incorrect. According to the docs it should default to null.

Yes, the default of blockGap should be null. It looks like the defaults of the other _wp_array_get calls need to be updated too, I think.

oandregal · 2022-11-29T10:23:56Z

I think I agree with the comments that ideally we wouldn't need to add a layer of caching to this function, since it's likely that many other blocks or block supports will need to perform global settings lookups. In my mind, it'd be good if the consuming code didn't need to worry about how expensive that lookup was, and could treat calling gutenberg_get_global_settings much like looking up an array to retrieve a value, rather than considering it an expensive operation.

I was on the fence, but given #45372 looks like in a solid path to be merged soon, I feel the same way.

Not opposed either. Though, I see how making this sort of change through the whole codebase may be unstable/fragile (consumers may forget to do it in some places, code is changed in the future to not use this after the settings cache is in place, etc.).

oandregal · 2022-11-30T10:15:52Z

FWIW, #45372 has been merged ☺️

Co-authored-by: Andrew Serong <[email protected]>

spacedmonkey

This looks good! I love that we are no longer adding another level of caching. Let's get this merge and into core ASAP.

andrewserong

Thanks for the updates @jorgefilipecosta! The new defaults now means the code is behaving the way we thought it was previously, even if it wasn't 😀

This is all looking good to me, just did a smoke test and confirmed:

✅ blockGap set to true or false outputs spacing rules, set to null it doesn't
✅ has-global-padding class is injected for constrained layout when useRootPaddingAwareAlignments is set to true on a theme, and isn't when it's set to false
✅ Output of layout styles still appears to be correct for the three layout types (flow, constrained, flex)

LGTM, and nice work getting this function down to only calling gutenberg_get_global_settings once per call! ✨

lib/block-supports/layout.php

jorgefilipecosta · 2022-12-06T10:50:01Z

Thank you all for the reviews 👍

…ordPress#46074) * Update: Improve performance of gutenberg_render_layout_support_flag. * Update lib/block-supports/layout.php Co-authored-by: Andrew Serong <[email protected]> * Update lib/block-supports/layout.php Co-authored-by: Andrew Serong <[email protected]> * Update lib/block-supports/layout.php Co-authored-by: Andrew Serong <[email protected]> * Fix indentation Co-authored-by: Andrew Serong <[email protected]> Co-authored-by: Ari Stathopoulos <[email protected]>

Backports WordPress/gutenberg#46074 into the core. render_layout_support_flag is run per block, and inside we called get_global_settings three times. get_global_settings calls get_merged_data, which is costly. render_layout_support_flag is a filter called during the block render. When the blocks start rendering, there is no expectation that the theme.json settings change during the block render, so the settings and their derived information should all be static information of this function. This simple change removes 3*NUMBER_OF_BLOCKS calls of get_merged_data to just one call. Props oandregal, aristath, felixarntz, tellthemachines, andrewserong, aaronrobertshaw, aaronrobertshaw. git-svn-id: https://develop.svn.wordpress.org/trunk@55167 602fd350-edb4-49c9-b593-d223f7449a82

Backports WordPress/gutenberg#46074 into the core. render_layout_support_flag is run per block, and inside we called get_global_settings three times. get_global_settings calls get_merged_data, which is costly. render_layout_support_flag is a filter called during the block render. When the blocks start rendering, there is no expectation that the theme.json settings change during the block render, so the settings and their derived information should all be static information of this function. This simple change removes 3*NUMBER_OF_BLOCKS calls of get_merged_data to just one call. Props oandregal, aristath, felixarntz, tellthemachines, andrewserong, aaronrobertshaw, aaronrobertshaw. Built from https://develop.svn.wordpress.org/trunk@55167 git-svn-id: https://core.svn.wordpress.org/trunk@54700 1a063a9b-81f0-0310-95a4-ce76da25c4cd

Backports WordPress/gutenberg#46074 into the core. render_layout_support_flag is run per block, and inside we called get_global_settings three times. get_global_settings calls get_merged_data, which is costly. render_layout_support_flag is a filter called during the block render. When the blocks start rendering, there is no expectation that the theme.json settings change during the block render, so the settings and their derived information should all be static information of this function. This simple change removes 3*NUMBER_OF_BLOCKS calls of get_merged_data to just one call. Props oandregal, aristath, felixarntz, tellthemachines, andrewserong, aaronrobertshaw, aaronrobertshaw. Built from https://develop.svn.wordpress.org/trunk@55167 git-svn-id: http://core.svn.wordpress.org/trunk@54700 1a063a9b-81f0-0310-95a4-ce76da25c4cd

jorgefilipecosta requested a review from spacedmonkey as a code owner November 25, 2022 19:27

jorgefilipecosta force-pushed the update/improve_performance_of_gutenberg_render_layout_support_flag branch from 015a5ab to 22ab467 Compare November 25, 2022 19:36

jorgefilipecosta mentioned this pull request Nov 25, 2022

Add WP_Object_Cache to the gutenberg_get_global_settings method #45372

Merged

aristath approved these changes Nov 28, 2022

View reviewed changes

oandregal reviewed Nov 28, 2022

View reviewed changes

spacedmonkey requested a review from felixarntz November 28, 2022 11:05

felixarntz reviewed Nov 28, 2022

View reviewed changes

andrewserong reviewed Nov 29, 2022

View reviewed changes

jorgefilipecosta and others added 3 commits December 5, 2022 14:38

Update: Improve performance of gutenberg_render_layout_support_flag.

f7027e5

Update lib/block-supports/layout.php

a16c4dd

Co-authored-by: Andrew Serong <[email protected]>

Update lib/block-supports/layout.php

cd4e43d

Co-authored-by: Andrew Serong <[email protected]>

jorgefilipecosta force-pushed the update/improve_performance_of_gutenberg_render_layout_support_flag branch from b743f01 to 2c0aa63 Compare December 5, 2022 14:44

Update lib/block-supports/layout.php

d5f575f

Co-authored-by: Andrew Serong <[email protected]>

jorgefilipecosta force-pushed the update/improve_performance_of_gutenberg_render_layout_support_flag branch from 2c0aa63 to d5f575f Compare December 5, 2022 17:33

spacedmonkey approved these changes Dec 5, 2022

View reviewed changes

andrewserong approved these changes Dec 6, 2022

View reviewed changes

aristath reviewed Dec 6, 2022

View reviewed changes

lib/block-supports/layout.php Outdated Show resolved Hide resolved

Fix indentation

f40ba0c

aristath merged commit a41ea81 into trunk Dec 6, 2022

aristath deleted the update/improve_performance_of_gutenberg_render_layout_support_flag branch December 6, 2022 10:45

github-actions bot added this to the Gutenberg 14.8 milestone Dec 6, 2022

jorgefilipecosta added the Needs PHP backport Needs PHP backport to Core label Dec 6, 2022

spacedmonkey added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Dec 6, 2022

andrewserong added the [Feature] Layout Layout block support, its UI controls, and style output. label Dec 7, 2022

andrewserong mentioned this pull request Dec 7, 2022

Tracking: additional layout options, design tools and refinements #44720

Closed

18 tasks

spacedmonkey mentioned this pull request Dec 17, 2022

wp_get_global_stylesheet: substitute 1-minute transient by non-persistent object cache WordPress/wordpress-develop#3712

Closed

Mamaduka mentioned this pull request Jan 17, 2023

Plugin: Backport PHP changes for WordPress 6.2 release #47187

Closed

85 tasks

jorgefilipecosta mentioned this pull request Jan 30, 2023

Update: Improve performance of gutenberg_render_layout_support_flag. WordPress/wordpress-develop#3945

Closed

tellthemachines mentioned this pull request Feb 2, 2023

Backport changes to wp_render_layout_support_flag dmsnell/wordpress-develop#2

Closed

Mamaduka removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Mar 29, 2023

ramonjd removed the Needs PHP backport Needs PHP backport to Core label Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: Improve performance of gutenberg_render_layout_support_flag. #46074

Update: Improve performance of gutenberg_render_layout_support_flag. #46074

jorgefilipecosta commented Nov 25, 2022

codesandbox bot commented Nov 25, 2022

aristath left a comment

oandregal commented Nov 28, 2022

oandregal Nov 28, 2022

aaronrobertshaw Nov 28, 2022

felixarntz Nov 28, 2022

andrewserong Nov 29, 2022

jorgefilipecosta Dec 5, 2022

oandregal Nov 28, 2022

spacedmonkey Nov 28, 2022

spacedmonkey Nov 28, 2022

felixarntz Nov 28, 2022

spacedmonkey Nov 29, 2022

felixarntz left a comment

andrewserong left a comment

andrewserong Nov 29, 2022

oandregal commented Nov 29, 2022

oandregal commented Nov 30, 2022

spacedmonkey left a comment

andrewserong left a comment

jorgefilipecosta commented Dec 6, 2022

Update: Improve performance of gutenberg_render_layout_support_flag. #46074

Update: Improve performance of gutenberg_render_layout_support_flag. #46074

Conversation

jorgefilipecosta commented Nov 25, 2022

Testing Instructions

codesandbox bot commented Nov 25, 2022

aristath left a comment

Choose a reason for hiding this comment

oandregal commented Nov 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

andrewserong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal commented Nov 29, 2022

oandregal commented Nov 30, 2022

spacedmonkey left a comment

Choose a reason for hiding this comment

andrewserong left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Dec 6, 2022