WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#34732 closed defect (bug) (invalid)

Memory exhaustion on posts with thousands of comments

Reported by: pento Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: Comments Keywords: reporter-feedback close
Focuses: performance Cc:

Description

The recent changes to WP_Comment_Query::get_comments() passes an array of all commend IDs to _prime_comment_caches(). This loads all of those comments, and tries to cache them.

On posts with thousands of comments, this causes memory exhaustion - we're seeing this on WP.com posts with (approximately) more than 10,000 comments.

This behaviour is a regression in 4.4 - previously, there were no memory exhaustion errors on these posts.

Change History (17)

#1 @boonebgorges
6 years ago

Do you have more details about when this is happening? Is it on a normal comment page load, as when viewing a single post? Or does it occur only in the context of custom calls to get_comments() or WP_Comment_Query?

#2 @pento
6 years ago

It seems to be happening on normal comment page load. We've just hacked in a hard limit for now, but users are noticing that their massive comment threads aren't completely loading anymore. :-)

function wpcom_limit_comment_query( &$obj ) {
	if ( isset( $obj->query_vars['number'] ) && $obj->query_vars['number'] === '' )
		$obj->query_vars['number'] = '10000';
}
add_action( 'pre_get_comments', 'wpcom_limit_comment_query' );

#3 @boonebgorges
6 years ago

I'm assuming that comment pagination is enabled on these sites? Can you share posts_per_page and page_comments settings?

#4 @pento
6 years ago

Yep, comment pagination is enabled.

comments_per_page: 30
posts_per_page: 25
page_comments: 1

#5 @boonebgorges
6 years ago

For the record, the purpose of #8071 was to *prevent* this from happening.

I've just run some preliminary tests, and I've confirmed that on a vanilla installation, only the proper comments from the current comment-page are being loaded into the cache. More specifically:

  • In comments_template(), the $comment_args['number'] var is being set from get_option( 'comments_per_page' )
  • This value is then passed to WP_Comment_Query
  • which is respected in WP_Comment_Query->get_comment_ids()
  • which determines what gets passed to _prime_comment_caches()

@pento Could you be a dear and help to debug where things are going astray on wordpress.com? Some things to check:

  • The logic that comments_template() uses to determine comments_per_page has changed a bit in 4.4. Have these changes been merged properly to .com? :-D
  • Are there any callbacks (probably on 'pre_get_comments' or 'parse_comment_query') doing anything weird with the default query vars?

Without more information, it's hard to tell whether the regression is due to an oversight in core, or a plugin doing something wonky.

#6 @boonebgorges
6 years ago

Another thing to check:

  • How does the theme initialize the comments loop? What I would call the "standard" technique is: (a) call comments_template() in the single-item template(s), then (b) in comments.php,if ( have_comments() ) { wp_list_comments(); }. But themes sometimes have other ways of doing it, and it could be that these "other ways" do not properly inherit the changes in 4.4

#7 @pento
6 years ago

Ah, I see. It looks like we haven't merged some relevant bits from comments_template().

Let's try that and we'll see what happens.

#8 @pento
6 years ago

  • Keywords reporter-feedback added
  • Severity changed from blocker to normal

Merging all these bits is proving to be a bit tricky. :-)

I'm fairly certain the changes to comments_template() will fix this, so I'm removing the blocker severity. Should hopefully be able to close it with a definite answer by tomorrow.

Last edited 6 years ago by pento (previous) (diff)

#9 @boonebgorges
6 years ago

Merging all these bits is proving to be a bit tricky. :-)

From https://wordpress.org/news/2008/12/coltrane/:

We heard how tired you were of doing upgrades for yourself and your friends, so now WordPress includes a built-in upgrade that will automatically notify you of new releases, and when you’re ready it will download them, install them, and upgrade your blog with a single click.

You guys should check that out for wordpress.com!!!1!

#10 follow-up: @pento
6 years ago

Sounds pretty gimmicky to me. I wouldn't trust these WordPress developers to not break everything.

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


6 years ago

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


6 years ago

#13 in reply to: ↑ 10 @afercia
6 years ago

Replying to pento:

Sounds pretty gimmicky to me. I wouldn't trust these WordPress developers to not break everything.

+1 :D

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


6 years ago

#15 @wonderboymusic
6 years ago

  • Keywords close added
  • Milestone changed from 4.4 to Awaiting Review

@pento is fired

#16 @rachelbaker
5 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Closing this ticket. @pento Please re-open if there still is something actionable here.

This ticket was mentioned in Slack in #core-comments by rachelbaker. View the logs.


5 years ago

Note: See TracTickets for help on using tickets.