[drupal-devel] [feature] Block query consolidation
I1uv4t4r
drupal-devel at drupal.org
Wed Sep 14 21:23:22 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: I1uv4t4r
Status: patch (code needs work)
Well, the patch (block.module_8.patch) works for me! Ready to be
committed I'd say.
I1uv4t4r
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.
------------------------------------------------------------------------
Tue, 13 Sep 2005 16:00:19 +0000 : mathias
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.
------------------------------------------------------------------------
Tue, 13 Sep 2005 16:18:07 +0000 : Allie Micka
For what it's worth, the benchmarks I submitted did not include the
key/index patch, they only included the query consolidation patch. The
performance impacts of the patch that hijacked this issue are unknown.
I'd suggest moving this issue back to http://drupal.org/node/27157 ,
where you can assess performance impacts and the back out or correct
that patch based on the results. Alternatively, continue using this
issue but supply a few benchmarks to justify adding and/or removing the
block table indexes.
Thanks!
------------------------------------------------------------------------
Tue, 13 Sep 2005 16:56:32 +0000 : m3avrck
Just chatted with mathias on IRC.
I think there was some slight confusion and I apologize for this. The
other issue is related to this issue indeed as it affects the block
query and that is why Dries above mentioned to incorporate that fix
into this one. Then both of these were rolled into the latest HEAD.
However, a small oversight on my part, the key wasn't unique in all
situations, and this above patch rectifies this situation.
Now, in terms of performance, the key doesn't affect performance in any
measurable way. Rather, it adds data integrity and structure to the
table, and enhances other *unknown and future* queries. I chatted with
a knowledgeable fellow on #mysql a while about this table and it was
determined that a key *was* beneficial, even if any performance gains
were not.
In hindsight, maybe these patches shouldn't have been put together, but
as such, they were, and this above patches fixes this issue and brings
us back up to speed. So basically this patch "consolidates block
queries *and* adds data integrity to the block table". :)
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:19:08 +0000 : I1uv4t4r
Attachment: http://drupal.org/files/issues/block.module_6.patch (966 bytes)
First I want to say that I am astonished that those patches have made it
into CVS. Both the database and block.module patches don't work for me
and cause significant errors. Firstly, a primary key of module, delta
and theme makes MySQL throw up a 'key too long' error. Secondly, in
block.module an array is fetched from the database and then it is used
as an object, and the blocks are added to the region the function is
called for. You'll understand when you see my patch.
I must say that my patch is not tested thouroughly, but I believe it
impossible that the patches above are even tested at all, but tell me
if I'm wrong. The patch I provide is solely for block.module by the
way.
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:59:01 +0000 : m3avrck
I1uv4t4r, I welcome your comments. However, this patch was thorougly
tested on this end. PHP5.0.5 with MySQL 4.1.11 yields *no* errors at
all. Everything works as expected.
The reason for the first hiccup was that I only had 1 theme enabled,
once I tried 2 themes, I ran into the duplicate key error, which was
easily fixed by modifying the key. Running that fine on my machine
here.
Can you be more specific as to the problems you encountered and which
version fo PHP / MySQL you are using?
------------------------------------------------------------------------
Tue, 13 Sep 2005 20:29:17 +0000 : Allie Micka
No, I1uv4t4r is right.
It would have been helpful if he provided more information about his
platform (PHP, MySQL versions, etc.) and less editorializing, but what
he says is true and the error is mine.
I'm a bit confused by Drupal's inconsistent use of db_fetch_array()
versus db_fetch_object(). It seems that the latter approach is more
common, so I do it that way myself. But that's no excuse for my
missing use of arrays in in block.module! My patch didn't break
anything on my 4.3.11 system but it might be a problem for an earlier
PHP version.
I have tested I1uv4t4r's patch and it doesn't change any behavior on my
local system, so I'd consider it "ready to commit".
Again, there are two patch threads moving along in this issue. I
believe that there are still one or two unresolved issues pertaining to
the key/index patch. Please commit block.module_6.patch so we can
narrow things down again.
Meanwhile, I1uv4t4r - please tell us which version of MySQL (and PHP)
you are working with. It may help us troublshoot your other problems.
Thanks, and sorry for all the drama!
------------------------------------------------------------------------
Wed, 14 Sep 2005 08:37:21 +0000 : Dries
This is somewhat hairy:
+ $blocks[$block['region']]["$block[module]_$block[delta]"] =
(object) $block;
Can't that be done in a cleaner fashion?
------------------------------------------------------------------------
Wed, 14 Sep 2005 12:44:36 +0000 : I1uv4t4r
I am using PHP 4.4.0 and MySQL 4.1.14.
The main problem was that all blocks were put in the region for which
the function is called first during a page request, in my case the left
region. That has been fixed by my patch.
The other problem was that the database server didn't really like a
primary key of the three columns /module, delta and theme/: #1071 -
Specified key was too long; max key length is 1000 bytes. I don't know
how to fix this, the column lenghts do not add up to 1000, so I don't
know why MySQL spits out this error.
I agree that that line of code is kinda 'hairy' as you call it, you
could consider assigning the value of $block['region'] to a variable
because it is used more than once.
I meant no offence to anybody, by the way, everybody can make mistakes.
I just found it weird that it made it into CVS.
------------------------------------------------------------------------
Wed, 14 Sep 2005 13:02:14 +0000 : I1uv4t4r
Attachment: http://drupal.org/files/issues/block.module_7.patch (968 bytes)
Here's an updated patch which decreases the 'hairyness'.
------------------------------------------------------------------------
Wed, 14 Sep 2005 14:22:34 +0000 : Allie Micka
Attachment: http://drupal.org/files/issues/block.module_8.patch (3.29 KB)
Actually, the root of all that hair is in the db_fetch_array() call.
This function is expected to return an array of objects, so why not
select it as such and keep the type consistent? I have even heard
stories about this inconsistency causing new Drupal developers to
introduce bugs with their patches ;)
There are only two other references to db_fetch_array() in block.module
- one for an $edit array internal to its function, and the other for
boxes. So this patch replaces the call and uses attributes
consistently with all block-returning code.
I have tested this patch with page specific settings, user preference
settings and by moving blocks to different regions and with different
weights. I have NOT tested this with throttling. I have also not
tested this on I1uv4t4r's machine, which seems necessary somehow.
Following this issue, we still have
- address m3avrck's multiple themes patch and I1uv4t4r's key length
concerns
- revise phpdoc for block_list() to update/remove todo item for the
database key
------------------------------------------------------------------------
Wed, 14 Sep 2005 14:36:58 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/database_5.patch (1.99 KB)
I looked into this issue and found this,
http://bugs.mysql.com/bug.php?id=4541 . Turns out to be a bug in MySQL
when dealing with MyISAM tables and UTF-8.
Also:
"
If you must use 4.1 (and your PHP installation is properly configured
for
it!) then make sure the database is created with latin-1 as the default
character set, not UTF-8. MySQL 4.1's character set support will
otherwise do strange things, including complaining about keys being too
long.
"
Right now because of this, it is in Drupal's best interest *not* put
any keys in the block table. Don't want this issue creeping up
elsewhere and the performance/data integrity benefits of this key
aren't worth the headache for other people. The attached patch fixes
this. Please roll this patch into the patch to fix the queries and we
can do a clean fix to HEAD.
More information about the drupal-devel
mailing list