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

mukeshpanchal27 · 2024-10-03T07:10:44Z

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.

github-actions · 2024-10-03T07:23:51Z

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 · 2024-10-03T07:29:39Z

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.

github-actions · 2024-10-03T07:30:27Z

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.

felixarntz

@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

mukeshpanchal27 · 2024-10-07T06:37:46Z

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

felixarntz

@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?

felixarntz · 2024-10-07T15:29:14Z

src/wp-includes/class-wp-theme-json.php

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


This looks like incorrect indentation? Same below.

felixarntz · 2024-10-07T15:30:02Z

src/wp-includes/class-wp-theme-json.php

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


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 · 2024-10-08T05:26:58Z

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!

src/wp-includes/class-wp-theme-json.php

mukeshpanchal27 · 2024-10-08T10:56:06Z

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

felixarntz

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

mukeshpanchal27 · 2024-10-09T05:45:31Z

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 · 2024-10-09T06:17:56Z

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 · 2024-10-09T09:53:30Z

@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 · 2024-10-09T17:54:26Z

@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.

felixarntz

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

src/wp-includes/class-wp-theme-json.php

andrewserong · 2024-10-09T22:05:04Z

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 · 2024-10-09T22:11:29Z

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 · 2024-10-09T22:33:00Z

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 · 2024-10-09T22:40:33Z

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.

src/wp-includes/class-wp-theme-json.php

… adding tests.

ramonjd · 2024-10-10T04:06:54Z

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:

Backport from WordPress Core: improve performance of WP_Theme_JSON::merge when merging background styles gutenberg#66002

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!

Co-authored-by: Felix Arntz <[email protected]>

mukeshpanchal27 · 2024-10-10T06:54:41Z

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

ramonjd · 2024-10-10T07:11:03Z

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

joemcgill

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?

joemcgill · 2024-10-10T20:05:32Z

src/wp-includes/class-wp-theme-json.php

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


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

joemcgill · 2024-10-10T20:07:35Z

src/wp-includes/class-wp-theme-json.php

-				'css'        => $selector,
-			);
+				$variation_selectors = array();
+				$include_variations  = $options['include_block_style_variations'] ?? false;


Similarly, this could also be set outside the loop

felixarntz · 2024-10-10T20:20:18Z

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

felixarntz · 2024-10-10T21:05:38Z

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

… adding tests.

