call_user_func_array() is substantially slower than calling a function directly. In fact, it's probably the slowest possible way to call a function. However, Drupal uses cufa()... a lot. All hook calls and theme calls involve multiple cufa() calls. Eeek! The only reason we can't use $function() is that we don't know in advance how many arguments a function is going to take, which means we need the indirect call.

Or do we?

If we accept 2 limitations, then we can switch to $function in several places:

1) An indirectly called function (hook or theme) may have no more than 10 (or some other arbitrary number) of arguments.

2) An indirectly called function will always be called with that same number of arguments, padded with NULL, with the unneeded ones just falling on the floor. That means we cannot rely on func_num_args()/func_get_args() there, which shouldn't be a problem with any current hook/theme implementations. We don't use those functions that much anyway.

The attached patch implements the above two limitations and then converts several cufa() calls along the critical path to $function calls. In my spot testing, thing seem to still be working. Yay! :-) Although it's only a few functions that change, they're all lines that are called dozens of times or more every page load so we're actually replacing hundreds of cufa() calls at runtime.

This does, of course, need a benchmarking guru.

Files: 
CommentFileSizeAuthor
#80 interdiff.txt4.64 KBsun
#80 module.invoke.80.patch44.14 KBsun
FAILED: [[SimpleTest]]: [MySQL] 63,502 pass(es), 78 fail(s), and 9 exception(s). View
#78 module.invoke.78.patch44.1 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#76 module.invoke.76.patch44.08 KBsun
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#66 drupal8.module-invoke.66.patch4.43 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.module-invoke.66.patch. Unable to apply patch. See the log in the details link for more information. View
#60 cufa.patch53.61 KBtorotil
PASSED: [[SimpleTest]]: [MySQL] 33,967 pass(es). View
#39 test21b.php_.txt5.07 KBjrchamp
#21 cufa.patch6.3 KBcatch
Unable to apply patch cufa_0.patch View
#12 cufa-2.patch6.91 KBCrell
Failed: Failed to apply patch. View
cufa.patch6.63 KBCrell
Failed: 6913 passes, 343 fails, 537 exceptions View

Comments

kbahey’s picture

Here is the benchmark results. This is using today's HEAD, with 10 concurrent users stressing the server for 2 minutes, with page cache and block cache off.

With APC on

       Resp Time   Trans Rate   Trans     OKAY    Failed   Elap Time   Concurrent
w/out       0.42        23.66    2836     2836        0      119.87         9.97
with        0.42        23.71    2838     2838        0      119.70         9.94

With APC off

       Resp Time   Trans Rate   Trans     OKAY    Failed   Elap Time   Concurrent
w/out       0.90        11.05    1325     1325        0      119.88         9.97
with        0.90        11.03    1325     1325        0      120.12         9.95

Two most columns are the most relevant (average response time, in seconds, and transaction rate, req/second).

No discernible difference that I can measure.

Crell’s picture

Well poopy. Those extra array_pad() calls must be eating up the difference.

I'm going to leave this open in case someone else has a suggestion for how to improve it. I still think there's benefit to be gained here if we can figure out how.

dmitrig01’s picture

COuld we add another function like drupal_call_user_func_array which does the same as above, except the parameters default to null?

Crell’s picture

How would that work? The problem is that cufa takes twice as long as a function call... so if we add 1-2 function calls in order to avoid it, we've just moved around the time, not eliminated it. (I was hoping that array_pad() was faster than that.)

moshe weitzman’s picture

Um, what page(s) did you test? Some pages do many more theme calls than others. And with contrib modules, even more. Please try with a page that hat a lot of form elements. The theme developer module has a template log feature that you can enable which lists all theme() calls.

kbahey’s picture

Here is what I tested:

31 pages total were tested, in sequence, by every user (of the 10 total) and going back to the start when it is done.

One page is the front page with 30 nodes on it, many with comments, and then each of the 30 nodes is visited too.

dmitrig01’s picture

I'm talking about 1 function call

dmitrig01’s picture

Something like this. we might be able to get rid of the switch if we don't care about the param count working.

