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

Files: 
CommentFileSizeAuthor
#18 comment-load-reset-1844146-18.patch3.37 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 55,642 pass(es).
[ View ]
#16 comment-load-reset-1844146-16.patch3.2 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 55,367 pass(es).
[ View ]
#12 comment-load-reset-1844146-12.patch3.21 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-load-reset-1844146-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 comment-load-reset-1844146-8.patch3.42 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-load-reset-1844146-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 comment-load-reset-1844146-6.patch2.82 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,352 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#4 comment-static-cache-1844146-4.patch2.37 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,398 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#1 comment-static-cache-1844146-1.patch553 bytesBerdir
FAILED: [[SimpleTest]]: [MySQL] 48,252 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new553 bytes
FAILED: [[SimpleTest]]: [MySQL] 48,252 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, comment-static-cache-1844146-1.patch, failed testing.

Priority:Normal» Major

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

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 49,398 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

This should fix the test fails.

Status:Needs review» Needs work

The last submitted patch, comment-static-cache-1844146-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.82 KB
FAILED: [[SimpleTest]]: [MySQL] 49,352 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Hah, 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..

Status:Needs review» Needs work

The last submitted patch, comment-load-reset-1844146-6.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-load-reset-1844146-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ah, another reset is necessary.

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

Issue tags:-Performance

#8: comment-load-reset-1844146-8.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Performance

The last submitted patch, comment-load-reset-1844146-8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-load-reset-1844146-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll, updated comments.

Issue tags:+Needs profiling

#12: comment-load-reset-1844146-12.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Performance

The last submitted patch, comment-load-reset-1844146-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.2 KB
PASSED: [[SimpleTest]]: [MySQL] 55,367 pass(es).
[ View ]

Re-roll.

Status:Needs review» Reviewed & tested by the community

RTBC if it passes, the changes look sane. One step closer to killing the comment_load wrapper

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.37 KB
PASSED: [[SimpleTest]]: [MySQL] 55,642 pass(es).
[ View ]

Re-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 :)

Status:Needs review» Fixed

Alright. Committed/pushed to 8.x!

Automatically closed -- issue fixed for 2 weeks with no activity.