Attached is a patch for advcache.module (5.x CVS HEAD), containing a patch for forum.module (5.1) that allows to cache the result of forum_get_forums() function which is a database consumming function.

Note that there is the same access issue as with the taxonomy_cache.patch. So be carefull.

CommentFileSizeAuthor
advcache_forum.patch4.9 KBaymerick

Comments

aymerick’s picture

aymerick commented
Status: Active » Needs review

Patch needs review :)

robertdouglass’s picture

robertdouglass commented
Status: Needs review » Needs work

@aymerick - committed, thanks! Please test installation, upgrading, and cases where you turn forum module on and off. Assign this issue to yourself and set to "fixed" when you've checked all of those aspects one more time.

robertdouglass’s picture

robertdouglass commented

ps I changed the code in advcache.install a bit from what was in the patch, so take a look there.

aymerick’s picture

aymerick commented
Status: Needs work » Fixed

Tested:

  • install of 5.x-1.3
  • install of 5.x-1.2, then update to 5.x-1.3
  • clearing of cache when
    • a comment is added / modified / deleted
    • a node is added / modified / deleted
    • a term is added / modified / deleted
    • forum vocabulary is modified
    • forum vocabulary is deleted (ouch, that's bad)
  • activation / deactivation / activation of forum.module

Done all those tests on a fresh drupal 5.1 filled thanks to devel_generate.module (patched with: http://drupal.org/node/148411).

And... no issue found :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

This seems superb!

But... I don't like what I read in the last comment.:

clearing of cache when
a comment is added / modified / deleted
a node is added / modified / deleted
a term is added / modified / deleted

Will this not adversely affect performance? If so (and I cannot think how it could not), how do we tackle this? I guess we just can't?

robertdouglass’s picture

robertdouglass commented

@Wim: only because Drupal has such nice hooks can we even consider caching strategies like the ones here. We absolutely have to clear the cache when the content changes, otherwise all sites will be broken =) The extra round of testing I requested is a tip-of-the-hat to the complexity of predicting all combinations of cache states plus enabled/disabled modules. It's all a bit complex. In the end, the cost of clearing and rebuilding the cache when new forum topics and comments are created is tiny compared to the reads that are done, so I expect this patch to make a difference on all sites, even those where the cache is cleared very very often.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I agree.

I was just thinking aloud about the edge cases.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)