Store last changed value in key instead of using it as a salt for query caches

[spacedmonkey]

This 'last changed' timestamp serves as a salt, implying that when this value changes, the cache key also changes, leading to the creation of a new cache. This method of cache invalidation has proven effective over the years but presents a drawback – it generates an excessive number of caches.

Imagine a high-traffic website constantly generating new content with new posts added every 10 minutes. This results in the 'last changed' cache being updated every time a new post or revision is created. Consequently, the WP_Query for the homepage could potentially generate a new cache each time a post is updated. This could lead to the creation of hundreds or even thousands of caches. The concept behind an object cache is to allow unused keys to expire and be removed from the cache. However, this process is not swift and may take hours or even days for these keys to naturally fall out of the cache. Furthermore, there's a risk that the object cache could become bloated and remove actively used cache keys.

The solution lies in reusing cache keys. Instead of using 'last changed' as a salt, store the 'last changed' values within the cache value itself. Then, retrieve the cache value and compare the 'last changed' value stored in the cache with the actual 'last changed' value. If they don't match, mark the cache as invalid, retrieve the values from the database, and resave the cache with an updated 'last changed' value.

This approach offers several benefits:

Cache keys are reused, ensuring that the number of cache queries remains stable, and the cache queries clean up after themselves.
Predictable cache keys allow caches to be primed efficiently. For instance, a caching plugin can discern the types of queries on a page and prime multiple cache keys in a single request.
However, there are some downsides to consider:

Third-party plugins may be reliant on salted cache keys.
When comparing the last updated value, the entire cache must be retrieved. This means that the first user after cache invalidation may experience a slightly larger request."

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

[spacedmonkey]

See attached ​PR of POC of this idea.

[tillkruess]

I think this is an excellent solution to the problem and would "truly" resolve #57625.

[nickchomey]

Perhaps I'm missing something fundamental, but it seems to me that (at least part of) the conversation here and in #57625 is focusing on the wrong thing - largely how the cache is configured. Specifically, focusing on cache key TTL rather than having an appropriate cache eviction policy for the cache data (e.g. LRU, which I believe is the default for Redis). ​https://redis.io/docs/reference/eviction/

If you use something like allkeys-lru, then the only way actively-used keys could get evicted is if your cache is too small - a hosting error, not WP. If you do want to use TTL/expire on specific keys, then perhaps volatile-lru could be used. It seems to me that this decision should be up to the site owner, not WP Core.

This all seems to roughly echo what Peter Wilson said in his initial comment: https://core.trac.wordpress.org/ticket/57625#comment:3

The proposed solution here seems to add unnecessary complexity and processing overhead. So, perhaps a solution the present problems is to give users and plugins more control over caching policies such as whether to apply a TTL or not to a specific cache key? Also, to provide documentation/guidance on proper server cache (redis) config - both for users who manage their own servers, as well as for hosting providers to do a better job at.

This article from Ruby on Rails goes into detail about using key-based cache expiration, so it seems worth reviewing for inspiration.
​https://signalvnoise.com/posts/3113-how-key-based-cache-expiration-works
I hope this is helpful rather than irrelevant/a distraction!

[owi]

Replying to nickchomey:

Perhaps I'm missing something fundamental, but it seems to me that (at least part of) the conversation here and in #57625 is focusing on the wrong thing - largely how the cache is configured. Specifically, focusing on cache key TTL rather than having an appropriate cache eviction policy for the cache data (e.g. LRU, which I believe is the default for Redis). ​https://redis.io/docs/reference/eviction/

As I was the original poster of the aforementioend ticket I wanted to clarify (if it was not clear from my description) that I had custom TTL for the query cache groups (24-72h) and eviction policy - and this did the job for me.

Nevertheless I still believed that the way Core handles keys for these groups was not proper. On busy sites the last_updated could be regenerated tens or hundreds of times per day (as I proved in original post - it was enough to open add new post screen to just regenerate it twice, without even clicking save). That could case the numbers of query cache keys grow crazy fast and then TTL would be just some magic number which you can set based on some trial and error (to balance the cache efficiency vs memory levels).

