In block_list() function, there are as many module_implements('node_grants') and module_invoke('throttle', 'status') as there are blocks, which is useless because the foreach only needs two boolean values to work flawlessly.

I did a simple performance patch that gains (on my box, with few, not reliable testing) between 5 and 10 ms per page request, using something like 30 site wide enabled blocks.

Hope you'd like it, it's a few millisecs, but isn't nothing.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, block.module-6.19-block_list_performance_fix.patch, failed testing.

pounard’s picture

Ok failed testing because I did not use the cvs diff command, still, the patch is good (working on my env and on two production sites actually).

pounard’s picture

New patch done with CVS.

jide’s picture

Status: Needs work » Needs review

Subscribing, and changing status for bot's pleasure.

Status: Needs review » Needs work

The last submitted patch, block.module-6.19-block_list_performance_fix.patch, failed testing.

Damien Tournoud’s picture

Version: 6.19 » 7.x-dev
Status: Needs work » Patch (to be ported)

This needs to be fixed in D7 first.

The function to patch is http://api.drupal.org/api/function/_block_render_blocks/7

From a code style perspective:

+  if ($cache_enabled === NULL) {
+    $grant_modules = module_implements('node_grants');
+    $cache_enabled = empty($grant_modules);
+    unset($grant_modules);
+    $do_throttle = module_invoke('throttle', 'status') > 0;
+  }
  • === NULL should be isset()
  • I see no reason not to use !count(module_implements('node_grants')) instead of the empty construct (array sizes are cached)
pounard’s picture

You are right about isset(), it works with booleans.

For the rest, this does not needs to fixed in D7 first, since it don't fix anything, it's a pure performance path over a frozen API method, which does not break any existing behavior.

This has no API changes, avoids (3 * block count) function calls, uses booleans instead of complexe operations, improves readability, and on my box, with a bit of testing, really gain a few millisecs over global execution time, so I don't really understand why you are arguing all this.

From a coder perspective, empty() is more likely what the original algorithm meant, and also is better than using count() since we don't really need the item count, avoid a function call and use a language keyword instead, which is logically faster.

jide’s picture

