Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance: Skip iteration in block supports elements when not necessary. #5411

Closed
wants to merge 8 commits into from
Closed

Performance: Skip iteration in block supports elements when not necessary. #5411

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dd8a479
Performance: Skip iteration in block supports elements when not neces…
dmsnell Oct 4, 2023
0f7a1d5
Create optimized predicate function to determine block eligibility.
dmsnell Oct 5, 2023
b35fda3
Inline logic to avoid function calls
dmsnell Oct 5, 2023
b5207cb
Un-staticise list of heading elements.
dmsnell Oct 6, 2023
714237b
Revert attempts at further optimization which didn't demonstrate impr…
dmsnell Oct 6, 2023
3a2f432
Revert attempts at further optimization which didn't demonstrate impr…
dmsnell Oct 6, 2023
d9242ef
Revert attempts at further optimization which didn't demonstrate impr…
dmsnell Oct 6, 2023
70e91de
Merge branch 'trunk' into performance/skip-needless-iteration-in-bloc…
felixarntz Oct 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 46 additions & 42 deletions src/wp-includes/block-supports/elements.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ function wp_get_elements_class_name( $block ) {
* @return string Filtered block content.
*/
function wp_render_elements_support( $block_content, $block ) {
if ( ! $block_content || empty( $block['attrs'] ) ) {
if ( ! $block_content || ! isset( $block['attrs']['style']['elements'] ) ) {
return $block_content;
}

Expand All @@ -41,42 +41,42 @@ function wp_render_elements_support( $block_content, $block ) {
'button' => array(
'skip' => wp_should_skip_block_supports_serialization( $block_type, 'color', 'button' ),
'paths' => array(
'style.elements.button.color.text',
'style.elements.button.color.background',
'style.elements.button.color.gradient',
array( 'button', 'color', 'text' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmsnell To avoid creating the arrays unnecessarily on each call, what do you think about moving the early return on lines 84-90 up to line 39 and changing them to this?

$skip_button_color  = wp_should_skip_block_supports_serialization( $block_type, 'color', 'button' );
$skip_link_color    = wp_should_skip_block_supports_serialization( $block_type, 'color', 'link' );
$skip_heading_color = wp_should_skip_block_supports_serialization( $block_type, 'color', 'heading' );

if ( $skip_button_color && $skip_link_color && $skip_heading_color) {
    return $block_content;
}

Then this will further reduce memory usage while having no functional or visual impact.

The $skip_* variables could be used for the 'skip' keys in the arrays below to avoid duplicate calls, but see the comment below for an alternative approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could further reduce memory usage by using the $skip_* variables to determine whether to add the property and its paths to the array at all, rather than create it and store whether to skip it later.

$element_color_properties = array();

if ( ! $skip_button_color ) {
    $element_color_properties['button'] = array( 
        array( 'heading', 'color', 'text' ),
        // etc.
    );
}

// same for link and heading.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a minor optimization, but I think it makes sense to add, given this PR is purely focused on performance in the first place 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @costdev - I explored this route but found diminishing returns. there are more optimization opportunities involving bailing out in a hierarchical manner too, such as saying, if there's no style.elements.heading attribute then don't check style.elements.heading.color.text but in my tests these all created marginal returns while making the code harder to understand or modify.

since these are all static we could eliminate the arrays entirely and fall back to static control structures.

again, things that I looked at but found no evidence that they are noteworthy optimizations, and things I felt could be addressed in a more normal development cycle where bigger changes can be examined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure where you all stand on adding these extra changes. I will try and create a new commit for them, but given the goals of this change and the timeline and that it's code coming in from a Gutenberg package I'm in favor of leaving this patch as-is, especially if we can't show a meaningful further improvement.

array( 'button', 'color', 'background' ),
array( 'button', 'color', 'gradient' ),
),
),
'link' => array(
'skip' => wp_should_skip_block_supports_serialization( $block_type, 'color', 'link' ),
'paths' => array(
'style.elements.link.color.text',
'style.elements.link.:hover.color.text',
array( 'link', 'color', 'text' ),
array( 'link', ':hover', 'color', 'text' ),
),
),
'heading' => array(
'skip' => wp_should_skip_block_supports_serialization( $block_type, 'color', 'heading' ),
'paths' => array(
'style.elements.heading.color.text',
'style.elements.heading.color.background',
'style.elements.heading.color.gradient',
'style.elements.h1.color.text',
'style.elements.h1.color.background',
'style.elements.h1.color.gradient',
'style.elements.h2.color.text',
'style.elements.h2.color.background',
'style.elements.h2.color.gradient',
'style.elements.h3.color.text',
'style.elements.h3.color.background',
'style.elements.h3.color.gradient',
'style.elements.h4.color.text',
'style.elements.h4.color.background',
'style.elements.h4.color.gradient',
'style.elements.h5.color.text',
'style.elements.h5.color.background',
'style.elements.h5.color.gradient',
'style.elements.h6.color.text',
'style.elements.h6.color.background',
'style.elements.h6.color.gradient',
array( 'heading', 'color', 'text' ),
array( 'heading', 'color', 'background' ),
array( 'heading', 'color', 'gradient' ),
array( 'h1', 'color', 'text' ),
array( 'h1', 'color', 'background' ),
array( 'h1', 'color', 'gradient' ),
array( 'h2', 'color', 'text' ),
array( 'h2', 'color', 'background' ),
array( 'h2', 'color', 'gradient' ),
array( 'h3', 'color', 'text' ),
array( 'h3', 'color', 'background' ),
array( 'h3', 'color', 'gradient' ),
array( 'h4', 'color', 'text' ),
array( 'h4', 'color', 'background' ),
array( 'h4', 'color', 'gradient' ),
array( 'h5', 'color', 'text' ),
array( 'h5', 'color', 'background' ),
array( 'h5', 'color', 'gradient' ),
array( 'h6', 'color', 'text' ),
array( 'h6', 'color', 'background' ),
array( 'h6', 'color', 'gradient' ),
),
),
);
Expand All @@ -89,32 +89,36 @@ function wp_render_elements_support( $block_content, $block ) {
return $block_content;
}

$element_colors_set = 0;
$elements_style_attributes = $block['attrs']['style']['elements'];

foreach ( $element_color_properties as $element_config ) {
if ( $element_config['skip'] ) {
continue;
}

foreach ( $element_config['paths'] as $path ) {
if ( null !== _wp_array_get( $block['attrs'], explode( '.', $path ), null ) ) {
++$element_colors_set;
if ( null !== _wp_array_get( $elements_style_attributes, $path, null ) ) {
/*
* It only takes a single custom attribute to require that the custom
* class name be added to the block, so once one is found there's no
* need to continue looking for others.
*
* As is done with the layout hook, this code assumes that the block
* contains a single wrapper and that it's the first element in the
* rendered output. That first element, if it exists, gets the class.
*/
$tags = new WP_HTML_Tag_Processor( $block_content );
if ( $tags->next_tag() ) {
$tags->add_class( wp_get_elements_class_name( $block ) );
Copy link
Contributor

@costdev costdev Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since $block doesn't change per iteration, we can avoid calling wp_get_elements_class_name() (and therefore md5() and serialize()) for each iteration by storing this above the foreach on line 94.

$elements_class_name = wp_get_elements_class_name( $block );

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was something I thought at first too but there's actually no iteration here so it only gets called once. once it realized it needs the class name, it finds the first opening HTML tag, adds the class, and then returns from the function.

}

return $tags->get_updated_html();
}
}
}

if ( ! $element_colors_set ) {
return $block_content;
}

// Like the layout hook this assumes the hook only applies to blocks with a single wrapper.
// Add the class name to the first element, presuming it's the wrapper, if it exists.
$tags = new WP_HTML_Tag_Processor( $block_content );
if ( $tags->next_tag() ) {
$tags->add_class( wp_get_elements_class_name( $block ) );
}

return $tags->get_updated_html();
// If no custom attributes were found then there's nothing to modify.
return $block_content;
}

/**
Expand Down