Replace ContainerAwareEventDispatcher with Symfony EventDispatcher

Sure, give it a try. Note: It was no longer a 1to1 representation of the original code. Maybe it would be worth to do some profiling as well, better be safe than sorry :)

Sure it needs profiling because more lazy instantiations

Agree with profiling, there might be surprises due to the fact that Drupal does handle the container dumping very differently from Symfony.

Please also see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Eve... it's the current master (same as Symfony 3.4).

What it does is that it registers listeners and subscribers using ServiceClosureArgument which means that listeners and subscribers are lazy instanciated at runtime when events are being fired (just as it worked until now) but it brings another improvements: listener and subscribers can be private services and thus be inlined during the container compile phase.

For Symfony itself, because they dump the container as PHP file, it means a smaller file, a smaller container, subsribers and listeners will all be inlined so their instanciation will be a lot faster.

For Drupal, because of the OptimizedPhpArrayDumper implementation, it may behave very differently.

For testing purposes, here is a patch that just removes Drupal custom implementation, no further optimisations. Did try with and without on a fresh install, nothing more in the site - did measure a 3% negative performance impact but that's not a viable measure.

requeued cos apcu issue again

@jibran

What is the next step here? Should we wait for #2874909: Update Symfony components to 3.3.* or #2927746: Update Symfony components to 3.4.* before doing further profiling?

This would be a great idea to profile with 3.3 and 3.4 patches altogether, yes. I'm not sure what would be the result, Symfony 3.4 did a lot of performance fixes but a lot are in the dumped container but Drupal does not uses it still does only use a few Symfony components.

Yes it does ! I have to wait to have less client work to do so I can proceed with a few hours of R&D, but it'll not happen before mid-february for me, so if anyone wan't to give it a shot it'd be great.

so if anyone wan't to give it a shot it'd be great.

I am looking at this now

This shouldn't be too hard, it's basically about removing a file, a few tests, and changing a class name somewhere :)

This reroll, is broken -

so this is a request for comments.. I am laying out my thinking because .. in the hope that the solution is the obvious to someone else.

I am aware of other issues related to upgrade symfony components and see pounard's question in

https://www.drupal.org/project/drupal/issues/2927746#comment-12430440

Instead of hardcoding services to public, would it be worthwhile to mark them as public in the services definition files ?

and the response was "Ahh crap that is going to break all the things". ( well alexpott's response more proper and very detailed. )

Back to the failing patch... When I try "drush si" it fails with

Unable to dump a service container if a parameter is an object without _serviceId.

When I put debug into OptimizedPhpArrayDumper.php

I can see it is failing to get the service definition ( see OptimizedPhpArrayDumper::getServiceDefinition() )

for the "cache_router_rebuild_subscriber" which is tagged as a event_subscriber.

From here my symfony knowledge is a little soggy... cache_router_rebuild_subscriber is a hard coded ( non-autowired ) name that the tagged service container factory is NOT picking up as expected ...
and I need to know how to do that ...

Is it a symfony thing that all tagged serivces need to be autowired?

Anyway here is the patch..

Nope, tagged services don't need to be autowired at all, I'd say that modifications from @alexpott to fix the php optimized dumper broke something.

Nope, tagged services don't need to be autowired at all

Thanks for the information...

When I look at the code snippet listed at the end ... just before the RuntimeException is thrown I dump out $value

$value is a ServiceClosureArguement object that holds a values array which does contain the service_id as "id".

Anyway I have my head in a symfony book trying to make progress .. but I am just posting what I see just in case anyone can see the solution before I get the spark of inspiration :)

ServiceClosureArgument {#1493 ▼
-values: array:1 [▼
0 => Reference {#1472 ▼
-id: "cache_router_rebuild_subscriber"
-invalidBehavior: 1
}
]
}

    }
    elseif (is_object($value)) {
      // Drupal specific: Instantiated objects have a _serviceId parameter.
      if (isset($value->_serviceId)) {
        return $this->getReferenceCall($value->_serviceId);
      }
      dump($value);
      throw new RuntimeException('Unable to dump a service container if a parameter is an object without _serviceId.');
    }

Actually, Drupal does it wrong somehow, the OptimizedPhpArrayDumper is a full custom rewrite, very specific to Drupal, of the container dumper, any bug in there is a Drupal bug, not a Symfony bug.