@pounard : Damien is right, all bugs and performance issues should be fixed in HEAD (so that's 7.x-dev) first and then backported.

jide’s picture

Issue tags: +Performance

Tagging.

pounard’s picture

@Damien
See the simple test PHP test I just did.

With those results on my environment (in CLI context, without Drupal around, and with APC, which means probably a lot faster code).

pounard@guinevere ~ $ php -f testArrayCountIssetPerf.php 
With a function call, with count().
Took 0.60606 ms
Without function call, with count().
Took 0.1571178 ms
With function call, with empty().
Took 0.4820824 ms
Without function call, with empty().
Took 0.0729561 ms
With boolean.
Took 0.0629425 ms
pounard@guinevere ~ $
pounard’s picture

Title: Performance patch for block_list() function » Performance patch for block_list() (D6) and _block_render_blocks() (D7) functions
Status: Patch (to be ported) » Needs review
FileSize
1.98 KB

Patch for D7, doing exactly the same thing. It seems that throttle is not used at all for blocks (in my cvs copy, throttle module directory is empty, is it normal?).

pounard’s picture

Re done the D6 patch, hoping that this one will be OK for the testing framework. Did some minor modification in order to get to a consensus, and moved some bits of inline documentation in order for whole code to be more readable.

Status: Needs review » Needs work

The last submitted patch, block.module-6.19-block_list_performance_fix.patch, failed testing.

pounard’s picture

Status: Needs work » Needs review
FileSize
2.01 KB

Patch for D7, with drupal_static() usage.

moshe weitzman’s picture

We do not need to cache a super fast php function that gets called only a dozen times per page. module_implements() is cached all over the place already. For me, this Won't fix. Feel free to add drupal benchmarks which confirm your intuition.

pounard’s picture

<flame>Rasmus would say : </flame>

IMHO any millisec gain is important, and good practice would tell me to avoid useless function calls. Plus this patch does not complexify the algorithm so this is pure free optimisation.

EDIT: Hidden flame war.
Re-EDIT: This is humour, I do not intend to hurt anyone, please tell my wife and children I probably won't get to diner today.

dalin’s picture

I agree with pounard on this. I think the patch both simplifies the code, and offers a small performance gain. But I'm unclear why !count() is preferable to empty(). That line will only be called once per page anyway so not a big deal either way.

moshe weitzman’s picture

Status: Needs review » Closed (won't fix)

It isn't free. Adding cache adds new complications around when to clear the cache and adds confusion for developers when they make a change and it has no effect.

pounard’s picture

I disagree with #18, this function fetches a "static" result from database, which is not altered that often. The static cache is kept only the client hit page rendering time. On most Drupal installation, this happens only once, at theme_page() call time.

I really hope no developers would ever alter this table content every client hit, it would be a great WTF.

Plus, it seems that the full Drupal core is doing it this way, this is why core has now the drupal_static() function in D7 btw.

EDIT: The only thing I would worry about is memory consumption.

Re-EDIT: If you disagree with D7 change, I would agree with you, but the patch for D6 still gives some improvements without breaking anything.

moshe weitzman’s picture

Drupal is more than a web app serving many page requests. We have drush applications and daemons (see David Strauss presentation at Drupalcon). Excessive caching hurts these sorts of applications.

There is some debate about the usefulness of drupal_static(). Talk there - #629452: Roll back drupal_static()

pounard’s picture

drupal_static() is an anti-pattern, on that it seems that we agree :)

But still, web application or not, regions are fixed, so are blocks, this kind of static cache does not hurt, this patch does not hurt anything.

pounard’s picture

Version: 7.x-dev » 8.x-dev
Status: Closed (won't fix) » Needs review
FileSize
1.81 KB

Redid the patch for 8.x

Notice that I removed totally the drupal_static() usage here because this function gets called at late page build, once no cache seems to be candidate for change anymore.

pounard’s picture

This is for code cleaning purposes, not fixiing any bug. I really don't like to see a function call in a loop, that returns the exact same result each loop: I think this kind of minor details is one of the first you learn in computer science courses.

msonnabaum’s picture

Here's a new version of the patch that moves ALL of the conditions that will be the same on each iteration outside of the loop. This actually makes the code clearer and reduces function calls, so I think it's a win regardless of the real world performance benefit.

I did not include the static cache however, as that does add code complexity. This check will still happen once per region, which is quite a bit better than once per block.

Status: Needs review » Needs work

The last submitted patch, block.module-8-dev-minor_performance_fix-24.patch, failed testing.

neclimdul’s picture

+++ b/core/modules/block/block.moduleundefined
@@ -354,18 +364,7 @@ function _block_get_renderable_region($list = array()) {
-    if (
-      $GLOBALS['user']->uid == 1 ||
-      count(module_implements('node_grants')) ||
-      !in_array($_SERVER['REQUEST_METHOD'], array('GET', 'HEAD')) ||
-      in_array($block->cache, array(DRUPAL_NO_CACHE, DRUPAL_CACHE_CUSTOM))
-    ) {
+    if ($cacheable && in_array($block->cache, array(DRUPAL_NO_CACHE, DRUPAL_CACHE_CUSTOM))) {

This was $cacheable || in_array() before the patch and $cacheable && in_array() after the patch. I assume that's why the tests failed. Seems an easy win though.

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Oops, good call.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

Easy performance win and code cleanup.

pounard’s picture

Yay \o/

catch’s picture

Status: Reviewed & tested by the community » Needs work
+    if ($cacheable || in_array($block->cache, array(DRUPAL_NO_CACHE, DRUPAL_CACHE_CUSTOM))) {

Please rename $cacheable to $not_cacheable (either that or beat me over the head for reading this incorrectly, but I don't think so).

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Good call.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

No we should have inverse'd all the boolean logic so we could still call it cacheable. :)

Green so moving back to RTBC.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

Committed/pushed to 8.x. this looks backportable to 7.x.

msonnabaum’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.64 KB

Here's a D7 version.

Dave Reid’s picture

Status: Needs review » Needs work

The backported logic looks incorrect. Patch in #31 is designed to specifically exclude user 1 from caching, but the patch in #34 will cause the $cacheable to be TRUE if user 1 is logged in, the opposite of what the D8 patch is doing. If we inverse it shouldn't it be !$GLOBALS['user']->uid == 1?

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
1.62 KB

Actually, the check for $GLOBALS['user']->uid == 1 is from D8 and doesn't belong here. All this patch is doing is moving the logic outside the loop.

msonnabaum’s picture

Accidentally removed a space.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Okay, tasted it, tested it, smelled it. The code is exactly the same, except that the things that are per-request (rather than per-block) are moved up out of the foreach.

I don't think this needs a test, as there's not a functional bug, and it's not like we're going to sit around counting calls to module_implements().

RTBC. :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay, performance!

Committed and pushed to 7.x. Thanks!

pounard’s picture

Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Performance, -Needs backport to D7

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