function drupal_call_user_func_array($function, &$a1 = NULL, &$a2 = NULL, &$a3 = NULL, &$a4 = NULL, &$a5 = NULL, &$a6 = NULL, &$a7 = NULL, &$a8 = NULL, &$a9 = NULL) {
  $num_args = func_num_args();
  switch ($num_args - 1) {
    case 0: $return = $function(); break;
    case 1: $return = $function($a1); break;
    case 2: $return = $function($a1, $a2); break;
    case 3: $return = $function($a1, $a2, $a3); break;
    case 4: $return = $function($a1, $a2, $a3, $a4); break;
    case 5: $return = $function($a1, $a2, $a3, $a4, $a5); break;
    case 6: $return = $function($a1, $a2, $a3, $a4, $a5, $a6); break;
    case 7: $return = $function($a1, $a2, $a3, $a4, $a5, $a6, $a7); break;
    case 8: $return = $function($a1, $a2, $a3, $a4, $a5, $a6, $a7, $a8); break;
    case 9: $return = $function($a1, $a2, $a3, $a4, $a5, $a6, $a7, $a8, $a9); break;
  }
}
Crell’s picture

Random idea I just had...

$args = /* ... */;

$args += array(NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL);
$function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5], $args[6], $args[7], $args[8], $args[9]);

Would that be faster than using array_pad()? += on indexed arrays will concatenate, which is fine here as we don't care if there are extra NULLs on the end of the $args array.

dmitrig01’s picture

Oops this removes the array part, how about something like

function drupal_call_user_func_array($function, $args) {
  switch (count($args)) {
    case 0: return $function(); break;
    case 1: return $function($args[0]); break;
    case 2: return $function($args[0], $args[1]); break;
    case 3: return $function($args[0], $args[1], $args[2]); break;
    case 4: return $function($args[0], $args[1], $args[2], $args[3]); break;
    case 5: return $function($args[0], $args[1], $args[2], $args[3], $args[4]); break;
    case 6: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5]); break;
    case 7: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5], $args[6]); break;
    case 8: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5], $args[6], $args[7]); break;
    case 9: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5], $args[6], $args[7], $args[8]); break;
  }
}
dmitrig01’s picture

@crell maybe that would work - what do you think of my approach?

Crell’s picture

FileSize
6.91 KB
Failed: Failed to apply patch. View

Here's a version using the mechanism in #9. Unfortunately I realized that we have to use array_shift() instead of unset() to ensure that the indexes are zero based, so I'm not sure I have very high hopes for it. :-(

kbahey’s picture

Can't see a difference still.

  Resp Time   Trans Rate   Trans     OKAY    Failed   Elap Time   Concurrent
Before       0.40        24.78    2972     2972        0      119.92         9.96
After         0.40        24.64    2958     2958        0      120.06         9.96

Would you like to roll a patch with dmitri's suggestion to see if that makes a difference?

moshe weitzman’s picture

kbahey - please try a complex form page. you might need to give anon some extra perms.

kbahey’s picture

I am enabling more core module (basically all, except ping, blogapi and throttle) for future tests.

Any specific form (from core) you suggest?

Crell’s picture

Something that invokes lots and lots of hooks and lots and lots of theme functions (not templates). That's where we should see the biggest difference, if there is going to be any.

catch’s picture

Permissions page seems like a good candidate, or maybe content type page with all core modules enabled since that has a few hook_form_alters in it?

moshe weitzman’s picture

Yes, permissions page. You will have fun giving anon users the right to administer users.

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

if you guys can make this happen then make sure that the upgrade docs have this documented throughly. 'Cos currently you can use something like module_invoke('taxonomy', 'get_tree') to call across modules but after this change if the called function have non-NULL default arguments, things will break. This is, however, quite acceptable as we now have modules that depend on other modules and also, you can instead just run drupal_function_exists('taxonomy_get_tree') before calling.

catch’s picture

Status: Needs work » Needs review
Issue tags: +Performance
FileSize
6.3 KB
Unable to apply patch cufa_0.patch View

Re-roll. Profiling a bit later.

catch’s picture

Status: Needs review » Needs work

Well it's just a straight re-roll due to whitespace changes but it fails pretty dismally.

Dave Reid’s picture

Subscribing. The reference implementation in drupal_alter is horrid and needs to be replaced.

xtfer’s picture

Two suggestions

1) An earlier suggestion with a default to catch more than 10 arguments:


function drupal_call_user_func_array($function, $args = array() ) {
  switch (count($args)) {
    case 0: return $function(); break;
    case 1: return $function($args[0]); break;
    case 2: return $function($args[0], $args[1]); break;
    case 3: return $function($args[0], $args[1], $args[2]); break;
    case 4: return $function($args[0], $args[1], $args[2], $args[3]); break;
    case 5: return $function($args[0], $args[1], $args[2], $args[3], $args[4]); break;
    case 6: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5]); break;
    case 7: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5], $args[6]); break;
    case 8: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5], $args[6], $args[7]); break;
    case 9: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5], $args[6], $args[7], $args[8]); break;
    default: return call_user_func_array($function,$args);
  }
}

2) A completely different approach that doesn't rely on arbitrary numbers of arguments:

function drupal_call_user_func_array($function, $args = array() ) 
  $argumentArray = array();
  $argumentKeys = array_keys($args);
  foreach($argumentKeys as $keys) {
    $argumentArray[] = "\$args[$argumentKeys[$keys]]";
  }
  $argumentString = implode($argumentArray, ', ');
   
  eval("\$result =({$argumentString});");
  // will spit out something like $result =& function($args[0],$args[1],$args[2]);
  return $result;
}

Either way though, call_user_func_array() seems to be pretty tolerant of having all sorts of things thrown at it, which is probably why its slow.

Crell’s picture

cufa() is not so slow that adding an extra user-space function call to replace it is a win. And eval() is well-known to be slow. I'm not sure that we are going to find a good workaround here. I thought we had, but we're burning too many cycles in setup. :-(

chx’s picture

#471326: call_user_func_array is slow is somewhat a duplicate of this -- but my issue inlines the switch instead of using a separate function to make it speedier.

cburschka’s picture

Yeah, using eval() in a performance patch brings to mind frying pans and fire.

Thought on seeing that ASCII-art pyramid: The performance boost on this had better be really worth it. *twitch*

By the way, "return" is by definition a break from the function. A separate "break" command to break from the case is completely redundant. ;)

Edit: Random brainstorm idea; standardize hooks into a kind of "registry" or "observer" model (perhaps both) that always acts on a specific number of arguments: 0 for registries, 1 for alter, etc.

I checked the number of hook declarations in core API files, and how many parameters they take:

$ echo "  Hooks Args";grep -h 'function hook_' -R *|perl -pi -e '$_=y/\$//."\n"'|sort|uniq -c
  Hooks Args
     39 0
     87 1
     35 2
     14 3
      9 4
      7 5
      4 6

Most take less than 2, and the vast majority less than three args. The ones with the most arguments are mostly field hooks.

jrchamp’s picture

Using inline as in #471326: call_user_func_array is slow is always going to be faster than wrapping it in a drupal_cufa(). I posted some test results in that issue.

Overall, there is a limited niche where not using cufa is faster. It must either have a predetermined number of arguments or have the number of arguments available for free. Even then, there is additional overhead if the "function name" can be either a string or array to allow for static function calls. It likely would not work for member function calls.

catch’s picture

While this is going to have limited to zero effect on actual performance, I'd still like us to do it for making function traces easier. Also marked #471326: call_user_func_array is slow as duplicate.

sun’s picture

Note that, with #593522-32: Upgrade drupal_alter(), we introduced a pattern of a maximum of 3 passed on arguments to drupal_alter().

$context = array(
  'related' => $related,
  'alterable object' => $object,
  'unalterable object' => clone $object,
  'alterable array' => &$array,
);
drupal_alter('foo', $foo, $parent, $context);

I don't really see why module_invoke_all() and module_invoke() should not follow this pattern. It would even allow to kill many lines of code that currently needs to use module_implements(), because arguments need to be passed by reference.

roynilanjan’s picture

In #24

foreach($argumentKeys as $keys) {
$argumentArray[] = "\$args[$argumentKeys[$keys]]";
}

it should like

$argumentArray[] = "\$args[$keys]";
because we have already made array_keys
also is it good idea to use eval()? as eval it self have performance overhead

other wise the code seems to me very good approach

jrchamp’s picture

@roynilanjan : #24 is not a good approach. eval() should be avoided if possible. It is only preferable to "file_put_contents(): include();". Rather, places where call_user_func_array() is used should be identified and replaced with normal function calls. Some functions will need to have their parameters redefined so that this is possible, which would require all code which calls those functions to pass the new parameters. If this changes the API, then it likely isn't applicable to 7.x-dev anymore and would need to be planned for 8.x-dev.

Gerhard Killesreiter’s picture

just a comment: it would be interesting to rm cufa also for usage of Drupal with the hiphop php compiler.

Damien Tournoud’s picture

Version: 7.x-dev » 8.x-dev

At this point, that's really a Drupal 8.

Vacilando’s picture

I've found this thread after analyzing a website with *five hundred* modules in the directory (though just a small minority are enabled). Using xhprof I've found out that 51.5% (685 ms) of the execution time is taken by call_user_func_array() (277 total calls). So it seems a solution to this sure will be noticed at least on some sites!

Vacilando’s picture

