Implement wp_cache_get_multiple() in wp_queue_posts_for_term_meta_lazyload()

[ocean90]

This is a follow-up to #36953 and #50352.

Quoting @boonebgorges:

WP_Query calls wp_queue_posts_for_term_meta_lazyload(), which in turn grabs the {$taxonomy}_relationships cache for each found post, for each taxonomy that they belong to. So if you're fetching 20 items, and each item is in 10 taxonomies, the process will require 200 cache gets. This is in addition to the dozens of other other cache priming gets that happens in WP_Query.

On very high traffic sites running an object cache backend, the high volume calls to the cache can cause performance issues.

This is something I have recently noticed for a site. Attaching a screenshot of the trace details of a single request. This is the result for the main query with 15 posts and each post can have one or more terms in six taxonomies.

This seems to be a place where core should also make use of wp_cache_get_multiple() so that cache implementations like the ​Redis Object Cache can combine the requests into a single request.

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

This is really looking good! Will do some more tests next week to make sure the terms and such are correctly fetched.

Before:

After:

[costdev]

This ticket was discussed in the recent bug scrub. There is a review request for @peterwilsoncc. Peter, are you available to take a look at the PR, or should others help out on this one?

Also, as all new code should ideally have tests, are PHPUnit tests possible for this change? I'm not quite sure how they would be written for this.

Props to @mukesh27

[flixos90]

@ocean90 @spacedmonkey @peterwilsoncc I think it's best at this point to punt this ticket to "Future Release" and continue work on it for 6.3.

There has not been any update in more than 2 months on this, and the ticket does not have an owner. That, together with all the other remaining features and enhancements in this milestone, makes me think this does not appear to have enough engineering support right now to make it in time for Beta next Tuesday.

[spacedmonkey]

@flixos90 Disagree here. We have some time before the beta. The issue here is that code is complete but we need some unit tests. But there are no good ways to test this. If we are okay with manual testing, then we can commit.

[flixos90]

@spacedmonkey Fair point. We should have an owner though that takes responsibility to get this ticket across the finish line. Would you be willing to manage that?

Regarding testing, could you please outline a way to test this?

[spacedmonkey]

@ocean90 Are you still working on this or do you want me to take over. We are a week out from beta 1 and I need to know soon, if we want to get this into 6.2.

[spacedmonkey]

Spent the night on this ticket. There is now unit tests.

[flixos90]

@spacedmonkey @ocean90 Is this something we can finalize before the 6.2 Beta 1 tomorrow? Otherwise this seems like a punt candidate.

[spacedmonkey]

@flixos90 Can you do a code review? I just pushed some changes requested by @ocean90. But I think this is a good to go.

[spacedmonkey]

In 55252:

Taxonomy: Implement wp_cache_get_multiple in wp_queue_posts_for_term_meta_lazyload.

In [47938] the wp_cache_get_multiple function was added to core. This function allows for multiple cache keys to be received from cache in a single function call. wp_queue_posts_for_term_meta_lazyload function does many calls to cache. To get taxonomy relationship for multiple posts and get all terms. Replace calls to get_object_term_cache with calls to wp_cache_get_multiple and _prime_term_caches. This improves performance on sites that implement the wp_cache_get_multiple in their object caching drop-in.

Props spacedmonkey, ocean90, SergeyBiryukov, costdev, flixos90, joemcgill, 10upsimon.
Fixes #57150.

This looks ready to be committed. Agreeing that the loop in the tests can be optimized.

That can be done in a follow up commit.

Committed

​https://github.com/WordPress/wordpress-develop/commit/35a678b0e60837c85a16005ec27b1bc867f5dd53

[skithund]

My guess is that [55252] broke multilingual sites with translated posts using Polylang Pro plugin. ​Regular non-Pro Polylang has 700+k installations.

Fatal error: Uncaught Error: Illegal offset type in isset or empty
in /app/wordpress/wp-includes/class-wp-metadata-lazyloader.php on line 95

var_dump() of $object_ids in that function call looks like this.

array(3) {
  [0]=>
  object(WP_Term)#12582 (10) {
    ["term_id"]=>
    int(7)
    ["name"]=>
    string(17) "pll_611b764508d29"
    ["slug"]=>
    string(17) "pll_611b764508d29"
    ["term_group"]=>
    int(0)
    ["term_taxonomy_id"]=>
    int(7)
    ["taxonomy"]=>
    string(17) "post_translations"
    ["description"]=>
    string(48) "a:3:{s:2:"fi";i:12;s:2:"sv";i:14;s:2:"en";i:25;}"
    ["parent"]=>
    int(0)
    ["count"]=>
    int(3)
    ["filter"]=>
    string(3) "raw"
  }
  [1]=>
  int(2)
  [2]=>
  int(7)
}

I'll try to get some extra bandwidth for later in the week to create reproduction steps and if it's reproducible with non-Pro version of Polylang. Error came from a production site pulled for development and core upgraded to nightly.

Downgrading back to 6.1.1 fixes the problem in the same (development) environment.

[skithund]

Reverting [55252] changes in wp-includes/post.php and site doesn't die with fatal error anymore.

[spacedmonkey]

Hey @ocean90 do you mind taking a looking into this. I am AFK after Wordcamp Asia.

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

[spacedmonkey]

I might have a fix at ​#4107. @skithund @ocean90 maybe you could take a look.

[peterwilsoncc]

cc @chouby for bug affecting polylang in comment #17 and a potential fix in the ​linked pull request.

[skithund]

@spacedmonkey that didn't fix it.

The problem is in line 7805, where you're reassigning $term_ids while doing foreach loop. Renaming $term_ids to $_term_ids like in row 7825 does the trick.

I'm unable to create a proper Github PR/patch now, but attachment:57510.patch worked.

[Chouby]

@peterwilsoncc Thank you for the ping.

On our side, we have been aware of this issue for a few days. Although we did not detect it in our PHPUnit tests because the Polylang developement branch was already fixed by @ocean90 ​(caching term ids instead of WP_Term objects) for WP 6.0+, it was detected by @chrystl in her tests and also reported by a few users :).

So you can consider that we will fix that on our side.

I have also tested 57150.patch​ proposed by @skithund. I confirm that it also fixes the issue on WordPress side. This could help for backward compatibility with potential other plugins still caching WP_Term objects.

https://core.trac.wordpress.org/attachment/ticket/57150/57150.patch

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

[ocean90]

@skithund Thanks for the report. I thought I had fixed this in ​https://github.com/WordPress/wordpress-develop/pull/3647/commits/2c62f430aa468abef215b4b23fe19b08f97374e6 but seems like it got reintroduced by some later changes.

Submitted a PR based on your patch at ​https://github.com/WordPress/wordpress-develop/pull/4111.

[ocean90]

In 55401:

Taxonomy: Rename temporary variable in wp_queue_posts_for_term_meta_lazyload().

This ensures the $term_ids variable only contains term IDs and is not filled with full term objects due to deprecated term caching.

Introduced in [55252].

Props skithund, Chouby, joemcgill, flixos90, ocean90.
Fixes #57150.

Closed in favour of ​https://github.com/WordPress/wordpress-develop/pull/4111.

[flixos90]

Drafted a note for this to be included in the Field Guide's notes section, as shared in ​https://wordpress.slack.com/archives/C02KGN5K076/p1677531978930639?thread_ts=1677512400.171959&cid=C02KGN5K076.