Block Supports: Re-use instance of Tag Processor when adding layout classes

[dmsnell]

This ticket tracks the merging of ​Gutenberg PR 54075 into Core.

Refactors block supports to use a single Tag Processor instance and to only send a single class at a time to add_class(). This is preparatory work for a change in the Tag Processor so that its behavior more closely matches its naming (add_class() is singular and should only accept a single class).

There should be no functional changes in this patch, though the re-use of the Tag Processor should have a negligible positive impact on performance.

Since I'm not the author of the block supports code I'm at a disadvantage for scrutinizing the changes, though the update has been running fine in Gutenberg for three weeks. That suggests the most critical review necessary here is whether this patch is a proper backport from the Gutenberg code, since the two block supports functions are different in each repository.

References:

• ​https://github.com/wordpress/gutenberg/pull/54075

Trac ticket: #59443

See WordPress/gutenberg#54075

Refactors block supports to use a single Tag Processor instance and to only send a single class at a time to add_class().

Trac ticket: #59443

See WordPress/gutenberg#54075

Refactors block supports to use a single Tag Processor instance and to only send a single class at a time to add_class().

Any reason this is still in draft?

not sure; I probably didn't have time at the end of the day to create the Trac ticket and so left it as a draft. created that now.

[isabel_brison]

In 56698:

Editor: make better use of Tag Processor in layout block support.

Refactors layout support to use a single Tag Processor instance and send one class at a time to add_class().

Props dmsnell, hellofromTonya.
Fixes #59443.

Committed in r56698.

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

Follow-up to #5252: replaces uses of null coalescing operator in that PR due to it not being officially supported in WP yet.

[isabel_brison]

In 56700:

Editor: remove null coalescing operator from layout.

Replaces uses of null coalescing operator in [56698].

Props dmsnell, mukesh27.
See #59443.

Committed in r56700.

[spacedmonkey]

While testing the WordPress 6.4 release, I noticed that wp_render_layout_support_flag is now using a lot of resources. This seems to be because this function gets calls a lot ( around 300-400 times per page load ) and uses WP_HTML_Tag_Processor to update html markup. See screenshot attached.

Reopenning ticket to see if we can fix the performance regression here.

CC @isabel_brison @dmsnell for their thoughts as they committed this. Also flagging to @joemcgill and @flixos90 for performance reviews of patches.

[dmsnell]

@spacedmonkey I think this probably is more related to #57584 which introduced the change from regex to HTML API. this patch is an attempt at removing some double-work from that patch.

[spacedmonkey]

Replying to dmsnell:

@spacedmonkey I think this probably is more related to #57584 which introduced the change from regex to HTML API. this patch is an attempt at removing some double-work from that patch.

That ticket was merged back in Feb, meaning it was in other release. The commit in the change, at the very least made it worse, as I am not seeing this performance issue in WP 6.3.

[spacedmonkey]

I have highlight wp_render_layout_support_flag as a possible issue.

Here is data from different themes and WP versions.

The data doesn't paint a clear picture. In some cases it faster and others much slower.

The most worrying thing here, is that the function time in TT4, where the function is called less but is much more function server time.

TT4 theme

Version	Function time	Times called	Total server time
WP 6.3 - TT4	11.8 ms	310	153ms
WP 6.4 beta 2 - TT4	18.1ms	220	174ms

Compare ​profiles

TT3 theme

Version	Function time	Times called	Total server time
WP 6.3 - TT3	4.8 ms	452	151ms
WP 6.4 beta 2 - TT3	5.69 ms	440	142ms

Compare ​profiles

TT1 theme

Version	Function time	Times called	Total server time
WP 6.3 - TT1	12 ms	808	237ms
WP 6.4 beta 2 - TT1	11.3 ms	797	227ms

Compare ​profiles.

[isabel_brison]

The only thing that changed here is that we're now calling the add_class method once for each classname, whereas before we were calling it once overall and adding all the classnames in one go. There are usually 2-4 classnames in each layout block, so that could explain the change.

I'm not sure there's much we can do about it, given that add_class has ceased supporting strings of multiple classes. @dmsnell I don't suppose the newly introduced class_list method would be any more performant?

[dmsnell]

I would like to see some benchmarks against the revert of this patch @isabel_brison before I personally make any conclusions - compare the performance with and without it. maybe I messed something up in the refactor.

[spacedmonkey]

