Forums performance improvements

Would adding the extra column and doing the JOIN on forum rather than term_node work? Seems like a better place for it to live if so.

they're essentially the same I think:

term_node: tid nid
forum: tid nid vid 

... but forum only deals with forum nodes and terms - which on my site means 8,500 rows instead of 25,000. Even on sites with different ratios of forum nodes and terms it's never going to be more.

Had a quick look at _forum_topics_unread.

Moved the join from term_node to forum, and removed the AND n.type = 'forum' because it's not necessary with the forum table I think.

devel module says:

before
138.46	1	_forum_topics_unread	SELECT COUNT(n.nid) FROM node n INNER JOIN term_node tn ON n.nid = tn.nid AND tn.tid = 732 LEFT JOIN history h ON n.nid = h.nid AND h.uid = 11 WHERE n.status = 1 AND n.type = 'forum' AND n.created > 1177262528 AND h.nid IS NULL
30.54	1	_forum_topics_unread	SELECT COUNT(n.nid) FROM node n INNER JOIN term_node tn ON n.nid = tn.nid AND tn.tid = 791 LEFT JOIN history h ON n.nid = h.nid AND h.uid = 11 WHERE n.status = 1 AND n.type = 'forum' AND n.created > 1177262528 AND h.nid IS NULL
23.48	1   _forum_topics_unread	SELECT COUNT(n.nid) FROM node n INNER JOIN term_node tn ON n.nid = tn.nid AND tn.tid = 748 LEFT JOIN history h ON n.nid = h.nid AND h.uid = 11 WHERE n.status = 1 AND n.type = 'forum' AND n.created > 1177262528 AND h.nid IS NULL

after:
61.87	1	_forum_topics_unread	SELECT COUNT(*) FROM node n INNER JOIN forum f ON n.nid = f.nid AND f.tid = 725 LEFT JOIN history h ON n.nid = h.nid AND h.uid = 11 WHERE n.status = 1 AND n.created > 1177263702 AND h.nid IS NULL
17.71	1	_forum_topics_unread	SELECT COUNT(*) FROM node n INNER JOIN forum f ON n.nid = f.nid AND f.tid = 732 LEFT JOIN history h ON n.nid = h.nid AND h.uid = 11 WHERE n.status = 1 AND n.created > 1177264013 AND h.nid IS NULL

Don't think I believe these results, but it's hopefully tidier sql, and I don't see any reason for the joins on term_node when forum works just as well with a smaller table. Would an index on nid, tid help? at the moment there's one for each, but not for both.

/me waits to be shot down for stupid oversight

...now I see it. I reckon adding the index to forum would still cut that WHERE clause, plus I don't see a good reason to add last_comment_timestamp and updated to term_node if it's only needed for forum nodes. It means any performance hit on adding is restricted only to forums, but with all the read benefits. Unless similar JOINS are happening elsewhere that could use those fields, but I can't think of anything obvious.

This is a patch.

in that case it'd need

ALTER TABLE {forum} ADD INDEX forum_somethingdescriptive (nid,tid)

somewhere, but I'm not sure exactly where that goes yet.

this goes into the forum.install file

read therefore about the new database schema api http://drupal.org/node/146843
http://drupal.org/node/146865

Thanks Tobias Maier.

Here's a new patch, quick summary:

in _forum_topics_unread it replaces the join on {term_node} with a join on {forum}, and adds an index to forum on tid,nid

This allows us to elimate type = 'forum' in _forum_get_topics_unread (because all nodes in the forum table should be in the forum anyway). It also paves the way for denormalisation into {forum} instead of {term_node} for the query David Strauss outlines above, which would lead to less of a performance hit on inserts - since it'd be restricted to nodes that appear in that table instead of every node.

As a side effect, it also allows for a future decoupling of forum.module from the forum node type, since this query would now work with other node types.

Taking a quick look, {term_node} is also used in:
_forum_new
theme_forum_topic_navigation (shudders)
forum_get_forums
forum_get_topics
and could be easily switched in all cases. I'll not do that yet in case I'm going in the wrong direction.

There's also a join on node_comment_statistics that requires a conditional on type = 'forum' in forum_block - which could be simplified by putting the latest_changed_timestamp into {forum} as well.

This one should fix the code style issue.

hello catch,

thanks for your patch.
You added the schema update, but you need also an update function for the .install file

thanks

tobi

thanks for the review.

This one hopefully with proper update function in .install

No idea how to work out which update number it is for forum, so stuck it at 1.

