Performance: cache function_exists() output in module_hook()

It's best to include, or at least summarize, benchmarks in the issue entry for both convenience and posterity rather than needlessly making everyone visit your blog. The benchmarks shows a performance improvement of about 2%.

How much does the performance improvement depend on the number of modules you have enabled (and how many functions they define)? You could even use PHP's get_defined_functions() to see how the performance gain varies with the exact number of defined functions for a few test setups (e.g. small, medium, and large number of modules enabled).

Also, your patch is difficult to apply in its current form. See http://drupal.org/patch for info on how to create a basic cvs patch. The IF statement needs a space in it too (see the coding standards).

That last } has a trailing white space, need to remove that.

Also, IMO all functions that implement static caching need to implement a $reset parameter to reset the static cache and Doxygen. For example, we may programmatically disable a module then run some other code that thinks this hook still exists (omitting a module_exists() call). We would have to add code to module_disable to reset this static cache. See http://drupal.org/node/106015 for a bit more explanation.

Good point, a reset parameter should be added.

i have no idea how php internals work, but can't see a benefit here. php knows in its symbol table about all functions that exist. you can't delete a function once it has been loaded, can you? that synbol table has got to be as fast as our static array. i see that benchmarks prove otherwise, but i'd like to sanity check.

This is really really low hanging fruit. The benchmark numbers show a delta of only 300ms, but yet the author claims linear correlation with number of modules loaded but offers no proof. IMHO, bogus so far.

Byte

Very interesting patch. Clearly the benchmarks are showing improvement which is great.

Moshe, I believe the speedup can be attested to the fact that isset() is extremely fast. This is apparent when compared to function_exists(). That would explain the speedup, and prove the sanity check :-)

Building upon this patch, we should repeat this logic in the theme layer, for example:

    if (function_exists($theme_engine .'_init')) {
      call_user_func($theme_engine .'_init', $themes[$theme]);
    }

function theme_get_function($function) {
  global $theme, $theme_engine;

  // Because theme() is called a lot, calling init_theme() only to have it
  // smartly return is a noticeable performance hit. Don't do it.
  if (!isset($theme)) {
    init_theme();
  }

  if (($theme != '') && function_exists($theme .'_'. $function)) {
    // call theme function
    return $theme .'_'. $function;
  }
  elseif (($theme != '') && isset($theme_engine) && function_exists($theme_engine .'_'. $function)) {
    // call engine function
    return $theme_engine .'_'. $function;
  }
  elseif (function_exists('theme_'. $function)){
    // call Drupal function
    return 'theme_'. $function;
  }
  return FALSE;
}

If we apply this same logic in the theme layer, we could *significantly* improve the performance of Drupal. Without a doubt, the theme area is the slowest area in Drupal. This trick that Danny points out, reused in the theme layer could show some significant performance boost.

Any updates? This patch seems pretty solid and worthwhile...

I can't reproduce any performance increase at all here:

ab -c1 -n500 on the homepage, default devel content generated and some blocks enabled.

First test:

Without patch 16.75 #/sec:
Mean 59, SD 3.4

With patch 16.63 #/sec:
Mean 59, SD 3.3

Second test:

Without patch 16.86 #/sec:
Mean 58, SD 3.4

With patch 16.64 #/sec:
Mean 59, SD 3.2

Bumping version

For impatient: patch seems non-relevant for current states of PHP/Drupal
Detailed version:

I have rerolled suggested patch for D8 and tested if it still relevant.
Server environment:
php 5.3.8 with suhosin patch (Debian testing), Apache 2.2.21
Drupal 8: 8.x-dev snapshot d78f6a6 - Patch #61456 by jeffschuler, edmund.kwok, jdefay, David_Rothstein, csevb10, alex_b, xjm, Steve Dondley: Aggregator titles display quotes and other characters with HTML entity equivalents badly (write tests).

Apache bench: ab 2.3 (bundled in Debian)

Tests were performed for the following configurations:
apc on, drupal cache off, CSS/JS aggregation off
apc on, drupal cache on, CSS/JS aggregation on
apc off, drupal cache off, CSS/JS aggregation off
apc off, drupal cache on, CSS/JS aggregation on

with drupal cache off, ab was run with -c1 -n500 parameters
with drupal cache on, ab was run with -c10 -n5000 parameters

after each configuration change, drupal cache was cleared (via admin/config/development/performance), and 5 identical runs of ab were performed.
First run was ignored to compensate cache filling effect, remaining 4 series combined to produce average result.

Finally, test results (less is better):

As you see, results differ within standard deviance.
My guess is that later versions of PHP made internal symbol table more effective.
I suggest to mark this "won't fix", please advise

The real reason is that the implementation cache kicks in: only direct hook invocations go through module_hook() in Drupal 7+.