Opened 5 weeks ago
Last modified 3 hours ago
#59719 assigned enhancement
Revise caching approach for theme files
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.5 | Priority: | high |
Severity: | normal | Version: | |
Component: | Themes | Keywords: | 2nd-opinion |
Focuses: | performance | Cc: |
Description
This is a follow up related to #59490, #59633, and #59600.
Specifically, the purpose of this ticket is to decide the path forward on the temporary approach of using a global cache group in #59633, which will also determine the path forward to take on #59600.
Questions to answer:
- Should we continue to use
wp_cache_*()
functions, or should we switch to transients to bring these benefits to sites without a persistent object cache too?- When relying on
WP_DEVELOPMENT_MODE
during theme development, using transients would be perfectly fine, as the caching gets (temporarily) disabled as long as the constant is set. - However, for e.g. agencies or freelancers that continuously deploy new theme versions, it may be more common to rely on flushing the cache when an update is made. This is arguably not about developing the theme in production, but rather continuous deployment without bumping the version number (the latter of which would automatically invalidate the cache).
- If we want to maintain the latter workflow of flushing the cache to refresh theme file caches, we either need to stick to
wp_cache_*()
, or we need to implement a way to "flush" certain transients even when not using a persistent object cache. (Flushing a persistent object cache would by definition flush transients too as in that case they're part of the object cache.)
- When relying on
- If we continue to rely on
wp_cache_*()
functions, which cache group should we use?- We should not use the
themes
cache group for this as it is non persistent by default, even for sites with a persistent object cache. - We should not use the global cache group as that is not encouraged.
- We could consider introducing a new cache group for this purpose, e.g.
theme_files
, ortheme_templates
, or similar.
- We should not use the
Change History (9)
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
5 weeks ago
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
4 weeks ago
This ticket was mentioned in Slack in #core-performance by joemcgill. 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 joemcgill. View the logs.
7 days ago
#7
@
7 days ago
I just had a good chat with @joemcgill about this, where we ended up taking a step back. While determining how to best cache this data continues to be relevant, it's worth considering other ways to improve the performance of the relevant logic regardless of caching. If there are opportunities for that, we could afterwards reassess how the caching here should behave to achieve a good compromise between reliability, consistency, and performance gain.
A few observations on existing theme caching first:
- The
themes
cache group (seeWP_Theme
class) is by default not persistent, but can be made persistent via thewp_cache_themes_persistently
filter. This filter additionally allows to define the cache expiration to use (in seconds). If no specific expiration is defined, it's set to 30 minutes. - This same filter is used to control whether to persistently cache the list of available themes (see
search_theme_directories()
function). The main difference is that here the WP Cache API isn't used, instead it uses a transient, which means even sites without a persistent object cache will receive these caching benefits. This in itself is inconsistent. - There is no invalidation logic for this cache group at all. This could be okay as the data naturally expires even if persistent, but it's worth highlighting that when you for example install a new theme, it would not show up in WP Admin until the cached data expired.
- Eventually, we could either use this cache group for all theme data or introduce a new more specific theme cache group depending on what is being cached. This decision depends on a few factors, including whether or not we think the approach of "non-persistent by default, persistent by opt-in" makes sense for this data.
Before we can decide that though, we identified another potential opportunity with block templates, block template parts, and block patterns (i.e. relevant for all tickets blocked by this ticket): Currently, when registering the data for them, that data includes the full contents of these files. Getting the file content accounts for some of the most expensive work, but the file contents actually only matter if the respective template/part/pattern is actually used on the current URL. There may be other data that isn't needed either unless the template/part/pattern is actually used. So we should consider reworking the template/part/pattern registration logic to only register the bare minimum data on every page load.
If we can find a solution to that, we can probably reduce the performance overhead significantly for themes that include a large number of templates/parts/patterns, without involving any caching.
An additional benefit would be that, even if we then cached this data, it wouldn't involve caching the actual file contents, which would mean that changes to those files would still be reflected immediately, plus it would keep cache data for individual keys from growing too large.
We should probably explore that further:
- Can we rework each of the block template/part/pattern registration logic to only register the bare minimum data needed by default?
- The relevant functions would still need to optionally support registering the full data, or there would need to be some "lazy-loading" mechanism to only run if any of those data fields is being accessed. This would be relevant for example in the block editor where templates/parts/patterns need to be previewable.
- For regular page loads though, only the bare minimum data would be relevant, and the extra data (such as the actual file content) would only be relevant for the few templates/parts/patterns actually used on the current URL.
It's worth noting that there is also a little bit of potential overlap with #57789. The two tickets are about caching different aspects of theme data, and while the relevant logic considerations are completely different, in terms of caching it may make sense to follow similar principles. It's worth noting that theme.json
data caching has the additional complexity of firing lots of filters which means the filtered data specifically cannot be cached as it could be dynamically adjusted by code from anywhere (not only the current theme).
#8
@
7 days ago
Thanks for summarizing the points we discussed, @flixos90. The couple of observations that stand out to me as the most relevant to theme caching strategy is that currently, the only place where we're using caching in the WP_Theme
class that isn't part of the 'themes' cache group is the new 'wp_theme_patterns_{$stylesheet}
key that was moved to the class in [56978].
Instead, that key is currently being cached to the global group so it is persistent in environments that support a persistent object cache. It would be more consistent to save that key to the themes
cache group, but not until we're able to see if can avoid the most expensive parts of getting all of the pattern data so that moving this to a non-persistent (by default) group is feasible.
If not, then I think it's worth exploring a similar approach to what search_theme_directories()
function does, which is to store that data in a transient with a default expiration time, and make use of the context argument on the wp_cache_themes_persistently
hook to enable more fine grained control over whether that value is persisted by default or not. For example, we could choose to have persistence set to true
by default for just this value and use the filter for people to opt out if needed (hypothetically—I'm not actually proposing this approach without more exploration).
Marking this as a high priority for the 6.5 release as it is partly a follow up, partly a blocker to other potential high impact tickets #59600 and #59314.
@joemcgill Let's continue the discussion here that was started as part of #59633.