thanks for your fast response.

I have some more remarks for you:

you can read here about the update naming convention
http://drupal.org/node/114774#update-n

you did the update function the old way.
see system_update_6019() at system.install
http://cvs.drupal.org/viewcvs/drupal/drupal/modules/system/system.instal...
there you can see how this is gets done using schema api

Thanks again Tobias.

OK I think I've got it this time. Last patch of the evening though, it's bed time!

forum_update_6001() is missing a brace and doesn't return $ret.

Thanks. Even my cut and pasting is sloppy :(

Here it is again.

OK this patch replaces all references to {term_node} with {forum}, and removes the redundant n.type = 'forum' checks from those queries.

I also managed to get rid of a join in _forum_get_topic_order($sortby); - it was using both {forum} and {term_node} in the same query, for no good reason as far as I can see.

It doesn't yet remove the references to n.type = 'forum' in forum_block because there's no reference to term_node, but that's not really the point of this issue. However, I'm waiting on David Strauss' denormalisation in to {forum} for that - since I reckon querying on {forum} instead of {node_comment_statistics} when it's got the 'last updated' column would have the same effect. Assuming the eventual result of this patch and related ones might be to remove node_comment_statistics altogether, this may well be necessary later, and would also be consistent with the rest of the module.

denormalisation in to {forum}

normally means denormalisation into {forum} right? ;)

Sorry, caught the wrong end of that question, and apologies for mild sarkiness!

Yes I meant timestamps as in your first post:

In the case of forum_get_topics ... term_node ... latest_change_timestamp

But as you know, doing that on {forum} instead so that can happen on a smaller result set, and avoid any unnecessary additional inserts on non-forum node types.

just seen this, will need to be re-rolled taking into account: http://drupal.org/node/144295

Add tag.

possibly being fixed at #401328: _forum_topics_unread: SQL performance problem can kill the database

Ran a couple of quick explains on forum_get_forums() queries while looking at #512864: "comment count" query in getForumStatistics() in \Drupal\forum\ForumManager is not limited to forum terms.

The first one isn't so bad:

mysql> EXPLAIN SELECT f.tid AS tid, COUNT(n.nid) AS topic_count, SUM(ncs.comment_count) AS comment_count FROM node n INNER JOIN node_comment_statistics ncs ON n.nid = ncs.nid INNER JOIN forum f ON n.vid = f.vid WHERE (status = 1) AND (f.tid IN (1, 2, 3)) GROUP BY tid;
+----+-------------+-------+--------+------------------------------+---------+---------+----------+------+--------------------------+
| id | select_type | table | type   | possible_keys                | key     | key_len | ref      | rows | Extra                    |
+----+-------------+-------+--------+------------------------------+---------+---------+----------+------+--------------------------+
|  1 | SIMPLE      | f     | index  | PRIMARY,tid                  | tid     | 4       | NULL     |    2 | Using where; Using index | 
|  1 | SIMPLE      | n     | eq_ref | PRIMARY,vid,node_status_type | vid     | 4       | d7.f.vid |    1 | Using where              | 
|  1 | SIMPLE      | ncs   | eq_ref | PRIMARY                      | PRIMARY | 4       | d7.n.nid |    1 |                          | 
+----+-------------+-------+--------+------------------------------+---------+---------+----------+------+--------------------------+
3 rows in set (0.00 sec)

However that's run once for the whole listing.

This next one is run per topic and does all kinds of nasty:

mysql> EXPLAIN SELECT ncs.last_comment_timestamp AS last_comment_timestamp, ncs.last_comment_uid AS last_comment_uid, IF (ncs.last_comment_uid != 0, u2.name, ncs.last_comment_name) AS last_comment_name FROM node n INNER JOIN users u1 ON n.uid = u1.uid INNER JOIN forum f ON n.vid = f.vid AND f.tid = 2 INNER JOIN node_comment_statistics ncs ON n.nid = ncs.nid INNER JOIN users u2 ON ncs.last_comment_uid = u2.uid WHERE (n.status = 1) ORDER BY last_comment_timestamp DESC LIMIT 1 OFFSET 0;
+----+-------------+-------+--------+----------------------------------+---------+---------+-------------------------+------+----------------------------------------------+
| id | select_type | table | type   | possible_keys                    | key     | key_len | ref                     | rows | Extra                                        |
+----+-------------+-------+--------+----------------------------------+---------+---------+-------------------------+------+----------------------------------------------+
|  1 | SIMPLE      | f     | ref    | PRIMARY,tid                      | tid     | 4       | const                   |    1 | Using index; Using temporary; Using filesort | 
|  1 | SIMPLE      | n     | eq_ref | PRIMARY,vid,node_status_type,uid | vid     | 4       | d7.f.vid                |    1 | Using where                                  | 
|  1 | SIMPLE      | u1    | eq_ref | PRIMARY                          | PRIMARY | 4       | d7.n.uid                |    1 | Using where; Using index                     | 
|  1 | SIMPLE      | ncs   | eq_ref | PRIMARY,last_comment_uid         | PRIMARY | 4       | d7.n.nid                |    1 |                                              | 
|  1 | SIMPLE      | u2    | eq_ref | PRIMARY                          | PRIMARY | 4       | d7.ncs.last_comment_uid |    1 | Using where                                  | 
+----+-------------+-------+--------+----------------------------------+---------+---------+-------------------------+------+----------------------------------------------+

