Performance improvements on append_to_selector method #47833

spacedmonkey · 2023-02-07T13:16:34Z

What?

The append_to_selector method is called up to 1200 times per page load.

This does an explode / implode, even if the string does not need it. Do a simple check to see if the string contains a comma before doing the explode.

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions · 2023-02-07T13:44:26Z

Flaky tests detected in bee781c.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4114291451
📝 Reported issues:

[Flaky Test] List #47632 in specs/editor/blocks/query.test.js
[Flaky Test] should always expand single line selection #39787 in specs/editor/various/multi-block-selection.test.js
[Flaky Test] should forward delete across blocks #44353 in specs/editor/various/multi-block-selection.test.js

Mamaduka

The fix looks good to me. Thank you, @spacedmonkey!

Do you have a performance graph after this change is applied?

felixarntz · 2023-02-09T18:58:13Z

@spacedmonkey @Mamaduka @aristath I gave this a test in WordPress core trunk (adding the same change directly in there), with TT3 theme. I used the Performance Lab Server-Timing API from WordPress/performance#553, registering a metric for the WP_Theme_JSON::append_to_selector() method that measures its execution. I then used the median based on 50 requests.

In my setup, the method is called 1179 times per page load.
1106 of those 1179 times (93.8%) don't need the implode + explode, so great catch there.
Without this fix, the overall time that method consumes is 8.5ms; with this fix it's 8.14ms. That's an increase of 4.3%, but overall it's less than a millisecond, so a very small benefit.
Either way, >8ms for a single function is a whole lot (about ~7% of total WP execution time in my testing). So maybe there's even more we can improve.

I looked into the problem a bit further, particularly in how this method is being called.

1023 of those 1179 times (86.7%) the method is called with $position set to left.
The method is pretty much always called with different values, so caching won't help here indeed.
However, the main problem here is that this method is called so often and runs those checks all the time. This is really unnecessary as we already know it when calling the method; this is one of the reasons why using parameters to control what a function does is a bad practice.

I think this change here looks good to merge for now. But I also think we should (in a separate PR) explore implementing this highly utilized method in a cleaner way. Instead of append_to_selector( $selector, $to_append, $position ), we should have the following much simpler utility methods:

append_to_selector( $selector, $to_append )
prepend_to_selector( $selector, $to_prepend )
append_to_selectors( $selectors, $to_append )
prepend_to_selectors( $selectors, $to_prepend )

With that, these's no if conditions anywhere, and only what is actually needed for each of the 4 scenarios is run. In all places where append_to_selector is called today, we'll know right away which of those 4 methods to call instead, so migrating to this logic should be trivial.

t-hamano · 2023-03-05T08:31:13Z

I came across this PR while researching #48706.
The issue is that in theme.json, when a style is applied to a heading element in a particular block, the correct selector is not generated. If I reverted this PR change, it fixes the issue reported in #48706.

t-hamano · 2023-03-05T08:32:13Z

lib/class-wp-theme-json-gutenberg.php

