Performance regression in comments_template in 4.4

[rogerhub]

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.

[SergeyBiryukov]

Introduced in [34561] and [34569].

[boonebgorges]

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?

[rogerhub]

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]

Fix for ticket #35025

[pavelevap]

Is this related?

​https://wordpress.org/support/topic/wp-44-update-comments-with-last-page-displayed-by-default-are-not-showing

[rogerhub]

@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.

[boonebgorges]

@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.

[boonebgorges]

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.

[boonebgorges]

Reopening for 4.4.1.

[boonebgorges]

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.

[rogerhub]

Wooo, thank you!