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: killes@www.drop.org Status: patch looks good, I suggest giving it a test run here on drupal.org and see how/if the load changes. killes@www.drop.org 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)

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: clydefrog Status: patch 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. clydefrog Previous comments: ------------------------------------------------------------------------ June 22, 2005 - 14: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 - 14:53 : chx Attachment: http://drupal.org/files/issues/comment_cache.patch (5.55 KB) ------------------------------------------------------------------------ June 22, 2005 - 15:19 : killes@www.drop.org looks good, I suggest giving it a test run here on drupal.org and see how/if the load changes. ------------------------------------------------------------------------ June 22, 2005 - 18: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.

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: clydefrog Status: patch 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. clydefrog Previous comments: ------------------------------------------------------------------------ June 22, 2005 - 14: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 - 14:53 : chx Attachment: http://drupal.org/files/issues/comment_cache.patch (5.55 KB) ------------------------------------------------------------------------ June 22, 2005 - 15:19 : killes@www.drop.org looks good, I suggest giving it a test run here on drupal.org and see how/if the load changes. ------------------------------------------------------------------------ June 22, 2005 - 18: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 22, 2005 - 19: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 22, 2005 - 22: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...

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: Dries Status: patch Please share some benchmark results. Note that on drupal.org (1) node/x pages are not in the top-20 most popular pages and (2) that they are among the fastest pages. I estimate that the performance gain is neglible on drupal.org but I'd be happy to test/evaluate it ... Dries 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@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. ------------------------------------------------------------------------ June 23, 2005 - 07:56 : chx 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.

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: stefan nagtegaal Status: patch *Dries wrote:* Note that on drupal.org (1) node/x pages are not in the top-20 most popular pages and (2) that they are among the fastest pages. I estimate that the performance gain is neglible on drupal.org but I'd be happy to test/evaluate it ... Doesn't this comment caching also effect the forums, which mostly hold a lot of the comments on a site? If so, the benefit maybe could be measured using the forums on drupal.org which are quite actve.. stefan nagtegaal 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@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. ------------------------------------------------------------------------ June 23, 2005 - 07:56 : chx 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. ------------------------------------------------------------------------ June 23, 2005 - 09:50 : Dries Please share some benchmark results. Note that on drupal.org (1) node/x pages are not in the top-20 most popular pages and (2) that they are among the fastest pages. I estimate that the performance gain is neglible on drupal.org but I'd be happy to test/evaluate it ... ------------------------------------------------------------------------ June 23, 2005 - 10:49 : Bèr Kessels In general, I think that the modules should care for clearing and setting of (parts of) the cache, for they are the only ones with the "knowledge" and "data" to do this in a smart way. A big +1 for this patch. And please no more settings. Just go for non dynamic filtering, But /if/ peopel really need it, then please consider using or expanding the existing cache-level setting in admin >> settings.