Improve cache key generation in WP_Term_Query

[spacedmonkey]

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.

[spacedmonkey]

CC @peterwilsoncc

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

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.

​Fix lints

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

[peterwilsoncc]

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.

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

[SergeyBiryukov]

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'] );
}

[SergeyBiryukov]

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.

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

[spacedmonkey]

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

#55460, #55461, #55462

[peterwilsoncc]

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.

[spacedmonkey]

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

[peterwilsoncc]

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.

[spacedmonkey]

Follow up ticket is created #55619.

[spacedmonkey]

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.

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

[spacedmonkey]

Reopenning as new PR to review - ​#2630

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!

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.

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.

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.

[spacedmonkey]

Dev note ready for ​review.

[spacedmonkey]

Has dev note - ​https://make.wordpress.org/core/2022/04/28/taxonomy-performance-improvements-in-wordpress-6-0/

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.

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

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?

[spacedmonkey]

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.

Committed