@@ -785,6 +785,9 @@ protected static function sanitize( $input, $valid_block_names, $valid_element_n
 	 * @return string The new selector.
 	 */
 	protected static function append_to_selector( $selector, $to_append, $position = 'right' ) {
+		if ( ! str_contains( ',', $selector ) ) {


Perhaps the order of the arguments is reversed?
See: https://www.php.net/manual/en/function.str-contains.php

felixarntz · 2023-05-02T21:46:43Z

Related: https://core.trac.wordpress.org/ticket/58193 and WordPress/wordpress-develop#4380

I'll work on porting this over to Gutenberg.

oandregal · 2023-05-05T12:19:43Z

Backport at WordPress/wordpress-develop#4412

Improve `append_to_selector` method in `WP_Theme_JSON` by checking to see if string contains a common before imploding / exploding string, which improves performance. Originally developed and tested in [WordPress/gutenberg#47833 Gutenberg PR 47833]. Props spacedmonkey, flixos90, mukesh27, joemcgill, wildworks, oandregal, mamaduka. Fixes #58231. git-svn-id: https://develop.svn.wordpress.org/trunk@55907 602fd350-edb4-49c9-b593-d223f7449a82

spacedmonkey · 2023-06-12T11:21:58Z

Backported https://core.trac.wordpress.org/changeset/55907

Improve `append_to_selector` method in `WP_Theme_JSON` by checking to see if string contains a common before imploding / exploding string, which improves performance. Originally developed and tested in [WordPress/gutenberg#47833 Gutenberg PR 47833]. Props spacedmonkey, flixos90, mukesh27, joemcgill, wildworks, oandregal, mamaduka. Fixes #58231. Built from https://develop.svn.wordpress.org/trunk@55907 git-svn-id: http://core.svn.wordpress.org/trunk@55419 1a063a9b-81f0-0310-95a4-ce76da25c4cd

Improve `append_to_selector` method in `WP_Theme_JSON` by checking to see if string contains a common before imploding / exploding string, which improves performance. Originally developed and tested in [WordPress/gutenberg#47833 Gutenberg PR 47833]. Props spacedmonkey, flixos90, mukesh27, joemcgill, wildworks, oandregal, mamaduka. Fixes #58231. Built from https://develop.svn.wordpress.org/trunk@55907 git-svn-id: https://core.svn.wordpress.org/trunk@55419 1a063a9b-81f0-0310-95a4-ce76da25c4cd

Do not run explode / implode if no comma is in the string.

bee781c

spacedmonkey requested review from Mamaduka, oandregal, aristath, felixarntz, tellthemachines and jorgefilipecosta February 7, 2023 13:16

spacedmonkey self-assigned this Feb 7, 2023

Mamaduka approved these changes Feb 7, 2023

View reviewed changes

aristath approved these changes Feb 9, 2023

View reviewed changes

felixarntz approved these changes Feb 9, 2023

View reviewed changes

spacedmonkey merged commit bb61b4c into trunk Feb 10, 2023

spacedmonkey deleted the fix/append_to_selector branch February 10, 2023 17:26

github-actions bot added this to the Gutenberg 15.2 milestone Feb 10, 2023

DaisyOlsen added [Type] Performance Related to performance efforts [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. labels Feb 14, 2023

t-hamano reviewed Mar 5, 2023

View reviewed changes

This was referenced Mar 5, 2023

Theme.json: Heading in selectors aren't set correctly at the block level #48706

Closed

Button Block: Style for pseudo-elements has priority #48393

Closed

Fix: Incorrect selector generated by append_to_selector method #48759

Merged

spacedmonkey added the Needs PHP backport Needs PHP backport to Core label Apr 28, 2023

felixarntz mentioned this pull request May 2, 2023

Introduce prepend_to_selector() to avoid additional if checks and follow single responsibility principle #50266

Merged

oandregal mentioned this pull request May 5, 2023

Backport change. WordPress/wordpress-develop#4412

Closed

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

Performance improvements on append_to_selector method #47833

Performance improvements on append_to_selector method #47833

spacedmonkey commented Feb 7, 2023

github-actions bot commented Feb 7, 2023

Mamaduka left a comment •

edited

Loading

felixarntz commented Feb 9, 2023 •

edited

Loading

t-hamano commented Mar 5, 2023 •

edited

Loading

t-hamano Mar 5, 2023

felixarntz commented May 2, 2023

oandregal commented May 5, 2023

spacedmonkey commented Jun 12, 2023

Performance improvements on append_to_selector method #47833

Performance improvements on append_to_selector method #47833

Conversation

spacedmonkey commented Feb 7, 2023

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Feb 7, 2023

Mamaduka left a comment • edited Loading

Choose a reason for hiding this comment

felixarntz commented Feb 9, 2023 • edited Loading

t-hamano commented Mar 5, 2023 • edited Loading

t-hamano Mar 5, 2023

Choose a reason for hiding this comment

felixarntz commented May 2, 2023

oandregal commented May 5, 2023

spacedmonkey commented Jun 12, 2023

Mamaduka left a comment •

edited

Loading

felixarntz commented Feb 9, 2023 •

edited

Loading

t-hamano commented Mar 5, 2023 •

edited

Loading