#8071 closed task (blessed) (fixed)
Refrain from querying all the comments on a post when paged
Reported by: | markjaquith | Owned by: | boonebgorges |
---|---|---|---|
Milestone: | 4.4 | Priority: | normal |
Severity: | normal | Version: | 2.7 |
Component: | Comments | Keywords: | dev-feedback has-patch |
Focuses: | Cc: |
Description
If a post has more than (comment_per_page) comments, we should query them specifically, not query all the comments and sort in PHP (doesn't scale well).
Logic:
if threading is off, use a simple limit query
elseif the page has equal or fewer comments than (comments_per_page), query them all
else query (comments_per_page) parent comments, and keep doing queries for their children, up to the threading limit (10).
Attachments (8)
Change History (78)
#3
@
16 years ago
- Milestone 2.8 deleted
- Resolution set to wontfix
- Status changed from new to closed
Closing as wontfix. As much as I agree this should really be dealt with in SQL, I dug into this one myself recently while redoing my theme's comments template.
Basically, the paging is so in WP that you the walker class grabs all of the comments. You then extract the correct ones based on filters (by type, etc.) and number of comments per page. As the stuff in there can use custom callbacks, there's absolutely no way this can land in SQL.
#4
@
14 years ago
- Keywords needs-patch added
- Milestone set to Future Release
- Resolution wontfix deleted
- Status changed from closed to reopened
That's not exactly true. The separation is done in wp_list_comments() and only after that it's sent to Walker_Comments.
So, I think it can be moved into SQL, when threading is disabled.
#8
follow-up:
↓ 10
@
14 years ago
For threaded comments, something like the following might or might not be worth implementing:
- Get the list of all ids and parent ids
- Build the hierarchy tree
- Figure out which items are to be displayed now
- Get all the info for those items, in one go
- Display
#10
in reply to:
↑ 8
@
12 years ago
Replying to scribu:
For threaded comments, something like the following might or might not be worth implementing:
- Get the list of all ids and parent ids
- Build the hierarchy tree
- Figure out which items are to be displayed now
- Get all the info for those items, in one go
- Display
This sounds like a great idea.
#11
@
12 years ago
I just did this (get all comments for post but just id=>parent / cache it) for my implementation of threaded replies in bbPress - once you do that, Walker can figure out hierarchy, but you need to do a Walker before you query paged comments so you can do an IN query for the cpage instead of calling get_comment( ) for every item in the comment loop
I'll attach code if what I did is portable
Doing the walker for the page will also get you a proper pagination matrix, and allow you to display 1-{total number of comments} vs 1-{total number of top level comments per page}
#13
@
9 years ago
- Milestone changed from Future Release to 4.4
- Owner set to wonderboymusic
- Status changed from reopened to assigned
#14
@
9 years ago
- Keywords has-patch dev-feedback added; needs-patch removed
Create 2 helper functions (these can be renamed, whatever):
wp_comments_per_page()
- figures out what the default per page value iswp_comment_pages_count( $total, $top_level )
- total can be read from$post->comment_count
,$top_level
is the count of a queried array of top level comment ids for a post that runs when comments are threaded
Add 2 query vars to WP_Comment_Query
:
parent__in
- for querying for comment_IDs that belong to a set of parentsparent__not_in
for completeness
Add a new orderby
value in WP_Comment_Query
: comment__in
- when querying by comment__in
, need to return them in the same order.
In comment_template()
:
- We're going to split the query - add
'fields' => 'ids'
to$comment_args
- If comments are threaded, query for all of the top level comment ids - else, get all of the comment IDs
- Figure out how many pages of comments there are using
wp_comment_pages_count()
- Save the number of pages on
$wp_query
as->comment_pages_count
- If we determine that there is more than 1 page of comments:
- save the current page on
$wp_query
as->comment_pages_page
- slice the list of comment IDs based on
page
andper_page
- if comments are threaded, use
parent__in
inwp_threaded_comment_ids()
to query for comment children (1 query for comment IDs per depth level)
- save the current page on
- set
comment__in
to the comment IDs, orderbycomment__in
, get all of the comments in 1 query
In wp_list_comments()
:
- If no comments are passed to the function (means we're in the loop) and
$wp_query->comment_pages_page
is set, reset$r['page']
to 1, so that the Walker doesn't try to create the offset that we created by slicing the comment_IDs incomments_template()
Any Comments? (Huzzah!)
This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.
9 years ago
#18
@
9 years ago
8071.3.diff scales a post that has 100,371 approved comments / 400,000 total comments bound to the post ID.
Without the patch, the page doesn't even load, even when paginated.
tl;dr Force comments pagination on posts that have either:
- 2 times
comments_per_page
- 100 comments (50 is the default, 2 pages)
If your comments are already paginated, and you have an extraordinary # of comments, these queries now scale. If you are not paginating comments, and all of a sudden, you get 100,000 comments, and you're not using an object cache, your page will now load.
Details
- Split The Query - get IDs first, then query for all fields for all relevant comments using an
IN
query - In
WP_Comment_Query
, mimic the->found_rows
implementation ofWP_Query
by usingSQL_CALC_FOUND_ROWS
and adding a->set_found_comments()
method and->max_num_pages
prop. This allows us to paginate our queries, rather than paginating in Walker after querying every single comment. - Set
$wp_query->max_num_comments_pages
equal to the new$wp_comments->max_num_pages
- When checking for
get_option( 'page_comments' )
, also check for$post->comment_count > $paging_threshold
- this is a filterable value equal to (See Above) - When getting comment IDs for threaded comments, get them one depth level at a time, versus having Walker sort through a gigantic list that causes memory exhaustion. This can be done using the
parent__in
query var, which is now possible via [34205] - After getting all of the comment IDs via the passed query args, query all of the relevant objects at once via
IN
and then order the results viacomment__in
, ditching the extra sorting values. This is now possible via [34212].
#19
@
9 years ago
- Owner changed from wonderboymusic to boonebgorges
Boone is going to shepherd my work here while I attend to other things
#20
@
9 years ago
Current plan is to move much of the guts of 8073.3.diff to WP_Comment_Query
, where it'll benefit all those who venture into WordPress comments, not just those using comments_template()
.
#21
@
9 years ago
8071.5.diff is the first step in generalizing wonderboymusic's patches - it splits the query in WP_Comment_Query
. In the process, it normalizes the way that single comments are cached to reduce cache misses in certain cases. Posting here for comment (LOLZ) before I move on to the next stage.
This ticket was mentioned in Slack in #core by boone. View the logs.
9 years ago
#25
@
9 years ago
8071.6.diff takes the spirit of wonderboymusic's patch and moves it deeper into the API. By moving some of the threading+pagination logic into WP_Comment_Query
rather than comments_template()
, we open up the performance improvements to other developers.
Notes:
- In order to maintain some separation of powers in
WP_Comment_Query
, I had to stash the SQL clauses in an object property. That's why you see all the red/green there. - I'm defaulting
no_found_rows
totrue
. This is in keeping with current behavior.SQL_CALC_FOUND_ROWS
can be very expensive, and I don't want to wreak havoc with people usingWP_Comment_Query
for weird things. - New param
hierarchical
takes three values:false
(current and default - ignores comment children),'flat'
(returns a flat array that includes descendants),'threaded'
(returns a tree) - To support the tree display, I added some
children
infrastructure toWP_Comment
(add_child()
,get_child()
,get_children()
). None of this threaded/tree stuff is used in core - we use a walker instead - but it seemed to round out the API. - I removed the 'found_comments' filter. A filter on the query seems like it's enough. (sometimes it's faster to do
SELECT COUNT(*)
, which I assume is why you put the filter on the SQL) - With this patch, the 'page_comments' setting becomes pretty meaningless - we stomp on it whenever we pass a certain threshold. I think it's a good idea to stomp on it, but it makes the UI setting "Break comments into pages..." fairly awkward. At some point - now or in the future - we should remove the checkbox, leaving only the 'comments_per_page' setting.
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.
9 years ago
#28
@
9 years ago
Thinking more about recent patches, I'm going to remove the filter on the SELECT FOUND_ROWS()
query altogether. SELECT FOUND_ROWS()
itself is not costly - it's SQL_CALC_FOUND_ROWS
that introduces overhead. For the time being, if a dev doesn't like the way WP gets a total comment count, it can be disabled with 'no_count_rows=true'. Alternative methods for counting (SELECT COUNT(*) WHERE...
) can be achieved independently.
#33
follow-up:
↓ 49
@
9 years ago
8071.7.diff is what remains to fix this issue.
The way that comment pagination works is complicated and not very intuitive. The 0th page of comments - what you see when reading a post with no 'cpage' query var - contains either the most recent or the oldest #per_page comments, depending on your 'default_comments_page' setting. However, the behavior of non-zero 'cpage' is *not* sensitive to 'default_comments_page'; cpage=2 always contains more recent comments than cpage=3, etc. In other words, when 'default_comments_page=newest', the order of comment pages (if you have 4 pages total) is 0, 3, 2, 1, where if you show the oldest page first, the order is 0, 1, 2, 3. (The magic, such as it is, is in Walker_Comment
.)
I don't think this makes a lot of sense, but I also don't know what would break if we changed the behavior. It's very likely that someone out there is building custom pagination links, and is expecting the behavior described above.
In order to introduce pagination that respects the new pagination powers of WP_Comment_Query
- which are the key to making comment pagination scale - without breaking backward compatibility for comment pagination URLs, I've put a couple of pretty awful hacks into comment_template()
. The hacks would be slightly less ugly if I put them into the Walker or into wp_list_comments()
, but both of these could have unexpected consequences, because of the various ways they're used. By isolating everything inside of the already hideous comments_template()
, I've greatly limited the possibility of collateral damage.
Primary problem at this point: I discovered that wonderboymusic's technique for forcing 'page_comments' is incomplete. When pretty permalinks are enabled, redirect_canonical()
will check to see if get_option( 'page_comments' )
is enabled. If not, it disregards 'comment-page-x' URL chunks, so that comment pagination links get redirected back to the post itself. No query has taken place at this point, so there's no way we can force the threshold for pagination based on comment count. In 8071.7.diff, I removed the get_option( 'page_comments' )
check from redirect_canonical()
. But, as I mentioned above, this effectively neuters the 'page_comments' settings. It may be time to kill the setting.
This ticket was mentioned in Slack in #core by boone. View the logs.
9 years ago
#41
follow-up:
↓ 43
@
9 years ago
- Keywords needs-patch added; has-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
r34561 has broken the order of comments. The comment_order
option is having the opposite of its intended effect.
#42
@
9 years ago
Actually it might not have been r34561, but one of the recent changes to comment behaviour.
#43
in reply to:
↑ 41
@
9 years ago
Replying to johnbillion:
r34561 has broken the order of comments. The
comment_order
option is having the opposite of its intended effect.
Are you able to give more precise instructions on how to reproduce? There are a number of discussion settings that interact in weird (and awful) ways. What are your values for 'comment_order' and 'default_comments_page'?
#45
@
9 years ago
This is reproducible on a brand new, fresh as daisies, vanilla install. Install WordPress, then leave two comments on the default post (you'll need to moderate the first comment).
#46
@
9 years ago
Thanks for the additional details. This has to do with the legacy logic in comments_template()
that separated the logic for multiple-page-threads from single-page-threads. It's no longer relevant, so I'm going to remove it. Some unit tests and a fix coming up.
#49
in reply to:
↑ 33
@
9 years ago
Replying to boonebgorges:
The way that comment pagination works is complicated and not very intuitive. The 0th page of comments - what you see when reading a post with no 'cpage' query var - contains either the most recent or the oldest #per_page comments, depending on your 'default_comments_page' setting. However, the behavior of non-zero 'cpage' is *not* sensitive to 'default_comments_page'; cpage=2 always contains more recent comments than cpage=3, etc. In other words, when 'default_comments_page=newest', the order of comment pages (if you have 4 pages total) is 0, 3, 2, 1, where if you show the oldest page first, the order is 0, 1, 2, 3. (The magic, such as it is, is in
Walker_Comment
.)
I don't think this makes a lot of sense, but I also don't know what would break if we changed the behavior. It's very likely that someone out there is building custom pagination links, and is expecting the behavior described above.
This was my doing when I helped write comment paging back in the day and it was done this way so that comment permalinks are actually permanent.
cpage=2 always contains more recent comments than cpage=3, etc
That's backwards. Page 1 is always the oldest comments, page 2 are slightly newer comments, and so forth.
It needs to be this way otherwise as new comments were added, older comments would be bumped off of one page number and onto the next, rendering permalinks useless. What page a comment is on shouldn't ever change regardless of new comments or what page is displayed by default.
The only exception to this is if you change the number of comments per page. Only way to avoid that is to have true comment permalinks instead of just page numbers with anchor tags.
#50
@
9 years ago
That's backwards. Page 1 is always the oldest comments, page 2 are slightly newer comments, and so forth
Right - I misspoke. Sorry about that. (Staring at oldest/newest, first/last, asc/desc for long enough will get to you.)
it was done this way so that comment permalinks are actually permanent.
Ah, this makes sense. Thanks for chiming in :)
This ticket was mentioned in Slack in #core by boone. View the logs.
9 years ago
#54
@
9 years ago
A lot of sites will currently (by choice) be displaying all comments on the page with the page_comments checkbox unchecked. Some of them may even be using themes which don't support paged comments because they have no need for this.
Will this change result in those sites 'breaking' until the comments per page setting is manually changed to a huge number? If so, should the WP update automatically change this to a 'huge number' if the checkbox was unchecked to prevent this?
#55
@
9 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
@smerriman - Thanks for your comment. I don't have a lot of sympathy for themes that don't have comment pagination built in - this should be a basic feature of any them. But I am sympathetic to sites that will suddenly have the behavior of their sites changed in a radical way. And thinking it over, I see that comment permalinks will be affected fairly broadly by this change, since comment-page-x is part of comment permalinks, and forcing pagination is going to move some comments to secondary pages.
One of the pillars of this ticket is that we must force pagination at some point. Originally, wonderboymusic had suggested that we set a threshold higher than comments_per_page. In working on the patch, I decided that forced pagination made the 'page_comments' setting pointless, so I pulled it out. But in retrospect, the problems described above suggest that we should be much more delicate about forcing pagination. I think I'd like to reintroduce page_comments, and continue to respect it, until we get to a fairly high threshold - maybe 250 or 500 - at which point we'll force pagination.
This ticket was mentioned in Slack in #meta by otto42. View the logs.
9 years ago
#59
@
9 years ago
- Keywords has-patch added; needs-patch removed
In an effort to minimize comment "permalink" breakage, 8071.8.diff does the following:
- Restores 'page_comments'
- Changes the default value for 'page_comments' from 0 to 1
- Introduces a couple of new helper functions.
wp_get_top_level_comments_count( $post_id )
does what it says.wp_get_comment_pages_count( $post_id )
gets the *canonical* comment-page count for a given post. By "canonical" I mean the count that is actually used when WP how to build comment pages (incomment_template()
). This means that it respects threading as well ascomments_per_page
. It also doesn't require full comment objects. In these ways, it differs from the existingget_comment_pages_count()
. - When
page_comments
is disabled, and the comment count for a post is above the (filterable) 'comment_paging_threshold', comment pagination is forced, with 'comment_paging_threshold' used as the per-page value.
This ticket was mentioned in Slack in #core by boone. View the logs.
9 years ago
#61
@
9 years ago
After some discussion, and lots of teenage angst, I've decided to revert the 'page_comments' changes. Forcing comment pagination raises lots of UX and backward compatibility issues that need to be addressed. After the revert, I'll move the latest patches to a new ticket, for consideration in a future release.
#63
@
9 years ago
- Resolution set to fixed
- Status changed from reopened to closed
[35331] stops forcing comment pagination. While this revert is a shame :( we did actually manage to accomplish some real improvements. Comments are loaded in a much more svelte manner [34310]. And we now only load the comments required by the page, instead of loading them all and letting Walker_Comment
do the pagination work; this part of [34561] survived, and it addresses the issue that the current ticket was originally opened for.
I've opened #34389 for further discussion of the performance problems that arise from setting page_comments
to 0. I think we can mark the current ticket as fixed.
Still think this is worth testing, to see how much of a performance benefit it yields.