Make WordPress Core

Opened 9 years ago

Closed 3 years ago

Last modified 3 years ago

#36904 closed enhancement (fixed)

Add caching to comment feed in WP_Query

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.0 Priority: normal
Severity: normal Version: 2.2
Component: Query Keywords: has-patch has-unit-tests commit
Focuses: performance Cc:

Description

In the WP_Query class, the comments table is queried. When WP_Comment_Query was introduced and added caching. Similar caching should be applied to WP_Query class.

Caching could be added to the WP_Query class, or WP_Query could use the WP_Comment_Query.

Attachments (7)

36904.patch (2.6 KB) - added by spacedmonkey 9 years ago.
36904.1.patch (8.2 KB) - added by spacedmonkey 9 years ago.
36904.2.patch (8.5 KB) - added by spacedmonkey 9 years ago.
36904.3.patch (9.6 KB) - added by spacedmonkey 9 years ago.
36904.4.patch (13.7 KB) - added by boonebgorges 8 years ago.
36904.diff (12.2 KB) - added by spacedmonkey 7 years ago.
36904-different.diff (2.6 KB) - added by spacedmonkey 7 years ago.

Download all attachments as: .zip

Change History (30)

@spacedmonkey
9 years ago

#1 @spacedmonkey
9 years ago

  • Keywords has-patch needs-unit-tests added

Added first patch, which is a patch for the WP_Query class.

#2 @boonebgorges
9 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to Future Release

Thanks for the patch. A couple thoughts:

  • The 'comment' cache group is non-persistent. Adding caching as suggested in 36904.patch won't do much, unless the same WP_Query is fired more than once on a given page load. See #36906.
  • Querying for comment IDs only, and then array_map( 'get_comment', $comment_id ), is going to result in a separate database query for each located comment (when the individual comment cache is not primed). We need something like _prime_comment_caches() here. It would reduce overhead if we could switch to WP_Comment_Query instead, though I haven't looked into how feasible this is.
  • Unless I'm mistaken, $wpdb->get_results( "SELECT wp_comments.comment_ID..." ) will return an array of objects with a single property (comment_ID). I think you meant get_col().
  • The two preceding points would be evident in the presence of test coverage :-D

#3 @spacedmonkey
9 years ago

  • Keywords needs-patch removed

So I have been hacking around and I think that converting to wp_comment_query might work. The biggest issue is backwards compablity with the filters. Otherwise It works much nice with wp_comment_query. Added a logical filter for the args as well. I think this second patch is the one we should be working towards.

#4 @spacedmonkey
9 years ago

Patch 2, does a better job of passing a valid query object in filter.

#5 @boonebgorges
9 years ago

  • Keywords needs-patch added

Thanks for your work on this, @spacedmonkey. I agree that this is a good strategy.

Instead of doing is_comment_feed() checks inside of WP_Comment_Query, I'd prefer to control this using a parameter. Something like do_comment_feed_filters is ugly sounding, but since it's really a back compat argument anyway, I think it's OK. This could take the place of suppress_filters, which I don't see the need for more generally in WP_Comment_Query. So, in WP_Query:

$comment_args = array(
    // ...
    'do_comment_feed_filters' => ! $q['suppress_filters'],
    // ...
);

and then in get_comment_ids() we just need to check if ( $this->query_vars['do_comment_feed_filters'] ).

Could you please move the filter docs over to get_comment_ids()?

I'm not convinced, at a glance, that setting post__in to the IDs of $this->posts is going to do the work of the existing SQL. For one thing, I'm pretty sure $this->posts hasn't yet been set at this point. It's possible that WP_Comment_Query doesn't have enough internal logic to handle things like WHERE ( post_status = 'publish' OR ( post_status = 'inherit' && post_type = 'attachment' ) ). If not, it may be possible to add it, either as general purpose parameters, or in the form of new parameters that are very specific to the use case of comment feeds. It'd be helpful to have tests for this, since it'll demonstrate more clearly what the cases are that need to be covered; looking at the code, I have a rough sense of what they are (though the generic $where is a wildcard), but having them explicit will make it easier to see what changes are required in WP_Comment_Query to make this change possible.

#6 @spacedmonkey
9 years ago

The latest patch adds back in the documentation. The biggest changed the wp_comment_query isn't called twice now. Is now only called after post_results have been setup. This makes the code a lot easier to read and stops repeated code. As it uses post__in, post status isn't an issue, as post status isn't part comment query unless you define it.

The value of the 'comment_feed_where' will be different from before, but I am not sure of how much of an issue this is.

#7 @spacedmonkey
9 years ago

  • Keywords has-patch added; needs-patch removed

#8 @boonebgorges
9 years ago

The biggest changed the wp_comment_query isn't called twice now. Is now only called after post_results have been setup.

Thanks. This is a step in the right direction. However, it won't work for values of fields like ids or id=>parent. I'm not sure if there are other ramifications for moving the comment query, so this needs some examination. As noted above, unit tests will be critical to prevent against regressions.

The value of the 'comment_feed_where' will be different from before, but I am not sure of how much of an issue this is.

A good first step here is to search the plugin repo to see who is using this filter, and for what.

#9 @boonebgorges
8 years ago

  • Keywords needs-patch added; needs-unit-tests has-patch removed

