Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patterns: Insert hooked blocks upon registration #5406

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Oct 4, 2023

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


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.

@ockham ockham self-assigned this Oct 4, 2023
@ockham ockham requested a review from gziolo October 4, 2023 12:49
@ockham
Copy link
Contributor Author

ockham commented Oct 4, 2023

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

@spacedmonkey
Copy link
Member

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

@spacedmonkey
Copy link
Member

I put together my own version of this PR for testing #5421

@ockham
Copy link
Contributor Author

ockham commented Oct 9, 2023

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

@ockham ockham closed this Oct 9, 2023
@ockham ockham deleted the update/patterns-insert-hooked-blocks-upon-registration branch October 9, 2023 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants