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.
Comment | File | Size | Author |
---|---|---|---|
#23 | database_5.patch | 1.99 KB | m3avrck |
#22 | block.module_8.patch | 3.29 KB | Allie Micka |
#21 | block.module_7.patch | 968 bytes | decafdennis |
#16 | block.module_6.patch | 966 bytes | decafdennis |
#12 | database_4.patch | 1.96 KB | m3avrck |
Comments
Comment #1
m3avrck CreditAttribution: m3avrck commentedAwesome 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... ;)
Comment #2
Souvent22 CreditAttribution: Souvent22 commentedGotta 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.
Comment #3
Dries CreditAttribution: Dries commentedHas 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.
Comment #4
m3avrck CreditAttribution: m3avrck commentedDries, 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.
Comment #5
Dries CreditAttribution: Dries commentedThis 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) ...
Comment #6
m3avrck CreditAttribution: m3avrck commentedOk 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.
Comment #7
Allie MickaThis 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)
Comment #8
m3avrck CreditAttribution: m3avrck commentedExactly running similar to what I was uncovering as well. So seems this patch is ready to go, time to commit :)
Comment #9
Dries CreditAttribution: Dries commentedThanks for the measurements! Patch committed to HEAD. Thanks.
Comment #10
asimmonds CreditAttribution: asimmonds commentedWith 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.
Comment #11
m3avrck CreditAttribution: m3avrck commentedEeek!!! Forgot to test with multiple themes, this patches fixes the key definition.
Comment #12
m3avrck CreditAttribution: m3avrck commentedNoticed a typo in the PGSQL.
Comment #13
matt westgate CreditAttribution: matt westgate commentedI 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.
Comment #14
Allie MickaFor 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!
Comment #15
m3avrck CreditAttribution: m3avrck commentedJust 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". :)
Comment #16
decafdennis CreditAttribution: decafdennis commentedFirst 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.
Comment #17
m3avrck CreditAttribution: m3avrck commentedI1uv4t4r, 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?
Comment #18
Allie MickaNo, 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!
Comment #19
Dries CreditAttribution: Dries commentedThis is somewhat hairy:
+ $blocks[$block['region']]["$block[module]_$block[delta]"] = (object) $block;
Can't that be done in a cleaner fashion?
Comment #20
decafdennis CreditAttribution: decafdennis commentedI 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.
Comment #21
decafdennis CreditAttribution: decafdennis commentedHere's an updated patch which decreases the 'hairyness'.
Comment #22
Allie MickaActually, 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
Comment #23
m3avrck CreditAttribution: m3avrck commentedI 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:
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.
Comment #24
decafdennis CreditAttribution: decafdennis commentedWell, the patch (block.module_8.patch) works for me! Ready to be committed I'd say.
Comment #25
m3avrck CreditAttribution: m3avrck commentedDries, please patch comment #22 and #23. This will fix the block queries and will remove the key to prevent any other users from receiving the error 'key index too long'. That is a tricky bug and that primary key() benefits don't away the bug issues.
Comment #26
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks.
Comment #27
(not verified) CreditAttribution: commentedComment #28
(not verified) CreditAttribution: commentedComment #29
Jürgen Depicker CreditAttribution: Jürgen Depicker commentedthe patch for postgre has an error:
+ PRIMARY KEY (module,detla)
should be:
+ PRIMARY KEY (module,delta)
Comment #30
decafdennis CreditAttribution: decafdennis commentedAre your looking at the latest patch (#23)? I can't find detla in there:
- PRIMARY KEY (module, detla)
(Which means this line was removed, actually.)Or is the bug in Drupal itself?
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedI think Jurgen mistakenly posted to the wrong issue. if not, please reopen