Here's a solution similar to dmitrig01's initial suggestion above, with a fallback built in (as proposed in #471326: call_user_func_array is slow): http://www.php.net/manual/en/function.call-user-func-array.php#100794 It looks promising, but needs testing for speed improvement.

mattyoung’s picture

~

jrchamp’s picture

@vacilando: Even the best part of Brad's comment is only true is there is a known number of arguments or a very low limit on the total number of arguments. As you can see in my comment on #471326: call_user_func_array is slow (http://drupal.org/node/471326#comment-1810480), I have attached a file which tests call_user_func_array() and shows that it is only worse when the argument count is predefined. At that point, the switch ceases to be useful, and the short format is not only the clear winner, but also comparatively understandable (and able to be profiled). The real solution is to modify the hook API to have a set number of arguments which follows the drupal context convention (see sun's comment #30 on this issue http://drupal.org/comment/reply/329012/3683108#comment-2320462).

jrchamp’s picture

FileSize
5.07 KB

I've attached an updated version to this issue which specifically points out how much worse the runtime is if count() is included as part of the measurement.

catch’s picture

Yes we should limit the number of arguments here the same way we did for drupal_alter().

@vacilando that 685ms is going to be the inclusive time for call_user_func_array() - which includes all the functions it calls. To see the expense of call_user_func_array() you need to look at the exclusive column (which is rarely more than a couple percent of the request if that). One of the big advantages of avoiding cufa() here would be saner profiler output - since the $function() approach registers the actual function call directly without the intermediary.

kbahey’s picture

Priority: Normal » Minor

@vacilando

What catch said is true. Use exclusive and see what eats up the time. In many cases, the slowness is external to PHP (network I/O, MySQL queries, file I/O, ...etc.) and xhprof will not report those correctly.

Also, if the site in question has a slow disk (e.g. using Virtuozzo vzfs, or having the web root on an NFS network share, then just accessing the directory tree will be slow, even if you have APC enabled.

kbahey’s picture

Priority: Minor » Normal

Reverting priority ...

Mixologic’s picture

+1 on awesomer function call stacks
+1 on killing cufa to have hip-hop for drupal
Just curious if anybody has attempted any of the solutions that get rid of cufa, but have negligible performance impact, and then tried to roll that against hip-hop to see what the performance benefit would be for that two prong solution? Maybe the average user wouldnt be affected, but it'd add another tool to the drupal performance hot-rodders out there.

sun’s picture

#353494: Remove node_invoke(), comment_invoke(), etc unfortunately attempted to do the same without recognizing this existing issue, entirely duplicated discussion.

jrchamp’s picture

Unless there is a specific reason to do otherwise, I'm inclined to agree with torotil's comment on the postponed issue that a single $context argument is all that's needed.

Damien Tournoud’s picture

HipHop does support call_user_func_array(). It doesn't support streamwrappers, which makes it currently useless for Drupal 7.

catch’s picture

While HipHop supports call_user_func_array(), iirc it's not able to provide any performance benefit when using it, as far as I know that's the same for $function() too - this was from a long time ago so that might have changed. As long as we don't break HipHop support with this I don't really care about the finer details at this point though.

Single $context argument seems fine, or potentially copy drupal_alter() and have two just for convenience/consistency.

Crell’s picture

If we do something like that, then

1) We should follow drupal_alter() and allow some fixed number, otherwise we make documenting hooks and functions even more painful than it is now.

2) For the love of god don't use the variable/word $context or I will hunt you down in your sleep and strangle you with your own entrails. That word already means too many completely different things as is. :-)

jrchamp’s picture

$args or whatever is fine too. The fixed number for drupal_alter() made sense because you knew what you wanted to do. If you can think of some static variables that make sense for the hooks, I don't have a problem with having more than one parameter. To me it wasn't obvious what those static variables would be for hooks other than the catch-all itself.

torotil’s picture

I did some performance tests on this issue and tried 4 different methods of invoking a hook:

The test candidates

  1. calling the method directly (so we have a baseline performance) directCall()
  2. using call_user_func_array userFuncArray()
  3. using drupal_call_user_func_array from above custom_user_func()
  4. using a hook that expects an arguments array and deals with it directly. argsArray()

The hook function should simply increment it's first argument.

The result

Function % of the scripts execution time in % of directCall()
directCall() 9.23 100
userFuncArray() 19.54 212
custom_user_func() 30.53 331
argsArray() 10.70 116

Conclusion

The proposed replacement for call_user_func_array() performs even worse than the original! The only solution that adds a minimal overhead (16%) is passing a fixed number of parameters to the hooks as in argsArray(). The biggest part of the overhead in argsArray() is caused by incrementArray() being slower than increment().

Test script and environment

The script that I've used is.


$x = 0;


function increment(&$a) {
	$a++;
}

function incrementArray($args) {
	$args[0]++;
}

function directCall($func, &$a) {
	$func($a);
}

