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.

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

Improve performance of WP_Theme_JSON::merge when merging background styles #7486

Conversation

mukeshpanchal27
Copy link
Member

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

See WordPress/performance#1572 (comment) for more details about the performance regression.


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.

Copy link

github-actions bot commented Oct 3, 2024

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.

@mukeshpanchal27
Copy link
Member Author

mukeshpanchal27 commented Oct 3, 2024

Server-timing After the PR

Mertic Beta 1 PR Diff % Diff abs.
Response Time 110.43 106.22 -3.81% -4.21
wp-before-template 68.19 70.02 2.68% 1.83
wp-before-template-db-queries 2.71 2.63 -2.95% -0.08
wp-template 41.12 34.52 -16.05% -6.60
wp-total 108.73 104.24 -4.13% -4.49
wp-template-db-queries 2.09 1.97 -5.74% -0.12

The benchmarks shows that it improve the performance.

Someone needs to verify the approach and the performance numbers.

@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review October 3, 2024 07:30
Copy link

github-actions bot commented Oct 3, 2024

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 mukesh27, flixos90, ramonopoly, joemcgill, andrewserong.

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

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 While it looks like this makes sense, I noticed there is one thing missing in this PR which would cause a bug.

Once you have implemented the fix, could you please rerun the profiles to see the updated impact? We need to make sure that this is not only faster because of what is currently missing.

src/wp-includes/class-wp-theme-json.php Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
@mukeshpanchal27
Copy link
Member Author

mukeshpanchal27 commented Oct 7, 2024

Server-timing After the PR implementation.

Mertic Trunk PR Diff % Diff abs.
Response Time 71.64 68.96 -3.74% -2.68
wp-before-template 30.99 30.74 -0.81% -0.25
wp-template 37.56 35.82 -4.63% -1.74
wp-total 69.78 66.58 -4.59% -3.20

@mukeshpanchal27 mukeshpanchal27 self-assigned this Oct 7, 2024
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 Thank you for the update.

It looks like this solution will work well, however we may want to reconsider the approach: While I initially suggested a new method, this introduces some duplicate code, which can cause problems in the future: We would need to make sure to always update the code in both methods if we want to make a change, which is not intuitive and could be easily missed.

I noticed that the existing get_block_nodes() method has an $options parameter. Maybe we could add an option like $node_paths_only or similar, and if it's set we would not run most of the logic of the method that populates the other keys. WDYT?


