Make WordPress Core

Opened 2 months ago

Closed 10 days ago

Last modified 3 days ago

#60120 closed defect (bug) (fixed)

Add cache group to block pattern cache

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.4
Component: Themes Keywords: has-patch has-unit-tests commit needs-dev-note
Focuses: performance Cc:

Description

In [56978] block patterns were cache in object cache. However, no cache group was provided. Example.

        private function get_pattern_cache() {
            if ( ! $this->exists() ) {
               return false;
            }
            $pattern_data = wp_cache_get( 'wp_theme_patterns_' . $this->stylesheet );

This makes it hard to control this cache. A cache group should be provided to bring inline with other caches.

Question: should this cache group be a global cache group.

Change History (25)

#1 @spacedmonkey
2 months ago

For your consideration. @joemcgill @flixos90 @hellofromTonya

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


2 months ago

#3 @joemcgill
2 months ago

  • Keywords needs-patch added
  • Milestone changed from Future Release to 6.5
  • Type changed from enhancement to defect (bug)

I agree that we should address this. Originally, block patterns were going to be cached to a transient, but no expiration strategy was set, nor are transients easily clearable. This concern was raised in #59633.

As a compromise, this was moved to the global cache group so that sites with an object cache would still get the benefits of persistent caching, while we considered a better wholistic caching strategy for expensive operations related to getting and parsing data from theme files in block themes (see: #59719).

Generally, it would make sense to cache all of these files to the themes cache group, but that group is non-persistent by default, unless someone has opted in to group being persistent by filtering the wp_cache_themes_persistently value.

Once a wholistic strategy is determined in #59719, we can apply that strategy to the pattern cache methods in WP_Theme.

Moving this to the 6.5 milestone.

#4 @flixos90
2 months ago

Agreed, this is somewhat blocked by #59719. We should make sure not to choose this cache group in isolation.

Let's aim to come to a decision on that broader direction ticket early in the new year so that we can start addressing the various tickets in 6.5.

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


7 weeks ago

#6 @flixos90
7 weeks ago

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

Assigning this in relation to https://core.trac.wordpress.org/ticket/59719#comment:13. Probably best to implement the suggested changes here first before proceeding with #59600, just to validate this actually works the way we want on the existing functionality.

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


7 weeks ago

#8 follow-up: @joemcgill
7 weeks ago

Thanks @flixos90. Based on https://core.trac.wordpress.org/ticket/59719#comment:13, let's both add a new theme_files cache group and use this as an opportunity to explore the idea of backing these caches up with a transient when a persistent object cache is not present.

A new PR should:

  1. Introduce a new theme_files cache group via wp_cache_add_global_groups()
  2. Apply the new cache group when setting the cached block pattern data in WP_Theme::set_pattern_cache
  3. Set the cache to a site transient, using set_site_transient() in WP_Theme::set_pattern_cache with an expiration time (e.g. HOUR_IN_SECONDS) if wp_using_ext_object_cache() returns false. In single sites, this will automatically fall back to a local transient.
  4. The expiration time should only be used for transients and not the persistent cache, and the value should be filterable. We should design the filter so the same hook can be extended to block templates and template parts, so the filter should pass additional context that identifies that the data the expiration applies to in this case is block patterns. Ex. apply_filters( 'wp_block_theme_files_cache_ttl', HOUR_IN_SECONDS, 'patterns' ) so that these TTL values can be independently controlled.
  5. Handle deleting the transient in WP_Theme::delete_pattern_cache when wp_using_ext_object_cache() returns false.

Note that WP_Theme::delete_pattern_cache is already called from WP_Theme::cache_delete and during switch_theme(), but additional cache invalidation may need to be considered for things like editing theme files directly in the legacy theme editor (/wp-admin/theme-editor.php). Additionally, WP_Theme::set_pattern_cache is already only called if WP_DEVELOPMENT_MODE is not set to theme or all, and that should remain the responsibility of the code calling WP_Theme::set_pattern_cache, not in the implementation of that helper method.

@flixos90 and @spacedmonkey, feel free to add anything that I missed.

#9 @flixos90
7 weeks ago

Thanks @joemcgill, LGTM.

Regarding cache invalidation when adding patterns in the theme file editor, I'm thinking we may want to (at least initially) leave this out of the PR here, as it ties in more closely with #59662 which is already on my list for next week. IMO it would make sense to use that ticket to tackle that part.

#10 in reply to: ↑ 8 ; follow-up: @spacedmonkey
7 weeks ago

Replying to joemcgill:

  1. Apply the new cache group when setting the cached block pattern data in WP_Theme::set_pattern_cache
  2. Set the cache to a site transient, using set_site_transient() in WP_Theme::set_pattern_cache with an expiration time (e.g. HOUR_IN_SECONDS) if wp_using_ext_object_cache() returns false. In single sites, this will automatically fall back to a local transient.
  3. The expiration time should only be used for transients and not the persistent cache, and the value should be filterable. We should design the filter so the same hook can be extended to block templates and template parts, so the filter should pass additional context that identifies that the data the expiration applies to in this case is block patterns. Ex. apply_filters( 'wp_block_theme_files_cache_ttl', HOUR_IN_SECONDS, 'patterns' ) so that these TTL values can be independently controlled.
  4. Handle deleting the transient in WP_Theme::delete_pattern_cache when wp_using_ext_object_cache() returns false.

All of this feel out of scope of what was being ask for in this ticket. This can, which I am personally unsure about, should be handled in a different ticket. Let’s keep this ticket to adding a cache group, or a larger ticket about improving caching here, which is a different discussion. Please read https://core.trac.wordpress.org/ticket/59719#comment:15. Until we have solved cache invalidation, we shouldn’t cache anything about caching here.

We should also consider, just making the cache group theme_files a non persistent group if development mode is enabled. This makes logic much cleaner.

#11 in reply to: ↑ 10 @joemcgill
7 weeks ago

Replying to spacedmonkey:

All of this feel out of scope of what was being ask for in this ticket.

I agree. 1 and 2 above are the minimum that needs to be done to close this ticket. However, I think it's worth trying to back data in this cache group with transients (3–5 above) at the same time, based on the conversation in #59719. Since there isn't a rush to create the new group only, I'd prefer to see us iterate on the full idea and scale back if needed rather than juggling various follow-up tickets during this release cycle.

We should also consider, just making the cache group theme_files a non persistent group if development mode is enabled. This makes logic much cleaner.

Good suggestion. That is worth exploring as part of this change.

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


7 weeks ago
#12

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

#13 @spacedmonkey
7 weeks ago

I created a PR based on the discussion in this ticket. Ready for review.

CC @flixos90 @joemcgill

#14 follow-up: @flixos90
7 weeks ago

@spacedmonkey @joemcgill
I'm not sure about the value of conditionally having the new group non-persistent. In all other places in core we avoid caching entirely based on WP_DEVELOPMENT_MODE. I think changing that warrants a bigger discussion.

I agree with @joemcgill that we need to consider this as part of the broader solution, though I also think it's fair to go with what you suggested @spacedmonkey and address only the minimum requirements that this ticket originally requested as part of the PR.

Given you previously suggested to limit the scope here, I'd say let's limit the PR to purely introducing the theme_files cache group, which was the only original purpose of this ticket.

We can then address the broader approach with a single PR against #59719.

@spacedmonkey commented on PR #5847:


7 weeks ago
#15

@felixarntz I have revert some of the changed based on your feedback.

#16 in reply to: ↑ 14 @spacedmonkey
7 weeks ago

Replying to flixos90:

Given you previously suggested to limit the scope here, I'd say let's limit the PR to purely introducing the theme_files cache group, which was the only original purpose of this ticket.

I have actioned your feedback.

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


6 weeks ago

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


5 weeks ago

#19 @spacedmonkey
3 weeks ago

@joemcgill I think the PR is ready to commit. Can you take another look?

#20 @joemcgill
3 weeks ago

  • Keywords commit added
  • Owner changed from joemcgill to spacedmonkey

Thanks @spacedmonkey. This does look ready to commit. Left some feedback on the PR, but not a blocker.

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


2 weeks ago

#22 @spacedmonkey
10 days ago

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

In 57608:

Themes: Add 'theme_files' cache group to block pattern cache operations.

Use 'theme_files' cache group for block pattern caches. Previously, block pattern cache data was not stored in a cache group and used the default group. This new cache group, is setup as a global cache group, meaning that sites using multisite, will have a single cache for block pattern data per theme. This change also no longer invalidate block pattern caches in multisite instances, meaning block pattern caches can be shared between sites on a network, meaning less repeated data in the object cache.

Props spacedmonkey, flixos90, joemcgill.
Fixes #60120.

#23 @spacedmonkey
10 days ago

  • Keywords needs-dev-note added

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


8 days ago

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


3 days ago

Note: See TracTickets for help on using tickets.