Remove call_user_func_array() from ModuleHandler::invoke() + invokeAll()

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.

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

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.

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.

I'm talking about 1 function call

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;
  }
}

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;
  }
}

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

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?

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

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

Any specific form (from core) you suggest?

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?

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

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.

Re-roll. Profiling a bit later.

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

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

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 =& {$function}({$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.

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

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.

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.

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.

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.

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

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

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

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

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!

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.

~

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

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.

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.

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

Reverting priority ...

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

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

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.

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

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.

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

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

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

With APC I get this results:

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

Could you run the benchmarks without xdebug enabled?

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

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:

with APC:

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.

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.

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.

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.

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.

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.

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

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.

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

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.

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.

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.

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

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

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.

$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

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

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.

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

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);

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.

Instead of limiting parameters the current patch provides, this could use variadic unpacking instead once 8.7.x development starts and PHP 5 support is removed.

Related work in #2616814: Delegate all hook invocations to ModuleHandler is trying to do things with closures.

Now that PHP 5 is gone, #89 is probably a good starting point for this...

There are 92 cufa's in core as of 10.0.x.

call_user_func_array\('(\w*)', (\$\w+)\)
call_user_func_array\((\$\w+), (\$\w+)\)
call_user_func_array\((\$\w+), (\$[\w\[\]\']+)\)
call_user_func_array\((\$[\w\[\]\']+), (\$[\w\[\]\']+)\)

$1(...$2)

The simplest replacements, with a literal/variable callback and a single variable as argument catch 4+15+1+2 occurrences.

call_user_func_array\('(\w*)', array_merge\(\[(\$\w+)\], (\$\w+)\)\)
call_user_func_array\((\$[\w\[\]\']+), array_merge\(\[&?\$(\w+)\], (\$[\w\[\]\']+)\)\)
$1($2, ...$3)

Literal/variable callback with an array merge for fixed arguments; 1+1 occurrences.

(There are some that use array_merge to append fixed arguments *after* variadic arguments, which ... cannot do.)

Appending a partial patch.

Git index wasn't clean; resubmitting.

Patch needs a reroll since #2616814: Delegate all hook invocations to ModuleHandler was committed and conflicts with this patch.

A re-rolled patch against the latest 10.0.x.

I made a patch trying to address all uses of call_user_func_array() (the patches from 98-101 were partial as stated in #98). I hope it works :) If so, I'm curious to see how it would perform with a benchmark test.

I can review.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

There's performance measures of calls to hooks, for me the winner is fast-callable syntax after replacing fast-call with $ff(...$args) as patch in #3259716-67: Replace usages of static::class . '::methodName' to first-class callable syntax static::method(...)

https://gist.github.com/donquixote/85efcca90056111e967dd14cb1f9de9c

	--- a/core/lib/Drupal/Core/Extension/ModuleHandler.php
		+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
		@@ -384,7 +384,7 @@ public function hasImplementations(string $hook, $modules = NULL): bool {
		    */
		   public function invokeAllWith(string $hook, callable $callback): void {
		     foreach (array_keys($this->getImplementationInfo($hook)) as $module) {
		-      $hookInvoker = \Closure::fromCallable($module . '_' . $hook);
		+      $hookInvoker = ($module . '_' . $hook)(...);
		       $callback($hookInvoker, $module);
		     }
		   }
		@@ -396,8 +396,8 @@ public function invoke($module, $hook, array $args = []) {
		     if (!$this->hasImplementations($hook, $module)) {
		       return;
		     }
		-    $hookInvoker = \Closure::fromCallable($module . '_' . $hook);
		-    return call_user_func_array($hookInvoker, $args);
		+    $hookInvoker = ($module . '_' . $hook)(...);
		+    return $hookInvoker(...$args);
		   } 

Only one test failed!

Found 1 more code example but 3 in comments that maybe should be addressed too?