Also I think (that's my personal opinion) that operating on the redis max memory all the time and relying on the eviction is not a healthy state and that's just covering the problem.

All of the above doesn't change the fact that I understand that the problem is not trivial to solve and query caches nature make it more difficult :)

This update introduces a wp_is_valid_last_changed() function to validate cache data against the last_changed value, improving cache reliability and removing timestamp dependency. Cache keys have been simplified by removing last_changed from their structure, and the cache values now store last_changed for consistency. Additionally, functions across core query classes are updated to implement these changes.

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

Closing in favour of ​https://github.com/WordPress/wordpress-develop/pull/8728

LGTM! I'd approve this if I'd had the permissions.

I think we should consider implementing two helper functions to read and write cache keys with $last_changed awareness, so that that logic is abstracted and doesn't need to be duplicated in all these places.

Excellent idea!

[sanchothefat]

Cross posting here from LinkedIn - an issue we hit with the recent core cache changes around option caching was really detrimental to performance because we’re using an object cache running on another server, a redis instance in our case. This isn't uncommon for larger sites.

Each cache lookup is still a remote request, so while often faster than a heavy db query ideally core would also optimise for not making too many cache requests when we know the data is likely to be stale.

This approach sounds like it might cause a lot of unnecessary cache requests, and cause us the same problem so I’m curious about that consideration and whether you have benchmarks for our kind of setup too?

[tillkruess]

Replying to sanchothefat:

Each cache lookup is still a remote request, so while often faster than a heavy db query ideally core would also optimise for not making too many cache requests when we know the data is likely to be stale.

This approach sounds like it might cause a lot of unnecessary cache requests, and cause us the same problem so I’m curious about that consideration and whether you have benchmarks for our kind of setup too?

It does not cause unnecessary cache requests. Please open a dedicated ticket for your problem, it's irrelevant to this ticket.

[spacedmonkey]

Thanks for the feedback, @sanchothefat.

This ticket was created following a discussion with your CTO, @rmccue. The remote cache was taken into consideration during its creation. Importantly, this patch does not increase the number of cache gets.

To clarify how the current and proposed approaches work:

Current Behaviour

• WP_Query is called.
• A cache lookup is performed, using the last changed value in the cache key.
• Cache is found.
• A post is updated.
• Last changed is updated.
• A new cache lookup is performed, now using the updated last changed value.
• Cache is not found (as the key has changed).
• A new cache is generated using the new key.

Proposed Behaviour

• WP_Query is called.
• A cache lookup is performed, comparing the last changed value in memory with the one stored in cache.
• Cache is found and deemed valid.
• A post is updated.
• Last changed is updated.
• A second cache lookup is performed, again comparing last changed.
• Cache is now considered invalid due to the change.
• The same cache key is reused and updated with new values.

Both approaches perform two cache lookups. The only difference lies in the second lookup: instead of a cache miss (which requires regenerating and storing new data), the proposed method performs an in-memory comparison. This is a very lightweight operation, as the cache in question typically contains only an array of post IDs—amounting to just a few bytes.

Moreover, in some caching systems (such as memcached, in my experience), a successful cache get is actually faster than a miss.

While it’s true that the first request in the proposed model may involve retrieving slightly more data, the ability to reuse cache keys is a significant improvement. It prevents the object cache from being filled with stale or rarely-used keys and allows query caches to persist longer, improving overall performance and cache efficiency.

I agree with @tillkruess, that improving database queries and remote caches, are out of scope of this discussed ticket and should be discussed else. Changing existing queries will be very hard, as these queries have filter that have to be maintained.

[sanchothefat]

Thanks for the clarification. I’m not talking modifying any db queries fwiw, but the impact on remote cache servers does feel relevant still.

Definitely a smart move where it’s only a small amount more data, just IDs retrieved at the same time, though for user, post or term queries returning a lot of rows (not good practice but happens) it could be a lot more data. I’ll see if I can pull some stats for that in case it’s helpful.

[sanchothefat]

Ok, I definitely misunderstood what's going into the cache there in some instances. Apologies @spacedmonkey - thanks for your work on this, looks like a great improvement :)

[rmccue]

I think I had originally suggested this when chatting with @spacedmonkey (although, I'm not the CTO :D), primarily because the introducing of this key-based caching started causing cache "churn" for us with data getting pushed out of Redis.

Most of WP is designed not to use key-based invalidation generally, so this was a pretty big conceptual change to the design of the cache system as a whole; moving back to regular keying I think makes the most sense and keeps it consistent. I can see a case for key-based invalidation, but WP was never really designed for that, so switching it has caused some issues.

(For reference, we use allkeys-lfu as the eviction policy.)

@sanchothefat Note that the data being stored here is only a list of IDs, as those then get pushed to _prime_{post,term,comment}_caches() and the various populate functions - so the typical size here is under a kilobyte.

[spacedmonkey]

Replying to rmccue:

I think I had originally suggested this when chatting with @spacedmonkey (although, I'm not the CTO :D),

Sorry for getting your job title wrong.

Most of WP is designed not to use key-based invalidation generally, so this was a pretty big conceptual change to the design of the cache system as a whole; moving back to regular keying I think makes the most sense and keeps it consistent. I can see a case for key-based invalidation, but WP was never really designed for that, so switching it has caused some issues.

So are you for or against this change? I find this message unclear.

BTW, I understand this is a big change, which is why I want the feedback from the community.

[rmccue]

Sorry, to be clear, I'm in favour of the change conceptually, as it avoids us needing to use extensive key-based invalidation.

[peterwilsoncc]

As a concept I like this idea for the *-queries groups, taking @rmccue, @tillkruess, @sanchothefat's word on the improved efficiency for large sites.

On the proposed implementation, I am wondeing if it would be better to introduce new wp_cache functions to do the checking of the last changed value as there are enough occurrences of the pattern in Core that it could be worthwhile (18, based on a quick check).

Something along the lines of:

<?php
wp_cache_get_query( ... $group ...) {
   $query_group = "$group-queries";
   $result = wp_cache_get( ... $query_group ... )
   $last_changed = wp_cache_get_last_changed( $group );
   if ( $result['last_changed'] !== $last_changed ) {
       return $false;
   }
   return $result['data'];
}

wp_cache_set_query( ... $group ... ) {
   $last_changed = wp_cache_get_last_changed( $group );
   wp_cache_set( $key, [ 'data' => $data, 'last_chagned' => $last_changed ], $group-queries )
}

[tillkruess]

Replying to peterwilsoncc:

On the proposed implementation, I am wondeing if it would be better to introduce new wp_cache functions to do the checking of the last changed value as there are enough occurrences of the pattern in Core that it could be worthwhile (18, based on a quick check).

A getter/setter function would be great, we'd just need to account for N timestamps, because some keys have multiple (posts+terms) for example.

[spacedmonkey]

@peterwilsoncc Have you had a chance to look at ​https://github.com/WordPress/wordpress-develop/pull/8728?

This introduces a new function wp_is_valid_last_changed. This is a very similar idea to what you mentioned in comment.

[peterwilsoncc]

@spacedmonkey Yes, I saw that. I'm just wondering whether it would be better to write the code once rather than duplicating it for each instance of a cache key using lastchanged. If it were once or two locations, I'd be happy to copy-paste but with 18ish occurrences in core it seems better to me to unify the code in a single location.

[spacedmonkey]

How about the following functions.

function wp_query_cache_get( $cache_key, $group, $last_changed )

function wp_query_cache_set( $cache_key, $data, $group, $last_changed ){
wp_cache_set( $key, [ 'data' => $data, 'last_chagned' => $last_changed ], $group)
}

I also consider wp_cache_get_query_data and wp_cache_set_query_data for the naming. Thoughts @peterwilsoncc @flixos90 ?

I want to avoid doing another wp_cache_get_last_changed, as this might result in a remote cache get ( like in the case of @rmccue ). That is why I am passing $last_changed into the function instead of passing $cache_group.

Update:

$last_changed has to be a passed in, as there is the following code in WP_Query.

$last_changed = wp_cache_get_last_changed( 'posts' );
if ( ! empty( $this->tax_query->queries ) ) {
        $last_changed .= wp_cache_get_last_changed( 'terms' );
}

Reviewing feedback, I see the need for multiple calls, so wp_cache_get_muliple_query_data or wp_query_cache_get_multiple.

[peterwilsoncc]

@spacedmonkey I'm happy if last changed needs to be passed to the functions given your and Till's comments.

As to naming, I think naming things is very hard. I can see a few ​plugins using the last changed pattern for cache invalidation so I think my original suggestion of using the keyword query isn't great. Note: there are a few false positives in the link provided.

Canonically, I think we'd want the naming convention to be with the cache API (ie, wp_cache_ prefixed) rather than wp_query prefixed as we'll eventually be using this elsewhere.

Maybe it's as simple as wp_cache_last_changed_set|get|etc but others will have opinions, I am sure.

[tillkruess]

Just my two cents, feel free to ignore.

Canonically, I think we'd want the naming convention to be with the cache API (ie, wp_cache_ prefixed) rather than wp_query prefixed as we'll eventually be using this elsewhere.

Agreed on sticking with wp_cache_*() prefixes for these functions.

Maybe it's as simple as wp_cache_last_changed_set|get|etc but others will have opinions, I am sure.

That might be confusing with the existing wp_cache_(get|set)_last_changed()?

I also consider wp_cache_get_query_data and wp_cache_set_query_data for the naming.

I don't mind wp_cache_(get|set)_query().

As for the signature, to accommodate multiple timestamps and an optional expiration, this would be my suggestion. That way the implementation can decide on how to handle multiple $last_changed.

<?php
function wp_cache_set_query(
    string $key,
    mixed $data,
    string $group,
    float|float[] $last_changed,
    int $expire = 0
) { }

[peterwilsoncc]

wp_cache_set_query etc works for me. If others suggestions come in, we can decide what colour to paint the bike shed at that time.

[spacedmonkey]

After feedback from @peterwilsoncc @tillkruess @sanchothefat @flixos90 I have rewritten the PR at ​https://github.com/WordPress/wordpress-develop/pull/8728.

This new PR adds the following functions.

wp_cache_get_query_data( $cache_key, $group, $last_changed )
wp_cache_set_query_data( $cache_key, $data, $group, $last_changed )
wp_cache_set_multiple_query_data( $data, $group, $last_changed )
wp_cache_get_multiple_query_data( $cache_keys, $group, $last_changed )

The updated PR, does not have unit tests and docs yet. It just a sense check on the approach, I will get the PR into a final stage to merge.

I am happy with the new functions, it makes it easier to update the code and add these style caches to your own code.

I have also moved this ticket into the milestone, as it looks like it is going ahead.

[flixos90]

#52582 was marked as a duplicate.