I know that it is what it is for performance and consistency reasons, mostly because Drupal needs the container to be dumped into cache and not into PHP files like Symfony would do, because it needs to be rebuildable via web interaction (module enable, etc..) - but it's a major pain to debug (I tried a few times).

So with lazing loading of services is good for performance
and apparently making services private leads to performance improvements

So short of ripping out OptimizedPhpArrayDumper going through the massively disruptive task of making services private as appropriate and then profiling so we can finally make drupal specific optimization from that point.

I am not sure what to suggest...

Related is closed, but has a lot of useful information

Reminder: https://www.drupal.org/files/issues/subsequent-dispatch-HEAD-vs-20-vs-35... in https://www.drupal.org/project/drupal/issues/1972300#comment-9216069 and then https://www.drupal.org/project/drupal/issues/1972300#comment-9235865

Obviously it is better if the Drupal project needs to maintain less code but less function / method calls in the hot code path is even better. If upstream reached performance parity or hopefully even better then all is well but I am surprised to see more than twenty comments (some with patches) since the "needs profiling" tag was added and I can't see any profiling. Did I miss something?

Revisiting this. ContainerAwareEventDispatcher has the following comment:

Faster instantiation of the event dispatcher service

Instead of calling addSubscriberService once for each
subscriber, a precompiled array of listener definitions is passed
directly to the constructor. This is faster by roughly an order of
magnitude. The listeners are collected and prepared using a compiler
pass.

Lazy instantiation of listeners

Services are only retrieved from the container just before invocation.
Especially when dispatching the KernelEvents::REQUEST event, this leads
to a more timely invocation of the first listener. Overall dispatch
runtime is not affected by this change though.

The lazy instantiation is resolved upstream. Symfony now wraps service references in a closure and these are resolved only when needed in the event dispatcher.

The faster instantiation is not resolved, and this is not currently possible to override; the $listeners property is private in the Symfony EventDispatcher and there is no way to inject a precompiled set of listeners. Perhaps Symfony would be amenable to addressing this upstream.

I have also addressed the issue in #20 where service closures could not be dumped by adding support for this to the dumper and container loader.

This still needs profiling but locally my site seems to work fine when using the upstream event dispatcher.

It would help if I actually switched the event dispatcher service.

+++ b/core/lib/Drupal/Core/CoreServiceProvider.php
@@ -21,13 +21,13 @@
-use Drupal\Core\DependencyInjection\Compiler\RegisterEventSubscribersPass;

AFAIK this is the only usage of RegisterEventSubscribersPass. Deprecate that as well?

#32 Great Work - I am signing off on the new service_closure addition to OptimizedPhpArrayDumper and PhpContainer. Implementation looks good to me.

Can you add tests please and also check that the Normal PhpArrayDumper dumps those correctly?

NW for #33/#35

Added a deprecation notice as per #33.
Still needs work for #35.

There are tests for the new service closure addition in #3108020: Support ServiceClosureArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterEventSubscribersPass.php
@@ -2,11 +2,18 @@
+@trigger_error('The ' . __NAMESPACE__ . '\RegisterEventSubscribersPass is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. Instead, use Symfony\Component\EventDispatcher\DependencyInjection\RegisterListenersPass. See https://www.drupal.org/project/drupal/issues/2909185', E_USER_DEPRECATED);
...
+ * @deprecated in drupal:9.4.0 and is removed from drupal:10.0.0.
+ * Use \Symfony\Component\EventDispatcher\DependencyInjection\RegisterListenersPass.

needs update

Also needs reroll following #3108020: Support ServiceClosureArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue, which added the new feature to the container plus tests (so I don't think we need additional tests here any more)

Filed CR https://www.drupal.org/node/3376090

Patch also deprecating ContainerAwareEventDispatcher

Fix CS

This is mentioned above, but https://github.com/symfony/symfony/pull/23008 was the eventual commit that hopefully made this obsolete in Symfony.

#1972300: Write a more scalable dispatcher has quite a lot of performance numbers.

We might want to port https://www.drupal.org/sandbox/znerol/2345951 to Drupal 10 and run the same thing again.

We might want to port https://www.drupal.org/sandbox/znerol/2345951 to Drupal 10 and run the same thing again.

I might have ported that already. And I'm still using the same machine, so I'm going to try and reproduce the numbers.

I see the RegisterListenersPass has hard-coded tag names like 'kernel.event_listener' and 'kernel.event_subscriber'.
This seems like a blocker to me. Am I wrong?