…erge when merging background styles (#66002) Backporting the current state of WordPress/wordpress-develop#7486 and adding tests. Co-authored-by: ramonjd <[email protected]> Co-authored-by: mukeshpanchal27 <[email protected]> Co-authored-by: felixarntz <[email protected]> Co-authored-by: andrewserong < [email protected]> Co-authored-by: joemcgill < [email protected]>

…erge when merging background styles (#66002) Backporting the current state of WordPress/wordpress-develop#7486 and adding tests. Co-authored-by: ramonjd <[email protected]> Co-authored-by: mukeshpanchal27 <[email protected]> Co-authored-by: felixarntz <[email protected]> Co-authored-by: andrewserong < [email protected]> Co-authored-by: joemcgill < [email protected]> Source: WordPress/gutenberg@2cd6929

…erge when merging background styles (#66002) Backporting the current state of WordPress/wordpress-develop#7486 and adding tests. Co-authored-by: ramonjd <[email protected]> Co-authored-by: mukeshpanchal27 <[email protected]> Co-authored-by: felixarntz <[email protected]> Co-authored-by: andrewserong < [email protected]> Co-authored-by: joemcgill < [email protected]>

…erge when merging background styles (WordPress#66002) Backporting the current state of WordPress/wordpress-develop#7486 and adding tests. Co-authored-by: ramonjd <[email protected]> Co-authored-by: mukeshpanchal27 <[email protected]> Co-authored-by: felixarntz <[email protected]> Co-authored-by: andrewserong < [email protected]> Co-authored-by: joemcgill < [email protected]>

Improve performance

c13a398

mukeshpanchal27 requested review from joemcgill, ramonjd and andrewserong October 3, 2024 07:29

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

mukeshpanchal27 mentioned this pull request Oct 3, 2024

Performance Benchmarks for WordPress 6.7 WordPress/performance#1572

Open

felixarntz requested changes Oct 3, 2024

View reviewed changes

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 added 2 commits October 7, 2024 11:17

Merge branch 'WordPress:trunk' into fix/61858-performance-regression

2b7be60

Introduce get_block_node_paths()

46640a7

mukeshpanchal27 requested a review from felixarntz October 7, 2024 06:33

mukeshpanchal27 self-assigned this Oct 7, 2024

felixarntz reviewed Oct 7, 2024

View reviewed changes

Add options that include node path and block elements

76e0140

mukeshpanchal27 commented Oct 8, 2024

View reviewed changes

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved

Apply suggestions from code review

82dc69a

mukeshpanchal27 requested a review from felixarntz October 8, 2024 10:57

felixarntz reviewed Oct 8, 2024

View reviewed changes

mukeshpanchal27 added 2 commits October 9, 2024 10:31

Remove "include_block_elements" parameter

867ae4a

Address review feedbacks

dbbd010

mukeshpanchal27 requested a review from felixarntz October 9, 2024 05:38

felixarntz approved these changes Oct 9, 2024

View reviewed changes

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved

ramonjd reviewed Oct 9, 2024

View reviewed changes

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved

src/wp-includes/class-wp-theme-json.php Show resolved Hide resolved

ramonjd added a commit to WordPress/gutenberg that referenced this pull request Oct 10, 2024

Backporting the current state of WordPress/wordpress-develop#7486 and…

482b786

… adding tests.

ramonjd mentioned this pull request Oct 10, 2024

Backport from WordPress Core: improve performance of WP_Theme_JSON::merge when merging background styles WordPress/gutenberg#66002

Merged

mukeshpanchal27 and others added 4 commits October 10, 2024 11:24

Merge branch 'WordPress:trunk' into fix/61858-performance-regression

bdf769c

Remove variable

2adb26a

Co-authored-by: Felix Arntz <[email protected]>

Update new variable description

58dfa83

Unit tests for new changes

35e4ed4

mukeshpanchal27 requested a review from ramonjd October 10, 2024 06:53

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

joemcgill reviewed Oct 10, 2024

View reviewed changes

felixarntz added 3 commits October 10, 2024 13:48

Merge branch 'trunk' into fix/61858-performance-regression

f1237f8

Add since annotation for the method change.

115dc4e

Move option variable assignments before the loops.

0eb736b

felixarntz closed this Oct 10, 2024

ramonjd added a commit to WordPress/gutenberg that referenced this pull request Oct 10, 2024

Backporting the current state of WordPress/wordpress-develop#7486 and…

9c21ff9

… adding tests.

ramonjd mentioned this pull request Nov 13, 2024

Theme JSON: include block style variations in path only output of get_block_nodes #7784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mukeshpanchal27 commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

mukeshpanchal27 commented Oct 3, 2024 •

edited

Loading

github-actions bot commented Oct 3, 2024 •

edited

Loading

felixarntz left a comment

mukeshpanchal27 commented Oct 7, 2024 •

edited

Loading

felixarntz left a comment

felixarntz Oct 7, 2024

felixarntz Oct 7, 2024

andrewserong commented Oct 8, 2024

mukeshpanchal27 commented Oct 8, 2024

felixarntz left a comment

mukeshpanchal27 commented Oct 9, 2024

andrewserong commented Oct 9, 2024

mukeshpanchal27 commented Oct 9, 2024

felixarntz commented Oct 9, 2024

felixarntz left a comment

andrewserong commented Oct 9, 2024

felixarntz commented Oct 9, 2024

andrewserong commented Oct 9, 2024

ramonjd commented Oct 9, 2024

ramonjd commented Oct 10, 2024

mukeshpanchal27 commented Oct 10, 2024

ramonjd commented Oct 10, 2024

joemcgill left a comment •

edited

Loading

joemcgill Oct 10, 2024

joemcgill Oct 10, 2024

felixarntz commented Oct 10, 2024

felixarntz commented Oct 10, 2024

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

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

Conversation

mukeshpanchal27 commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

Test using WordPress Playground

Some things to be aware of

mukeshpanchal27 commented Oct 3, 2024 • edited Loading

Server-timing After the PR

github-actions bot commented Oct 3, 2024 • edited Loading

felixarntz left a comment

Choose a reason for hiding this comment

mukeshpanchal27 commented Oct 7, 2024 • edited Loading

Server-timing After the PR implementation.

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz Oct 7, 2024

Choose a reason for hiding this comment

felixarntz Oct 7, 2024

Choose a reason for hiding this comment

andrewserong commented Oct 8, 2024

mukeshpanchal27 commented Oct 8, 2024

felixarntz left a comment

Choose a reason for hiding this comment

mukeshpanchal27 commented Oct 9, 2024

Server-timing After the PR implementation.

andrewserong commented Oct 9, 2024

mukeshpanchal27 commented Oct 9, 2024

felixarntz commented Oct 9, 2024

felixarntz left a comment

Choose a reason for hiding this comment

andrewserong commented Oct 9, 2024

felixarntz commented Oct 9, 2024

andrewserong commented Oct 9, 2024

ramonjd commented Oct 9, 2024

ramonjd commented Oct 10, 2024

mukeshpanchal27 commented Oct 10, 2024

ramonjd commented Oct 10, 2024

joemcgill left a comment • edited Loading

Choose a reason for hiding this comment

joemcgill Oct 10, 2024

Choose a reason for hiding this comment

joemcgill Oct 10, 2024

Choose a reason for hiding this comment

felixarntz commented Oct 10, 2024

felixarntz commented Oct 10, 2024

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

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

mukeshpanchal27 commented Oct 3, 2024 •

edited

Loading

github-actions bot commented Oct 3, 2024 •

edited

Loading

mukeshpanchal27 commented Oct 7, 2024 •

edited

Loading

joemcgill left a comment •

edited

Loading