On a site with a large number of nodes, the query to generate the counts for each forum is quite expensive. For example from a 6.x install, devel indicates the following on /forum on my local instance of a site with X nodes:

Executed 244 queries in 1726.65 milliseconds. Queries taking longer than 1000 ms and queries executed more than once, are highlighted. Page execution time was 2009.72 ms.

1321.66
1
advanced_forum_get_forums
SELECT DISTINCT r.tid, COUNT(n.nid) AS topic_count, SUM(l.comment_count) AS comment_count FROM node n INNER JOIN node_comment_statistics l ON n.nid = l.nid INNER JOIN term_node r ON n.vid = r.vid WHERE n.status = 1 GROUP BY r.tid

That's 77% of the total time spent in the DB for one query. For reference, the EXPLAIN:

id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
1	SIMPLE	n	ref	PRIMARY,vid,node_status_type	node_status_type	4	const	55652	Using temporary; Using filesort
1	SIMPLE	l	eq_ref	PRIMARY	PRIMARY	4	rupal_blizzard.n.nid	1	
1	SIMPLE	r	ref	vid	vid	4	drupal_blizzard.n.vid	2	Using index

Adding n.type = 'forum' to the query gives us the same forum count results, but a dramatically faster query by reducing the number of rows from {node} - only 30ms on my local.

Patches forthcoming.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deviantintegral’s picture

Here they are, for all three branches. I have not tested the 6.x-2.x patch.

troky’s picture

FileSize
1.3 KB

Interesting.

7.x doesn't need array_walk(ing)... everything else looks good.

troky’s picture

Version: 7.x-2.x-dev » 6.x-2.x-dev
Status: Needs review » Patch (to be ported)

Committed to 7.x.

mcdruid’s picture

Thanks for the patches deviantintegral, and sorry it's taken me a long time to look at them in detail.

I've concentrated on the 6.x-2.x patch, and will look at the 1.x branch later; I think the code is going to be more-or-less the same.

The advanced_forum_get_forums function is "is copied from the forum module and adapted". The original code was probably a good candidate for a couple of comments to explain what's going on :)

So a couple of problems with the patch (which you did say you hadn't tested); you added the AND n.type IN condition to one of the multiple versions of $sql, but it looks like in many cases this version gets thrown away. If db_rewrite_sql doesn't change the first $sql, it gets replaced by a slightly different query. In some basic tests with permissions set a couple of different ways, and accessing the forum list as different users, I found db_rewrite_sql was not changing $sql, so your addition was always being discarded. There must be circumstances under which the query gets rewritten, but I didn't find one.

There was also a stray 'AND', and I was keen to avoid the addition of the array_walk callback if possible; if we were being really paranoid we could ask whether we can trust the $node_types enough to put them straight into a query.

So here's my rewrite of the patch - which seems to work okay. I've tested it with 0, 1, and 2 $node_types.

Would appreciate it if people could test this - particularly anyone using forum_access or other types of access control. It would also be good to hear whether it does improve performance.

mcdruid’s picture

Tweak to avoid an undefined variable - this had been removed in a tidy-up.

mcdruid’s picture

Another tweak which really should avoid the undefined variable this time.

mcdruid’s picture

Status: Patch (to be ported) » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.