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

Fast follow: post parent ID cache #5386

Conversation

peterwilsoncc
Copy link
Contributor

@peterwilsoncc peterwilsoncc commented Oct 3, 2023

https://core.trac.wordpress.org/ticket/59188

  • primes parent id cache in update_post_cache()
  • adds @since annotation
  • renames function for clarity --- it is currently a little unclear and could imply the parent post object is cached in its entirity
  • adds parameter to _prime_post_caches for priming the post parent ID caches if not already primed.

The docblock for the new parameter needs some work.

Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like these changes. Changing update_post_cache, would prime a cache that is not used in most cases. This is just another memcache / memory writing call, which is wasteful.

I also don't see the point of adding a new parameter to _prime_post_caches, you can use call
_prime_post_parent_id_caches after calling _prime_post_caches

I would consider the name change of the function.

}
wp_cache_add_multiple( $data, 'posts' );
wp_cache_add_multiple( $parent_ids, 'post_parent' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cache will not be used in most caches. I don't think it is a good idea to prime it on all requests.

@peterwilsoncc
Copy link
Contributor Author

I don't like these changes. Changing update_post_cache, would prime a cache that is not used in most cases. This is just another memcache / memory writing call, which is wasteful.

In this case, I'd question the value of introducing the new cache and instead change the shape of the cache in WP_Query calls requesting id=>parent fields to include the post parent.

$cache_value = array(
'posts' => $post_ids,
'found_posts' => $this->found_posts,
'max_num_pages' => $this->max_num_pages,
);

For backcompat, the change of the cache's shape would need to be considered wehn getting the data.

@joemcgill
Copy link
Member

@peterwilsoncc and @spacedmonkey, it seems like there's consensus here to commit the function name change and the @since annotation updates, but some disagreement about whether the new cache should be primed or if it should be removed and replaced with updates to the main WP_Query cache, if I'm understanding @peterwilsoncc's comment above correctly:

I'd question the value of introducing the new cache and instead change the shape of the cache in WP_Query calls requesting id=>parent fields to include the post parent

Can you two clarify what is needed to move this forward?

@spacedmonkey
Copy link
Member

@joemcgill

What we are trying to avoid from the original ticket, was lookuping the whole post object if all you need is a parent id. This is why this new cache was introduced.

So here is something I considered in the original PR.

  • We could change all id queries to get id and parent id. Then cache the parent ids in as another key in cache the array. Okay, this would help id=>parent queries, but make id queries slower, as you are requested more from the database. As id queries are FAR more common, I think we should optimize for id queries over the rare id=>parent.
  • On the first run on the query, the parent cache is primed if id=>parent. These caches are not expiring. There are only invalided if you update the post or flush the whole cache. Meaning, if you are using object caching, then the first time you load the post list page in wp-admin, the parent will be primed and cache will live forever. If not there a couple of posts that were updated, the new function will do a very small query to get the missing caches.
  • Priming this cache when it is not used or needed, takes resources. Adding this priming may negatively affect performance.

How about just updating the post parent cache on wp_insert_post, that way the value would be updated in cache just once and not primed all the time?

@peterwilsoncc peterwilsoncc deleted the follow-up/58368-post-parent-caches branch October 9, 2023 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants