[drupal-devel] [feature] Block query consolidation
mathias
drupal-devel at drupal.org
Tue Sep 13 16:00:23 UTC 2005
Issue status update for
http://drupal.org/node/30801
Post a follow up:
http://drupal.org/project/comments/add/30801
Project: Drupal
Version: cvs
Component: block.module
Category: feature requests
Priority: critical
Assigned to: Anonymous
Reported by: Allie Micka
Updated by: mathias
Status: patch (ready to be committed)
I can recreate this bug when running update.php but I question the
necessity of this index in the first place. I couldn't find one SELECT
query in block.module that benefit from it.
mathias
Previous comments:
------------------------------------------------------------------------
Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka
Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB)
The blocks module is running a separate query for each region, e.g.:
SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND
region='left' ORDER BY weight, module
SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND
region='right' ORDER BY weight, module
SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND
region='header' ORDER BY weight, module
SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND
region='footer' ORDER BY weight, module
Ultimately, we're going to need all of these so let's save a few
queries. I have changed the query to:
SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER
BY region, weight, module
And set all results to the static $blocks array on the first pass.
------------------------------------------------------------------------
Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB)
Awesome patch! Noticed the version was set wrong so changed this to CVS.
Also, rerolled the patch, there was an extraneous $region in there no
longer needed. Reducing 4 queries is definetly a great start in
optimizing Drupal :)
Take this patch along with this one (might need to be updated now with
this query rewrite I'm thinking): http://drupal.org/node/27157 ... and
we'll have Drupal's loading of blocks pretty much maxed out with
optimizations, then on to the slooow alias ones... ;)
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22
Gotta love when something is just logical. Makes sense, and the patch
worked. I'd say this is ready. Get these blocks sped up a bit. This
coupled with 'block primary key' patch should make the blocks solid.
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:36:00 +0000 : Dries
Has this been benchmarked? I would expect this to be faster, but it's
good practice nonetheless. Feel free to throw that other block query
patch into the mix.
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck
Dries, I ran some simple benchmarks with the devel.module. Queries with
a default install were reduced from 125 to 121. Query times averaged
around 50ms and this was reduced to roughly 47-48ms. It's not a *huge*
savings, but on a a dev machine, with only one user, this is still good
nonetheless. I imagine on a hard hit Drupal site this would start to
scale up nicely with time/load savings. I'll reroll a patch that
combines the two in a few.
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:26:08 +0000 : Dries
This patch reduces the SQL overhead but adds some CPU-overhead and
memory-overhead. Hence, it's better to benchmark the number of
requests/second (for example using ab or ab2) ...
------------------------------------------------------------------------
Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB)
Ok here is an updated patch, along with the blocks table modification of
adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing
best practice for this table and adding a primary key is the best bet.
Any indexes would not improve performance since this table will
probably almost never have more than 30 rows in it. The primary key
itself is slightly neglible as well, however, if any query were to
perform advanced queries on this table, the primary key would benefit,
so in best practice, we should have this as well.
I'll try to get some benchmarks up soon backing these simple changes.
------------------------------------------------------------------------
Mon, 12 Sep 2005 17:49:34 +0000 : Allie Micka
This patch will not increase memory overhead unless a theme has one or
more regions that are defined but not in use. Otherwise, the static
$blocks array will be identical with or without the patch applied.
The patch also does not increase CPU overhead because less code is
processed. Without the patch, the entire query and parsing routine is
executed 5 or more times (everything between if
(!isset($blocks[$region])) { } ). With the patch, the same code
executes only once.
But the proof is in the pudding. I'm not sure what your standards are
for ab, but I like to do 1 serial check and 1 check with concurrency so
I can rule out performance gained by increased memory consumption, which
bogs down concurrent requests.
This patch appears to save an average of 9-12ms per request and
increase performance by .05 requests per second.
The relevant bits from my ab results are as follows:
Before Patching
ab -n 100 -c 1 http://localhost/
Requests per second: 2.32 [#/sec] (mean)
Time per request: 430.20 [ms] (mean)
ab -n 100 -c 10 http://localhost/
Requests per second: 2.16 [#/sec] (mean)
Time per request: 4631.00 [ms] (mean)
Time per request: 463.10 [ms] (mean, across all concurrent
requests)
After Patching
ab -n 100 -c 1 http://localhost/
Requests per second: 2.37 [#/sec] (mean)
Time per request: 421.08 [ms] (mean)
ab -n 100 -c 10 http://localhost/
Requests per second: 2.22 [#/sec] (mean)
Time per request: 4510.90 [ms] (mean)
Time per request: 451.09 [ms] (mean, across all concurrent
requests)
------------------------------------------------------------------------
Mon, 12 Sep 2005 18:04:27 +0000 : m3avrck
Exactly running similar to what I was uncovering as well. So seems this
patch is ready to go, time to commit :)
------------------------------------------------------------------------
Mon, 12 Sep 2005 18:27:32 +0000 : Dries
Thanks for the measurements! Patch committed to HEAD. Thanks.
------------------------------------------------------------------------
Tue, 13 Sep 2005 07:37:26 +0000 : asimmonds
With this patch committed, and updating my HEAD install with update.php,
I get the error
"Duplicate entry 'user-0' for key 1" from mysql.
I have two themes configured and there is a primary key (module,delta)
clash between the two themes.
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:39:43 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/database_3.patch (1.96 KB)
Eeek!!! Forgot to test with multiple themes, this patches fixes the key
definition.
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:41:46 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/database_4.patch (1.96 KB)
Noticed a typo in the PGSQL.
More information about the drupal-devel
mailing list