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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

troky’s picture

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

Michelle’s picture

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

mcdruid’s picture

Thanks 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:

function advanced_forum_statistics_replies($delta = NULL, $refresh = FALSE) {

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

num_topics=150
num_comments=50
num_reps=20
x=1

while [ $x -le $num_reps ]
do
  echo "looping: $x of $num_reps"
  time drush genc --add-terms --types=forum $num_topics $num_comments
  drush sqlq "SELECT COUNT(1) AS forum_nodes FROM node WHERE type='forum'; SELECT COUNT(1) AS total_comments FROM comments;"
  x=$(( $x + 1 ))
done

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.

nodes comments before after
150 7500 4m7.254s 3m52.696s
300 15000 4m12.326s 3m53.161s
450 22500 4m26.307s 3m55.173s
600 30000 4m34.963s 3m54.974s
750 37500 4m51.205s 4m15.130s
900 45000 4m59.467s 4m0.096s
1050 52500 5m14.949s 3m56.673s
1200 60000 5m26.542s 3m54.062s
1350 67500 5m39.357s 3m57.766s
1500 75000 5m50.648s 3m59.094s
1650 82500 6m12.135s 3m55.324s
1800 90000 6m23.583s 3m55.857s
1950 97500 6m38.983s 3m57.211s
2100 105000 6m53.493s 3m59.223s
2250 112500 7m6.132s 4m1.999s
2400 120000 7m17.921s 3m59.292s
2550 127500 7m33.481s 4m3.622s
2700 135000 7m42.120s 4m3.625s
2850 142500 7m59.738s 3m59.502s
3000 150000 8m14.522s 3m59.457s

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 ;)

mcdruid’s picture

Oh, and - as discussed - I also added conditionality to the hook_comment and hook_nodeapi implementations so that AF ignores non-forum content types.

troky’s picture

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

mcdruid’s picture

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

mcdruid’s picture

Issue summary: View changes

markup tweak