Make WordPress Core

Opened 7 years ago

Last modified 16 months ago

#39242 accepted enhancement

Add caching to count_user_posts()

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: whyisjake's profile whyisjake
Milestone: Future Release 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 (5)

39242.diff (1.4 KB) - added by milindmore22 6 years ago.
Cache for count_user_posts
39242.2.diff (1.3 KB) - added by desrosj 4 years ago.
39242.3.diff (3.4 KB) - added by johnregan3 22 months ago.
Not fully functional. Unit tests added, but others are failing.
39242.4.diff (4.7 KB) - added by wpgurudev 16 months ago.
Refactor count_user_posts function
39242-unit-tests.diff (8.9 KB) - added by wpgurudev 16 months ago.
Refactored function with unit tests

Download all attachments as: .zip

Change History (22)

#1 @DJPaul
7 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
6 years ago

Cache for count_user_posts

#2 @milindmore22
6 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
4 years 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
4 years 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
4 years ago

#5 @desrosj
4 years 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
4 years ago

  • Keywords needs-unit-tests needs-testing added

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

This ticket was mentioned in Slack in #core by metalandcoffee. View the logs.


4 years ago

#8 @hellofromTonya
4 years ago

Hey @whyisjake, With beta 1 coming fast, is this ticket still on your radar for 5.6 milestone? If yes, what's the next step for it?

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

#10 @hellofromTonya
4 years ago

  • Milestone changed from 5.6 to Future Release

In today's core scrub, decision made to punt this ticket to Future Release as 5.6 Beta 1 is 5 days (next Tuesday 20th Oct) and this ticket needs tests.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#11 @TimothyBlynJacobs
4 years ago

#46249 was marked as a duplicate.

#12 @Krstarica
2 years ago

REST API is still terribly slow, is it possible to fix this issue? Thanks.

@johnregan3
22 months ago

Not fully functional. Unit tests added, but others are failing.

#13 @johnregan3
22 months ago

Modified the code so that if an array is passed as the $post_type param, the cache key name will be generated correctly.

I also composed initial Unit Tests.

However, the tests currently come back as such:

There were 3 failures:

1) Tests_Media::test_wp_get_attachment_image_should_use_wp_get_attachment_metadata
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<img width="999" height="999" src="http://example.org/wp-content/uploads/2022/07/test-image-testsize-999x999.jpg" class="attachment-testsize size-testsize" alt="" decoding="async" loading="lazy" srcset="http://example.org/wp-content/uploads/2022/07/test-image-testsize-999x999.jpg 999w, http://example.org/wp-content/uploads/2022/07/test-image-large-150x150.jpg 150w" sizes="(max-width: 999px) 100vw, 999px" />'
+'<img width="999" height="999" src="http://example.org/wp-content/uploads/2022/07/test-image-testsize-999x999.jpg" class="attachment-testsize size-testsize" alt="" decoding="async" loading="lazy" srcset="http://example.org/wp-content/uploads/2022/07/test-image-testsize-999x999.jpg 999w, http://example.org/wp-content/uploads/2022/07/test-image-large-1-150x150.jpg 150w" sizes="(max-width: 999px) 100vw, 999px" />'

/var/www/tests/phpunit/tests/media.php:2646

2) WP_Test_REST_Users_Controller::test_get_item_published_author_pages
Failed asserting that 401 is identical to 200.

/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:3128
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:1152

3) Tests_User::test_count_many_users_posts
Failed asserting that '1' matches expected 2.

/var/www/tests/phpunit/tests/user.php:573

I'll continue tracking down these issues, but feel free to jump in and add feedback or your own ideas.

#14 @johnregan3
22 months ago

The reason unit test # 2 above is failing is because a post is being created and the value returned by count_user_posts() is returning the cached (former) value.

Looks like this will need an action hooked into (probably) 'save_post' that clears this cache. However, as the cache key is dependent upon the post type(s) are requested, it's trickier to guess the precise cache key we need to delete when a post is saved.

I'd love some input on the best way to approach this.

#15 @Krstarica
18 months ago

We have $cache_key = "count_user_{$post_type_label}_{$userid}"; plus taking into account $public_only param.

On save_post we actually don't know if post_type or user_is has changed, so the safest would be to delete cache for all count_user_% keys, although wp_cache_delete function does not support wildcards.

#16 @wpgurudev
17 months ago

  1. I applied the patch from @johnregan3 , I think the overall approach is fine. The only issue seems that it is not handling certain events which may need to invalidate this cache and recalculate the count.
  1. IMO, we can introduce an additional parameter $force = false to count_user_posts function as the last parameter with default value false to be backward compatible.
  1. If $force is set to true, function would recalculate the user posts count and set it as a new entry in the cache.
  1. This will be useful because we can simply call count_user_posts function with $force set to true on several hooks like save_post, deleted_post etc. to recalculate user posts count. This will further abstract the entire logic of calculating the user posts count without need to know the cache key and group names.

Further Challenges

  1. The main issue which I can observe is when we have multiple post types. Consider a situation where we pass
array( 'post', 'page', 'product' )

as $post_type to count_user_posts function.

  1. Now, consider if we delete a product then we need to update the cache count_user_post_page_product, but the challenge is how do we identify this combination, because we can easily update count_user_product, but we cannot be sure about post_page_product combination as it can be anything like post_product, page_product etc. as well.

@wpgurudev
16 months ago

Refactor count_user_posts function

#17 @wpgurudev
16 months ago

I have refactored the count_user_posts function to two smaller functions

  1. count_user_posts_for_single_type: checks count of posts for a single post type. This function leverages the wp cache.
  1. count_user_posts_for_multiple_types to check count of posts for multiple post types. This iterates over each post type and calls count_user_posts_for_single_type function in turn.

In addition to this, I have also added clear_count_user_posts_cache function which clears the cache. This function can be called on several conditions like author change of posts, visibility change etc.

I have added one such instance where author change will clear the cache. Some other instances can include

  1. Post visibility change from public to private or vice-versa.
  2. User deletion.
  3. User role change.

and so on..

We can keep adding functions and hook them to appropriate actions to clear the cache using clear_count_user_posts_cache function.

As of now, I'm clearing both the public and non-public posts count cache to keep things simplified.

@wpgurudev
16 months ago

Refactored function with unit tests

Note: See TracTickets for help on using tickets.