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.

CommentFileSizeAuthor
#14 t.patch2.95 KBcatch
#12 t.patch1.85 KBcatch
#12 t.png445.1 KBcatch
#5 t.patch1.07 KBcatch
#4 t_patch.png424.57 KBcatch
#2 t.patch1.75 KBcatch
#2 t_head.png75.29 KBcatch
#2 t_patch.png459.45 KBcatch
patch.png445.37 KBcatch
head.png411.76 KBcatch
t.patch1.69 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Title: Make t() twice as fast » Speed up t() five times
Status: Needs work » Needs review
FileSize
459.45 KB
75.29 KB
1.75 KB

Additionally 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
424.57 KB

static for module_exists() isn't good, so just replaced module_exists() with function_exists(), still a very good speedup.

catch’s picture

FileSize
1.07 KB

patch.

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

chx’s picture

Status: Fixed » Active

*UM* we have just changed this to module_exists some research here checking that issue would be good...

catch’s picture

@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?

catch’s picture

Status: Active » Fixed

Found 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.

1.639        (goba     04-May-07):   // Translate with locale module if enabled.
1.896        (dries    09-May-09):   // We don't use drupal_function_exists() here, because it breaks the testing
1.896        (dries    09-May-09):   // framework if the locale module is enabled in the parent site (we cannot
1.896        (dries    09-May-09):   // unload functions in PHP).
1.896        (dries    09-May-09):   elseif (module_exists('locale') && $langcode != 'en') {
1.641        (goba     15-May-07):     $string = locale($string, $langcode);
1.377        (dries    11-Aug-04):   }
catch’s picture

Damien Tournoud’s picture

Category: task » bug
Priority: Normal » Critical
Status: Fixed » Active

@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.

catch’s picture

Status: Active » Needs review
FileSize
445.1 KB
1.85 KB

@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):

Time taken for tests:   140.144 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      18581000 bytes
HTML transferred:       18116000 bytes
Requests per second:    7.14 [#/sec] (mean)
Time per request:       140.144 [ms] (mean)
Time per request:       140.144 [ms] (mean, across all concurrent requests)
Transfer rate:          129.48 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   129  140  13.3    136     235
Waiting:      124  135  12.9    130     228
Total:        129  140  13.3    136     235

Time taken for tests:   140.134 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      18581000 bytes
HTML transferred:       18116000 bytes
Requests per second:    7.14 [#/sec] (mean)
Time per request:       140.134 [ms] (mean)
Time per request:       140.134 [ms] (mean, across all concurrent requests)
Transfer rate:          129.49 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   131  140   7.6    138     197
Waiting:      125  135   7.3    132     190
Total:        131  140   7.6    138     197

Reverting the patch:

Time taken for tests:   142.920 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      18581000 bytes
HTML transferred:       18116000 bytes
Requests per second:    7.00 [#/sec] (mean)
Time per request:       142.920 [ms] (mean)
Time per request:       142.920 [ms] (mean, across all concurrent requests)
Transfer rate:          126.96 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   132  143  11.6    140     331
Waiting:      127  137  11.3    135     318
Total:        132  143  11.6    140     331


Time taken for tests:   143.168 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      18581000 bytes
HTML transferred:       18116000 bytes
Requests per second:    6.98 [#/sec] (mean)
Time per request:       143.168 [ms] (mean)
Time per request:       143.168 [ms] (mean, across all concurrent requests)
Transfer rate:          126.74 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   132  143  10.9    140     228
Waiting:      127  138  10.6    134     219
Total:        132  143  10.9    140     228

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.

Time taken for tests:   139.227 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      18581000 bytes
HTML transferred:       18116000 bytes
Requests per second:    7.18 [#/sec] (mean)
Time per request:       139.227 [ms] (mean)
Time per request:       139.227 [ms] (mean, across all concurrent requests)
Transfer rate:          130.33 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   129  139  11.5    135     215
Waiting:      124  134  11.3    130     209
Total:        129  139  11.5    135     215

Time taken for tests:   139.680 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      18581000 bytes
HTML transferred:       18116000 bytes
Requests per second:    7.16 [#/sec] (mean)
Time per request:       139.680 [ms] (mean)
Time per request:       139.680 [ms] (mean, across all concurrent requests)
Transfer rate:          129.91 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   130  140   9.6    137     222
Waiting:      117  134   9.3    132     213
Total:        130  140   9.6    137     222

Enabled locale on my head install, ran locale and path tests and all passes.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

static needs resetting both ways, same problem as the original patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
Dries’s picture

That is some pretty ugly code, IMO.

sun’s picture

+  static $locale;
-    t($name, array(), array('langcode' => $langcode));
+    t($name, array(), array('langcode' => $langcode, 'reset' => TRUE));
+    t('', array(), array('reset' => TRUE));
+      // Reset the static variable for module_exists('locale') in t().
+      t('', array(), array('reset' => TRUE));

Why can't we use drupal_static() here?

This review is powered by Dreditor

catch’s picture

Because using drupal_static() would be as slow as the module_exists().

See http://drupalbin.com/10770

catch’s picture

catch’s picture

Title: Speed up t() five times » Speed up t() five times when xdebug profiling is enabled.
Priority: Critical » Minor
Status: Needs review » Closed (won't fix)

Turns 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 :(