Replace call_user_func_array() with $callback() for performance, where possible.

It would be great to see a before and after of xhprof to see how much this actually improves.

+++ b/core/includes/theme.inc
@@ -530,11 +530,13 @@ function _theme($hook, $variables = array()) {
-  foreach (array_reverse($suggestions) as $suggestion) {
+  end($suggestions);
+  while ($suggestion = current($suggestions)) {
     if ($theme_registry->has($suggestion)) {
       $info = $theme_registry->get($suggestion);
       break;
     }
+    prev($suggestions);
   }

How much faster is that?

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -1015,8 +1015,8 @@ public function doBuildForm($form_id, &$element, &$form_state) {
+      foreach ($element['#process'] as $callable) {
+        $element = $callable($element, $form_state, $form_state['complete_form']);

@@ -1080,7 +1080,7 @@ public function doBuildForm($form_id, &$element, &$form_state) {
-        $element = call_user_func_array($callable, array($element, &$form_state));
+        $element = $callable($element, $form_state);

@@ -1224,7 +1224,7 @@ protected function handleInputElement($form_id, &$element, &$form_state) {
-            $element['#value'] = call_user_func_array($value_callable, array(&$element, $input, &$form_state));
+            $element['#value'] = $value_callable($element, $input, $form_state);

@@ -1240,7 +1240,7 @@ protected function handleInputElement($form_id, &$element, &$form_state) {
+          $element['#value'] = $value_callable($element, FALSE, $form_state);

I'm pretty sure we purposefully allow callables here. I didn't think that $callable = array($object, 'method'); $callable(); worked?

Learn something new every day! Thanks for the tip @sun.

Just tagging for #5

Re #5.

I've generated 50 nodes with 3 commments using drush genc 50 3.
Then did clear cache vs warm cache json_encode export of the $suggestsions of what's now ThemeManager:render() on ThemeManager.php:255

This should be much better performance test than xhprof because it's real results but localized to this case.

To export I just dropped in:

file_put_contents('suggestions.txt', json_encode($suggestions) . "\n", FILE_APPEND);
file_put_contents('suggestions-warm.txt', json_encode($suggestions) . "\n", FILE_APPEND);

Suggestions Cold Cache

Reverse Foreach Times:

Mean: 2.8772787614302E-6
Mean: 2.8772787614302E-6
Median: 2.1457672119141E-6
Range: 4.0054321289062E-5

End While Times:

Mean: 6.1731446873058E-6
Mean: 6.1731446873058E-6
Median: 3.0994415283203E-6
Range: 7.2240829467773E-5

Suggestions Warm Cache

Reverse Foreach Times:

Mean: 3.7806374686105E-6
Mean: 3.7806374686105E-6
Median: 2.1457672119141E-6
Range: 2.4080276489258E-5

End While Times:

Mean: 6.4600081670852E-6
Mean: 6.4600081670852E-6
Median: 5.0067901611328E-6
Range: 2.0027160644531E-5

So essentially that change is twice as slow and also fairly negligible. but should be removed from this patch in my opinion because it's less clear as well as slower.

Running the script change it to test.php and run it with php or drush...
drush scr test.php

Here's a re-roll without that foreach to while change and using this regular expression to find similar array callables.

call_user_func_array\([^,]*, array\(

Made a typo missed $ and reroll for installer batch changes.

Very interesting I wonder why the callback would fail in the AJAX request and before it didn't... wrapped them both in a check to see if we have a callbable.

PHP Fatal error:  Call to undefined function \Drupal\Core\Render\Element\FormElement::valueCallback() in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Form/FormBuilder.php on line 1082

+++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
@@ -101,7 +101,7 @@ public function has($key) {
     // are not registered, just check the existence of the key in the registry.
     // Use array_key_exists() here since a NULL value indicates that the theme
     // hook exists but has not yet been requested.
-    return array_key_exists($key, $this->storage);
+    return isset($this->storage[$key]) || array_key_exists($key, $this->storage);

The comment indicates that this was on purpose

@dawehner re #16 yes it does but that logic change shouldn't change that fact it will just make it faster by doing isset() first if the key exists. NULL will failover to the slower array_key_exists().

@dawehner re #16 yes it does but that logic change shouldn't change that fact it will just make it faster by doing isset() first if the key exists. NULL will failover to the slower array_key_exists().

Oh you are right. Do you mind adapting the comment a bit?

I don't mind, just won't be looking at this realistically for another week as I'll be doing safemarkup at MWDS.

Though I'll pick this up again if nobody has and might squeeze it in somewhere between.

Most of these changes are about replacing call_user_func_array(). Which I support very much.

Should we focus on these changes only and then change the issue title?
I almost created a new issue for the same purpose, because I could not find this one with this very generic title.

I change the title myself for now, so at least it can be more easily found in searches.
If we actually decide to move the two other improvements to a different issue, we can simplify the title.

The main problem with replacing call_user_func_array() is callbacks of the format 'C::foo'. These are supported by call_user_func_array(), but not by $callback().

Only in PHP 7 is this fixed.
https://3v4l.org/KFAGV

For callbacks that are processed with $form_state->prepareCallback($callback), we could extend the prepareCallback() to also cover this case, with little extra overhead.

  public function prepareCallback($callback) {
    if (!is_string($callback) || FALSE === $pos = strpos($callback, '::')) {
      return $callback;
    }
    elseif ($pos === 0) {
      return [$this->getFormObject(), substr($callback, 2)];
    }
    else {
      return explode('::', $callback);
    }
  }

We would have to verify which combination of explode(), strpos() and substr() is fastest. I think it depends on which case we optimize for.
Btw even faster would be if the form state had a method that invokes the method instead of just preparing it. And/or if it could prepare or invoke multiple of those methods in one call. So the loop over $element['#process'] would be in the new FormState::invokeProcessCallbacks() method, not in the FormBuilder::doBuildForm() method.

-------

For the other cases, we would simply have to wait for a release that allows a BC break like disallowing the 'S::foo' syntax for callbacks. Or wait until Drupal requires PHP 7.

See also comment #149 in #1499596-149: Introduce a basic entity form controller, the issue where the call_user_func_array() was introduced.

re #22 at the point where we cover the exception are we just making this more complicated and slower? strpos is pretty fast but explode isn't.

re #22 at the point where we cover the exception are we just making this more complicated and slower? strpos is pretty fast but explode isn't.

We would have to profile this and compare. The other option is

      return [substr($callback, 0, $pos), substr($callback, $pos + 2)];

This would be two function calls instead of one, but possibly it is still faster. Only benchmarks can tell.

But more important is that the different if branches have a different likeliness. If e.g. only 2% of callbacks have the 'C::foo' format, then the cost of explode() does not really matter as much.

Broke off the small little change for array_key_exists()
#2770065: array_key_exists() micro-optimization

So it can be removed form this and make the title change more accurate from #21

Benchmark code:

$strings_all = [];

$n = 10000;

foreach (['explode', 'substr'] as $k) {
  for ($i = 0; $i < $n; ++$i) {
    $strings_all[$k][$i] = uniqid('', false) . '::' . uniqid('', false);
  }
}

$dtss = [];
$t = microtime(true);

for ($j = 0; $j < 100; ++$j) {

  $strings = $strings_all['explode'];
  for ($i = 0; $i < $n; ++$i) {
    $str = $j . $strings[$i];
    if (false !== $pos = strpos($str, '::')) {
      $pieces = explode('::', $str);
    }
  }

  $t = ($dtss['explode'][] = microtime(TRUE) - $t) + $t;

  $strings = $strings_all['substr'];
  for ($i = 0; $i < $n; ++$i) {
    $str = $j . $strings[$i];
    if (false !== $pos = strpos($str, '::')) {
      $pieces = [substr($str, 0, $pos), substr($str, $pos + 2)];
    }
  }

  $t = ($dtss['substr '][] = microtime(TRUE) - $t) + $t;
}

$percentiles = [];
foreach ($dtss as $k => $dts) {
  sort($dts);
  foreach ([10, 50, 90] as $percent) {
    $percentiles[$percent . '%'][$k] = $dts[$percent - 1] * 1000;
  }
}

print json_encode($percentiles, JSON_PRETTY_PRINT) . "\n";

Output:

{
    "10%": {
        "explode": 7.6808929443359,
        "substr ": 9.6120834350586
    },
    "50%": {
        "explode": 7.7648162841797,
        "substr ": 9.6838474273682
    },
    "90%": {
        "explode": 7.9150199890137,
        "substr ": 9.9670886993408
    }
}

Which means that the explode() solution is faster.

EDIT: I added a $pos = before the first strpos(), and posted the new numbers. Still same conclusion.

It turns out that explode() is even faster for the case of '::foo'.

$strings_all = [];

$n = 10000;

foreach (['explode 1', 'explode 2', 'substr'] as $k) {
  for ($i = 0; $i < $n; ++$i) {
    $strings_all[$k][$i] = '::' . uniqid('', false);
  }
}

$obj = new \stdClass();

$dtss = [];
$t = microtime(true);

for ($j = 0; $j < 100; ++$j) {

  $strings = $strings_all['explode 1'];
  for ($i = 0; $i < $n; ++$i) {
    $str = $strings[$i] . $j;
    $fragments = explode('::', $str);
    if (isset($fragments[1])) {
      if ($fragments[0] === '') {
        $fragments[0] = $obj;
        $pieces = $fragments;
      }
      else throw new \Exception();
    }
    else throw new \Exception();
  }

  $t = ($dtss['explode 1'][] = microtime(TRUE) - $t) + $t;

  $strings = $strings_all['explode 2'];
  for ($i = 0; $i < $n; ++$i) {
    $str = $strings[$i] . $j;
    $fragments = explode('::', $str);
    if (isset($fragments[1])) {
      if ($fragments[0] === '') {
        $pieces = [$obj, $fragments[1]];
      }
      else throw new \Exception();
    }
    else throw new \Exception();
  }

  $t = ($dtss['explode 2'][] = microtime(TRUE) - $t) + $t;

  $strings = $strings_all['substr'];
  for ($i = 0; $i < $n; ++$i) {
    $str = $strings[$i] . $j;
    if (false !== $pos = strpos($str, '::')) {
      if (0 === $pos) {
        $pieces = [$obj, substr($str, $pos + 2)];
      }
      else throw new \Exception();
    }
    else throw new \Exception();
  }

  $t = ($dtss['substr   '][] = microtime(TRUE) - $t) + $t;
}

$percentiles = [];
foreach ($dtss as $k => $dts) {
  sort($dts);
  foreach ([10, 50, 90] as $percent) {
    $percentiles[$percent . '%'][$k] = $dts[$percent - 1] * 1000;
  }
}

print json_encode($percentiles, JSON_PRETTY_PRINT) . "\n";

{
    "10%": {
        "explode 1": 6.5970420837402,
        "explode 2": 8.5539817810059,
        "substr   ": 7.8849792480469
    },
    "50%": {
        "explode 1": 6.67405128479,
        "explode 2": 8.6469650268555,
        "substr   ": 7.9388618469238
    },
    "90%": {
        "explode 1": 6.788969039917,
        "explode 2": 8.7661743164062,
        "substr   ": 8.2519054412842
    }
}

But for the 'foo' case (regular function), strpos() is faster.

$strings_all = [];

$n = 10000;

foreach (['explode 1', 'explode 2', 'substr'] as $k) {
  for ($i = 0; $i < $n; ++$i) {
    $strings_all[$k][$i] = uniqid('', false);
  }
}

$obj = new \stdClass();

$dtss = [];
$t = microtime(true);

for ($j = 0; $j < 100; ++$j) {

  $strings = $strings_all['explode 1'];
  for ($i = 0; $i < $n; ++$i) {
    $str = $strings[$i] . $j;
    $fragments = explode('::', $str);
    if (isset($fragments[1])) {
      throw new \Exception();
    }
    $callback = $str;
  }

  $t = ($dtss['explode 1'][] = microtime(TRUE) - $t) + $t;

  $strings = $strings_all['explode 2'];
  for ($i = 0; $i < $n; ++$i) {
    $str = $strings[$i] . $j;
    $fragments = explode('::', $str);
    if (isset($fragments[1])) {
      throw new \Exception();
    }
    $callback = $str;
  }

  $t = ($dtss['explode 2'][] = microtime(TRUE) - $t) + $t;

  $strings = $strings_all['substr'];
  for ($i = 0; $i < $n; ++$i) {
    $str = $strings[$i] . $j;
    if (false !== $pos = strpos($str, '::')) {
      throw new \Exception();
    }
    $callback= $str;
  }

  $t = ($dtss['substr   '][] = microtime(TRUE) - $t) + $t;
}

$percentiles = [];
foreach ($dtss as $k => $dts) {
  sort($dts);
  foreach ([10, 50, 90] as $percent) {
    $percentiles[$percent . '%'][$k] = $dts[$percent - 1] * 1000;
  }
}

print json_encode($percentiles, JSON_PRETTY_PRINT) . "\n";

{
    "10%": {
        "explode 1": 4.5981407165527,
        "explode 2": 4.6041011810303,
        "substr   ": 3.2799243927002
    },
    "50%": {
        "explode 1": 4.6591758728027,
        "explode 2": 4.6849250793457,
        "substr   ": 3.3488273620605
    },
    "90%": {
        "explode 1": 4.8019886016846,
        "explode 2": 4.9130916595459,
        "substr   ": 3.4840106964111
    }
}