Performance issues in _register_theme_block_patterns

[spacedmonkey]

During the testing of the TT4 theme in preparation for the WordPress 6.4 release, it came to our attention that the _register_theme_block_patterns function is imposing a significant resource overhead. This issue primarily stems from themes, such as TT4, that register a substantial number of block patterns. These patterns necessitate numerous file operations, including file lookups, file reading into memory, and related processes. To provide an overview, the _register_theme_block_patterns function currently performs the following file operations:

• is_dir
• is_readable
• file_exists
• glob
• file_get_contents (utilized via get_file_data)

Additionally, the function reads the entire pattern into memory through an inclusion process. However, it is important to note that these pattern files remain unchanged unless theme files are modified or updated. Consequently, conducting these file operations on every page request appears unnecessary and contributes to increased resource consumption.

To address this issue, we propose the implementation of caching mechanisms and lazy loading techniques within the _register_theme_block_patterns function. This optimization will substantially reduce the frequency of file lookups, enhance performance, and alleviate the resource-intensive nature of the function.

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

[spacedmonkey]

Updated my PR with unit tests and improved code. It is ready for review.

For my PR, I run benchmarks, PHP 7.4, 1000 runs.

Theme 2021

	Trunk	PR
Response Time (median)	59.61	59.39
wp-load-alloptions-query (median)	0.93	0.93
wp-before-template (median)	23.64	23.46
wp-template (median)	32.78	32.61
wp-total (median)	56.46	56.21

Theme 2023

	Trunk	PR
Response Time (median)	87.47	82.8
wp-load-alloptions-query (median)	0.93	0.92
wp-before-template (median)	39.24	36.03
wp-template (median)	44.43	43.36
wp-total (median)	83.72	79.39

Theme 2024

	Trunk	PR
Response Time (median)	124.19	98.33
wp-load-alloptions-query (median)	0.93	0.94
wp-before-template (median)	59.31	36.22
wp-template (median)	60.53	58.19
wp-total (median)	120.44	94.51

As you can see from these profiles, there is a net benefit for all themes ( as it save the file lookups ). But the benefit is clean for TT4 theme.

As this change has unit tests, I believe it should be considered for commiting in WP 6.4.

CC @flixos90 @joemcgill for review.

Spent some time on working on. Using transinets didnt make sense. Saving the cache to a network option, means the in single site, it results in option that is not autoloaded. Same with site transients.

Using network options, means that the cache is network wide, but does single site, caches are invalidated in on switch to theme. This is because on a single site, there is a no need to keep this cache in an autoloaded.

Some years ago caching of theme files was introduced in r41806 for WP#6531 but subsequently removed in r42242 as it was causing more problems than it solved, so you'll need to be cautions about doing this.

cc @westonruter as I think you were involved on the previous ticket.

Some years ago caching of theme files was introduced in r41806 for WP#6531 but subsequently removed in r42242 as it was causing more problems than it solved, so you'll need to be cautions about doing this.

cc @westonruter as I think you were involved on the previous ticket.

Something else that has happened since Core-6531 is the introduction of wp_is_development_mode(). Now that you can turn off theme caching by defining WP_DEVELOPMENT_MODE to theme, the issue of stale caches should be pretty much a non-issue. Or at least, there is an established way to address the issue.

@peterwilsoncc @westonruter It is worth noting that the file content is not cached, just the headers. This means the developers that update the file content, that will be reflected. Just removing and adding files might be cached.

I'll also note that Core-6531 was really only about the performance of the theme/plugin editors in the wp-admin. Caching block patterns is much more important because it impacts frontend performance.

@spacedmonkey @westonruter Thanks both, I figure it's best to ask these questions than regret it later ;)

@felixarntz Re switching to transients: I think *_site_transient() functions may be better so the data is shared on multisite installs. I don't have a strong opinion whether options or transients are best so I'll leave you and the team to figure that out.

@peterwilsoncc I am not opposed to using network transients (*_site_transient()), but on the side of caution I think it's safer to go with regular transients, since network wide storage comes with additional considerations, e.g. in terms of autoloading and, more importantly, cache invalidation. Multisite networks also tend to be very customized, e.g. maybe different sites use different theme roots to not use the same themes etc. Last but not least, the existing theme data caching in the WP_Theme class is per site too, so I think it's safer to follow that.

If we feel confident about using network transients for this, I'm not going to block it, but to err on the side of caution and go with the simpler approach, I prefer regular transients for now.

Question: Why do we even need to register block-patterns on init?
Do we need to register these patterns when viewing the frontend of a site? 🤔 I can't think of a reason why we'd need to register them when rendering the front... If that is the case, then instead of adding caches we should examine _why_ and _when_ we load patterns?

Question: Why do we even need to register block-patterns on init?

