Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#55352 closed enhancement (fixed)

Improve cache key generation in WP_Term_Query

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.0 Priority: normal
Severity: normal Version: 4.7
Component: Taxonomy Keywords: has-patch has-unit-tests has-dev-note
Focuses: performance Cc:

Description

Using the WP_Term_Query, caches are generated using the args passed to the class instance. However, there are a number of params passed, that would not affect the cache but would create a new cache key. Couple of example of these are.

  • pad_counts
  • update_term_meta_cache

These params can be removed from the cache key with no affect to cache.

There are other params such as slug and term_taxonomy_id, that can be both string and array. If all these were convert to arrays, it would improve the likelyhood that would be a cache hit.

There are likely more wins to be had to improve caches here.

Change History (29)

#1 @spacedmonkey
2 years ago

  • Keywords needs-patch needs-unit-tests added

CC @peterwilsoncc

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


2 years ago
#2

  • Keywords has-patch added; needs-patch removed

peterwilsoncc commented on PR #2398:


2 years ago
#3

As this is on future release, I'm putting this on my ignore list while doing reviews for 6.0.

Based on a quick glance it looks sensible but I'd want to spend some additional time on it.

#4 @spacedmonkey
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Milestone changed from Future Release to 6.0
  • Owner set to spacedmonkey
  • Status changed from new to assigned

peterwilsoncc commented on PR #2398:


2 years ago
#5

Fix lints

I promise that wasn't a deliberate troll. Let's get this puppy in

#6 @peterwilsoncc
2 years ago

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

In 52970:

Taxonomy: Increase cache hits in WP_Term_Query.

Increase the number of cache hits in WP_Term_Query by normalizing data included in the cache key.

Arguments that do not affect the SQL query, eg update_term_meta_cache, are removed from cache key generation. Arguments that are accepted in multiple formats, eg a string and an array, are normalized for both the cache key and the SQL query.

Props spacedmonkey.
Fixes #55352.

peterwilsoncc commented on PR #2398:


2 years ago
#7

Committed in e6eaa58741ba751ba08b8bcccf13298428d69f3e (canonically https://core.trac.wordpress.org/changeset/52970)

#8 @SergeyBiryukov
2 years ago

Lines like this seem a bit too hard to read for me without spending some time to parse the logic:

$args['term_taxonomy_id'] = is_string( $args['term_taxonomy_id'] ) && 0 === strlen( $args['term_taxonomy_id'] ) ? array() : array_map( 'intval', (array) $args['term_taxonomy_id'] );

I think this can be simplified to:

if ( '' === $args['term_taxonomy_id'] ) {
	$args['term_taxonomy_id'] = array();
} else {
	$args['term_taxonomy_id'] = array_map( 'intval', (array) $args['term_taxonomy_id'] );
}

#9 @SergeyBiryukov
2 years ago

In 52972:

Coding Standards: Simplify some long conditions in wp-includes/class-wp-term-query.php.

This aims to improve readability and make the logic easier to parse at a glance.

Follow-up to [40293], [52970].

See #55352, #54728.

#10 follow-up: @johnjamesjacoby
2 years ago

I’d like to kindly suggest, rather than inlining everything in existing methods, that a new method be used to reshape these arguments for improved cache key usage.

#11 @spacedmonkey
2 years ago

Created some related tickets to improve cache be generation for other Query classes.

#55460, #55461, #55462

#12 in reply to: ↑ 10 @peterwilsoncc
2 years ago

Replying to johnjamesjacoby:

I’d like to kindly suggest, rather than inlining everything in existing methods, that a new method be used to reshape these arguments for improved cache key usage.

What benefit does this provide?

Most of the changes were to normalize the DB query as it's sanitized. While nessesary for the cache key, this also improves hits to the DB servers query cache.

Normalizing the WP cache is about three lines of code.

#13 follow-up: @spacedmonkey
2 years ago

I did some more testing, I noticed, I was getting duplicate queries in the admin.

This was fix by changing this line to

$key          = md5( serialize( $cache_args ) . serialize( $taxonomies ) . $this->request );

to

$key          = md5( serialize( asort( $cache_args ) ) . serialize( $taxonomies ) . $this->request );

Seems like a small different in the cache args, can result in an new cache key.

I know @peterwilsoncc had a flagged something similar. As this is a small change, I think that can made it into 6.0

#14 in reply to: ↑ 13 @peterwilsoncc
2 years ago

Replying to spacedmonkey:

I know @peterwilsoncc had a flagged something similar. As this is a small change, I think that can made it into 6.0

Agreed, it's a bug on this enhancements.

When patching, are you able to write up some tests and also check if $taxonomies needs to be sorted too?

I'll leave you to decide whether you wish to reopen this ticket or create a follow up.

#15 @spacedmonkey
2 years ago

Follow up ticket is created #55619.

#16 @spacedmonkey
2 years ago

After even more testing, it seems like my first findings are wrong. The duplicate query came from something else.

Exclude can be both a string and an array. I will push a PR up now.

#18 @spacedmonkey
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopenning as new PR to review - #2630

hellofromtonya commented on PR #2630:


2 years ago
#19

Hey @spacedmonkey, can you share manual testing step-by-step instructions please including what code is needed to test it? This will help testers like me reproduce the issue and then validate the changes resolve it. Thanks!

spacedmonkey commented on PR #2630:


2 years ago
#20

If you go the category list page in the wp-admin, then you will see a duplicate query, which is where I found the issue. See screenshot.
https://i0.wp.com/user-images.githubusercontent.com/237508/165471463-12ab1e76-537e-4bfa-95a4-051ed1738357.png

To test manually, I wrote this script.

function test_init(){

	get_terms(array(
		'fields'           => 'all',
		'taxonomy'         => 'category',
		'exclude'          => '',
		'include'          => '',
	));
       get_terms( array(
		'taxonomy'         => 'category',
		'fields'           => 'all',
		'exclude'          => array(),
		'include'          => array()
      ));
}

add_action('init', 'test_init');

Then I used query monitor to see if a got duplicate queries.

spacedmonkey commented on PR #2630:


2 years ago
#21

My review is basically "what Tonya said" with some tests of those cases in the data provider too.

There are extra example of this in the data provider? So this is tested.

#22 @spacedmonkey
2 years ago

  • Keywords needs-dev-note added

Dev note ready for review.

peterwilsoncc commented on PR #2630:


2 years ago
#24

There are extra example of this in the data provider? So this is tested.

Sorry, I was unclear -- I was thinking of 0 and maybe even '0' to make sure the result is as expected.

peterwilsoncc commented on PR #2630:


2 years ago
#25

I pushed up a switch to empty() and added a array [?] vs no [?] and array [?] vs zero [?] test for both include and exclude.

spacedmonkey commented on PR #2630:


2 years ago
#26

I pushed up a switch to empty() and added a array ? vs no ? and array ? vs zero ? test for both include and exclude.

I am okay with that. Should we get this committed?

#27 @spacedmonkey
2 years ago

In 53309:

Taxonomy: Increase cache hits in WP_Term_Query when using include and exclude parameters.

Ensure that empty values of include and exclude passed to the parameters of WP_Term_Query, reused existing caches by
resetting values to an empty array.

Props Spacedmonkey, peterwilsoncc, hellofromtonya.
Follow-up to [52970].
See #55352.

#28 @spacedmonkey
2 years ago

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

spacedmonkey commented on PR #2630:


2 years ago
#29

Committed

Note: See TracTickets for help on using tickets.