Make WordPress Core

Opened 7 months ago

Closed 2 months ago

Last modified 8 weeks ago

#59532 closed defect (bug) (fixed)

Avoid registering block-patterns on the frontend

Reported by: aristath's profile aristath Owned by: thekt12's profile thekt12
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.4
Component: Editor Keywords: has-patch has-unit-tests
Focuses: performance Cc:

Description

Registering block patterns from a theme is an expensive process - especially when the theme includes many patterns (like the default twentytwenty-four theme).

Block patterns are not needed for frontend requests, so we can simply skip registering them for requests that don't need them.

Change History (34)

This ticket was mentioned in PR #5397 on WordPress/wordpress-develop by @aristath.


7 months ago
#1

  • Keywords has-patch added

#2 @spacedmonkey
7 months ago

  • Keywords has-patch removed

To be clear, are you talking about the including the markup of the contents of the file? What about block patterns register from a plugin?

#3 @aristath
7 months ago

  • Keywords has-patch added

#4 @spacedmonkey
7 months ago

Registering block patterns from a theme is an expensive process -

@aristath Can you explain what is expensive about it? Storing an array of data is pretty cheap. To me the expensive part is the output buffering, something we could do lazily.

#6 @spacedmonkey
7 months ago

@aristath I have put together a PR to lazily load block patterns.

https://github.com/WordPress/wordpress-develop/pull/5398

Btw, block patterns are used on the front end. I am seeing block patterns being using in TT4 theme.

@aristath commented on PR #5397:


7 months ago
#7

Closing in favor of https://github.com/WordPress/wordpress-develop/pull/5398
Lazy-loading patterns is a better solution.

#8 @gziolo
7 months ago

Last edited 7 months ago by gziolo (previous) (diff)

#9 @Bernhard Reiter
7 months ago

Tangentially related: #59541.

#10 @hellofromTonya
6 months ago

  • Component changed from General to Editor
  • Milestone changed from Awaiting Review to 6.5

Pulling into 6.5.

#11 @joemcgill
4 months ago

Given that patterns can be referenced in templates and will need to be registered in order to render properly, we either need to find a way of registering these on demand (i.e., lazy loading) rather than loading them all on every request, or find a way of making this registration process more performant.

For the latter, improving the caching strategy for block patterns (related #60120) should help, but finding additional ways of making this process more performant without persistently caching patter data would be good to explore.

This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


3 months ago

This ticket was mentioned in PR #5941 on WordPress/wordpress-develop by @thekt12.


3 months ago
#13

This is just an enhancement made to to original PR - https://github.com/WordPress/wordpress-develop/pull/5398, to adapt to the new changes in the trunk.

The idea is to load the pattern file's content only when the pattern is required.

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

## Performance Improvement

We do see an improvement of ~5 ms for TT4. This improvement mainly comes from running include $files_path only when we need the content. I feel this is the best we can do for now as the functions were optimised to get extended and parsing the content was already happening in a JIT fashion.

Metric PR (mean) TT4 Trunk (mean) TT4
Response Time (median) 413.06 418.38
wp-load-alloptions-query (median) 3.05 3.065
wp-before-template (median) 190.84 196.95

#14 @thekt12
3 months ago

I have worked on @spacedmonkey POV and improved upon the code to fix some BC issues.

New PR is good for review https://github.com/WordPress/wordpress-develop/pull/5941

Performance benchmarking shows a small by consistent improvement for TT4 theme. However, little to no change was observed for TT3. (check PR for metrics)

I also do see an opportunity to improve performance further by persistent caching WP_Block_Patterns_Registry::prepare_content()

These are referred to in both WP_Block_Patterns_Registry::get_all_registered and WP_Block_Patterns_Registry::get_registered.

Next step:

  • Use the profiler to check overhead on WP_Block_Patterns_Registry::prepare_content() which seems to have some expensive operations like block parsing.
  • Add unit test

This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


3 months ago

#16 @mukesh27
3 months ago

  • Owner set to thekt12
  • Status changed from new to assigned

#17 @spacedmonkey
3 months ago

@thekt12 I recommend testing this patch with the themes highlighted in this comment https://core.trac.wordpress.org/ticket/59490#comment:16. They had ALOT of patterns. These themes are popular and well used.

@thekt12 commented on PR #5941:


3 months ago
#18

We tried getting values if we cache prepare_content values - https://github.com/10up/wordpress-develop/pull/82/files.

The initial result shows caching has slowed down the result, but I feel it was because of the use of serialize on a large array.

@joemcgill commented on PR #5398:


3 months ago
#19

Closing in favor of #5941. Thanks for the good start here!

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


2 months ago

@thekt12 commented on PR #5941:


2 months ago
#21

@joemcgill I have made a slight code refactor here after your last review. Instead of load_content_from_filepath I have introduced get_content, this will handle common code from get_registered and get_all_registered

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


2 months ago

#23 @joemcgill
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 57683:

Editor: Load pattern content only when used.

Previously, the content for all registered patterns would get loaded on each request when the patterns are registered. Instead, this stores the path the pattern file during registration and reads the content the first time the pattern is used, which is a performance optimization.

Props thekt12, spacedmonkey, gziolo, aristath, joemcgill.
Fixes #59532.

@joemcgill commented on PR #5941:


2 months ago
#24

Merged in https://core.trac.wordpress.org/changeset/57683. Thanks @kt-12. If you had additional tests you wanted to add, we can do those as follow-up commits, but I don't think they are necessary given that the public API hasn't changed and the existing tests cover these changes.

This ticket was mentioned in PR #6154 on WordPress/wordpress-develop by @thekt12.


2 months ago
#25

  • Keywords has-unit-tests added

Trac ticket: https://core.trac.wordpress.org/ticket/59532
Added unit test for #59532

@thekt12 commented on PR #5941:


2 months ago
#26

@joemcgill I have added 2 unit tests. Couldn't test it earlier due to mysql docker issue. Also, I had to open a

new one https://github.com/WordPress/wordpress-develop/pull/6154/files

@thekt12 commented on PR #6154:


2 months ago
#27

@joemcgill Doesn't set_up and tear_down work after each test? At the moment, even if I am not resetting the values after test case it's still working (else my second test case would have failed )

@thekt12 commented on PR #6154:


2 months ago
#28

@joemcgill Doesn't set_up and tear_down work after each test? At the moment, even if I am not resetting the values after test case it's still working (else my second test case would have failed )

#29 @joemcgill
2 months ago

In 57703:

Editor: Improve PHPUnit tests for block pattern registration.

This is a followup to [57683], which adds additional unit test coverage to ensure block pattern content is not loaded from files during registration, but instead when those patterns are accessed. This also improves the set_up and tear_down methods for the Tests_Blocks_wpBlockPattersRegistry test class to ensure that any modifications made to registered blocks during the tests are reset after each test.

Props thekt12, joemcgill.
See #59532.

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


8 weeks ago

#33 @joemcgill
8 weeks ago

In 57731:

Docs: Improve docblock for WP_Block_Patterns_Registry::register.

This documents the new filePath property supported by WP_Block_Patterns_Registry::register and also updates the property name to camel case formatting to be consistent with other block pattern properties.

Props thekt12, spacedmonkey, joemcgill.
See #59532.

Note: See TracTickets for help on using tickets.