Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#12 | remove-join-667100-12.patch | 1.35 KB | Berdir |
#9 | 667100-3.patch | 3.05 KB | catch |
#3 | 667100-3.patch | 3.05 KB | catch |
#2 | 667100-2.patch | 1.79 KB | catch |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedsubscribe. seems a reasonable approach.
Comment #2
catchSo 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.
Comment #3
catchI 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).
Comment #4
Dries CreditAttribution: Dries commentedI 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.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedTempted to won't fix that. Those are straight joins that should be blazing fast.
Comment #6
jolos CreditAttribution: jolos commentedI can be wrong on this, but shouldn't
be:
Comment #8
catch@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.Comment #9
catchComment #11
dixon_Comment #12
BerdirWow, 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.
Comment #13
catchCan't get better than that.
Comment #14
xjm12: remove-join-667100-12.patch queued for re-testing.
Comment #16
Berdir12: remove-join-667100-12.patch queued for re-testing.
Comment #17
andypostTested with devel module - no more joins happens.
\Drupal\comment\CommentStorage::loadThread()
cares to query onlycomment_field_data
table