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: normal +Priority: critical Assigned to: Anonymous Reported by: Allie Micka Updated by: m3avrck -Status: active +Status: patch (ready to be committed) 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. m3avrck 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.