The main query in comment_load_multiple() with 50 comments takes 25 ms on my machine, this is unacceptably long, and is likely to degrade a lot with even slightly large datasets.

It currently joins on two tables - {node} and {user}, the first place to look would be removing those joins and replacing them with separate queries - it means three queries instead of one, but if they take 1ms instead of 25ms then that's fine.

CommentFileSizeAuthor
#12 remove-join-667100-12.patch1.35 KBberdir
#9 667100-3.patch3.05 KBcatch
#3 667100-3.patch3.05 KBcatch
#2 667100-2.patch1.79 KBcatch

Comments

moshe weitzman’s picture

moshe weitzman commented

subscribe. seems a reasonable approach.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs work
StatusFileSize
new 667100-2.patch1.79 KB

So today I'm getting about 4ms to query 50 comments (these are the only comments on the site). We can get the node type from node_load() - since the node is already in memory in virtually any situation comment_load() will be called. Feels a bit dirty putting this in the loader but not sure where else it can go.

Generating a bigger dataset now before working on the user table join to see if I can reproduce the bad query time a bit more reliably.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 667100-3.patch3.05 KB

I still can't reproduce the 25ms query time with 26000 comments. However here's a patch which keeps the node->type changes and adds user_comment_load() for the rest. This seems to be bringing things down from 3-4ms to 1-2ms for me (although a lot of variation). Would be good if someone else could test on a site with a lot of comments (and if we had a working upgrade path so I could test on my 350k comments site rather than running devel generate).

dries’s picture

dries commented

I agree that this patch needs more benchmarking before it can be committed. If we only gain 1-2ms on the total request (instead of 25ms), it might not be worth it.

damien tournoud’s picture

damien tournoud commented

Tempted to won't fix that. Those are straight joins that should be blazing fast.

jolos’s picture

jolos commented

I can be wrong on this, but shouldn't

foreach ($uids as $cid => $uid) {
  $comments[$cid]->name = $cid ? $user_fields[$uid]->name : $comments[$cid]->name;
  ...
}

be:

foreach ($uids as $cid => $uid) {
  $comments[$cid]->name = $uid ? $user_fields[$uid]->name : $comments[$cid]->name;
  ...
}
catch’s picture

catch
he/him
Primary language English
commented
Priority: Critical » Normal

@Damien. Yeah I wrote the original query and thought that was fine, but 25ms is a lot on < 10,000 rows (although not being able to reproduce it today means it might be a red herring). However, we have #373897: admin/content still has sortable author column resulting in SQL errors which produced a fair bit of data from large tables and was a similar query so I'd prefer to leave this open until it's verified either way.

But until someone can reproduce > 10ms query time this isn't critical.

@jolos: no this is checking for uid == 0 - in which case the comment module stores a custom username if that's given, so $uid is the correct check there. Too much sake, new patch in next post.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 667100-3.patch3.05 KB

Status: Needs review » Needs work

The last submitted patch, 667100-3.patch, failed testing.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented
Version: 7.x-dev » 8.x-dev
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: comment_load_multiple() query joins on two tables and is slow » comment_load_multiple() query joins users table and is slow
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new remove-join-667100-12.patch1.35 KB

Wow, old issue :)

This is also a problem with caching consistency, if comment caches the username itself then the user might change and comments still display the old username.

We almost got rid of this already, the only remaining part is that it queries users to get the username, but we later on load the users anyway to render them. So let's try to see what happens if we just remove it.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Can't get better than that.

xjm’s picture

xjm
she/her
Primary language English
commented

12: remove-join-667100-12.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: remove-join-667100-12.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

12: remove-join-667100-12.patch queued for re-testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Closed (cannot reproduce)

Tested with devel module - no more joins happens.
\Drupal\comment\CommentStorage::loadThread() cares to query only comment_field_data table