Unnecessary writes to cache in `WP_Query`

[spacedmonkey]

Follow on from [55035] and #57373.

In WP_Query, the class calls the update_post_caches function. This updates post caches. However, this function does not check to see if the post is already in cache. For site using persistent object caches this can result in lots of writes to cache that are unnecessary.

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

[johnbillion]

Is this a candidate for 6.1.2?

[spacedmonkey]

Replying to johnbillion:

Is this a candidate for 6.1.2?

Could be. @flixos90 seemed keen to get it in. As performance lead, I will defer to him.

[johnbillion]

Moving for visibility

[OllieJones]

FWIW, the persistent cache implementations don't push through unchanged values to the object store. They compare first ( serialize($old) !== serialize($new) ) That's not a super-cheap operation, but it saves object store traffic.

See ​https://github.com/WordPress/wordpress-develop/pull/3793#discussion_r1063023404 for the idea that led to this.

After adding caching to WP_Query in https://core.trac.wordpress.org/changeset/53941, the follow up change https://core.trac.wordpress.org/changeset/54352 aimed to eliminate excessive cache writes by relying on _prime_post_caches(), to only refresh those caches for posts that are not in cache yet. That on the other hand resulted in an additional database query, since _prime_post_caches() only supports being passed IDs, which led to a partial revert in https://core.trac.wordpress.org/changeset/55035.

This changeset restores the state in [54352] in a way that avoids both the excessive cache writes and the additional database query. It does so by adding support for passing post objects to _prime_post_caches(): The first parameter can now be either an array of post IDs or an array of post objects.

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

[flixos90]

@spacedmonkey @peterwilsoncc I opened a PR ​https://github.com/WordPress/wordpress-develop/pull/3868 for your review.

Given the parameter change, I would say this is more suitable for 6.2 instead of 6.1.2.

The real issue in 57498 is that when split queries are run, posts are not primed in caches. This change, only primes that cache if _prime_post_cache has not need called already.

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

[spacedmonkey]

I have another solution for this that can be found ​here.

I am personally not a fan of changing the params passed to _prime_post_caches. This function is a api in core, as they are other functions like _prime_comment_caches etc. If we make this change here, we should really make the change elsewhere.

@spacedmonkey

I am personally not a fan of changing the params passed to _prime_post_caches. This function is a api in core, as they are other functions like _prime_comment_caches etc. If we make this change here, we should really make the change elsewhere.

The change is a backward compatible enhancement, so IMO reasonable. I agree with your point on consistency: We could definitely make this change also for the other _prime_*_caches() functions, though if we do that it should be in a separate ticket/PR.

@spacedmonkey

I am personally not a fan of changing the params passed to _prime_post_caches. This function is a api in core, as they are other functions like _prime_comment_caches etc. If we make this change here, we should really make the change elsewhere.

The change is a backward compatible enhancement, so IMO reasonable. I agree with your point on consistency: We could definitely make this change also for the other _prime_*_caches() functions, though if we do that it should be in a separate ticket/PR.

Can you explain how this is different from ​https://github.com/WordPress/wordpress-develop/pull/3868 in terms of behavior? What is this implementation optimizing for? I'm not sure I follow.

So there two ways posts are loaded.

1. A query for just ​ids is run and _prime_post_caches is called to prime posts in cache.
2. A query​is run to get the whole post row. Posts are primed in cache by ​calling update_post_caches.

update_post_caches is called whatever query is run, even if _prime_post_caches already been called. This is wasteful and result in more calls and more cache sets that are not needed.

We only need to update_post_caches for posts not in cache for case 2 ( see above ). In case 1, posts will always be in cache as _prime_post_caches. I want to above calling _prime_post_caches lots of times. If wp_cache_get_mulitple is not implemented, it can result in lots and lots of calls to cache that are unneeded.

Change WP_Query is stressful and hard, I get it. I have made lots of changes to it lately. I don't want to add complexity to this massive class, maybe moving this new logic out to a new function would be help.

Sharing here for extra context ​https://github.com/WordPress/wordpress-develop/pull/3871#issuecomment-1397416664

[flixos90]

This would benefit from another review as there are 3 alternative PRs with different approaches for a fix. @peterwilsoncc @johnbillion Would you mind taking a look at them?

[spacedmonkey]

Doing some research into this, look at different add methods for different object caching drop-ins

• ​Automattic memcached
• ​pantheon-systems redis
• ​W3 Edge memcached
• ​SQlite object cache
• ​Memcached redux

Some both to check if the exist exists before writing it and others do not. I am starting to think this is not a big problem. It is kind of up to the object cache implementor to check if it exists before writing.

[flixos90]

@spacedmonkey Fair point. I think we can keep thinking about this a bit more, but this doesn't seem critical for 6.1.2, and I'd argue it's not even a bug, rather an enhancement.

I'll move this to "Future Release" for now, but it may also end up as a wontfix if that's what we land on.

[spacedmonkey]

While working on this ticket, I found an issue with the core implementation of object-cache.php included in the test suite. See #57963.

This may not be a problem with core, but this testing file. Once this is fixed, we can close this ticket out.

[spacedmonkey]

I am now no longer seeing this issue now that [55577] was merged.

I am going to mark this as invalid and close this ticket out.