The faster instantiation is not resolved, and this is not currently possible to override; the $listeners property is private in the Symfony EventDispatcher and there is no way to inject a precompiled set of listeners. Perhaps Symfony would be amenable to addressing this upstream.

A first idea is that this does not scale well with many events and listeners.

Consider a project with 100 different event types, with 10 listeners each.
To load the event dispatcher, we need 1000 times of resolving a service argument, creating the service closure, and calling ->addListener().
This happens on every request that needs the event dispatcher, even if only one event is fired, or perhaps no event at all.

We do _not_ load all the services, which is good. But still there is an overhead that grows with the number of total registered listeners.
It seems the overhead should be proportional to the total number of listeners.

needs profiling

Yes!
So, i applied the patch, and did some hacking to make it _somewhat_ work:
It still breaks because there is an object parameter that cannot be compiled.
But we can make measurements before the compilation.
So:
- I changed the tag name 'kernel.event_subscriber' to just 'event_subscriber' in RegisterListenersPass in symfony.
- I added a $container->get('event_dispatcher'); in DrupalKernel, after the container is compiled.
- I added microtime(TRUE) in various places.
- with xdebug enabled, I run "drush cr" to inspect the number of calls in the event_dispatcher service.
- with xdebug fully disabled (phpdismod), I run "drush cr" and other commands for the actual profiling.
- I profile with and without the patch.

I see 121 calls to ->addListener() in the definition for 'event_dispatcher'.
This means there are 121 event listener methods in core. (this is the actual value for the placeholder number 1000 from above)
In a real-world project, this number could grow quite a lot. But would it grow significantly > 1000?

Profiling results:

Without the patch, drush uli (as good as any other command or request), Container->get('event_dispatcher'):

• Container->get('event_dispatcher') uses ~0.49 milliseconds.
• Biggest part is including the actual class file in Container->createService(), which consumes ~0.47 milliseconds.
  (Maybe my opcache is misconfigured?)
• Resolving the arguments takes ~0.002 milliseconds.

Without the patch, drush uli (as good as any other command or request), Container->get('event_dispatcher'), but with the class file pre-loaded:

• Container->get('event_dispatcher') uses ~0.007 milliseconds.

Without the patch, in drush cr after container is compiled, ContainerBuilder->get('event_dispatcher'):

• ContainerBuilder->get('event_dispatcher') uses ~0.51 milliseconds.
• The biggest part is for resolving arguments in ContainerBuilder->createService().
  This could actually be sped up, because we don't need the container to go through all the ids, we just want the unchanged array value.
  But, it does not really matter, because normally we don't deal with ContainerBuilder but with Container.
• Loading the actual class file for EventDispatcher.php takes no time, because with drush cr, that class is already loaded.

With the patch, in drush cr, after container is compiled, ContainerBuilder->get('event_dispatcher'):

• ContainerBuilder->get('event_dipatcher') uses ~0.84 milliseconds.
• In ContainerBuilder->createService(), the biggest part is 121 calls to EventDispatcher->addListener(), which takes ~0.78 milliseconds.
  EDIT: With all core modules enabled, it would probably be more.
• Loading the actual class file for EventDispatcher.php takes no time, because with drush cr, that class is already loaded.
• Unfortunately, we can only measure ContainerBuilder, not the real Container, unless we fix the compilation.

So this means, with ContainerBuilder, with the patch, there is a cost proportional to the number of listeners.
It seems that ~150 listeners cost 1 millisecond.
Far from dramatic, but still worth avoiding.

How would this number look like in the real container?
It might be smaller, but it would still grow proportionally with the number of listeners, because we still need to call ->addListener() that many times.

The above only profiles getting the event dispatcher from the container.
This is relevant on requests that only fire a few events, which might be the majority of all requests.

I don't think it makes sense to profile total page load time, the difference would disappear in the margin of error.

However, if we do want to profile page load, we could pump up the subscribers/listeners to a significant number. E.g. 150.000 listeners would mean ~1 second to get the dispatcher, and this would definitely make a difference in the page load time.
In the same spirit, we can fire a high number of different events, or fire a single event a high number of times, and compare page load time with and without.
But for all of this, we should fix the compiler first, so that we profile the real Container and not ContainerBuilder.

In a real-world project, this number could grow quite a lot. But would it grow significantly > 1000?