Some early finds for my research.

Of the 20ms that wp_render_layout_support_flag takes on a TT4 page.

• wp_get_layout_style - 5.25 ms
• wp_get_global_settings - 4.06ms
• sanative_title - 3.06ms
• WP_HTML_Tag_Processor::get_updated_htm - 2.46ms.

It seems this change creates lots of WP_HTML_Tag_Processor instance, where before ( in WP 6.3 ) those instances were only created if needed. WP_HTML_Tag_Processor is done on innner content in a while loop. A while loop is also used to next tag.

More logic is run if disable-layout-styles theme supports in place. So different themes will see different results. Which explains some of the different noted in findings above.

Running so many html modifications on every block, feels like it would not be good for performance. Some of these blocks could contain lots of html / text, making the html parsing process slow.

I don't have a context to what these changes did. I wonder if we could add some conditional logic to make sure that expressive function / class classes are avoid, I think that would be helpful for performance.

[dmsnell]

WP_HTML_Tag_Processor is done on innner content in a while loop.

@spacedmonkey are you referring to do_blocks() here? to which while loop are you referring?

A while loop is also used to next tag.

this is normative within the HTML API; an unfortunate requirement to avoid breaking the HTML is to scan through it.

I wonder if we could add some conditional logic to make sure that expressive function / class classes are avoid, I think that would be helpful for performance.

absolutely true, but it would also be good to remember why these are in place. the expressive functions are not the goal, but a happy side-effect of building a safe system. the motivation for and guiding principles of the HTML API are eliminating the security vulnerabilities and data corruption that have plagued WordPress for the past couple decades, and which are continually added in new code.

if we can find a way to avoid processing HTML that would be even better; until then, there's a tradeoff between moving faster and avoiding breakage.

I do agree that this function could use some help; particularly the bit where it tries to find an inner-block wrapping element, since that's not only non-deterministic here, but it's not guaranteed in any way to be correct, which is something I tried to note in this refactor (which itself was motivated by things you are raising: before the refactor we had some superfluous instantiation of the class and needless re-parsing and iteration).

I'm not sure of any immediate solutions, but it does seem like we could find a way to communicate when we're entering and existing the render of inner blocks, and that would eliminate the need to search for a fuzzy target the way this is.

Thanks for your continued measurements and sharing. Have you compared this performance against the revert of this patch yet?

[spacedmonkey]

@dmsnell @isabel_brison Can I understand why this function was changed and no unit tests seem to have been added? It is hard to refactor this code to improve performance when there are no unit tests to prove how the function is meant to work and that your change has not broken anything.

I will provide number of before and after this commit when I get sometime.

[spacedmonkey]

Still trying to understand the point of this ticket. This seems like a code refactor. Considering there are rules around this, see the ​docs.

Code refactoring should not be done just because we can.

What new functionality or bug does this code fix?

[hellofromTonya]

Noting:
#59544 has been opened as a possible performance fix to the problem noted by @spacedmonkey for reopening this ticket (see comment:14).

[spacedmonkey]

@spacedmonkey are you referring to do_blocks() here? to which while loop are you referring?

While loop here.

                $first_chunk_processor = new WP_HTML_Tag_Processor( $first_chunk );
                while ( $first_chunk_processor->next_tag() ) {
                        $class_attribute = $first_chunk_processor->get_attribute( 'class' );
                        if ( is_string( $class_attribute ) && ! empty( $class_attribute ) ) {
                                $inner_block_wrapper_classes = $class_attribute;
                        }
                }

[dmsnell]

thanks for your continued diligence in this @spacedmonkey - I do suggest first though that you revert the patch and run your benchmarks against that revert, as I've asked several times already, as that will give us a clear answer to the questions about this particular refactor and you won't be spending your time trying to refactor if that doesn't turn up any results.

I'm still confused why we keep discussing this when we don't have any comparisons before and after this patch applies to demonstrate that this is a problem or introduced a regression.

on your other questions though:

Can I understand why this function was changed and no unit tests seem to have been added?

As stated in the ​original description of the changes, this function was updated to address some hygiene issues with the HTML API in Core and to prevent future breakage because of the way this was written. I didn't catch it when the Tag Processor was initially introduced and I am trying to ensure that Core sets proper examples because people will be copying them. In the description above I also repeated that this change is necessary to prevent a breakage to Core when an oddity in the Tag Processor is fixed later on.

