Make WordPress Core

#57498 closed enhancement (invalid)

Unnecessary writes to cache in `WP_Query`

Reported by: spacedmonkey's profile spacedmonkey Owned by: flixos90's profile flixos90
Milestone: Priority: normal
Severity: normal Version: 6.1
Component: Query Keywords: has-patch
Focuses: performance Cc:

Description

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.

Change History (19)

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


16 months ago
#1

  • Keywords has-patch added

#2 follow-up: @johnbillion
16 months ago

Is this a candidate for 6.1.2?

#3 in reply to: ↑ 2 @spacedmonkey
16 months ago

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.

#4 @johnbillion
16 months ago

  • Milestone changed from 6.2 to 6.1.2

Moving for visibility

#5 @OllieJones
16 months ago

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.

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


16 months ago
#6

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

#7 @flixos90
16 months ago

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

@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.

Last edited 16 months ago by flixos90 (previous) (diff)

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


16 months ago
#8

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

#9 @spacedmonkey
16 months ago

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

@spacedmonkey commented on PR #3868:


16 months ago
#10

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.

@flixos90 commented on PR #3868:


16 months ago
#11

@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.

@flixos90 commented on PR #3868:


16 months ago
#12

@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 commented on PR #3871:


16 months ago
#13

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 queryis 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.

#15 @flixos90
16 months ago

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?

#16 @spacedmonkey
16 months ago

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

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.

#17 @flixos90
16 months ago

  • Milestone changed from 6.1.2 to Future Release
  • Type changed from defect (bug) to enhancement

@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.

#18 @spacedmonkey
14 months ago

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.

#19 @spacedmonkey
14 months ago

  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.