I've been doing some basic profiling / performance analysis of AF - initially investigating #529388: Adding an index to comments table speeds performance of /forum a great deal.
I used devel_generate to add a fairly large amount of content to a test site (500,000+ topics with 1-50 comments on each).
The top result in the slow query log was a bit of a surprise; it turned out to be this one:
SELECT SUM(s.comment_count) FROM {node_comment_statistics} s INNER JOIN {forum} f ON (s.nid = f.nid)
...from the advanced_forum_statistics_replies function (non- nodecomment version).
Digging into this, I found a couple of potentially quite serious performance issues:
1) advanced_forum_statistics_replies(TRUE) is called to refresh the count of replies every time:
* advanced_forum_nodeapi is invoked with an op of 'update', 'insert' or 'delete' regardless of the node in question's content type.
* advanced_forum_comment is invoked with an op of 'update', 'insert' or 'delete' regardless of the node in question's content type.
...so this expensive query is potentially called unnecessarily when non-forum content is being changed.
2) the cache mechanism used in advanced_forum_statistics_replies is a variable_get / variable_set
...so every time any node or comment is inserted, updated or deleted, AF performs an expensive query, and then does a variable_set, which leads to the variable cache being cleared.
I would have thought there might be some room for improvement on both these fronts:
1) advanced_forum_statistics_replies(TRUE) calls
* be more selective about refreshing the count; only do so when forum nodes / comments are being affected.
* would it be cheaper to calculate the delta to the total_replies count which the update/insert/delete will result in, and apply this to the cached total?
* additionally, how up-to-date does the figure need to be (bearing in mind the performance cost of up-to-date accuracy on sites with a lot of nodes and comments)? Perhaps add an option to recalculate this total periodically on the cron, rather than doing it on the fly with every insert / update / delete.
2) use the actual cache API to store the total, rather than getting/setting a variable.
I note that in #801260: advanced_forum_statistics_replies performance dawehner already contributed an optimisation for the query itself - not sure if we could squeeze any more out of it?
I'll put some work in on this issue ASAP, but wanted to post my notes on it now.
Troky, I believe at least part of this issue also applies to the D7 branch.
Comment | File | Size | Author |
---|---|---|---|
#3 | advanced_forum_statistics_replies-performance_optimisation-1632260.patch | 4.7 KB | mcdruid |
Comments
Comment #1
troky CreditAttribution: troky commentedHere is what I would do (talking about 7.x which has different node/comment insert/update/delete hooks):
1. in hook_node_xxx and hook_comment_xxx check for content type: $node->type="forum" and $comment->node_type="comment_node_forum"
2. D7 doesn't use nodecomments so hook_node_xxx are not needed for calculation and we can remove them.
3. remove call to advanced_forum_statistics_replies from above hooks and change nodeapi code like this (pseudo code):
in hook_comment_insert: advanced_forum_stats_replies++
in hook_comment_update: /* nothing */
in hook_comment_delete: advanced_forum_stats_replies--
... no need to recalculate totals every time since we always deal with delta of +/-1
4. leave variable_get/set(advanced_forum_stats_replies) as is or introduce advanced_forum_statistics table
5. add advanced_forum_statistics_replies(TRUE) to hook_cron() to keep numbers in sync.
Comment #2
MichelleI haven't been around much lately, but I just wanted to say you guys rock! Performance has always been a weak area for me. I am so thankful to have you guys not only keeping this project going but continuing to make it better.
Comment #3
mcdruidThanks Michelle :)
Attached is a messy patch with my initial stab at some optimisations here. I've...
1) Altered advanced_forum_statistics_replies so that it takes two params:
...so that hook_comment (and hook_nodeapi for nodecomments) can send a delta which should be applied to the running total (+1 on op="insert" and -1 on op="delete").
2) Replaced the variable_get / variable_set with cache_get / cache_set - I'd expect this to make more of a difference with memcache or similar in place - but even with db-based cache backend, it should avoid the variable cache being cleared unnecessarily.
Regardless of which is more performant, I think this belongs in cache, as it's a calculable number which we want to avoid calculating when we don't have to, rather than a setting.
3) Implemented hook_cron to send refresh = TRUE to advanced_forum_statistics_replies, which will run the expensive COUNT query to ensure the stats are up-to-date.
My initial tests are (not very scientific but) quite encouraging. I ran some loops with devel_generate to create forum topics with a fixed number of comments (not nodecomment initially), and timed how long each chunk of generating content took. e.g. (in bash)
Early on with not many nodes / comments in the db, the difference between the existing code and my patched version wasn't significant, but as the db filled up with nodes and comments, the difference was quite dramatic.
N.B. the actual numbers are not important - this is a slow VM on my laptop - it's the trend I'm interested in.
N.B. also that I've hacked devel_generate so that it always adds exactly $max_comments to each node for consistency (it usually does a random number up to the max)
The before and after columns show the figure time gave for real time it took to generate each block of 150 nodes / 7500 comments.
So (with a little allowance for wobbles caused by the fact that this is a VM running on a desktop OS with other things going on) it looks like the existing implementation gets gradually, and quite dramatically, slower as more content is added. Whereas the tweaked version which avoids running the slow COUNT query with every new comment seems to keep on trucking at a fairly constant speed.
I plan to re-run the tests, and add a few more (e.g. make sure nodecomment still works).
I think it's looking good though - especially if your site gets 7500 new comments every few minutes from a php_cli script ;)
Comment #4
mcdruidOh, and - as discussed - I also added conditionality to the hook_comment and hook_nodeapi implementations so that AF ignores non-forum content types.
Comment #5
troky CreditAttribution: troky commentedFew things to consider:
- when node/topic is inserted reply count is not incremented since inserted node doesn't have any comments
- when node/topic is deleted drupal calls hook_comment with op=delete (or hook_comment_delete in d7) where counter updates with delta=-1
... ergo, hook_nodeapi (or hook_node_XXX in d7) should not refresh or call advanced_forum_statistics_replies() at all
- replies counter is increased on comment insert and comment publish
- replies counter is decreased on comment delete and comment unpublish
In d7 there is hook_comment_publish but unpublishing is catched via hook_comment_update where !$comment->status... I don't know how it is done in d6.
Comment #6
mcdruidThanks troky.
The hook_nodeapi implementation is only required for nodecomment (and therefore only in D6) - as you say, the replies stats don't include topics themselves.
I hadn't considered publish/unpublish for comments as well as insert/delete - will have a think about how this needs to work in D6 - with and without nodecomment.
Comment #6.0
mcdruidmarkup tweak