Make WordPress Core

Opened 10 months ago

Last modified 9 months ago

#59594 new enhancement

Remove $taxonomies from cache key generation in WP_Term_Query

Reported by: spacedmonkey's profile spacedmonkey Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: good-first-bug dev-feedback has-patch
Focuses: performance Cc:

Description

When generating cache keys in generate_cache_key method in WP_Term_Query, a serialized version of $taxonomies is used. This is not needed as the array of taxonomies are stored in the arguments used to generate the $cache_args variable. This part of the cache key can safely be removed.

Before

        $taxonomies = (array) $args['taxonomy'];
        $key        = md5( serialize( $cache_args ) . serialize( $taxonomies ) . $sql );

After

        $key = md5( serialize( $cache_args ) . $sql );

Change History (4)

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


10 months ago

#2 @joemcgill
10 months ago

  • Keywords dev-feedback added

Good first step here would be to do some archival work through the commit history to see why the taxonomies were added in the first place to validate that it's safe to remove.

#3 @flixos90
10 months ago

This cache key has been used basically forever: It was present in the initial version of WP_Term_Query (see https://github.com/WordPress/wordpress-develop/commit/8b9bdaccd28a95092b14c71df5523ffcd71a321f#diff-e11115d9cf3918f9f3436b7be4935810f97d4834a0863b8344b83377b50f1800R614), where it was simply taken over from the cache key previously applied in get_terms() (see https://github.com/WordPress/wordpress-develop/commit/8b9bdaccd28a95092b14c71df5523ffcd71a321f#diff-7813f38c6b905b75c4b11bf2750c63ba0f5a1227ffc4f012deeeb22999cee68bL1615).

I think this is worth revisiting. My hunch is that it was simply taken over out of caution, as that was a massive change. Even back then, I believe the separate $taxonomies consideration wasn't necessary in WP_Term_Query due to [36614], which had already landed before the WP_Term_Query introduction in [37572].

$taxonomies used to be a separate parameter in get_terms(), which is the reason this code exists. However for WP_Term_Query that was never the case, there is only $args['taxonomy']. So including that in the cache key here is redundant indeed.

That said, changing that would only be an improvement in terms of cleaning up old code. Functionally, it wouldn't make a difference, and there is a single potentially negative consideration that making this change would make lots of existing caches invalid / outdated. Not necessarily a blocker, but worth considering particularly since the benefits of this change would IMO be negligible.

This ticket was mentioned in PR #5577 on WordPress/wordpress-develop by nirav7707.


9 months ago
#4

  • Keywords has-patch added
Note: See TracTickets for help on using tickets.