As a refactor with no functional changes I was relying on existing tests, and also didn't know what should be tested or how. My goal was to make things clearer and remove an obvious inefficiency.

Still trying to understand the point of this ticket. This seems like a code refactor. Considering there are rules around this, see the docs…What new functionality or bug does this code fix?

These have been repeated in multiple places, so what's still unclear? I'm sorry I'm having trouble clearly communicating this.

WP_HTML_Tag_Processor is done on inner content in a while loop
While loop here.

Ah okay, well this isn't creating new tag processor instances here. It's scanning through the HTML tags to try and find the wrapping HTML element for the inner blocks. As noted in this patch, after spending much effort to figure out what the original purpose of the code was, this is not a reliable or particularly great way to do this, and something better surely exists.

Don't shoot me: I only tried to make it clearer after my own investigation.

Also please take note that before this change it was normative to create three instances of the Tag Processor and afterwards there's only one.

---

@hellofromTonya I opened #59544 as part of addressing the overall performance issues in the 6.4 branch, but unrelated to this ticket. I'm still waiting to see evidence that this ticket introduces a performance regression. It addresses the rendering pipeline, where I have been seeing extra time spent during the render of the homepage for twentytwentyfour.

For the big changes to this function, around which this discussion is taking place, introduced in #57584, there's an obvious oddity in how it's trying to find the inner block wrapper, assuming it exists. That's a great question that maybe @isabel_brison or @flixos90 can help elucidate. We're scanning HTML on render to find a place that we ostensibly could indicate directly at a different stage of the render process. I'm sure this is hard stuff and other options were tried. Before #57584 the inherent logic was simpler, so it's not a surprise to me that we're seeing different performance characteristics afterwards.

This being said, I have some ideas for how to address this more broadly, but probably not in a way that we can address it for 6.4. I think there's a reasonable idea in there somewhere to mark block boundaries during render and then remove them on final output for pages. There are some opportunities with Unicode ranges that allow us to do this in a safe and benign way, but any broad fix is going to need lots of input and vetting before pushing it out; not a beta-timeline process IMO.

[isabel_brison]

Thanks for your detailed reply @dmsnell!

For the big changes to this function, around which this discussion is taking place, introduced in #57584, there's an obvious oddity in how it's trying to find the inner block wrapper, assuming it exists.

For context, the relevant changes in Gutenberg were from this PR: ​https://github.com/WordPress/gutenberg/pull/44600 (which was incidentally the first use of the HTML tag processor, before it landed in core even). There's a fair amount of discussion in the PR, but ​https://github.com/WordPress/gutenberg/pull/44600#issuecomment-1269334562 points to why we ended up with such a roundabout solution: InnerBlocks couldn't reliably be used for all blocks.

This is not to say we can't come up with a better solution, given time! There are many different implementations and states of container blocks that have to be taken into account though, so that's definitely not a task for this release.

[spacedmonkey]

As requested, here is data from reverting this change.

Trunk is core commit [56766]. Here is data from different block themes.

TT4

Version	Function time	Times called	Total server time
Trunk	18.9 ms	279	176ms
Revert	30.4 ms	279	184ms

TT3

Version	Function time	Times called	Total server time
Trunk	5.98 ms	444	152ms
Revert	10 ms	444	148ms

Ollie

Version	Function time	Times called	Total server time
Trunk	17.3 ms	509	226ms
Revert	33.4 ms	509	242ms

Frost

Version	Function time	Times called	Total server time
Trunk	8.68 ms	66	111ms
Revert	14.4 ms	66	114ms

So, just reverting this change, we are seeing a net benefit. Which is confusing, is in the first run the function was slower. What other changes could have effect this function in WP 6.4. This function also result is around 7-17% of server time. If we can look at the performance of this function and make some improvements, it will be well worth it.

[isabel_brison]

@spacedmonkey I might be missing something, but it seems like in that data trunk is faster than revert.

[spacedmonkey]

To be clear, I am seeing this function be faster is some cases and slower in others. Reverting this single function back to the WP 6.3 release, it seems it is faster the version in trunk. It does mean the original observations were incorrect, it just means that it is unclear that this commit was the thing that changed to make this function slower.

As there is no clear path forward to fix this function or make it faster and it may just be slower in some context ( which is not always a bad thing ).

Closing, will create a new ticket if there is an actionable fix.