I've spent some time with the latest patch, and it seems to do pretty well with single-post comment feeds - example.com/2017/05/foo/comments/feed/. 36904.4.patch is a slightly cleaned-up version, with an initial pass at a technique for having at least a tiny bit of test coverage.

But the patch, as written, breaks the main comment feed: example.com/comments/feed/. This feed should fetch the latest 10 comments from all published posts (10 being the default posts_per_rss):

SELECT  wp_comments.* FROM wp_comments JOIN wp_posts ON ( wp_comments.comment_post_ID = wp_posts.ID ) WHERE ( post_status = 'publish' OR ( post_status = 'inherit' AND post_type = 'attachment' ) ) AND comment_approved = '1'  ORDER BY comment_date_gmt DESC LIMIT 10

With 36904.4.patch, it fetches the last 10 comments *that belong to the last 10 posts*. Any comments from posts 11+ will not be included.

@spacedmonkey Could you take this back to the drawing board a bit? It seems that removing the // Comment feeds block and doing all processing after the main posts query is responsible for this problem. It might turn out that we'll need to continue to do something in that part of the query generation, whether it's setting some sort of flag (it's not clear to me what that'd look like, but maybe you can figure it out) or just calling WP_Comment_Query in both places.

If we get a version with everything working, it seems like a good call for 4.9.

@spacedmonkey
7 years ago

#10 @spacedmonkey
7 years ago

36904.diff fixes the above issue.

Key changes.

  • New param,do_comment_feed. When passed, it added the following
    ( post_status = 'publish' OR ( post_status = 'inherit' AND post_type = 'attachment' ) )
    

to the comment query. This brings it in line current sql. Seems the current comment query check to see if the posts / attachments are public before, using the comments.

  • Fixed formatting after code standards change.
  • no_found_rows is set to true.

The results are good.

On main comment feed, sql query is
Before

SELECT  wp_comments.* FROM wp_comments JOIN wp_posts ON ( wp_comments.comment_post_ID = wp_posts.ID ) WHERE ( post_status = 'publish' OR ( post_status = 'inherit' AND post_type = 'attachment' ) ) AND comment_approved = '1'  ORDER BY comment_date_gmt DESC LIMIT 10

After

SELECT  wp_comments.comment_ID FROM wp_comments JOIN wp_posts ON wp_posts.ID = wp_comments.comment_post_ID WHERE ( comment_approved = '1' ) AND ( post_status = 'publish' OR ( post_status = 'inherit' AND post_type = 'attachment' ) )  ORDER BY wp_comments.comment_date_gmt DESC, wp_comments.comment_ID DESC LIMIT 0,10

Single feed queries are
Before

SELECT wp_comments.* FROM wp_comments  WHERE comment_post_ID = '1' AND comment_approved = '1'  ORDER BY comment_date_gmt DESC LIMIT 10

After

SELECT  wp_comments.comment_ID FROM wp_comments  WHERE ( comment_approved = '1' ) AND comment_post_ID = 1  ORDER BY wp_comments.comment_date_gmt DESC, wp_comments.comment_ID DESC LIMIT 0,10

The two queries are basically the same, apart from ordering and this only due to comment query changing ordering.

Looking good again, I think. Over to you @boonebgorges !

#11 @spacedmonkey
7 years ago

  • Keywords has-patch added; needs-patch removed

#12 @spacedmonkey
7 years ago

  • Keywords needs-unit-tests added

In 36904-different.diff I got back to the original patch and make it work better.

I am begining to believe keeping filters and same or similar queries running is until with WP_Comment_Query is impossible.

Last edited 7 years ago by spacedmonkey (previous) (diff)

#14 @spacedmonkey
3 years ago

  • Milestone changed from Future Release to 6.0
  • Owner set to spacedmonkey
  • Status changed from new to assigned

#15 @spacedmonkey
3 years ago

I have created a PR for at #2181. I am happy with this solution in this, it just needs unit tests.

This ticket was mentioned in PR #2194 on WordPress/wordpress-develop by spacedmonkey.


3 years ago
#16

  • Keywords has-unit-tests added; needs-unit-tests removed

#19 @peterwilsoncc
3 years ago

  • Keywords commit added

PR#2194 looks good to me. I've merged in a fresh copy of trunk to get it ready for commit.

#20 @peterwilsoncc
3 years ago

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

In 53065:

Query: Cache comments feeds in WP_Query.

Cache queries to the comments table in WP_Query for various comments feeds. Only comment IDs are stored for each feeds cache to avoid doubling up caching with each individual comment's cache.

Props spacedmonkey, boonebgorges, pbearne.
Fixes #36904.

#21 @peterwilsoncc
3 years ago

In 53066:

Query: Add tests following [53065].

Props spacedmonkey, boonebgorges, pbearne.
See #36904.

peterwilsoncc commented on PR #2194:


3 years ago
#22

Committed in https://core.trac.wordpress.org/changeset/53065 and finished up in https://core.trac.wordpress.org/changeset/53066 because I forgot to run svn add.

peterwilsoncc commented on PR #2194:


3 years ago
#23

Committed in https://core.trac.wordpress.org/changeset/53065 and finished up in https://core.trac.wordpress.org/changeset/53066 because I forgot to run svn add.

Note: See TracTickets for help on using tickets.