function userFuncArray($func, &$a) {
	call_user_func_array($func, array(&$a));
}

function argsArray($func, &$a) {
	$func(array(&$a));
}

function drupal_call_user_func_array($function, $args = array() ) {
	switch (count($args)) {
		case 0: return $function(); break;
		case 1: return $function($args[0]); break;
		case 2: return $function($args[0], $args[1]); break;
		case 3: return $function($args[0], $args[1], $args[2]); break;
		case 4: return $function($args[0], $args[1], $args[2], $args[3]); break;
		case 5: return $function($args[0], $args[1], $args[2], $args[3], $args[4]); break;
		case 6: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5]); break;
		case 7: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5], $args[6]); break;
		case 8: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5], $args[6], $args[7]); break;
		case 9: return $function($args[0], $args[1], $args[2], $args[3], $args[4], $args[5], $args[6], $args[7], $args[8]); break;
		default: return call_user_func_array($function,$args);
	}
}

function custom_user_func($func, &$x) {
	drupal_call_user_func_array($func, array(&$x));
}

for ($i = 1; $i <= 100000; $i++) {
	directCall('increment', $x);
	userFuncArray('increment', $x);
	custom_user_func('increment', $x);
	argsArray('incrementArray', $x);
}

I've used PHP without APC:

PHP 5.3.8-pl0-gentoo (cli) (built: Sep  1 2011 16:11:06) 
Copyright (c) 1997-2011 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2011 Zend Technologies
    with Xdebug v2.1.0, Copyright (c) 2002-2010, by Derick Rethans
torotil’s picture

With APC I get this results:

Function % of the scripts execution time in % of directCall()
directCall() 9.40 100
userFuncArray() 20.24 215
custom_user_func() 30.00 319
argsArray() 10.00 106

Both results were consistent within a few % when repeating them on the same machine.

catch’s picture

Could you run the benchmarks without xdebug enabled?

torotil’s picture

@catch I use the xdebug profiler to get the results. The debug functionality of xdebug is disabled.

torotil’s picture

Another benchmark. It's basically the same as above but this time I use microtime to get the execution times instead of the xdebug profiler. I always did the test 5 times with 1.000.000 iterations for each function.

without APC:

function execution time [s] (std deviation [s]) execution time relative directCall() [per cent]
directCall 1.972 ± 0.03 100
userFunc 3.716 ± 0.06 188
custom_user_func 4.834 ± 0.28 245
argsArray 3.25 ± 0.14 165

with APC:

function execution time [s] (std deviation [s]) execution time relative directCall() [%]
directCall 2.082 ± 0.08 100
userFunc 3.75 ± 0.05 180
custom_user_func 4.8 ± 0.11 230
argsArray 3.25 ± 0.11 156

The overall picture is the same with argsArray being faster than userFunc and custom_user_func performing worst. The difference seems to be smaller though.

jrchamp’s picture

Don't worry torotil, CUFA and custom_user_func weren't on the table anymore.

The question now appears to be:

One argument with all parameters.

foreach ($hooks as $hook_function => $hook_params) {
  $hook_function($hook_params);
}

vs.

Some number (e.g. three) of arguments with one of them being the "catch all" parameter argument.

foreach ($hooks as $hook_function => $hook_params) {
  $hook_function($hook_params['param_one'], $hook_params['param_two'], $hook_params['other_params']);
}

I believe that's what Crell was indicating, to make them behave more like drupal_alter(). Both of these should perform similar to the "directCall" metric.

Crell’s picture

Assigned: Crell » Unassigned
sun’s picture

Title: Performance: Remove call_user_func_array() » Replace call_user_func_array() with hook_HOOKNAME($arg1, $arg2, $context)

Right. What's actually on the table for D8 is to introduce consistency by adopting the drupal_alter() pattern/limitation:

hook_HOOKNAME($arg1, $arg2, $context);

Adjusting title accordingly.

Crell’s picture

sun: Expect a visit from me per #48. :-)

IMO we should probably have more than 2 "normal" arguments by default, since hooks likely have more arguments than an alter, but that's something worth investigating our current hooks for first to see what is "typical". Switching to that format, though, is probably the only way we're going to get more performance out of that operation short of completely changing the entire concept, which we're not doing.

torotil’s picture

The catch-all in #55 looks like it would be based on a previous call to func_get_args() which would be very bad:
func_get_args() is what currently breaks references in module_invoke*(). The broken references are the reason for loads of custom MODULE_invoke()-functions. Without func_get_args() there is no way to implement a catch-all parameter, at least if it should be transparent to all module_invoke* invocations.

