WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 5 weeks ago

#39242 accepted enhancement

Add caching to count_user_posts()

Reported by: johnjamesjacoby Owned by: whyisjake
Milestone: 5.6 Priority: normal
Severity: normal Version: 3.0
Component: Users Keywords: has-patch dev-feedback needs-unit-tests needs-testing
Focuses: rest-api, performance Cc:

Description

The count_user_posts() function does not cache the results of its query.

On the surface, this seems easy enough, but since it accepts multiple parameters and intersects with page/post editing, it starts to get complex quickly as authors change, posts are transitioned from public to private, and custom post types do more elaborate things.

Since some themes use this directly (Twenty Fourteen, et al) and the REST API exposes this data too, there's at least a small win from not hitting the database for each and every check.

Eventually, count_many_users_posts() may be able to check for cached counts for existing users, and only query for (and prime the caches of) users who do not already have cached counts.

Attachments (2)

39242.diff (1.4 KB) - added by milindmore22 3 years ago.
Cache for count_user_posts
39242.2.diff (1.3 KB) - added by desrosj 5 weeks ago.

Download all attachments as: .zip

Change History (8)

#1 @DJPaul
3 years ago

We're seeing this uncached function be an issue on a A8C VIP hosted site when querying the REST API users endpoint.

@milindmore22
3 years ago

Cache for count_user_posts

#2 @milindmore22
3 years ago

  • Focuses rest-api performance added

Yes, performances of websites using REST is highly affected.

I have added a patch based on WP VIP function below.

<?php

/**
 * Cached version of count_user_posts, which is uncached but doesn't always need to hit the db
 *
 * count_user_posts is generally fast, but it can be easy to end up with many redundant queries
 * if it's called several times per request. This allows bypassing the db queries in favor of
 * the cache
 */
function wpcom_vip_count_user_posts( $user_id ) {
    if ( ! is_numeric( $user_id ) ) {
        return 0;
    }

    $cache_key = 'vip_' . (int) $user_id;
    $cache_group = 'user_posts_count';

    if ( false === ( $count = wp_cache_get( $cache_key, $cache_group ) ) ) {
        $count = count_user_posts( $user_id );

        wp_cache_set( $cache_key, $count, $cache_group, 5 * MINUTE_IN_SECONDS );
    }

    return $count;
}

Do we need proper cache invalidation or 5 Min expiration timeout will do ?

I think we should handle this in 5.0 thoughts ?

#3 @RogerTheriault
7 weeks ago

Just came here to chime in that this (or a similar) improvement is still needed for sites that have many rows in the posts table and use the REST API.

#4 @whyisjake
7 weeks ago

  • Keywords has-patch dev-feedback added; needs-patch 2nd-opinion removed
  • Milestone changed from Awaiting Review to 5.6
  • Owner set to whyisjake
  • Status changed from new to accepted
  • Version set to trunk

@desrosj
5 weeks ago

#5 @desrosj
5 weeks ago

  • Version changed from 5.5 to 3.0

Changing the version to 3.0 because that's the first version the function existed without caching.

Looking at 39242.diff, it doesn't take into account the $post_type or $public_only parameters.

Also, looking at other cached values in core, it seems that not specifying an expiration in favor of invalidating the cache when appropriate is preferred. In this case, when a post is updated (published/unpublished, or visibility is changed).

39242.2.diff adds the post type to the cache key, and introduces an additional group for when $public_only is true.

I toyed around with where to invalidate caches for a while. Somewhere inside clean_user_cache() feels like the right spot. But there is no way to pass the two additional parameters. Inside wp_insert_post() should be more feasible.

#6 @johnbillion
5 weeks ago

  • Keywords needs-unit-tests needs-testing added

This would benefit greatly from tests that cover repeated calls, cache invalidation, etc.

Note: See TracTickets for help on using tickets.