foreach ( $theme_json['styles']['blocks'] as $name => $node ) {
$nodes[] = array(
'path' => array( 'styles', 'blocks', $name ),
Copy link
Member

Choose a reason for hiding this comment

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

This looks like incorrect indentation? Same below.

* @since 6.7.0
*
* @param array $theme_json The theme.json converted to an array.
* @return array The block nodes in theme.json.
Copy link
Member

Choose a reason for hiding this comment

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

Given this is a separate function with the only purpose to return the node paths, it would make more sense to return an array of strings, not an array of associative node arrays with only the path in it.

But we may want to reconsider the approach, per my overarching comment.

@andrewserong
Copy link
Contributor

Not sure if it helps, but my understanding is that background images aren't supported for element styles yet (i.e. they're only supported for blocks themselves). Would it make it simpler to not include support for elements at this stage, then we could go with something like the first commit in this PR? Then we could revisit further down the track if or when we wished to add support for background images on elements.

Just an idea, though, in case it helps wrangle the complexity. I don't want to derail the discussion if the current path is a better fix!

@mukeshpanchal27
Copy link
Member Author

In 76e0140 i introduce two option.

  • include_node_paths_only only return path for the nodes, Default false
  • include_block_elements per @andrewserong comment if we want to remove it then we have to pass false in option array.

@felixarntz @andrewserong Could you please take look when you have moment. Thanks

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @mukeshpanchal27, this almost looks good to me. Just a bit of polishing needed at this point, but almost good to commit IMO.

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
@mukeshpanchal27
Copy link
Member Author

Server-timing After the PR implementation.

Mertic Trunk PR Diff % Diff abs.
Response Time 68.94 66.46 -3.60% -2.48
wp-before-template 30.17 29.74 -1.43% -0.43
wp-template 36.26 34.57 -4.66% -1.69
wp-total 67.03 64.52 -3.74% -2.51

@andrewserong
Copy link
Contributor

Thanks for the updates here. Apologies if I missed some context, but it looks like this adds a fair bit of additional complexity, where the original commit took a simpler approach. Was there a reason we need to make the additional changes? Since we're only using this flag in one place, I wondered if it's worth it to try to reduce the changeset. Or were there other instances where you think we might re-use this flag?

@mukeshpanchal27
Copy link
Member Author

@andrewserong In my quick search I don't find any place where we can use these option, happy to update other places as it return lightweight version of array values.

Per #7486 (comment), to add support for elements we updated the approach.

Ping @ramonjd as he added this feature in core and have more context.

@felixarntz
Copy link
Member

@andrewserong

Thanks for the updates here. Apologies if I missed some context, but it looks like this adds a fair bit of additional complexity, where the original commit took a simpler approach. Was there a reason we need to make the additional changes? Since we're only using this flag in one place, I wondered if it's worth it to try to reduce the changeset. Or were there other instances where you think we might re-use this flag?

Can you clarify what you consider additional complexity in relation to what other version of the code? I'm not sure I understand what you're referring to.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 This looks good to me. One minor nit-pick, but not a blocker at all.

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
@andrewserong
Copy link
Contributor

Can you clarify what you consider additional complexity in relation to what other version of the code? I'm not sure I understand what you're referring to.

The original commit in c13a398 looked a little simpler to me. Not a blocker at all, but I was just trying to wrap my head around whether that approach would have been viable instead of adding the $include_node_paths_only option. I don't have any objections to the approach here, it overall sounds reasonable to me — this was really just a comment because we now have three checks for if ( $include_node_paths_only ) and if the condition ($include_node_paths_only) was only used in one place, I wasn't too sure how necessary it was.

But these questions were only asked out of curiosity / to confidence check the approach. Please don't let my comments here hold up this performance improvement!

@felixarntz
Copy link
Member

The original commit in c13a398 looked a little simpler to me. Not a blocker at all, but I was just trying to wrap my head around whether that approach would have been viable instead of adding the $include_node_paths_only option. I don't have any objections to the approach here, it overall sounds reasonable to me — this was really just a comment because we now have three checks for if ( $include_node_paths_only ) and if the condition ($include_node_paths_only) was only used in one place, I wasn't too sure how necessary it was.

Thank you for clarifying. I think the three checks make sense, because $include_node_paths_only should still mean only that - you want only the paths for every node, but you still want every node. There was originally only one check because it was excluding element nodes, but that would be unexpected behavior. In that case, the parameter would be more like $include_node_paths_only_and_only_for_block_nodes.

If we wanted to only get this data for block nodes, we could consider another parameter like you had originally proposed, but that would not really simplify the PR code, and it would introduce another parameter which we don't really know whether we need it. Hope that makes sense.

@andrewserong
Copy link
Contributor

Thanks for explaining!

If we wanted to only get this data for block nodes, we could consider another parameter like you had originally proposed

Just to clarify, I wasn't meaning to advocate for another parameter — the original commit didn't include any as it skipped calling get_styles_block_nodes altogether and was iterating over $incoming_data. From reading the comments here, I was idly wondering if we could get away with not adding any parameters. But it was just a thought, and nothing worth blocking the good work here. The added parameter is clear and the conditions read well to me 🙂

@ramonjd
Copy link
Member

ramonjd commented Oct 9, 2024

Thank you for working on this, folks.

Not sure if it helps, but my understanding is that background images aren't supported for element styles yet (i.e. they're only supported for blocks themselves).

This is correct. I don't see any argument to support them until we support them 😄

This is a new feature in 6.7 so I assume backwards compatibility is not on the table.

the original commit didn't include any as it skipped calling get_styles_block_nodes altogether and was iterating over $incoming_data.

I think iterating over $incoming_data is a perfectly fine approach - it's small, targeted and doesn't introduce extra changes to get_block_nodes().

Having said that, I don't want to be a blocker either. The parameter addition is not a huge footprint. I just think this PR needs unit tests to support it.

@ramonjd
Copy link
Member

ramonjd commented Oct 10, 2024

Will there be unit tests for the changes in this method?

If it helps, I've added one to cover the new option in the Gutenberg backport PR, if you want to port it over:

Now that I've had time to reflect, I think the new option has merit as it will future-proof background image support for elements et. al.

Thanks again to all!

@mukeshpanchal27
Copy link
Member Author

@felixarntz @ramonjd The feedback is addressed and PR is ready for commit.

@mukeshpanchal27 mukeshpanchal27 changed the title Improve performance for Background images: resolve ref and ensure appropriate default values Improve performance of WP_Theme_JSON::merge when merging background styles Oct 10, 2024
@ramonjd
Copy link
Member

ramonjd commented Oct 10, 2024

Looking good to me. If no-one gets to it first, I can retest and commit tomorrow (I'm end of day).

Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

A couple of small nitpicks, but not blockers.

It would be good to backport this to the Gutenberg plugin as well.

I'm also not seeing a big impact in the latest Performance Tests for this PR. Can we confirm that this approach still has the intended performance impact?

if ( isset( $selectors[ $name ]['duotone'] ) ) {
$duotone_selector = $selectors[ $name ]['duotone'];
}
$include_node_paths_only = $options['include_node_paths_only'] ?? false;
Copy link
Member

Choose a reason for hiding this comment

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

This could be moved outside of the foreach loop since that option doesn't change

'css' => $selector,
);
$variation_selectors = array();
$include_variations = $options['include_block_style_variations'] ?? false;
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, this could also be set outside the loop

@felixarntz
Copy link
Member

@joemcgill There's already a Gutenberg backport PR, see WordPress/gutenberg#66002.

@felixarntz
Copy link
Member

Committed in https://core.trac.wordpress.org/changeset/59213

@felixarntz felixarntz closed this Oct 10, 2024
ramonjd added a commit to WordPress/gutenberg that referenced this pull request Oct 10, 2024
ramonjd added a commit to WordPress/gutenberg that referenced this pull request Oct 10, 2024
…erge when merging background styles (#66002)

Backporting the current state of WordPress/wordpress-develop#7486 and adding tests.

Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: andrewserong < andrewserong@git.wordpress.org>
Co-authored-by: joemcgill < joemcgill@git.wordpress.org>
peterwilsoncc pushed a commit to peterwilsoncc/gutenberg-build that referenced this pull request Oct 11, 2024
…erge when merging background styles (#66002)

Backporting the current state of WordPress/wordpress-develop#7486 and adding tests.

Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: andrewserong < andrewserong@git.wordpress.org>
Co-authored-by: joemcgill < joemcgill@git.wordpress.org>

Source: WordPress/gutenberg@2cd6929
ciampo pushed a commit to WordPress/gutenberg that referenced this pull request Oct 14, 2024
…erge when merging background styles (#66002)

Backporting the current state of WordPress/wordpress-develop#7486 and adding tests.

Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: andrewserong < andrewserong@git.wordpress.org>
Co-authored-by: joemcgill < joemcgill@git.wordpress.org>
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
…erge when merging background styles (WordPress#66002)

Backporting the current state of WordPress/wordpress-develop#7486 and adding tests.

Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: andrewserong < andrewserong@git.wordpress.org>
Co-authored-by: joemcgill < joemcgill@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done 😃
Development

Successfully merging this pull request may close these issues.

5 participants