[drupal-devel] [feature] Cache rendered comment threads

[chx]

Issue status update for http://drupal.org/node/25563

 Project:      Drupal
 Version:      cvs
 Component:    comment.module
 Category:     feature requests
 Priority:     normal
 Assigned to:  chx
 Reported by:  chx
 Updated by:   chx
 Status:       patch
 Attachment:   http://drupal.org/files/issues/comment_cache_1.patch (3.82 KB)

Ah, yes, it's true. So we do not need to lose dynamic filters, very
good.




chx



Previous comments:
------------------------------------------------------------------------

June 22, 2005 - 23:45 : chx

First problem to tackle: comments may have dynamic filters. No more.
Comments are not nodes because they should be simple and fast and
dynamic filters are killing this.


So now we have static content built every two minutes on drupal.org,
which is IMO bad. I do CACHE_PERMANENT and take care of killing the
thread only when the particular thread is invalidated.


If you find the patch worthy, I will extend it so that there is no
multiple cache ie. the comments themselves are not cached when called
from comment thread view.


Maybe a similar patch could be introduced for cacheable nodes?




------------------------------------------------------------------------

June 22, 2005 - 23:53 : chx

Attachment: http://drupal.org/files/issues/comment_cache.patch (5.55 KB)




------------------------------------------------------------------------

June 23, 2005 - 00:19 : killes at www.drop.org

looks good, I suggest giving it a test run here on drupal.org and see
how/if the load changes.




------------------------------------------------------------------------

June 23, 2005 - 03:59 : moshe weitzman

just to clarify, this patch caches the output of comment_render() as a
large HTML hunk. That saves  a lot of computation when we show a node.
Further, the cache stays valid for a long time (until another comment
is posted to that thread - quite unlike cache_clear_all()).


The downside is that we can't have php evaluated in comments or other
dynamic filters. At first blush, I am OK with that loss since the
performance gain should be significant.




------------------------------------------------------------------------

June 23, 2005 - 04:29 : clydefrog

The performance gain should indeed be large, but there may be use cases
where PHP in comments is required. Perhaps a config option (on/off)
would be useful in this case.


Another idea is having filters label themselves as "dynamic" or
"static", so caching can be disabled when a dynamic filter is used on
one of the comments. But that's outside the scope of this patch.




------------------------------------------------------------------------

June 23, 2005 - 07:02 : chx

Attachment: http://drupal.org/files/issues/comment_cache_0.patch (5.6 KB)

I think we only cache for anon. users.


"Another idea is having filters label themselves as "dynamic" or
"static", so caching can be disabled when a dynamic filter is used on
one of the comments. "
 the filters are labeled but the problem is that if you look up the
cacheabiility every comment when comment_render is called, then you
won't gain much from caching...




------------------------------------------------------------------------

June 23, 2005 - 07:26 : clydefrog

It doesn't matter that the cache is only valid for anon users: I was
suggesting an admin option, so the admin makes the tradeoff choice
between performance and dynamic filters.


"if you look up the cacheabiility every comment when comment_render is
called, then you won't gain much from caching...

"
I think you only need to look up the cacheability at render time -
don't set the cache if it's not cacheable. So if the cache exists, it's
usable. And cacheability of an individual comment can be set at comment
post time, so I don't see why looking it up when rendering would have a
significant performance impact.