Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve generate_cache_key to reduce cache miss while avoiding collision #5347

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

kt-12
Copy link
Member

@kt-12 kt-12 commented Sep 28, 2023

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


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@kt-12 kt-12 marked this pull request as ready for review October 2, 2023 12:16
@peterwilsoncc
Copy link
Contributor

When generating the cache key, the SQL statement is normalized to use the same fields regardless of the processed SQL statement:

$new_request = str_replace( $fields, "{$wpdb->posts}.*", $this->request );
$cache_key = $this->generate_cache_key( $q, $new_request );

However, in the following lines the actual cached data an be altered depending on the arguments used in the query in the code immediately following the lines linked above.

You're right, it might be possible to optimize the cache key generation to increase the number of cache hits but the cache key will need to consider some of the options used. It may be worth considering:

  • normalizing arguments a little more, for example the post type argument 'post' and [ 'post' ] are equivalent; as are [ 'page', 'post' ] and [ 'post', 'page' ]
  • whether additional arguments can be removed from the cache key

I think we'd need a bunch of tests to make sure there aren't cache collisions too.

@kt-12 kt-12 changed the title Remove args seralization for generate_cache_key Improve generate_cache_key to reduce cache miss while avoiding collision Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants