Make WordPress Core

Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#59541 closed defect (bug) (wontfix)

Patterns: Insert hooked blocks upon registration

Reported by: bernhard-reiter's profile Bernhard Reiter Owned by: bernhard-reiter's profile Bernhard Reiter
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: performance Cc:

Description

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.

Change History (15)

This ticket was mentioned in PR #5406 on WordPress/wordpress-develop by @Bernhard Reiter.


9 months ago
#1

  • Keywords has-patch added

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

@Bernhard Reiter commented on PR #5406:


9 months ago
#2

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

#3 @joemcgill
9 months ago

  • Focuses performance added
  • Type changed from enhancement to defect (bug)

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.

@spacedmonkey commented on PR #5406:


9 months ago
#4

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

#6 @spacedmonkey
9 months ago

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

@spacedmonkey commented on PR #5406:


9 months ago
#7

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

@gziolo commented on PR #5421:


9 months ago
#8

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.

@Bernhard Reiter commented on PR #5406:


9 months ago
#9

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

#10 @Bernhard Reiter
9 months ago

  • Milestone 6.4 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

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.

@Bernhard Reiter commented on PR #5421:


9 months ago
#11

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

@flixos90 commented on PR #5421:


9 months ago
#12

@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 commented on PR #5421:


9 months ago
#13

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

@spacedmonkey commented on PR #5421:


9 months ago
#14

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.

@gziolo commented on PR #5421:


9 months ago
#15

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 .

Note: See TracTickets for help on using tickets.