When we originally added this, hooks to events was still in contention, but I think it almost entirely out of contention now with hux-style hooks, we might even be able to convert some core events (back) to hooks once that lands. So I think it's probably fine to assume that we'll have core subscribers to Symfony events, and some contrib subscribers to Symfony events, and some of the existing core/contrib events for a while, but not a big expansion from where we are.

To add to my previous post with the profiling:
I did not have all the core modules enabled when i ran this test. So the number 121 would be higher.

I now tested with a real-world project where I get 245 listeners/subscribers.

There might be some contrib modules that make excessive use of events.
For a regular project we might lose one or two milliseconds if we go with symfony EvenDispatcher, but it would also depend on the infrastructure.

My opinion: If we can make the ContainerAwareEventDispatcher do what we need, let's continue working with that. At least in #3376163: [PP-1] Support #[AsEventListener] attribute on classes and methods I will see what we can do with ContainerAwareEventDispatcher.

@donquixote is that just with container builder or also with the compiled container?

@donquixote is that just with container builder or also with the compiled container?

The profiling was done with ContainerBuilder, because I was not able to make it work with the compiled container.
But the calls to ->addListener() won't go away with the compiled container.
So there is still going to be a linear cost factor with the number of subscribers.

But actually I should be able to make it work with the compiled container!
I will give it a try now.

Actually, now with the compiled container, I get ~0.12 milliseconds for the 121 calls.
This is much better, at the point where we no longer have to worry about it.

It works now thanks to #3108020: Support ServiceClosureArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue.
But we still need to change 'kernel.event_subscriber' to 'event_subscriber' in RegisterListenersPass, unless we want to rename the tag everywhere else.

More soon.

But we still need to change 'kernel.event_subscriber' to 'event_subscriber' in RegisterListenersPass, unless we want to rename the tag everywhere else.

The ability to configure the tags used by the compiler pass was removed in https://github.com/symfony/symfony/pull/41937 and the reasoning listed in https://github.com/symfony/symfony/pull/40468 - "nobody uses this possibility anyway"

However the suggestion there is a good workaround for us, without having to copy/paste the entire class: we add another pass that either renames or adds kernel.event_subscriber to all services that are tagged with event_subscriber.

However the suggestion there is a good workaround for us, without having to copy/paste the entire class: we add another pass that either renames or adds kernel.event_subscriber to all services that are tagged with event_subscriber.

Good point.

Somehow it fails, maybe because core is not using tags or pass should come later

The related CR https://www.drupal.org/node/3357408

PHP Fatal error:  Uncaught Error: Cannot use object of type Symfony\Component\DependencyInjection\ChildDefinition as array in /var/www/html/core/lib/Drupal/Core/DependencyInjection/Compiler/RenameTagsPass.php:44
Stack trace:
#0 /var/www/html/vendor/symfony/dependency-injection/Compiler/Compiler.php(80): Drupal\Core\DependencyInjection\Compiler\RenameTagsPass->process(Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#1 /var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php(767): Symfony\Component\DependencyInjection\Compiler\Compiler->compile(Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#2 /var/www/html/core/lib/Drupal/Core/DrupalKernel.php(1335): Symfony\Component\DependencyInjection\ContainerBuilder->compile()

It looks more readable

I was looking for a ->deleteTag() :)
So ->clearTag() it shall be.

Somehow it fails, maybe because core is not using tags or pass should come later

It was a mistake.

Simplified the history.
This reveals that most of the required changes were already covered by the patch in #45.

The branch contains a commit that introduces profiling statements, which is later reverted.
I am curious about profiling results from other people.

I think we need a kernel test for event subscribers / listeners.

We have ContainerAwareEventDispatcherTest, but this only tests the class itself, not how it is used.
Event subscribers are covered indirectly by other tests, but we should have a dedicated test for this.

Test run took 45mins, which means no slowdown overall

Gonna profile tomorrow, but I bet if any regressions appear the whole test-suite exposed it

Looking at https://git.drupalcode.org/project/drupal/-/commit/553666589ad34a1b80bf7...

We can deprecate the event_subscriber tag and use autoconfiguraton with proper tag (kernel.event_subscriber) for SF dispatcher to minimize amount of renames

We can deprecate the event_subscriber tag and use autoconfiguraton with proper tag (kernel.event_subscriber) for SF dispatcher to minimize amount of renames

How do we deprecate a tag?