edit: quick note that these are probably using 1 row because I didn't run these with a proper forum dataset, rather than because they always use one row.

Using index; Using temporary; Using filesort is solely caused by ORDER BY last_comment_timestamp DESC

However, not sure how you managed to get only 1 row in that first reference. On a moderately large forum, I get 11576 rows for a single forum/term here, which ultimately results in the bad performance.

I've already tried a couple of workarounds (adding indexes here and there, storing a comma-separated list of tids in {ncs}, ...) with no luck.

We need to solve the ORDER BY challenge, somehow.

On a related note, we are needlessly joining two times on {users} via $query->join('users', 'u1', 'n.uid = u1.uid');, or resp.

INNER JOIN users u1 ON n.uid = u1.uid

However, that has a marginal effect only.

Why not just add a changed column to forum this bring ability to "up" some posts without commenting and improve performance

EDIT related #877020: _forum_topics_unread() broken

I'm having heavy performance problems with the forums' listings on 6.19 that seem to be exactly like the addressed in this issue. In the same time their are plenty of "slow-forum&node_access"-related issues opened on drupal.org and it is not clear which one is the solution if any:)

Can anybody tell if the patch #26 is ready and worth applying on D6 to solve the problem at least partially?
Was it ever committed to 6.x-dev?

I think we fixed the topic listing in D7 but not the forums listing. Can anyone confirm?

Actually, I am still seeing problems in both. Mainly forum_forum_load() and _forum_topics_unread().

Subscribe

Haven't looked into this properly (which is sad since it's 11 months after I posted that comment..), but I'm sure that

EXPLAIN SELECT ncs.last_comment_timestamp AS last_comment_timestamp, ncs.last_comment_uid AS last_comment_uid, IF (ncs.last_comment_uid != 0, u2.name, ncs.last_comment_name) AS last_comment_name FROM node n INNER JOIN users u1 ON n.uid = u1.uid INNER JOIN forum f ON n.vid = f.vid AND f.tid = 2 INNER JOIN node_comment_statistics ncs ON n.nid = ncs.nid INNER JOIN users u2 ON ncs.last_comment_uid = u2.uid WHERE (n.status = 1) ORDER BY last_comment_timestamp DESC LIMIT 1 OFFSET 0;

Could make use of the new {forum_index} table.

{forum_index} does have the 'last_comment_timestamp' field but I think we'd need to put the 'cid' field in there to be able to get the uid of the last commenter.

There's a patch at #1008410: Forum list header sorting broken that seriously refactors the query in question, and if it doesn't fix the performance issue, ought to give us a great idea what's missing. I haven't had time to properly review that patch though.

Now that #100840 is in, this issue gets clearer.

There are two queries that can benefit from the new index table, both in forum_forum_load():

1) topic count per forum
current:

SELECT f.tid AS tid, COUNT(n.nid) AS topic_count, SUM(ncs.comment_count) AS comment_count FROM node n
INNER JOIN node_comment_statistics ncs ON n.nid = ncs.nid
INNER JOIN forum f ON n.vid = f.vid WHERE (status = 1) GROUP BY tid

can change to:

SELECT f.tid AS tid, COUNT(f.nid) AS topic_count, SUM(f.comment_count) AS comment_count FROM node n
INNER JOIN forum_index f ON n.nid = f.nid WHERE (status = 1) GROUP BY tid

Explain of latter:

id	select_type  table  type	possible_keys	key		key_len	ref	rows	Extra
1	SIMPLE	n	ref	PRIMARY,node_status_type node_status_type 4	const	47133	Using index; Using temporary; Using filesort
1	SIMPLE	f	ref	forum_topics		forum_topics	4	n.nid	1	

2) 'last comment' query
as mentioned by catch/#38 -- and meatsack/#39 is right that a 'cid' field needs to be added to the table in this case.

current:

SELECT ncs.last_comment_timestamp AS last_comment_timestamp, ncs.last_comment_uid AS last_comment_uid, n.nid AS nid, n.title AS title, n.type AS type, CASE ncs.last_comment_uid WHEN 0 THEN ncs.last_comment_name ELSE u2.name END AS last_comment_name
FROM node n INNER JOIN forum f ON n.vid = f.vid AND f.tid = :tid
INNER JOIN node_comment_statistics ncs ON n.nid = ncs.nid 
INNER JOIN users u2 ON ncs.last_comment_uid = u2.uid 
WHERE (n.status = 1) ORDER BY last_comment_timestamp DESC LIMIT 1 OFFSET 0

can change to:

SELECT f.last_comment_timestamp AS last_comment_timestamp, c.uid AS last_comment_uid, n.nid AS nid, n.title AS title, n.type AS type, COALESCE(c.name, u2.name) AS last_comment_name
FROM node n INNER JOIN forum_index f ON n.nid = f.nid 
INNER JOIN comment c ON f.cid = c.cid 
INNER JOIN users u2 ON c.uid = u2.uid 
WHERE (f.tid = :tid) AND (n.status = 1) ORDER BY last_comment_timestamp DESC LIMIT 1 OFFSET 0

explain of latter:

id	select_type  table  type	possible_keys	key	key_len	ref	rows	Extra
1	SIMPLE	f	ref	forum_topics,tid	tid	4	const	1736	
1	SIMPLE	u2	index	PRIMARY			name	182	NULL	2	Using index; Using join buffer
1	SIMPLE	c	eq_ref	PRIMARY,comment_uid	PRIMARY	4	f.cid	1	Using where
1	SIMPLE	n	eq_ref	PRIMARY,node_status_type PRIMARY 4	f.nid	1	Using where

results / stuff for review

I just migrated 90000 topics into a new site. With the database populated, Above queries took 50-75% of the original time, according to devel.module query statistics. The speed gain was obvious, but timing results varied every time I ran a test.

Note: I'm not an expert on indexes. I added an index on 'tid' because this makes the second query run faster and is also mentioned in the 'explain'.
Adding an index on 'nid' didn't seem to make a speed difference for the first query, though I had expected that.
And I have no idea why the 'forum_index' is on 'nid + tid + sticky + last_comment_timestamp'. (#100840: security against injection attacks and bulk emailing for tellafriend last changed that index; it added the nid.)

....OK, the first query change is no good. {forum_index} contains shadow copies too (i.e. several rows with different tid, for a given nid.)
These should not get counted in the per-forum comment count.

Later.

marked as duplicate of this issue #597742: Optimize forum.module query performance

the patch seems has a little problem and can be more optimized.
downgrade the version to D7 to see testbots reviews first. (because it's easier to reroll a D7 patch. sorry for any inconvenience)

Great.

following explains do not reflect teh real world usages.

- patch avoid to join all nodes vids.
- comment name is empty if that is a user submitted comment, should not use COALESCE

D7 with devel generated contents

Original

Not sure how generally this issue wants to tackle forums performance improvements, should this be a new issue? On D7, when the forum module is enabled, deleting comments in bulk (as in a migration rollback) suffers tremendously from forum_comment_delete's call to _forum_update_forum_index() - even when there are no forum topic nodes (let alone comments on them). The heavy hitter is:

    $last_reply = db_query_range('SELECT cid, name, created, uid FROM {comment} WHERE nid = :nid AND status = :status ORDER BY cid DESC', 0, 1, array(
      ':nid' => $nid,
      ':status' => COMMENT_PUBLISHED,
    ))->fetchObject();

This is to setup a db_update on forum_index, which will do nothing anyway if the parent node wasn't a forum topic. So, we want to avoid this query unless we know the node is already in forum_index - probably just do an inner join to forum_index on the initial comment query in the function. Let me try that... Yes, in a very rough informal test the average time of forum_comment_delete() goes from 54ms to 1ms (this is with 73k comments in the comment table).

Let me know if I should add the optimization to this patch, or open a separate issue.

Thanks.

@mikeryan,

this thread mainly focus on forum topic display. please open a new issue for the deletion tweak, thanks :)

OK, see #1715402: Poor performance of _forum_update_forum_index() when passed a non-forum node.

Thanks.

Re-rolled patch in #44 against latest 7.x.

Fix integrity constraint violation in #51.

This is pretty similar to my quick solution for Drupal.org at #1543530-11: Drupal.org D7 performance testing. We'll be doing some profiling for D7, but I think the explains speak for themselves.

Attached is a revision of #53, which I will be applying for Drupal.org's codebase, with:

• I removed n.status = 1 conditions from both queries, forum_index only has published nodes already.
• I removed a join on users (u1), as suggested by sun in #32.
• I added ->orderBy('NULL'), we don't care about the ordering that groupBy implies, removing a filesort.

Explains:

(It would be ideal to remove the useless joins on node when node access modules are not used.)

tags

Looks nice

Needs forward porting to 8.x. And at least explain before/after, maybe profiling if the committers require it.

I don't have exact numbers handy, but #54 indeed helped Drupal.org a lot.

affects D.o tag is enough.

54: 145353.diff queued for re-testing.

The logic from #54 works great and this is a very very huge problem for larger forums. So what's the plan to bring this forward? Will this be backported to D7 anyway?

The patch in #54 is for D7, so it needs to be updated for D8.

And needs to be applied on D7 :)

