Comments currently don't have the entity static cache enabled, which wasn't necessary in 7.x because comments were not really loaded multiple times in 7.x before.
However, a comment_load($comment->pid) was added to template_preprocess_comment() recently. While doing some profiling for #1778178: Convert comments to the new Entity Field API, where I was displaying 300 comments, of which 290 were attached to a parent comment, I noticed that this resulted in an initial entity load multiple of all these comments and then every parent comment was re-loaded 29 times. That's... ouch.
So, we should probably just enable static caching for comments....
Comment | File | Size | Author |
---|---|---|---|
#18 | comment-load-reset-1844146-18.patch | 3.37 KB | Berdir |
#16 | comment-load-reset-1844146-16.patch | 3.2 KB | Berdir |
#12 | comment-load-reset-1844146-12.patch | 3.21 KB | Berdir |
#8 | comment-load-reset-1844146-8.patch | 3.42 KB | Berdir |
#6 | comment-load-reset-1844146-6.patch | 2.82 KB | Berdir |
Comments
Comment #1
BerdirEnabling static caching for comments is easy enough, and I think we still want to get persistent entity caching into core, where it would make sense to do this anyway.
Comment #3
catchWe should profile the memory impact of adding the static caching, 300 entities is potentially a lot although in practice entity_load() tends to be a lot less than things like field metadata so it might well be worth it.
Comment #4
BerdirThis should fix the test fails.
Comment #6
BerdirHah, would help if comment_load() would pass the reset argument forward :)
Not sure about those, actually, might make sense to get rid of those, I once started work on a entity_reset() helper function and dropping these $reset arguments...
Anyway, this should really pass now..
Comment #8
BerdirAh, another reset is necessary.
Comment #9
olli CreditAttribution: olli commentedLooks good, we need to update @param $reset in comment_load and comment_load_multiple docblocks. With $reset it is easy to reset the entire cache, compared to using entity_load_unchanged().
Tagging per #3.
Comment #10
Berdir#8: comment-load-reset-1844146-8.patch queued for re-testing.
Comment #12
BerdirRe-roll, updated comments.
Comment #13
xjmComment #14
Berdir#12: comment-load-reset-1844146-12.patch queued for re-testing.
Comment #16
BerdirRe-roll.
Comment #17
tim.plunkettRTBC if it passes, the changes look sane. One step closer to killing the comment_load wrapper
Comment #18
BerdirRe-roll, did not apply anymore.
Did a memory comparison on a custom page callback that renders 300 comments
8.x:
No parents: 23.09 MB.
Each comment has the previous comment as parent:24.28 MB
This patch:
No comment parents: 23.11 MB
Each comment has the previous comment as parent: 23.66 MB
So not much of a difference for no parents and a bit of a difference for the second case.
More interesting is the CPU difference for the parent case:
8.x: 0.63 [#/sec] (1577.902 ms)
Patch: 1.07 [#/sec] (932.077 ms)
Not surprisingly an obvious winner there :)
Comment #19
catchAlright. Committed/pushed to 8.x!