Cache rendered comment threads

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

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.

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.

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.

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

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.

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

Anyone willing to benchmark this?

I think that no single node/x page is popular on drupal.org but as a whole they consist of the majority of views.

patching file modules/comment.module
Hunk #1 succeeded at 270 (offset -4 lines).
Hunk #2 succeeded at 641 with fuzz 2 (offset 99 lines).
Hunk #3 succeeded at 745 with fuzz 2 (offset 97 lines).
Hunk #4 FAILED at 932.
Hunk #5 succeeded at 952 (offset 73 lines).
Hunk #6 succeeded at 977 with fuzz 2 (offset 62 lines).
Hunk #7 FAILED at 1081.
Hunk #8 FAILED at 1362.
Hunk #9 FAILED at 1698.
4 out of 9 hunks FAILED -- saving rejects to file modules/comment.module.rej

I commit to benchmarking it, if I get a new patch. :)

There still doesn't seem to be any caching of entire comment threads, so perhaps this is still needed? Not sure.

Moving to 6.x-dev queue.

patch updated, benchmark coming

Lookign at the patch in its current state I find that it isn't really usable. It caches html, which already has been generated by the theme system. Due to this it is forced to be only cached for anon users which find not as usefull for drupal.org as I want it.

The aim should be to rewrite comment_render so that it generates some array of comments which are then themed by another function. The structure generated by comment_render would then be cached.

Here is some work in progress towards that aim.

Won't make it into D6 unfortunately.

That latest patch should have set it to CNR, but anyway, after testing it needs a reroll against HEAD:

$ patch -p0 < comment_cache_3.patch
patching file modules/comment/comment.module
Hunk #1 succeeded at 918 (offset -13 lines).
Hunk #2 FAILED at 949.
Hunk #3 FAILED at 988.
2 out of 3 hunks FAILED -- saving rejects to file modules/comment/comment.module.rej

Subscribing

Unless we can do this for auth users, it doesn't give us much over the page cache IMO, so marking won't fix. We'd need a way to intelligently handle things templates formatting comment submitted as time ago via javascript to do this properly.

A site can do this now with #cache but you would have to deal with any personalization in the comment threads like links that say '2 new' and so on. you probably have to alter out the personalization. only a given site knows what modules are running and personalizing so this is not a job for core.