Regarding references there is also a major advantage of having just a single argument-array instead of supporting a certain fixed number (+catchall): There is no point in deciding which of the arguments should be references. To clarify that a bit I give an example (pseudo-code):

function hook_something(&$a, $b);

// this will break references for hook_something()
function module_invoke1($func, $arg1, $arg2, $args);

// this will break calls like module_invoke2('something', $var, 'something static');
function module_invoke2($func, &$arg1, &$arg2, $args);

Working around this by adding yet more magic to module_invoke makes it slower and harder to understand for developers.

Because of that I'd suggest the most simple version:


function hook_something($args);

function module_invoke_all($hook, $args) {
  foreach (module_implements($hook) as $module) {
    $func = $module.'_'.$hook;
    $func($args);
  }
}

// just works
module_invoke('something', array($mixed, &$references, &$with, $copies));

This would mean that all existing hook-implementation/-invocations need to be migrated, but I think that is something that a rather simple script can do.

torotil’s picture

FileSize
53.61 KB
PASSED: [[SimpleTest]]: [MySQL] 33,967 pass(es). View

Me again …

I had another thought. Maybe it would be a lot easier to make special purpose invoke_all-functions for some common cases. These are easier to implement and will allow us to evaluate the performance impact. I've prepared a patch that simply replaces the two most common cases:

  1. hooks without arguments
  2. hooks with exactly one argument (no reference)

This two cases cover 129 of 200 calls to module_invoke_all(). If they don't have a measurable performance impact it would put the results of #35 in doubt.

torotil’s picture

Status: Needs work » Needs review
jrchamp’s picture

