Add caching to get_page_of_comment()

[Viper007Bond]

Ack. get_page_of_comment() lacks caching which means if you call get_comment_link() for the same comment ID on the same page multiple times (and outside of the loop), it will query multiple times. You also can't cache the result between page loads (the big "uh oh").

Attached patch introduces a cache that stores comment ID => older comment count (in short, the result of the function's query) on a per-post basis.

When any comment's status is changed, the entire cache for that comment's parent post is deleted as it will likely affect other comments (for example, deleting a comment can change the page of newer comments).

I opted to make a new cache flag (group), but I'm not sure if using an existing one would be better or not.

Oh, and this patch needs through testing and/or a good review.

[dd32]

Oh, and this patch needs through testing and/or a good review.

For that entire reason..

[tott]

looks good, but should also consider custom comment types. i don't see this in the switch statement. apart of this it seems to work

[wmertens]

I just applied this on a 3.5.1 installation; it made our database happy. We have a couple of threads with way too many comments and despite the caching plugin our db was still getting hammered; now everything is humming along.

The patch didn't apply cleanly, I had to add it manually. I'll try to attach the patch here. If I can't manage that, it was simple to apply manually.

I don't understand the custom comment types comment - the previous code doesn't seem to handle them either.

[wmertens]

applies to 3.5.1

[wmertens]

With this patch though, we get the following warnings every time we delete a piece of spam:

Warning: Missing argument 2 for clear_page_of_comment_cache() in /home/pjaminet/public_html/perfecthealthdiet/wp/wp-includes/comment.php on line 913

Warning: Missing argument 3 for clear_page_of_comment_cache() in /home/pjaminet/public_html/perfecthealthdiet/wp/wp-includes/comment.php

I don't see how that can happen :-/

Other than that things seem to work fine.

[SergeyBiryukov]

Replying to wmertens:

With this patch though, we get the following warnings every time we delete a piece of spam

You need to specify the number of arguments in the add_action() call:

add_action( 'transition_comment_status', 'clear_page_of_comment_cache', 10, 3 );

[wmertens]

Applies to 3.6

[wmertens]

New patch for 3.6 with the correct call for add_action (thanks SergeyBiryukov)

Please add this, it slows down all sites that use paged comments, by a lot.

[wmertens]

Replying to wmertens:

Please add this, it slows down all sites that use paged comments, by a lot.

One example: If you use the recent comments widget, then every page view of the widget will generate 10 expensive queries.

[jeremyfelt]

11334.diff​ is a refresh against trunk with a couple notes.

• Tweaked code style a bit, clarified variable names.
• The patch makes use of wp_cache_replace() and would become the only place in core that does so if committed. Not sure if that says something or if there is a better approach now.

Not sure if this is critical enough to make it in 3.7. May want to consider punting so that there is more time to test.

[jeremyfelt]

11334.2.diff​ adds some basic unit testing for get_page_of_comment() to the previous patch.

[nacin]

Not sure if this is critical enough to make it in 3.7. May want to consider punting so that there is more time to test.

[boonebgorges]

In 31289:

Add basic unit tests for get_page_of_comment().

See #11334.
Props jeremyfelt.

[boonebgorges]

11334.3.diff​ cleans up the patch and adds caching-specific unit tests.

Critically, the patch also changes older-comment-count caching so that it's comment-specific rather than post-specific. If we cache on a post-by-post basis, then every comment associated with that post will return the same value for get_page_of_comment(), which is clearly wrong. Can I get a sanity check on that? :)

[DrewAPicture]

Patch still applies.

Looking at the switch statement on $comment_type, should the comment types all be the same for the 'comment', 'pingback', 'trackback', and 'pings' types? If so, we can reduce that down to assigning $comment_type_where for all four cases at once.

[boonebgorges]

Replying to DrewAPicture:

Patch still applies.

Looking at the switch statement on $comment_type, should the comment types all be the same for the 'comment', 'pingback', 'trackback', and 'pings' types? If so, we can reduce that down to assigning $comment_type_where for all four cases at once.

Nope, the comment types should not be the same - bad copypasta on my part. Fix is coming up.

[boonebgorges]

[11334.4.diff] updates 3.diff as follows:

• fixes 'type' regression noted by DrewAPicture above
• improves function documentation
• improves cache invalidation. Looking the patch over, I realized that changing changing the status of a comment has the potential to change the pagination values for any other comment on the post. So the cache-clearing callback now loops through all comments on the post in question.

I'm leaving the patch to simmer here for a few days before I circle back around to recheck my work :)

[boonebgorges]

I'd like to go back to the drawing board with this one. get_page_of_comment() is a depressing function that duplicates a bunch of the logic of WP_Comment_Query. This is probably because WP_Comment_Query didn't used to support everything that get_page_of_comment() needed - a 'date_query', and 'fields=ids'. 11334.5.diff​ tears all the custom SQL from get_page_of_comment() and uses the comment API instead.

The problem is that WP_Comment_Query does not cache fields=ids queries. I've added support for it in 5.diff, but I don't think it's a good long-term solution. We could probably restore some sanity to the situation - and fix a number of other performance bugs (stuff like #31072) - if we did a more serious overhaul of comment query caching. More specifically, we should split the comment query like we do in WP_Query, and then cache both halves of the split: the comment IDs matching the function arguments, and the individual comment arguments.

This is too much for 4.2, but it's a worthwhile project.

[boonebgorges]

The problem is that WP_Comment_Query does not cache fields=ids queries

It does as of [34310].

[boonebgorges]

In 34535:

Use WP_Comment_Query in get_page_of_comment().

This change allows get_page_of_comment() to use WP_Comment_Query's native
caching mechanisms.

Props boonebgorges, Viper007Bond, wmertens, jeremyfelt.
Fixes #11334.

[boonebgorges]

In 34540:

Don't run get_page_of_comment() cache test on Multisite.

get_page_of_comment() uses get_option(), and WP_INSTALLING earlier in the
test suite causes get_option() to miss the cache. See #31130.

See #11334.

[boonebgorges]

In 34805:

Only count top-level comments when calculating threaded pagination.

The change in [34535] did not properly account for threading.

See #13939, #11334.