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.
Comment | File | Size | Author |
---|---|---|---|
#37 | block-perf-fix-d7-917490-37.patch | 1.62 KB | msonnabaum |
#36 | block-perf-fix-d7-917490-36.patch | 1.62 KB | msonnabaum |
#34 | block-perf-fix-d7-917490-34.patch | 1.64 KB | msonnabaum |
#31 | block.module-8-dev-minor_performance_fix-31.patch | 2.13 KB | msonnabaum |
#27 | block.module-8-dev-minor_performance_fix-27.patch | 2.13 KB | msonnabaum |
Comments
Comment #2
pounardOk 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).Comment #3
pounardNew patch done with CVS.
Comment #4
jide CreditAttribution: jide commentedSubscribing, and changing status for bot's pleasure.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis 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:
=== NULL
should be isset()!count(module_implements('node_grants'))
instead of the empty construct (array sizes are cached)Comment #7
pounardYou 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.
Comment #8
jide CreditAttribution: jide commented@pounard : Damien is right, all bugs and performance issues should be fixed in HEAD (so that's 7.x-dev) first and then backported.
Comment #9
jide CreditAttribution: jide commentedTagging.
Comment #10
pounard@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).
Comment #11
pounardPatch 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?).
Comment #12
pounardRe 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.
Comment #14
pounardPatch for D7, with drupal_static() usage.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedWe 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.
Comment #16
pounard<flame>Rasmus would say : </flame>
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.
Comment #17
dalinI 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.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedIt 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.
Comment #19
pounardI 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.
Comment #20
moshe weitzman CreditAttribution: moshe weitzman commentedDrupal 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()
Comment #21
pounarddrupal_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.
Comment #22
pounardRedid 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.
Comment #23
pounardThis 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.
Comment #24
msonnabaum CreditAttribution: msonnabaum commentedHere'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.
Comment #26
neclimdulThis 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.
Comment #27
msonnabaum CreditAttribution: msonnabaum commentedOops, good call.
Comment #28
neclimdulEasy performance win and code cleanup.
Comment #29
pounardYay \o/
Comment #30
catchPlease rename $cacheable to $not_cacheable (either that or beat me over the head for reading this incorrectly, but I don't think so).
Comment #31
msonnabaum CreditAttribution: msonnabaum commentedGood call.
Comment #32
neclimdulNo we should have inverse'd all the boolean logic so we could still call it cacheable. :)
Green so moving back to RTBC.
Comment #33
catchCommitted/pushed to 8.x. this looks backportable to 7.x.
Comment #34
msonnabaum CreditAttribution: msonnabaum commentedHere's a D7 version.
Comment #35
Dave ReidThe 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
?Comment #36
msonnabaum CreditAttribution: msonnabaum commentedActually, 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.
Comment #37
msonnabaum CreditAttribution: msonnabaum commentedAccidentally removed a space.
Comment #38
xjmOkay, 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. :)
Comment #39
webchickYay, performance!
Committed and pushed to 7.x. Thanks!
Comment #40
pounardThanks!