Patterns: Insert hooked blocks upon registration

[Bernhard Reiter]

It has been brought up that performance for block themes that make heavy use of patterns is currently lacking.

The new Block Hooks feature (introduced in #59313) needs to have hooked blocks automatically inserted into patterns. Since the API surface for patterns is fairly small (it's basically just the WP_Block_Patterns_Registry class, which has only a few methods to register/unregister and look up patterns), this was originally done at lookup time, i.e. in that class's get_registered and get_all_registered methods.

As an alternative, we could consider inserting hooked blocks during registration of patterns -- assuming that a given pattern might be looked up multiple time, but will typically only be registered once.

As of [56733], we have some test coverage for the patterns registry, which should provide a certain level of protection against regressions.

It has been brought up that performance for block themes that make heavy use of patterns is currently lacking.

The new Block Hooks feature (introduced in #5261) needs to have hooked blocks automatically inserted into patterns. Since the API surface for patterns is fairly small (it's basically just the WP_Block_Patterns_Registry class, which has only a few methods to register/unregister and look up patterns), this was originally done at lookup time, i.e. in that class's get_registered and get_all_registered methods.

As an alternative, we could consider inserting hooked blocks during registration of patterns -- assuming that a given pattern might be looked up multiple time, but will typically only be registered once.

As of #5299, we have some test coverage for the patterns registry, which should provide a certain level of protection against regressions.

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

Not working as expected 😕 I guess it's a loading order problem -- patterns might be registered prior to (3rd-party) hooked blocks? 🤔

[joemcgill]

Adding the performance focus for visibility. @bernhard-reiter given that you added this to the 6.4 milestone, I'm thinking this should be tracked as a bug for an existing feature added in this release, rather than an enhancement. Feel free to change back if I'm incorrect.

@ockham I am seeing this PR being much slower than trunk in my testing. See ​https://blackfire.io/profiles/compare/98fe898f-c932-4299-bab8-1d2faa6a51e0/graph

Basically for TT4, there are 50 patterns. Before it was only parsing the patterns if was using. Now it is parsing the lot!

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

[spacedmonkey]

@bernhard-reiter I put together my ​own version of your PR, that caches the parsed blocks are demand. Can you take a look?

I put together my own version of this PR for testing ​https://github.com/WordPress/wordpress-develop/pull/5421

In my testing ​https://github.com/WordPress/wordpress-develop/pull/5399, I also extracted the logic for updating the content of the pattern to a new method. I was validating other refactoring, though. So, both PRs separately could potentially improve performance.

@ockham I am seeing this PR being much slower than trunk in my testing. See ​https://blackfire.io/profiles/compare/98fe898f-c932-4299-bab8-1d2faa6a51e0/graph

Thank you for profiling, @spacedmonkey! That, alongside the fact that the change is actually breaking block insertion, is a pretty compelling reason for me to close this PR 😅

Basically for TT4, there are 50 patterns. Before it was only parsing the patterns if was using. Now it is parsing the lot!

Indeed 😕 FWIW, the patterns REST API controller uses WP_Block_Patterns_Registry::get_all_registered(), so I was a bit concerned about running hooked block insertion for all patterns whenever the editor is used; but I agree that that's still the right tradeoff to make if the alternative is to slow down the frontend.

[Bernhard Reiter]

Since @spacedmonkey's profiling has shown that this PR actually slows down the frontend, and, worse, actually breaks hooked block insertion (likely due to loading order issues), I'm going to close this ticket.

This is a promising approach 🤔 Maybe I closed the ticket too early 😅 (although arguably, this PR does something different than what the ticket says, so it might be worth filing a new one).

Let's maybe run some numbers on this one, and rebase it once @gziolo's ​https://github.com/WordPress/wordpress-develop/pull/5399 lands (in order to use the extracted parsing and hooked blocks insertion logic).

@spacedmonkey Now that #5399 has been committed, there is a merge conflict here. Could you please address it?

Curious to run benchmarks on this once it's up to date with latest trunk.

@spacedmonkey Now that #5399 has been committed, there is a merge conflict here. Could you please address it?

Curious to run benchmarks on this once it's up to date with latest trunk.

I will provide benchmarks tomorrow.

So after some testing, it seems like there is little performance benefit from this change now. All the performance benefit seems to have been committed in ​https://github.com/WordPress/wordpress-develop/pull/5399.

This PR might be useful for times where there the same pattern is used multiple times on a page. Not in all the example themes I found, I could not find an example where it did this. I am also worried that the filters would not run if this result was cached.

I still think there is something here, but it needs more work and is unlikely to make it's way into core in this release. Closing for now.

I see that the ticket got reopened. It would be great to run the benchmarks again with the latest trunk. Related Slack comment with a better context (accessing the link might require registration at ​https://make.wordpress.org/chat/):
​https://wordpress.slack.com/archives/C02RQBWTW/p1696950609355169?thread_ts=1696880738.627909&cid=C02RQBWTW .