Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#35025 closed defect (bug) (fixed)

Performance regression in comments_template in 4.4

Reported by: rogerhub's profile rogerhub Owned by: boonebgorges's profile boonebgorges
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Comments Keywords: fixed-major
Focuses: performance Cc:

Description

I'm experiencing a significant performance regression from the 4.4 upgrade, related specifically to this code in wp-includes/comment_template.php in the comments_template() function:

<?php
$comment_query = new WP_Comment_Query( $comment_args );
$_comments = $comment_query->comments;

// Trees must be flattened before they're passed to the walker.
$comments_flat = array();
foreach ( $_comments as $_comment ) {
    $comments_flat = array_merge( $comments_flat, array( $_comment ), $_comment->get_children( array(
        'format' => 'flat',
        'status' => $comment_args['status'],
        'orderby' => $comment_args['orderby']
    ) ) );
}

The call to $_comment->get_children seems to generate a query for every top-level comment in a post. This seems to be new code introduced in 4.4. I don't have much context on the new comment caching stuff, but is there any way these queries can be avoided, especially for sites that don't even use nested comments?

For more context, I'm experiencing these issues on a page that has 7000+ comments, and for various reasons, they can't be paginated.

Attachments (2)

35025-test.diff (887 bytes) - added by boonebgorges 9 years ago.
35025-fix.diff (807 bytes) - added by rogerhub 9 years ago.
Fix for ticket #35025

Download all attachments as: .zip

Change History (12)

#1 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 4.4.1

Introduced in [34561] and [34569].

#2 @boonebgorges
9 years ago

  • Keywords reporter-feedback added

Hi @rogerhub - Thanks for the report, and welcome to Trac!

When the system works the way it was designed in 4.4, $comment->get_children() should result in no extra database queries, when called on comments that were fetched using WP_Comment_Query with 'hierarchical' => 'threaded'. Here's the rundown:

  1. comment_template() calls WP_Comment_Query with 'hierarchical' => 'threaded'. https://core.trac.wordpress.org/browser/tags/4.4/src/wp-includes/comment-template.php?marks=1288,1327#L1283
  2. WP_Comment_Query::get_comments() then calls the fill_descendants() method https://core.trac.wordpress.org/browser/tags/4.4/src/wp-includes/class-wp-comment-query.php?marks=443-445#L440
  3. fill_descendants() does only a single DB query for each hierarchy level across all located comments. In your case, this should mean just two queries: one for the top-level comments (ie, all of them), and then one for children of those comments (should return none, at which point db queries should stop)
  4. When fill_descendants() then builds the comment tree, it marks each comment with the populated_children flag. https://core.trac.wordpress.org/browser/tags/4.4/src/wp-includes/class-wp-comment-query.php?marks=925-928#L906
  5. When populated_children is set, the get_children() method of WP_Comment should *not* do a database query: https://core.trac.wordpress.org/browser/tags/4.4/src/wp-includes/class-wp-comment.php?marks=290-296#L279 See [34730].

(phew)

So, that's how it's supposed to work. (I'm going to attach a unit test that demonstrates that what I've described above is working, at least in the simple case.) If it's not working for you, it must be short-circuiting at one of these steps. With the above sketch in mind, do you think you could do some more debugging to see at which point the process is failing?

#3 @rogerhub
9 years ago

  • Keywords has-patch added; reporter-feedback removed

Hi, thanks for the quick response. I did some PHP profiling, and it looks like the performance hit I was experiencing was because of the use of the array_merge function. I looked on Google for "array_merge slow" and it seems like a lot of people say that array_merge is slow and shouldn't be used in loops. I rewrote the code with foreach/"$array[] =" and the running time of the loop I mentioned in the OP went from 10~11 seconds to 0.3 seconds. I've attached a patch that fixes the issue for me. I've never submitted a patch for WordPress before, so if there's something extra I need to do or a code style issue, please let me know.

Thanks for your time!

@rogerhub
9 years ago

Fix for ticket #35025

#5 @rogerhub
9 years ago

  • Keywords dev-feedback added

@pavelevap I don't think it's related to this particular bug, but it could be related to the new comments code in 4.4.

#6 @boonebgorges
9 years ago

@rogerhub Thanks for this - some testing suggests that your approach is correct. Glad to hear it was not more complicated than this :)

@pavelevap Thanks for the link to the issue. As @rogerhub notes, I think it's unrelated to the issue in this ticket, but it's probably related to #35068 and/or #35078, which I'll look into.

#7 @boonebgorges
9 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 35931:

Don't use array_merge() when building comment children arrays.

array_merge() is much slower than building the combined array using a
foreach loop. The performance difference was causing a speed regression with
the get_children() functionality introduced in 4.4.

Props rogerhub.
Fixes #35025.

#8 @boonebgorges
9 years ago

  • Keywords fixed-major added; has-patch dev-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.4.1.

#9 @boonebgorges
9 years ago

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

In 35932:

Don't use array_merge() when building comment children arrays.

array_merge() is much slower than building the combined array using a
foreach loop. The performance difference was causing a speed regression with
the get_children() functionality introduced in 4.4.

Merges [35931] to the 4.4 branch.

Props rogerhub.
Fixes #35025.

#10 @rogerhub
9 years ago

Wooo, thank you!

Note: See TracTickets for help on using tickets.