Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Adding a static for the module_exists('locale') check cuts out 428 function calls from the default front page, webgrind has t() a third faster for self time and twice as fast incl.
Comments
Comment #2
catchAdditionally we shouldn't use drupal_static() for $custom_strings - adding a function call to save a function call isn't very clever. Reverted back to static, if that's not acceptable for some reason (I really hope it is), then we should remove the static altogether and just do variable_get() every time, but this makes it 5-6 times as fast.
Comment #4
catchstatic for module_exists() isn't good, so just replaced module_exists() with function_exists(), still a very good speedup.
Comment #5
catchpatch.
Comment #6
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #7
chx CreditAttribution: chx commented*UM* we have just changed this to module_exists some research here checking that issue would be good...
Comment #8
catch@chx I found http://drupal.org/node/334283#comment-1674002 - which just moves around the module_exists() rather than changing from a function_exists(). Did you mean another issue?
Comment #9
catchFound it. As mentioned on the url() issue, I'd rather we didn't introduce performance regressions into HEAD (not that drupal_function_exists() wouldn't be as slow as module_exists(), but still) due to simpletest issues.
Comment #10
catchAnd the issue #407256: Make t() more robust.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commented@catch: do NOT make micro-optimizations at the expense of functionning or testability of the code. This module_exists() is critical. I'm asking for a revert.
Comment #12
catch@Damien, I could say the same about making changes to functions in the critical path without profiling.
I also dispute that 5% of page execution time is a micro-optimization - micro-optimization is when something is only measurably faster when isolated and run tens or hundreds of thousands of times. Even with ab rather than the profiler, this comes out at 2% faster, and this is on a page without a great deal of translatable strings compared to some. Measurable with ab on the default front page with content != micro-optimization.
Front page, ten nodes, ab -c1 -n1000 - two runs each.
HEAD (as of today):
Reverting the patch:
Only a couple of percent, but there's 4-5 patches like this in the queue, and adding them up I measured 7% improvement with ab last night #513984: Find bottlenecks in HEAD - meta issue.
Either way, here's a (hopefully) drupal_web_test_case() friendly version, not sure about putting reset in $options, but can't imagine anyone other than simpletest wanting to reset this, we swap the function_exists() for a static and two isset()s, if anything a teensy bit faster.
Enabled locale on my head install, ran locale and path tests and all passes.
Comment #14
catchstatic needs resetting both ways, same problem as the original patch.
Comment #16
catchComment #17
Dries CreditAttribution: Dries commentedThat is some pretty ugly code, IMO.
Comment #18
sunWhy can't we use drupal_static() here?
This review is powered by Dreditor
Comment #19
catchBecause using drupal_static() would be as slow as the module_exists().
See http://drupalbin.com/10770
Comment #20
catchPossible solution: #537724: Consider $GLOBALS['drupal_static'] instead of drupal_static().
Comment #21
catchTurns out it's only 5 times faster when you look at xdebug profiling data (because of the extra time measuring function calls). microbenchmarks confirm it's no change otherwise :(