Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 9 months ago

#59490 closed defect (bug) (fixed)

Performance issues in _register_theme_block_patterns

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.0
Component: Editor Keywords: has-patch commit has-dev-note
Focuses: performance Cc:

Description

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.

Change History (48)

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


10 months ago
#1

  • Keywords has-patch added

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


10 months ago

#3 @spacedmonkey
10 months ago

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.

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


10 months ago

@spacedmonkey commented on PR #5342:


10 months ago
#5

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.

@peterwilsoncc commented on PR #5342:


10 months ago
#6

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.

@westonruter commented on PR #5342:


10 months ago
#7

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.

@spacedmonkey commented on PR #5342:


10 months ago
#8

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

@westonruter commented on PR #5342:


10 months ago
#9

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.

@peterwilsoncc commented on PR #5342:


10 months ago
#10

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

@flixos90 commented on PR #5342:


10 months ago
#11

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

@aristath commented on PR #5342:


10 months ago
#12

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?

@spacedmonkey commented on PR #5342:


10 months ago
#13

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

@aristath commented on PR #5342:


10 months ago
#14

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?

@spacedmonkey commented on PR #5342:


10 months ago
#15

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.

#16 @spacedmonkey
10 months ago

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

This ticket was mentioned in Slack in #core by spacedmonkey. View the logs.


10 months ago

#18 @spacedmonkey
10 months ago

  • Keywords commit added
  • Owner set to spacedmonkey
  • Status changed from new to assigned

#19 @spacedmonkey
10 months ago

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

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

#22 @flixos90
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#24 @spacedmonkey
10 months ago

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

#26 @spacedmonkey
10 months ago

  • Keywords needs-dev-note added

#27 @flixos90
10 months ago

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

In 56771:

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

Follow up to [56765].

Props spacedmonkey.
Fixes #59490.

#29 @flixos90
10 months ago

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

#30 @spacedmonkey
10 months ago

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

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


9 months ago
#31

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

@flixos90 commented on PR #5463:


9 months ago
#32

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

@spacedmonkey commented on PR #5463:


9 months ago
#33

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.

@flixos90 commented on PR #5463:


9 months ago
#34

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

@spacedmonkey commented on PR #5463:


9 months ago
#35

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.

@flixos90 commented on PR #5463:


9 months ago
#36

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

@flixos90 commented on PR #5463:


9 months ago
#37

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

@spacedmonkey commented on PR #5463:


9 months ago
#38

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

@spacedmonkey commented on PR #5463:


9 months ago
#39

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.

@spacedmonkey commented on PR #5463:


9 months ago
#40

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.

@spacedmonkey commented on PR #5463:


9 months ago
#41

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

@flixos90 commented on PR #5463:


9 months ago
#42

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

#43 @flixos90
9 months ago

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.

joemcgill commented on PR #5463:


9 months ago
#44

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.

#45 follow-up: @webcommsat
9 months ago

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.

#46 in reply to: ↑ 45 @flixos90
9 months ago

  • Keywords has-dev-note added; needs-dev-note removed

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.

#47 @joemcgill
9 months ago

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.

#48 @joemcgill
9 months ago

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.

Note: See TracTickets for help on using tickets.