Optimize ModuleHandler::invokeAll()

Have you tested this recently? Some of them are definitely no longer valid.

Thanks for reviewing this, here is my initial take with some questions!

Inline the invokeAllWith() call in invokeAll(), don't call invokeAllWith(), don't use a closure.

I'd love a deeper dive in this bit!

Replace isset($result) with NULL !== $result.

Reviewing this, it seems that the benefits are negligible and can throw warnings where isset cannot.

Provide a version of NestedArray::mergeDeep() which accepts exactly two parameters and uses its own implementation instead of calling self::mergeDeepArray(). There is unnecessary indirection and parameter juggling.

That seems worth it.

The recursive result merging is not necessary in many cases.

I wonder if the performance boost is worth the extra methods. It's worth checking for sure.

call_user_func_array($hook, $args);

There is an old issue about replacing this and it's not just references, it's also how many do you add and most attempts to optimize it are slower than cufa.

First version does not do anything about NestedArray::mergeDeep().
Imo we could postpone this to a separate issue, just to keep things incremental.

Replace isset($result) with NULL !== $result.

Reviewing this, it seems that the benefits are negligible and can throw warnings where isset cannot.

The performance benefit would be negligible, true.
isset() treats undefined like null, it does not produce a warning if a variable or array offset does not exist.

Whenever we know for sure that a variable exists, we should use === NULL or !== NULL over isset(), it is more explicit and we learn quickly if a variable is misspelled, both from static analysis (IDE, phpstan) and from the php warning.

Everything old is new again

This code was created in https://www.drupal.org/project/drupal/issues/2616814#comment-12065522 📌 Delegate all hook invocations to ModuleHandler Fixed and then in #25 the performance question was raised, acknowledged and never addressed.

Everything old is new again

Right, but now is as good a time as any to address it..
we might even backport to 10.x and/or 11.1.x pre OOP hooks.

Profiling PRs

I pushed two branches for profiling:
2865609-invokeAll-profile - adds profiling and does not include the change
2865609-inline-invokeAll-profile - adds profiling and includes the change

The profiling is for an invokeAll('theme') that actually never happens like this.
I only picked hook_theme() because it is the first that came to mind that has plenty implementations and can be executed without doing lots of other stuff first.

Arguably hook_theme() is a bad choice, because a lot of the time will be spent on the array merging, which we are not optimizing in this version of the MR. Still, if we can measure a difference here, then the experiment is not completely useless.

The way to run the profiling is to execute the kernel test.
The failure message reports different profiling metrics.

To compare, repeatedly switch branches and run the profiling kernel test.

Profiling results

For me, I get:
- Without the change, I get 'repeat.best' in the range of 54 to 56 microseconds.
- With the change, I get 'repeat.best' in the range of 52 to 55 microseconds.

There are outliers, but it is relatively consistent.
The difference between testHookTheme() and testHookThemeWithBogusArguments() is quite small, for me it was around 1 microsecond.

Conclusion

So, the improvement is very small, but it is reproducible.
I also see no reason to assume that it would cause slowness in other areas.

For complexity, I think the changed version is actually better, because you don't need to read two methods to see what is going on.

That is less than I expected, 1-4 microseconds.

I think we would need more test cases.

At least one for each branch and ideally some that cross multiple like a real request would.

I don't think ghost was advocating against this change, just commenting.

I am not sure this would be eligible for backport either.

If I'm being honest unless there is more improvement in other logic branches I'm not sure this is worth it, yes it inlines the call, but now you have to maintain the logic in two functions, especially if we are going to provide return type specific invokes in the other issue, we'd be duplicating the logic multiple times.

I think it's worth testing the other paths and getting more opinions.

Can issue summary be updated to use standard template please.

Also if profiling findings could be added the proposed solutions section and we can remove that tag.

Thanks all!

I would advocate for closing this, 1-4 micro seconds isn't worth chasing. There are far larger bottlenecks.

If that’s the route I am saving some credit for the work that was done.

Closed issues get credit now

I would advocate for closing this, 1-4 micro seconds isn't worth chasing. There are far larger bottlenecks.

The measurements so far don't look promising, but I would not say they are conclusive (yet).
The 1-4 ms are some kind of artificial scenario.
Also, some of this might change after the changes from 📌 Hook ordering across OOP, procedural and with extra types Active , when omitting the $modules could provide other benefits.