Make WordPress Core

Opened 19 months ago

Closed 15 months ago

Last modified 15 months ago

#57150 closed enhancement

Implement wp_cache_get_multiple() in wp_queue_posts_for_term_meta_lazyload()

Reported by: ocean90's profile ocean90 Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch dev-feedback has-unit-tests commit needs-dev-note
Focuses: performance Cc:

Description (last modified by SergeyBiryukov)

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.

Attachments (2)

57150.png (242.7 KB) - added by ocean90 19 months ago.
57150.patch (750 bytes) - added by skithund 15 months ago.

Download all attachments as: .zip

Change History (33)

@ocean90
19 months ago

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


19 months ago
#1

  • Keywords has-patch added

@ocean90 commented on PR #3647:


19 months ago
#2

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

Before:
https://i0.wp.com/user-images.githubusercontent.com/617637/202765795-574a1b76-d00e-4a2b-805c-11839b427fd9.png

After:
https://i0.wp.com/user-images.githubusercontent.com/617637/202765750-7134dcff-c5de-4bd4-8063-f48b4e683d79.png

#3 @SergeyBiryukov
19 months ago

  • Description modified (diff)
  • Summary changed from Implement wp_cache_get_multi() in wp_queue_posts_for_term_meta_lazyload() to Implement wp_cache_get_multiple() in wp_queue_posts_for_term_meta_lazyload()

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


16 months ago

#5 @costdev
16 months ago

  • Keywords dev-feedback added

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

#6 @flixos90
16 months ago

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

#7 @spacedmonkey
16 months ago

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

#8 @flixos90
16 months ago

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

#9 @spacedmonkey
16 months ago

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

#10 @spacedmonkey
16 months ago

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

#11 @spacedmonkey
16 months ago

  • Keywords has-unit-tests added

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

#12 @flixos90
16 months ago

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

#13 @spacedmonkey
16 months ago

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

#14 @spacedmonkey
16 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

@spacedmonkey commented on PR #3647:


16 months ago
#15

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.

#17 @skithund
15 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#18 @skithund
15 months ago

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

#19 @spacedmonkey
15 months ago

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

#21 @spacedmonkey
15 months ago

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

#22 @peterwilsoncc
15 months ago

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

@skithund
15 months ago

#23 @skithund
15 months ago

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

#24 @Chouby
15 months ago

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

#26 @ocean90
15 months ago

  • Keywords commit added

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

#27 @ocean90
15 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#29 @milana_cap
15 months ago

  • Keywords has-dev-note added

#30 @milana_cap
15 months ago

  • Keywords needs-dev-note added; has-dev-note removed

#31 @flixos90
15 months ago

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.

Note: See TracTickets for help on using tickets.