WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 4 years ago

Last modified 4 years ago

#11334 closed enhancement (fixed)

Add caching to get_page_of_comment()

Reported by: Viper007Bond Owned by: boonebgorges
Milestone: 4.4 Priority: high
Severity: major Version: 2.9
Component: Comments Keywords: needs-patch
Focuses: performance Cc:

Description

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.

Attachments (9)

11334.patch (4.2 KB) - added by Viper007Bond 10 years ago.
11334-3.5.1.patch (4.4 KB) - added by wmertens 6 years ago.
applies to 3.5.1
11334-3.6.patch (4.0 KB) - added by wmertens 6 years ago.
Applies to 3.6
11334.diff (4.6 KB) - added by jeremyfelt 6 years ago.
11334.2.diff (6.8 KB) - added by jeremyfelt 6 years ago.
11334.3.diff (7.0 KB) - added by boonebgorges 5 years ago.
31355.diff (10.7 KB) - added by boonebgorges 5 years ago.
11334.4.diff (10.7 KB) - added by boonebgorges 5 years ago.
11334.5.diff (8.8 KB) - added by boonebgorges 4 years ago.

Download all attachments as: .zip

Change History (35)

@Viper007Bond
10 years ago

#1 @dd32
10 years ago

  • Milestone changed from 2.9 to 3.0

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

For that entire reason..

#2 @tott
10 years ago

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

#3 @nacin
10 years ago

  • Milestone changed from 3.0 to Future Release

#4 @wonderboymusic
7 years ago

  • Keywords needs-refresh added

#5 @wmertens
6 years ago

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
6 years ago

applies to 3.5.1

#6 follow-up: @wmertens
6 years ago

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.

#7 in reply to: ↑ 6 @SergeyBiryukov
6 years ago

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
6 years ago

Applies to 3.6

#8 follow-up: @wmertens
6 years ago

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.

#9 in reply to: ↑ 8 @wmertens
6 years ago

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.

#10 @SergeyBiryukov
6 years ago

  • Keywords needs-refresh removed
  • Milestone changed from Future Release to 3.7

@jeremyfelt
6 years ago

#11 @jeremyfelt
6 years ago

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
6 years ago

#12 @jeremyfelt
6 years ago

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

#13 @nacin
6 years ago

  • Milestone changed from 3.7 to Future Release

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.

This ticket was mentioned in IRC in #wordpress-dev by JPry. View the logs.


5 years ago

#15 @SergeyBiryukov
5 years ago

  • Keywords 4.0-early added

#16 @boonebgorges
5 years ago

In 31289:

Add basic unit tests for get_page_of_comment().

See #11334.
Props jeremyfelt.

@boonebgorges
5 years ago

#17 @boonebgorges
5 years ago

  • Focuses performance added
  • Keywords 4.0-early needs-testing removed
  • Milestone changed from Future Release to 4.2

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? :)

#18 follow-up: @DrewAPicture
5 years ago

  • Keywords dev-feedback added

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.

#19 in reply to: ↑ 18 @boonebgorges
5 years ago

  • Keywords dev-feedback removed

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
5 years ago

@boonebgorges
5 years ago

#20 @boonebgorges
5 years ago

[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
4 years ago

#21 @boonebgorges
4 years ago

  • Milestone changed from 4.2 to Future Release

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.

#22 @boonebgorges
4 years ago

  • Keywords needs-patch added; has-patch removed

#23 @boonebgorges
4 years ago

  • Milestone changed from Future Release to 4.4
  • Owner set to boonebgorges
  • Status changed from new to assigned

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

It does as of [34310].

#24 @boonebgorges
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#25 @boonebgorges
4 years ago

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.

#26 @boonebgorges
4 years ago

In 34805:

Only count top-level comments when calculating threaded pagination.

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

See #13939, #11334.

Note: See TracTickets for help on using tickets.