| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | Performance |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| cufa.patch | 6.63 KB | Idle | Failed: 6913 passes, 343 fails, 537 exceptions | View details | Re-test |
Comments
#1
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 Concurrentw/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 Concurrentw/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.
#2
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.
#3
COuld we add another function like drupal_call_user_func_array which does the same as above, except the parameters default to null?
#4
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.)
#5
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.
#6
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.
#7
I'm talking about 1 function call
#8
Something like this. we might be able to get rid of the switch if we don't care about the param count working.
<?phpfunction 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;
}
}
?>
#9
Random idea I just had...
<?php
$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.
#10
Oops this removes the array part, how about something like
<?phpfunction 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;
}
}
?>
#11
@crell maybe that would work - what do you think of my approach?
#12
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. :-(
#13
Can't see a difference still.
Resp Time Trans Rate Trans OKAY Failed Elap Time ConcurrentBefore 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?
#14
kbahey - please try a complex form page. you might need to give anon some extra perms.
#15
I am enabling more core module (basically all, except ping, blogapi and throttle) for future tests.
Any specific form (from core) you suggest?
#16
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.
#17
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?
#18
Yes, permissions page. You will have fun giving anon users the right to administer users.
#19
The last submitted patch failed testing.
#20
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 rundrupal_function_exists('taxonomy_get_tree')before calling.#21
Re-roll. Profiling a bit later.
#22
Well it's just a straight re-roll due to whitespace changes but it fails pretty dismally.
#23
Subscribing. The reference implementation in drupal_alter is horrid and needs to be replaced.
#24
Two suggestions
1) An earlier suggestion with a default to catch more than 10 arguments:
<?phpfunction 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:
<?php
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.
#25
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. :-(
#26
#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.
#27
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 -cHooks 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.
#28
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.
#29
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.
#30
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.
#31
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
#32
@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.
#33
just a comment: it would be interesting to rm cufa also for usage of Drupal with the hiphop php compiler.
#34
At this point, that's really a Drupal 8.
#35
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!
#36
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.
#37
~
#38
@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).
#39
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.
#40
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.
#41
@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.
#42
Reverting priority ...
#43
+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.
#44
#353494: Remove node_invoke(), comment_invoke(), etc unfortunately attempted to do the same without recognizing this existing issue, entirely duplicated discussion.
#45
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.
#46
HipHop does support
call_user_func_array(). It doesn't support streamwrappers, which makes it currently useless for Drupal 7.#47
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.
#48
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. :-)
#49
$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.
#50
I did some performance tests on this issue and tried 4 different methods of invoking a hook:
The test candidates
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.
<?php
$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
#51
With APC I get this results:
Both results were consistent within a few % when repeating them on the same machine.
#52
Could you run the benchmarks without xdebug enabled?
#53
@catch I use the xdebug profiler to get the results. The debug functionality of xdebug is disabled.
#54
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.
#55
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.
<?phpforeach ($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.
<?phpforeach ($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.
#56
#57
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.
#58
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.
#59
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):
<?php
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:
<?php
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.
#60
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:
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.
#61
#62
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.
#63
@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.
#64
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.