Do we need to register these patterns when viewing the frontend of a site? 🤔 I can't think of a reason why we'd need to register them when rendering the front... If that is the case, then instead of adding caches we should examine why and when we load patterns?

@aristath Sadly, now that this is how this work, is going to extremely hard to undo it and not break anything. I agree, if we knew about this when this functionality went into core, we could have done something. For now let try and fix is with a cache and we can then look a lazily registering this patterns

Sadly, now that this is how this work, is going to extremely hard to undo it and not break anything

If we know what we're doing is wrong, perhaps we should treat the _cause_ instead of the symptom...
I understand that adding this cache improves performance, but in essence, it just masks a deeper issue.

If registering patterns is not necessary on the frontend, then we could add a condition and only load them when in the dashboard - or change the hook from init to something else?

If we know what we're doing is wrong, perhaps we should treat the cause instead of the symptom...

I understand that adding this cache improves performance, but in essence, it just masks a deeper issue.

@aristath With limited time left in the beta, I don't think we have time to solve this. If you want to take a bash at it, feel free.

[spacedmonkey]

I have done some testing and found a number of block themes that have number patterns register. For testing, here is a list.

• ​Ollie
• ​Frost
• ​Powder

[spacedmonkey]

In 56765:

Editor: Improve performance of _register_theme_block_patterns function.

The _register_theme_block_patterns function imposed a significant resource overhead. This issue primarily stems from themes, such as TT4, that register a substantial number of block patterns. These patterns necessitate numerous file operations, including file lookups, file reading into memory, and related processes. To provide an overview, the _register_theme_block_patterns function performed the following file operations:

• is_dir
• is_readable
• file_exists
• glob
• file_get_contents (utilized via get_file_data)

To address these issues, caching using a transient has been added to a new function call _wp_get_block_patterns. If theme development mode is disabled and theme exists, the block patterns are saved in a transient cache. This cache is used all requests after that, saving file lookups and reading files into memory. Cache invalidation is done, when themes are switched, deleted or updated. Meaning that block patterns are not stored in the cache incorrectly.

Props flixos90, joemcgill, peterwilsoncc, costdev, swissspidy, aristath, westonruter, spacedmonkey.
Fixes #59490

Committed ​https://github.com/WordPress/wordpress-develop/commit/3ad20183b0f54b79ae7a2bbf449ea523d50fd687

@spacedmonkey What about my feedback from ​https://github.com/WordPress/wordpress-develop/pull/5342#pullrequestreview-1655363697?

[flixos90]

See ​https://github.com/WordPress/wordpress-develop/pull/5342#pullrequestreview-1655363697 and ​https://wordpress.slack.com/archives/C02RQBWTW/p1696346290050229 for additional feedback that still should be addressed.

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

[spacedmonkey]

@flixos90 Feedback actionedin ​https://github.com/WordPress/wordpress-develop/pull/5371. Ready for your review.

@felixarntz Feedback actioned in ​https://github.com/WordPress/wordpress-develop/pull/5371

[flixos90]

In 56771:

Editor: Simplify return shape and logic of _wp_get_block_patterns().

Follow up to [56765].

Props spacedmonkey.
Fixes #59490.

Committed in https://core.trac.wordpress.org/changeset/56771

[flixos90]

@spacedmonkey Why does this change need a dev note? I don't think there's anything that developers need to change to benefit from this improvement?

[spacedmonkey]

@flixos90 because now block patterns are cached unless you have develop mode on. Develop maybe confused why they are getting cached results.

Using a cache for these file content lookups results in a major performance boost for block themes. Here are some Server-Timing benchmarks, using 100 runs with the benchmark-server-timing tool (with the transient cache populated):

• TT4 theme: 95.31ms (PR) vs 104.89ms (trunk) → 9.1% faster
• TT3 theme: 72.8ms (PR) vs 80.37ms (trunk) → 9.4% faster
• TT2 theme: 72.21ms (PR) vs 79.31ms (trunk) → 9.0% faster
• testing with a few third-party block themes:
  • Frost theme: 71.13ms (PR) vs 75.9ms (trunk) → 6.3% faster
  • Neve theme: 94.97ms (PR) vs 102.74ms (trunk) → 7.6% faster
  • Ollie theme: 99.52ms (PR) vs 105.31ms (trunk) → 5.5% faster

