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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
553 bytes

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.

catch’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.37 KB

This should fix the test fails.

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.82 KB

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.42 KB

Ah, another reset is necessary.

olli’s picture

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.

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.21 KB

Re-roll, updated comments.

xjm’s picture

Issue tags: +needs profiling
Berdir’s picture

Issue tags: -Performance, -needs profiling

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

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.2 KB

Re-roll.

tim.plunkett’s picture

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

Berdir’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.37 KB

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

catch’s picture

Status: Needs review » Fixed

Alright. Committed/pushed to 8.x!

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