Any "solution" using func_get_args() would get a -1 from me (and I didn't suggest it). Special purpose functions is a backpedal and makes it a maintenance nightmare. All hooks need to be broken and made to use the new hook argument pattern.

torotil’s picture

@jrchmp I only proposed the patch to have something to run benchmarks against.

For me it doesn't look like cufa is even the problem. While nearly all of the processing time is spent in some function that's called by cufa - cufa itself doesn't even take up 0.01% of the resources.

catch’s picture

Yeah cufa() is slower than regular function calls, but we do not really call it very many times during a request for it to make any difference really. It's possible you might see it take more time if you have a real site where a lot of hooks are actually being executed (like hook_url_outbound_alter() and lots of calls to url()), but even then the savings here are very small, and there are worse performance issues when that happens than this.

Having said that, there would be some advantages to moving to $function() when looking at Drupal with a code profiler. Currently everything leads back to cufa() (which is used in a lot of places other than module_invoke_all()), but that is pretty minor in the scheme of things.

moshe weitzman’s picture

Issue tags: -Performance

Removing tag based on catch's comments of a year ago. I also think this is helpful when profiling.

sun’s picture

FileSize
4.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.module-invoke.66.patch. Unable to apply patch. See the log in the details link for more information. View

Note that the main offenders - field API "hooks" (callbacks) with their massive amount of arguments - slowly cease to exist.

I actually wonder how much really breaks today.

Status: Needs review » Needs work

The last submitted patch, drupal8.module-invoke.66.patch, failed testing.

sun’s picture

As suspected, the primary remaining offenders are mainly Field Storage API hooks (callbacks):

function hook_field_storage_load($entity_type, $entities, $age, $fields, $options) {

function field_sql_storage_field_storage_load($entity_type, $entities, $age, $fields, $options) {

function field_sql_storage_field_storage_write($entity_type, $entity, $op, $fields) {

function field_sql_storage_field_storage_purge($entity_type, $entity, $field, $instance) {

Once field storage has been converted into plugins, I could very well imagine that the patch in #66 will pass.

hefox’s picture

Mentioning it here as it's somewhat relevant to this topic

Was looking to see if changing module_invoke_all in d6 to do:

function module_invoke_all() {
  ...
  $direct = empty($args);
  foreach (module_implements($hook) as $module) {
    $function = $module .'_'. $hook;
    $result = $direct ? $function() : call_user_func_array($function, $args);

would net any differences, as there is a lot of module_invoke_all('no_args') (init, exit, boot). Nope, the empty eats it, I'm guessing.

pillarsdotnet’s picture

@#69 -- What was your testing methodology? The empty() function call shouldn't impose appreciable overhead.

hefox’s picture

*scratches head* I was surprised about this also, so I redid (remade) my test... and now they're coming like I'd originally suspected. I wish I kept yesterday's code so I could see what I'd done wrong...

(I'm sorta informal tests-- aka I turned on devel and visited devel/php )

function module_invoke_all1() {
  $args = func_get_args();
  $hook = $args[0];
  unset($args[0]);
  $return = array();
  foreach (module_implements($hook) as $module) {
    $function = $module .'_'. $hook;
    $result = call_user_func_array($function, $args);
    if (isset($result) && is_array($result)) {
      $return = array_merge_recursive($return, $result);
    }
    else if (isset($result)) {
      $return[] = $result;
    }
  }

  return $return;
}

function module_invoke_all2() {
  $args = func_get_args();
  $hook = $args[0];
  unset($args[0]);
  $return = array();
  $direct = empty($args);
  foreach (module_implements($hook) as $module) {
    $function = $module .'_'. $hook;
    $result = $direct ? $function() : call_user_func_array($function, $args);
    if (isset($result) && is_array($result)) {
      $return = array_merge_recursive($return, $result);
    }
    else if (isset($result)) {
      $return[] = $result;
    }
  }

  return $return;
}
$times = 10;
$hooks = 10;
$code = '';
$rand = rand();
for ($k = 0; $k < $hooks; $k++) {
  foreach (module_list() as $module) {
    $code .= 'function ' . $module . '_fakehook' . $rand . '1_' . $k . '(){ return '  . $k . '; }';
    $code .= 'function ' . $module . '_fakehook' . $rand . '2_' . $k . '(){ return '  . $k . '; }';
    $code .= 'function ' . $module . '_fakehook' . $rand . '3_' . $k . '(){ return '  . $k . '; }';

    $code .= 'function ' . $module . '_fakehookargs' . $rand . '1_' . $k . '($arg1, $arg2){ return '  . $k . '; }';
    $code .= 'function ' . $module . '_fakehookargs' . $rand . '2_' . $k . '($arg1, $arg2){ return '  . $k . '; }';
    $code .= 'function ' . $module . '_fakehookargs' . $rand . '3_' . $k . '($arg1, $arg2){ return '  . $k . '; }';  }
}
eval($code);

for ($k = 0; $k < $hooks; $k++) {
  module_implements('fakehook' . $rand . '1_' . $k);
  module_implements('fakehook' . $rand . '2_' . $k);
  module_implements('fakehook' . $rand . '3_' . $k);
}

$arg1 = node_load(1);
$arg2 = user_load(1);

timer_start('m3');
for ($i = 0; $i < $times; $i++) {
  for ($k = 0; $k < $hooks; $k++) {
    module_invoke_all('fakehook' . $rand . '3_' . $k);
    module_invoke_all('fakehookargs' . $rand . '3_' . $k, $arg1, $arg2);
  }
}

$m3 = timer_read('m3');

timer_start('m1');
for ($i = 0; $i < $times; $i++) {
  for ($k = 0; $k < $hooks; $k++) {
    module_invoke_all1('fakehook' . $rand . '1_' . $k);
    module_invoke_all1('fakehookargs' . $rand . '1_' . $k, $arg1, $arg2);
  }
}

$m1 = timer_read('m1');

timer_start('m2');
for ($i = 0; $i < $times; $i++) {
  for ($k = 0; $k < $hooks; $k++) {
    module_invoke_all2('fakehook' . $rand . '2_' . $k);
    module_invoke_all2('fakehookargs' . $rand . '2_' . $k, $arg1, $arg2);
  }
}
$m2 = timer_read('m2');

dsm($m3 . ' ' . $m1 . ' ' . $m2);

Results:
core: 305.76
copy of core: 304.44
empty: 272

However, since it does add the !empty and the ternary check, it does slow down the case of module_invoke_all (above with the module_invoke_all2('fakehook' . $rand . '2_' . $k); commented out: 160.85 162.71 166.68), so whether a given drupal page load is faster is depends on the ratio of with and without argument use of module_invoke_all. The core hooks on a d6 site executed each page load are init (no args), exit (1 arg), footer (1 arg).

Replaciating that sorta ($times = 1; $hooks = 1; and do the fakehookarg twice), is 6.34 6.12 5.79, so a blank page load will gain speed, but once you start adding other module_invoke_all with args it'll likely eat any change.

The think most performant way for every-page-load hooks to go is foreach-ing foreach(module_implements('their_hook) as $module) themselves or having a separate function that knows there's no args.

Grayside’s picture

Separate function that knows there's no args is what I was thinking. I also seem to remember a notion around having module_invoke_all_n() for a number of args 0-9.

hefox’s picture

$code = '';
foreach (module_implements('init') as $module) {
  $code .= 'function ' . $module . '_init2(){ return '  . $k . '; }';
}

foreach (module_implements('exit') as $module) {
  $code .= 'function ' . $module . '_exit2($arg1){ return '  . $k . '; }';
}

foreach (module_implements('footer') as $module) {
 $code .= 'function ' . $module . '_footer2($arg1){ return '  . $k . '; }';
}
eval($code);


  module_implements('init2');
  module_implements('exit2');
  module_implements('footer2');

timer_start('m3');
module_invoke_all('init2');
module_invoke_all('exit2', 'somearg');
module_invoke_all('footer2', 'somearg');
$m3 = timer_read('m3');

timer_start('m1');
foreach ( module_implements('init2') as $module) {
  $function = $module . '_init2';
  $function();
}
foreach ( module_implements('exit2') as $module) {
  $function = $module . '_exit2';
  $function('somearg');
}
foreach ( module_implements('footer2') as $module) {
  $function = $module . '_footer2';
  $function('somearg');
}
$m1 = timer_read('m1');


dsm($m3 . ' ' . $m1);

~.3MS difference... Not worth it

jibran’s picture

Status: Needs work » Needs review

66: drupal8.module-invoke.66.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 66: drupal8.module-invoke.66.patch, failed testing.

sun’s picture

Title: Replace call_user_func_array() with hook_HOOKNAME($arg1, $arg2, $context) » Remove call_user_func_array() from ModuleHandler::invoke() + invokeAll()
Component: base system » extension system
Status: Needs work » Needs review
FileSize
44.08 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Oh yay - this is finally possible now! :)

Performance of cufa() is no longer relevant in PHP 5.3, even less so in PHP 5.4.

However, we unfortunately baked the variable list of arguments into the new ModuleHandler methods, which is not only a very strange function signature, but also requires you to explicitly pass arrays by reference now:

$this->moduleHandler->invokeAll('foo', array($thing, &$some_array));

That is ugly and makes no sense. It should be:

$this->moduleHandler->invokeAll('foo', $thing, $some_array);

So attached patch is a bit larger than the ones before, because it additionally has to update all existing calls to ModuleHandler (only the legacy/procedural module_invoke* functions take a variable amount of arguments, the ModuleHandler methods accept a single array).

I wanted to stick to 3 arguments first, but there are 1-2 offending calls for both invoke() and invokeAll(), so I had to increase to 4.

Status: Needs review » Needs work

The last submitted patch, 76: module.invoke.76.patch, failed testing.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
Issue tags: +DX (Developer Experience)
FileSize
44.1 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Oopsie, canceled previous patch. Diff was incomplete as I forgot to commit some changes.

Status: Needs review » Needs work

The last submitted patch, 78: module.invoke.78.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
44.14 KB
FAILED: [[SimpleTest]]: [MySQL] 63,502 pass(es), 78 fail(s), and 9 exception(s). View
4.64 KB

Oh, hm:

Fatal error: Only variables can be passed by reference in install_begin_request().

caused by:

  $requirements = module_invoke($module, 'requirements', 'install');

So that is the reason for why module_invoke* always passed arguments by value. Whereas the new ModuleHandler cufa() indirection allows to pass a value by reference within the argument array.

I wonder what breaks if we revert that to legacy style. Grepping the patch, I can only see the following 3 instances:

/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -412,10 +412,9 @@ protected function prepareInvokeAll($hook, array &$form_state) {
-        $args = array($this->entity, $this->getFormDisplay($form_state), $this->operation, &$form_state);
-        call_user_func_array($function, $args);
+        $function($this->entity, $this->getFormDisplay($form_state), $this->operation, $form_state);
 
/core/modules/views/lib/Drupal/views/ViewExecutable.php
@@ -1379,15 +1379,15 @@ public function render($display_id = NULL) {
     // Let modules modify the view output after it is rendered.
-    $module_handler->invokeAll('views_post_render', array($this, &$this->display_handler->output, $cache));
+    $module_handler->invokeAll('views_post_render', $this, $this->display_handler->output, $cache);
 
@@ -1465,7 +1465,7 @@ public function preExecute($args = array()) {
     // Let modules modify the view just prior to executing it.
-    \Drupal::moduleHandler()->invokeAll('views_pre_view', array($this, $display_id, &$this->args));
+    \Drupal::moduleHandler()->invokeAll('views_pre_view', $this, $display_id, $this->args);

Status: Needs review » Needs work

The last submitted patch, 80: module.invoke.80.patch, failed testing.

The last submitted patch, 80: module.invoke.80.patch, failed testing.

torotil’s picture

That is ugly and makes no sense. It should be:

On what base? I think passing the arguments as array might be a bit uglier but it is even more in line with template_[pre]process functions, safes a lot of hassle with cufa, references and what not and makes the caller explicitly specify which variables might be changed by hooks.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.