Make WordPress Core

Opened 5 months ago

Last modified 17 hours ago

#59600 assigned defect (bug)

Block themes: Use a cache for block template files

Reported by: flixos90's profile flixos90 Owned by: joemcgill's profile joemcgill
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch has-unit-tests
Focuses: performance Cc:

Description (last modified by joemcgill)

Updated Feb 28 by @joemcgill

On every page load for a block theme, several block templates, template parts, and patterns are used, each of which results in a file_get_contents() operation to get the block contents before parsing and rendering the blocks included in each. Getting these file contents is an expensive operation, resulting in a performance bottleneck which gets greater the more block template parts are being used in a theme.

Since these files are unlikely to change often in production, we should attempt to avoid these redundant operations by the use of the caching strategy outlined in #59719.

Solutions need to respect WP_DEVELOPMENT_MODE settings, so that persistent caching does not happen when developing a theme, and should also be forgiving for use cases where theme files are modified directly in production through various methods.

Original issue description

While reviewing the performance impact of #59583, I noticed that a large chunk of the cost of parsing block templates and block template parts comes from simply the file_get_contents() calls for those files.

On every page load for a block theme, several block templates and template parts are used, each of which results in a file_get_contents() operation. Getting these file contents is an expensive operation, that results in a performance bottleneck which gets greater the more block template parts are being used in a theme.

I have prepared a draft PR with a solution for this and based on Server-Timing benchmarks, it notably improves the overall response time by a solid 5-10% for various block themes.

Given this is a major performance win and relies on a pattern already established via #59490 / [56765], I think it is worth prioritizing even this late in the release cycle.

Change History (18)

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


5 months ago

#2 @flixos90
5 months ago

  • Keywords has-patch needs-unit-tests added

See PR https://github.com/WordPress/wordpress-develop/pull/5463 for the proposed solution and benchmarks showing its impact.

#3 @flixos90
5 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

https://github.com/WordPress/wordpress-develop/pull/5463 now has unit tests added, to ensure the cache is relied upon and set correctly, but only when it should.

@flixos90 commented on PR #5467:


5 months ago
#5

@spacedmonkey This approach caches more data than my PR #5463, but the concerns that you shared in https://github.com/WordPress/wordpress-develop/pull/5463#pullrequestreview-1673559100 apply to this PR as well, as far as I can tell.

Can you please clarify what the benefits of this PR are over #5463?

#6 @flixos90
5 months ago

  • Milestone changed from 6.4 to 6.5

Per the PR conversations, punting this to 6.5 to give ourselves a bit more time to consider different approaches.

#7 @flixos90
4 months ago

  • Priority changed from normal to high

Marking this as a high priority due to having significant performance impact with the additional caching.

Note that this is blocked by making a decision on #59719 first.

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


4 months ago

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


3 months ago

#10 @flixos90
2 months ago

  • Owner changed from flixos90 to joemcgill

Reassigning in relation to https://core.trac.wordpress.org/ticket/59719#comment:13. Let's figure out the direction there first, and then we can work on this ticket as well as #60120. Maybe best to do #60120 first as it would be about migrating existing functionality to the new approach, i.e. slightly more urgent than this one here, which is a new enhancement.

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


3 weeks ago

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


2 weeks ago

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


6 days ago

#14 @joemcgill
6 days ago

  • Description modified (diff)
  • Milestone changed from 6.5 to 6.6
  • Priority changed from high to normal
  • Summary changed from Block themes: Use a cache for block templates and block template parts content to Block themes: Use a cache for block template files

Moving this to 6.6 so we can land caching for this and pattern files early in the release cycle and not just before RC. I'm updating the issue title to also cover persistent caching of block patterns, which was discussed in 60120, but not implemented in that issue.

@thekt12 already has an approach in progress for caching patterns, which is currently related to #59719, but should be updated to point to this issue. That way we can close out #59719, since it's purpose was to agree on a consistent strategy for caching to block theme files, which was documented in this comment.

Further implementation details can be discussed related to the PRs for this issue.

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


6 days ago

@thekt12 commented on PR #6219:


17 hours ago
#18

Oh, this is interesting. I'm going to think on it a bit.

My knee-jerk reaction is that making such a big change the the main wp_cache_ functions, is likely not the right approach, but there is something kind of clever about trying to make it easier to optionally back caches in non-persistent environments with transients without a lot of repeated code.

I wonder if it would be easier to handle from the opposite perspective, given that transients already get saved to external object cache when available? I think the main things that are missing for transients are the ability to set a cache group besides 'transient' which is where they all get stored now, and a way to ensure transients that are stored in the DB options table get flushed whenever the cache group they belong to gets flushed.

The reason I was inclined to the changes here is :

  • Consistency - mainly in naming convention. If we have a consistent name, in future it will be easy to implement flushing.
  • Secondly, we will have too many filters to manage and a lot of repetitive code.

I was analysing both the functions wp_cache_ side functions and transient side functions.
The way I have written this code is transient cache is conditionally enabled only when $transient_cache flag is set and object_cache is not enabled. So older code will work the same way. This shift of logic applies only to places where we want that logical check, so it's easy to ensure BC.

The problem I felt with making changes to transient side functions is that;

  • We will have to be extra careful while adding support for groups in caching code as there might be some sites reliant on object cache. Ensuring BC might be a bit more complex here as compared to wp_cache base approach.
  • Handling the expiry situation might become complex (I cannot say for sure though) as we will have to add a bunch of code to differentiate between regular transient and transient for caching.
  • We will have to replace wp_cache_ at all the places with transient functions, which might get confusing. Also, key name logic will be bit more complex inside transient functions.

Lastly, I felt it was more of a caching problem than a transient problem (even though they mean the same), so I added my code to wp_cache_.

Note: See TracTickets for help on using tickets.