#54, ported to D8, modified for D8 table structure, and more.

I'll first await testbot results and if it's actually green, post separate interdiffs with various explanations (so that someone can profile/'explain' separate parts if they want.)

This patch has one small change (I forgot the 'private' directive).

Separate interdiffs. It would be good if someone with an active forum on D8 can do some measurements of the differences between baseline, 1, 2 and 3 (note 3 fires less queries). My own quick tests with only one forum with generated comments, looking at the devel webprofiler, showed;

• visible improvement on part 1
• visible improvement on part 2 for the 'node/comment' count query; barely/not for the last-post query.
• no improvement on part 3 because I made only one forum.

1): the ported patch

There's a difference with D7 / #41 / #54:

• The last-post query joins node -> forum_index -> comment -> users in D7
• and node node -> forum_index -> comment_entity_statistics -> users in D8. Also, we do not add the extra forum_index.cid field to forum_index

...because the comment table in D8 contains no uid. (That's been moved into comment_field_data, which we don't want to use because it can have multiple rows per nid, prompting unresolved questions about display language of comments which are beyond the scope of this issue.)

To be honest I don't know anymore, why we don't keep joining against comment_entity_statistics in the D7 patch and why I added forum_index.cid in #41. Maybe just because #40 told me to. But a D7 forum can probably re-profile this after the D8 patch is committed; I don't have a real-world D7 forum available anymore.

2): removing node

drumm said in #54 "It would be ideal to remove the useless joins on node when node access modules are not used.". So I did, in a separate interdiff so that it can be profiled.

This has unfortunately made me call 'global' functions from within the ForumManager service, so I separated that out into a commented private method and will await critique.

3): reducing last-post queries for multiple forums

It seems strange to me that we (also on D7) run one query to get all the node/comment counts for all forums, and then run individual queries to get the last-post data for each forum. It doesn't seem necessary (anymore?)

So I changed the code so that only one query for last-post data will be made on a forum list page. It's still inconsistent with the node/comment counts however, because it only queries data for all forums where we actually need the data. I'm not sure what the time difference would be time-wise if we queried data for all forums to make the code easier, and afraid to make that change.

Attached is a new version of #54 that removes a couple more joins on the node table.

Could use testing with a node access module enabled, it may need conditional joins added on the node table, when necessary, like the D8 patch has. The D8 patch is looking good in general.

The last patch was for D7 is this needed for D10?

@smustgrave thanks, the patch in #75 was for Drupal 8 already, so I guess yes. But I also guess it needs a reroll.

I'd suggest creating the 10.x as MR, so we can keep the Drupal 7 patch separate (while I think it should better be a separate issue, if that's ever to be committed?

Fixed phpcs error.

Also not sure if the change for forum.services.yml was intended?

>Also not sure if the change for forum.services.yml was intended

@smustgrave : The change was intentional, as the user object is being used in new method introduced. Constructor also changed.

if service getting new argument it should be optional and required in 11.0 with BC-shim to prevent site failures on upgrade

Forum is approved for removal. See #1898812: [policy] Deprecate forum module for removal in Drupal 11

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

It will be moved to the contributed extension once the Drupal 11 branch is open.