The approach used here has already been established for block patterns (see https://core.trac.wordpress.org/ticket/59490 / https://core.trac.wordpress.org/changeset/56765), and it makes sense to apply it here as well.

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

@joemcgill

Ideally, I think we should add some intentional expiry to these transients to ensure they get cleaned up during transient garbage collection. We should also try to set them to not autoload, since we don't need all of this data on every pageload, only the template data that is in play for that pageload.

Good catch! Rather than using an expiry though, which isn't needed given the name won't change (the version isn't part of the transient name), I think we should follow the same approach that is used for the recently introduced theme pattern cache (see https://core.trac.wordpress.org/changeset/56765): When switching the theme, delete the caches for the relevant themes. This will ensure a maximum of 2 of those transients are present (and autoloaded). I have added this in ​https://github.com/WordPress/wordpress-develop/pull/5463/commits/ad18c944d61a79dce8a0b15e8fdd612309b7a5a3.

I'm also wondering if there is a way we can incorporate these enhancements with _get_block_templates_files, which is a previously noted performance botte neck (see https://core.trac.wordpress.org/ticket/58196#comment:26)

This PR is actually already benefitting block templates: _get_block_templates_files() itself doesn't retrieve the file contents, but the only place it is used is get_block_templates(), and that function calls _build_block_template_result_from_file() for every block template, which is the function enhanced here. So the caching benefits retrieving both block templates and block template parts.

Just to reflect a ​conversion on slack. I am strongly against this change being merged into core in the WP 6.4 cycle. Caching the file_get_content has clear performance benefits, but this is not a bug fix, it is an enhancement. This should have it's own ticket and big discussed there. If this is every committed, it needs to be committed early in a release cycle, so it get tested. But at the moment, there is no cache invalidation, for files being uploaded via FTP. Uploading files via FTP is how many developers work on client sites. I didn't always update the theme version number when making changes. We can't break that workflow.

@spacedmonkey

This would be a breaking change. This would REQUIRE users to enable development mode. This would not allow to upload new files to the server and for those to read.

The behavior here is exactly the same as the pattern caching change that you committed last week after us agreeing on it. I acknowledge that this led to a bug, but the problem of loading non-existing files isn't possible here, since this cache literally caches the file contents. Related to ​https://github.com/WordPress/wordpress-develop/pull/5462#issuecomment-1759671030, we should however ensure that any existing file caches are invalidated as soon as someone sets WP_DEVELOPMENT_MODE to "theme", to prevent stale caches being used once WP_DEVELOPMENT_MODE is turned off again.

Uploading files via FTP is how many developers work on client sites. I didn't always update the theme version number when making changes. We can't break that workflow.

Users that are making changes to a theme (except updating to a new version) _are_ developing the theme and therefore should set WP_DEVELOPMENT_MODE to "theme". This is why that constant was introduced. If we now change our opinion on it, the constant has literally no value. Theme development should happen on development or staging instances. We shouldn't encourage developing on production as that is clearly a problematic pattern. If a developer wants to do that though, they can still set WP_DEVELOPMENT_MODE accordingly.

Caching the file_get_content has clear performance benefits, but this is not a bug fix, it is an enhancement.

Again, this is similar to the pattern caching change that you committed last week. Can you please clarify why you think it is okay to commit that change this late in the cycle, but not the change here, even though they do something similar? I acknowledge that now it is even later in the beta cycle, and maybe it's too late. But the argument of this being an enhancement when the pattern caching change https://core.trac.wordpress.org/ticket/59490 was a bug makes no sense to me.

This change is similar to but not exactly same as.
If we are going cache all the contents of files, this would make the transient very big. Those files could have mbs work of markup in them. We are trying to avoid large cached, autoloaded options. Caching large blobs of markup in transient is something I am not sure about.

Are filter run, block parsed and strings translated if we cache like this. Didn't we just have an issue where hooks were not applied? @ockham @gziolo can you give context here.

Again, I will say, adding caching here is a good idea. But we could see a performance benefit from an in memory cache, like in my PR. If there is no breakage from the in memory cache, we could work at making that in memory cache persistent. Going straight to persistent, will likely bring up errors. Doing this so late in the process, is far to risky. I do think it something we can explore in WP 6.5.

@spacedmonkey

If we are going cache all the contents of files, this would make the transient very big. Those files could have mbs work of markup in them. We are trying to avoid large cached, autoloaded options. Caching large blobs of markup in transient is something I am not sure about.

That is a fair point. We could add an expiration to the transient and thus not autoload it. That would lead to 1-2 more DB queries in the frontend, but those are way faster than all the file_get_contents() calls, so that would be fine. Using only object cache will leave a lot of sites out of getting the performance benefits.

Are filter run, block parsed and strings translated if we cache like this. Didn't we just have an issue where hooks were not applied?

This change caches only file_get_contents() results, so there is zero chance of any filters being skipped by this caching.

@spacedmonkey I pushed two import changes related to your feedback:

• ​https://github.com/WordPress/wordpress-develop/pull/5463/commits/4414d8b1c5c6e4245ff47e791e820f30d1f62e44 ensures that existing caches are wiped when theme development mode is enabled, to prevent stale caches from being served if theme development mode is disabled again later.
• ​https://github.com/WordPress/wordpress-develop/pull/5463/commits/d0f971ffc2808de62fc5198275298de679beab02 adds an expiration to the transient. This ensures periodic refresh, natural expiration over time, as well as prevents the transient from being autoloaded. I benchmarked it, and the additional DB query indeed doesn't matter. DB queries are generally much faster than file system lookups. I see the same improvements as before.

@felixarntz Other than a performance improvement, what bug is this fixing again? Why does this need to go in RC?

In my testing here are the size of autoload options after this cache was added.

Theme	Size of transient
TT2	5.261 kb
TT3	5.429 kb
TT4	14.876 kb
Frost	5.518 kb
Ollie	22.651 kb

I am worried about these cache growing large, for complex these. Autoloading a large option may worse then file operations.

Was it considered, to prime this transient cache on theme activation. Saying by doing a glob on the /templates directory and saving the cache. This way caches would be loaded by a request in admin or wp cli and front end users could have to prime the caches.

Added some people that worked on this function before. Hope we can get some more context.

@spacedmonkey

In my testing here are the size of autoload options after this cache was added.

The options are no longer autoloaded based on your previous feedback. Please make sure to read my explanations in ​https://github.com/WordPress/wordpress-develop/pull/5463#issuecomment-1759892697.

Was it considered, to prime this transient cache on theme activation. Saying by doing a glob on the /templates directory and saving the cache.

On first thought, I like this idea. However, it wouldn't actually work because of theme updates. We will need to be able to refresh the caches on demand, to cover the scenario of the theme version being different. We can't hook into the update process for that since new theme versions are often deployed in other ways, e.g. Composer, git, FTP.

[flixos90]

In 56931:

Themes: Clear existing pattern cache when in theme development mode and prevent PHP warning due to missing file.

Follow up to [56765].

Props spacedmonkey, afercia, jrf, flixos90.
Fixes #59591.
See #59490.

Sorry for the delay in responding here yesterday.

For the record, I'm still a very big fan of the idea of trying to avoid these types of redundant file parsing lookups during template rendering. That said, I think there are a few different approaches to consider here, like saving these files to a post type, different considerations of how transients get cached and loaded, and exploring methods of ensuring that cache busting could be in play for site owners who are directly editing their theme files on their live site 🙈🤠.

To give us time to properly consider and test these scenarios, I think it's best that we consider this along with other template loading enhancements that have been discussed for 6.5 rather than trying to rush this into this release during the final week of the beta period before RC. Ideally, this (and things like it) should benefit from being in place at the start of the beta period.

In the meantime, I'm retracting my original review as no longer applicable, since my original blocking concerns have been addressed.

[webcommsat]

As this looks like it is being moved to 6.5, we are removing it from the list of items for dev notes for 6.4. Can the milestone on this trac ticket be adjusted please. It can be revisited for 6.5 dev notes. Thanks.

[flixos90]

Replying to webcommsat:

As this looks like it is being moved to 6.5, we are removing it from the list of items for dev notes for 6.4.

I don't think that's the case, where did you get that information from?

Note that this ticket is already covered in the existing 6.4 dev note for template loading optimization.

[joemcgill]

In 56978:

Themes: Make caches for block patterns clearable.

In [56765], theme block pattern files were cached to a transient as a performance enhancement. However, transients are not easily clearable when caches are flushed on environments not using a persistent cache, which can lead to errors if the theme files are renamed, edited, or moved.

This changes the caching mechanism to use wp_cache_set() instead, and caches these values to the global group so they are still persistent on environments using an object cache, and will be cleared by a cache flush.

In addition, the helper _wp_get_block_patterns has been moved WP_Theme::get_block_patterns for consistency with other block related theme methods and cache helpers for these values, WP_Theme::get_pattern_cache and WP_Theme::set_pattern_cache, have been made private.

Relevant unit tests updated.

Props: afercia, flixos90, mukesh27, joemcgill.
Fixes #59633. See #59591, #59490.

[joemcgill]

In 56979:

Themes: Make caches for block patterns clearable.

In [56765], theme block pattern files were cached to a transient as a performance enhancement. However, transients are not easily clearable when caches are flushed on environments not using a persistent cache, which can lead to errors if the theme files are renamed, edited, or moved.

This changes the caching mechanism to use wp_cache_set() instead, and caches these values to the global group so they are still persistent on environments using an object cache, and will be cleared by a cache flush.

In addition, the helper _wp_get_block_patterns has been moved WP_Theme::get_block_patterns for consistency with other block related theme methods and cache helpers for these values, WP_Theme::get_pattern_cache and WP_Theme::set_pattern_cache, have been made private.

Relevant unit tests updated.

Props afercia, flixos90, mukesh27, joemcgill.
Merges [56978] to the 6.4 branch.
Fixes #59633. See #59591, #59490.