If we rename all the existing tags (which goes beyond #64), we should probably add a test to prove that the old tag still works.

Test run took 45mins, which means no slowdown overall

I think we should expect that the performance impact will be small compared to an average page load, and very small compared to other operations that happen during a test run.
The most relevant impact might be for very simple requests that are otherwise very fast, where one millisecond could make a measurable difference.
For a feature like this, the usual margin of error for page load time is higher than the difference we want to measure and optimize.

We should add a kernel test for event subscribers and listeners, and to cover the old tag.

It was initially renamed in #1824400: Rename the service tag 'kernel.event_subscriber' to just 'event_subscriber'

OTOH we can use to check for EventSubscriberInterface of service and #3376163: [PP-1] Support #[AsEventListener] attribute on classes and methods

Yes.
But we still need to decide what to do if we encounter the old tag:

1. Just rename the tag, and don't show a deprecation notice.
2. Show a deprecation notice and recommend the new tag.
3. Show a deprecation notice and recommend to use either EventSubscriberInterface with autoconfiguration, or the new tag.

And if we show a deprecation notice, we have to decide how and when it should be displayed.
I would prefer to have it only once on or after container rebuild, not on every request.
I also would want it to be in Drupal's log system.

I tried to call trigger_error(.., E_USER_DEPRECATED) during container rebuild, but on drush cr I saw nothing. Maybe wrong config in php? I think it is because we change the error handler during container rebuild. And anyway we should assume that messages from container rebuild don't reach the usual logger, because the logger requires a working container.

Other instances of deprecated service or deprecated tag produce a E_USER_DEPRECATED message every time the respective service is instantiated, not just once on container rebuild. This seems too much - or is it not?

If we want to trigger the E_USER_DEPRECATED after container rebuild, but not repeat it, we could set a container parameter with a collection of messages, and then have a mechanism that will log these messages once after a container rebuild. Then something in state to make sure it happens only once. But this seems too much for the scope of this issue.

It was initially renamed in #1824400: Rename the service tag 'kernel.event_subscriber' to just 'event_subscriber'

I do like the motivation for the rename.
But now I guess we have to go back to the old tag name.

OR we continue fully supporting the old tag name, without deprecation, but we recommend autoconfigure with the EventSubscriberInterface.

Just keep in mind, autoconfigure does not remove the tag, it only removes the need to explicitly add it to a service definition.

Thinking about this again..
I don't think we should deprecate the 'event_dispatcher' tag name.

It will cause lots of disruption for little gain.
Imagine all the issues and tickets that will be opened in contrib and in client projects.
The cost of renaming only happens on cache rebuild, and will vanish with autoconfigure.

Contrib and custom code can slowly move to autoconfigure. Until then they just use the old tag name.

Perhaps in the future, symfony will re-add the option to use a different tag name instead of 'kernel.event_subscriber'.

(To be clear: I am talking about the tag name here, _not_ about the ContainerAwareEventDispatcher class!)

New issue: #3379488: Add kernel tests for event system
Let's add these tests _before_ we do this big change.

Actually, in https://github.com/symfony/symfony/pull/40468, Nicolas suggests to add a decorator instead of a separate pass.

A decorator would allow to only temporarily rename the tag, and then rename it back.
This would take more time during container rebuild, but would be more clean, in that we would only support the existing 'event_subscriber' tag.

With the current MR with the RenameTagPass, we run into the following "problems":

• We allow to use 'kernel.event_subscriber' and 'event_subscriber' interchangeably. This means if we ever want to go back to only supporting 'event_subscriber', we now have to add BC support 'kernel.event_subscriber'.
• Compiler passes that run _after_ the event subscriber pass and look for services tagged with 'event_subscriber' will find nothing. I don't know _why_ there would be any compiler pass like this, but what to I know.

Using a decorator to rename and then revert would solve these two problems, but at a cost of (a bit) slower rebuild.

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

Rebased against 11.x

I am torn between whether changing the tag name is a good idea. On one hand it makes it easier for Symfony developers to use Drupal as it's the tag they already know. But on the other hand it is a lot of busy work for Drupal developers if we deprecate the existing tag just to add kernel. in front of it; I also agree with the reasoning in #1824400: Rename the service tag 'kernel.event_subscriber' to just 'event_subscriber' for renaming it in the first place. It also seems like scope creep from the original issue where all we wanted to do was replace the dispatcher.

Perhaps the simpler solution is take inspiration from grumphp who had the same issue, and borrow their decorator code, which looks fairly trivial to implement: https://github.com/phpro/grumphp/blob/v2.x/src/Configuration/Compiler/Re...

Postponed #3376163: [PP-1] Support #[AsEventListener] attribute on classes and methods on this issue.

Retitled this issue with proper name of class.

MR!6657 starts again from #45 and borrows the GrumPHP approach of decorating the Symfony compiler pass to temporarily rename the tag. It also renames it back again afterwards, just in case.

I think an IS update would help lesser gods, certainly like myself, to figure out what and why we're doing here.

Updated the IS. I don't think we need any additional testing but profiling the difference in instantiation time when a large number of subscribers are in use would be helpful.

Is profiling still required?

The comments from #26, #46, #47 are still valid. If @znerol still has the script that was mentioned that would help here, otherwise we need to port the script or write something new.

Leaving at NR in the meantime to try and get more eyes on this.

Except profiling it probably needs empty update hook to make sure container rebuild for 10.3

For minor-only things we've been skipping empty update hooks because there's always at least one non-empty update in a minor release, 10.3 has quite a lot already.

Used znerol's script and I see no difference in before and after on standard profile

/var/www/html/web $ for i in $(seq 50); do php modules/znerol-2345951/scripts/test.php >>ed-after; done

missed to enable the module, but there's no viable diff

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.

but there's no viable diff

So is this something to still pursue?

I think this is still worth pursuing, plus it will unlock further possible improvements such as #3432805: Make event subscribers private by default which will let us inline most subscribers as noted in #5.

Personally I think if there is a performance issue with the way listeners are injected, it would have been resolved upstream already, and so far we haven't figured out a measurable difference between the two dispatchers.

Tried to do some profiling in xhprof on an install of the standard profile and I don't think there's much in it at all. Symfony has split the work across more methods but we can still compare them.

After cache rebuild, first page load:

Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch: 12 calls, 619µs

vs

Symfony\Component\EventDispatcher\EventDispatcher::dispatch: 12 calls, 57µs
Symfony\Component\EventDispatcher\EventDispatcher::optimizeListeners: 8 calls, 97µs
Symfony\Component\EventDispatcher\EventDispatcher::callListeners: 11 calls, 452µs
Total: 606µs

Second page load:

Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch: 9 calls, 583µs

vs

Symfony\Component\EventDispatcher\EventDispatcher::dispatch: 9 calls, 46µs
Symfony\Component\EventDispatcher\EventDispatcher::optimizeListeners: 5 calls, 61µs
Symfony\Component\EventDispatcher\EventDispatcher::callListeners: 9 calls, 426µs
Total: 533µs

Invoking the actual service is done differently in both - inside a closure in Symfony, calling Container::get() then call_user_func() from dispatch() in Drupal but as far as I can see these look comparable as well.

In terms of initially constructing the event dispatcher there is a difference: Symfony has 116 calls to Symfony\Component\EventDispatcher\EventDispatcher::addListener() which take 76µs (first run) or 53µs (second run), these are not required with the Drupal dispatcher because we send the prebuilt set of listeners to the constructor. The constructors themselves take 1-2µs. But as per the above it seems that the Symfony dispatcher is slightly faster anyway which negates some of this difference.

Overall I don't think this is worth worrying about - it might be "an order of magnitude" slower to instantiate the dispatcher but we are talking about less than 100 microseconds here, and the Symfony dispatcher appears slightly faster anyway, which on a setup with more calls might mean Symfony actually has an advantage here.

CR outdated, and I think it needs examples from conversions

Updated CR: https://www.drupal.org/node/3376090

Not sure what else there is to say, this shouldn't affect anyone unless they were replacing the dispatcher for some reason, which seems an unlikely thing to do?

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.

Rebased.

Thank you!

If we end up doing #3366083: [META] Hooks via attributes on service methods (hux style) and go the 'hook as event' route there, then we'll have a lot more registered listeners and it's possible Symfony\Component\EventDispatcher\EventDispatcher::addListener() might turn into an issue after all, but... I think that's something we can look at if and when we get there.

Committed 9dbd2a6 and pushed to 11.x. Thanks!

We need a 10.3.x version of the MR as this does not apply cleanly to RegisterEventSubscribersPass

The 10.3.x version looks good.

Committed 6e73386 and pushed to 10.3.x. Thanks!