Reintroduce the container aware dispatcher for a 10% speedup

It looks from the reddit thread that the resolveServicesAndParameters calls take about 2-3ms - the problem with this on page cache hits is that a page cache hit only takes 2-3ms at most, so it's doubling the request time.

(edit: I'm seeing a lot more than this locally, more like 10-20ms, but some of that might be xhprof overhead).

Also 11.1 only has core hooks converted, once you have a site with 2-300 modules, all of which have OOP hooks, that 2ms could turn into 5-10ms or more, which will be noticeable on dynamic page cache hits too (dynamic page cache hits are usually more like 60-70ms, so increasing those by 10-20%).

One issue I'm seeing profiling is this is running for discovered procedural non-hooks still - uploading some dump output from having a look around. And because those go from collection to service closure, it looks like it results in two calls each.

Here's a quick go at a revert, it also doesn't work. I get "Unable to find the controller for path "/". The route is wrongly configured." after a drush cr.

To describe the basic issue:

We add hooks to the event dispatcher via the 'calls' parameter to the event dispatcher, which calls ::addListener(), this happens every time event dispatcher is instantiated, e.g. on every single request.

So even though we don't instantiate the individual hook services, we iterate over every hook implementation in core to create a service closure. This involves many recursive calls to ::resolveServicesAndParameters().

ContainerAwareEventDispatcher on the other hand only creates the service definition lazily - e.g. when a specific event is dispatched.

Even though the original reason we added ContainerAwareEventDispatcher is no longer an issue (e.g. services are not actually constructed for every listener), the sheer number of event listeners in Drupal core now after OOP hooks means that even creating the service closures on every request is causing a measurable performance regression.

Here's a before/after xhprof screenshot with HEAD vs. #3506930: Separate hooks from events.

Note that profiling differs whether you have dev dependencies installed or not, at least partly due to OpenTelemetry SDK, I think this was with dev dependencies installed, but the reduction is about the same even if the totals are different.

So #3506930: Separate hooks from events is removing about 2/5ths of the function calls and about half the wall time.

If #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher is going to be non-trivial to revert, then we could potentially profile 10.3.x just before/after that commit to see what the impact on cached pages is there. That would give us an idea whether it really is worth reverting or not.

One thing I noticed during debugging that a lot of event listeners we have in core (that aren't hooks) are core event listeners for core events, we could also reduce that list by converting some core events to hooks 😇.

catch is good at predicting the future: #2909185-103: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher

This MR combines the calls to addListener() which removes ~670 method calls locally, but it doesn't really make that much difference.

There is nothing stopping us having multiple event dispatchers; I am wondering if we should put hooks on their own, separate dispatcher.

MR!12850 is promising locally. Testing on the Umami homepage, warm cache:

11.x:

Wall Time 11,910 µs
CPU Time 9,726 µs
Memory Usage 3,249,088 bytes
Peak Memory Usage 4,462,840 bytes

670 calls to EventDispatcher::addListener()

MR!12850:

Wall Time 8,083 µs
CPU Time 6,179 µs
Memory Usage 2,082,648 bytes
Peak Memory Usage 2,481,720 bytes

151 calls to EventDispatcher::addListener()

edit: just saw #3506930: Separate hooks from events, is this what that is trying to do, but a different/simpler implementation?

The page that you are testing, are any hooks called on a warm cache?
And is ModuleHandler instantiated at all in these requests?

If ::addListener() calls are what slows down these requests, maybe we could go even further and have a separate event dispatcher with only those events that are commonly invoked in requests with warm cache?

It's the Umami homepage as anonymous, so on a warm cache no hooks should be invoked, hence the drop in calls to addListener. As I understand it from the Reddit thread and the issue summary it's cached pages that are the problem here?

As soon as a hook is invoked I think this will make no difference, because instantiating the hook event dispatcher goes back to being expensive.

The problem is when can we make these changes. The bigger the change the more likely we have to wait for 11.3 but given this is a regression maybe we want a stopgap fix in 11.2?

I'm not too worried about an 11.2 backport, this is a regression for cached pages, but it's a regression from very fast to still quite fast. It'll be barely measurable for requests that don't hit the page cache (partly why we haven't noticed it until now) and it's all CPU rather than i/o. If we end up with something backportable that's great but more interested in the longer term solutions.

Either we revert to container aware or we pull hooks out of event dispatcher (or both).

So I think if we revert to container aware event dispatcher, then we should have almost zero overhead from having hooks in event dispatcher, but we could still move them out for other reasons.

If we move hooks out first, then the regression from not using container aware will be whatever it was in 10.3 when the initial issue to not use it was committed, which seems relatively small but also worth fixing. While I might have predicted the future I also wasn't thinking about cached pages in that comment.

The problem with that issue and the MRs here is they are both just treating the symptom not the cause.

This is true but also I think if we know it doesn't scale to thousands of events and we already have a plan to stop putting thousands of events in it, we should definitely do that.

There. I reintroduced the container aware dispatcher but with much better tie-in via a new ContainerAwareEventDispatcherDefinition which YamlFileLoader uses. In the original version a new pass was needed instead of Symfony's. No longer. The very definition of the service captures the addMethodCall calls from the Symfony RegisterListenersPass and changes them into the argument structure required by ContainerAwareEventDispatcher. This allows keeping all the functionality of RegisterListenersPass including multiple event dispatchers with very little overhead. Tests could be resurrected of course and/or tests could be written for ContainerAwareEventDispatcherDefinition but since all of core flows through it, I haven't felt the need.

The performance regression was introduced in D10.3, this needs to be backported widely.

If we inject the container and read from it at runtime I think this means we will never be able to make event listener services private by default. I am not sure if this is truly important, although @alexpott and I have discussed it as a long term aim in the past.

Note the changes do not invalidate the performance work, they are cosmetic / build time. But now the code doesn't look like something the cat barfed up. That ladder of ifs was hard to comprehend, this is linear.

Event dispatcher shouldn't be instantiated for cached pages at all. Filed: #3538294: Regression: All stack middlewares are constructed at the same time even for cached pages.

That's even better but a performance regression is present since 10.3 for any site with a large number of listeners regardless of cached pages. It's possible this issue needs to be retitled for that and the issue znerol linked is the urgent page cache performance regression which is independent after all.

a performance regression is present since 10.3 for any site with a large number of listeners regardless of cached pages.

The performance regression was reported for 11.1 and is clearly measurable on 11.1 on cached responses. Is there any evidence that 10.3 suffered a performance regression on cached pages of the same scale?

Not on cached pages. The performance regression exists on every page if you have enough listeners.

Not on cached pages. The performance regression exists on every page if you have enough listeners.

Then please submit your PR as a separate issue.

@znerol yes, it's possible this issue should be retitled to "revert the event dispatcher" and the issue title from here needs to move to yours. Sorry, we didn't realize the event dispatcher shouldn't even be there in the first place. However, fixing middlewares to be constructed on demand only avoids constructing the event dispatcher in core, the moment anyone adds a middleware firing before page cache which has the event dispatcher as an argument will be hit by the issue here. I believe both issues need to be fixed and quite likely backported widely.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Trying a re-title.

Based on the performance benchmark in the issue summary there is a 10% hit on the admin performance page from the Symfony event dispatcher just with standard without hooks. So I am not sure whether "hundreds" are even needed.

A few small comments, mostly about typing.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

I can't imagine still seeing a regression now between #3538294: Regression: All stack middlewares are constructed at the same time even for cached pages and #3506930: Separate hooks from events. If anything, thanks to the first issue on page cache scenario, it should actually be considerably faster than 11.2. You should also no longer see a difference in this scenario between 11.x and this MR because event dispatcher is no longer initialized. You would need a different scenario, where event subscriber is invoked, for example a dynamic page cache hit, with a considerable amount of regular events. So I have my doubts this is still a release priority.

Yeah I think we can untag it.

Some early numbers from our project, comparing 11.2 against 11.x, as well as #3551605: Injecting ConfigFactory into BanIpManager is expensive, use a closure as that's necessary to get the full benefit here.

It's quite a difference. As usual, times vary a lot per request, but here's what I'm seeing:

Time: -6.71 ms (-53%)
12.8 ms → 6.05 ms

I/O Wait: -159 µs (-12%)
1.31 ms → 1.15 ms

Peak Memory: -7.51 MB (-76%)
9.84 MB → 2.34 MB

Network-68 kB (-35%)
194 kB → 126 kB

This is with redis, but with ban enabled, so it opens the database question and runs one query

This was on ddev, which unfortunately doesn't create php.access.log files by default which contain time and memory usage, so I'll see if I can either enable this or run it somewhere where I can that information without that.

It was also withou_ optimized autoloader. Using that I can push the absolute times even lower

Looking at numbers it looks bad idea to reintroduce and maintain custom dispatcher

@andypost @berdir do I misunderstand #66 because it seems the speedup is substantial?

I think #66 is 11.2.x vs 11.x, not vs this issue.

10% is viable so ++

We should re-measure the change here after #3583505: Use Symfony PhpDumper instead of a serialized array container structure as that will likely have an impact, and I have no idea in which direction it will be.