I think I left the order of methods as they were before:

master:
public function __construct(ContainerInterface $container)
public function addListenerService($eventName, $callback, $priority = 0)
public function removeListener($eventName, $listener)
public function hasListeners($eventName = null)
public function getListeners($eventName = null)
public function addSubscriberService($serviceId, $class)
public function dispatch($eventName, Event $event = null)
public function getContainer()
protected function lazyLoad($eventName)

modified:
public function __construct(ContainerInterface $container)
public function addListenerService($eventName, $callback, $priority = 0)
public function removeListener($eventName, $listener)
public function addSubscriberService($serviceId, $class)
public function getContainer()

The patience algorithm yields a more readable patch: git diff master --patience

Ok, fixed the coding style issues and also prevent removeListener() from instantiating services if it is not necessary.

I think this will break the integration with the TraceableDispatcher (used for the profiler panel). All lazy listeners will not be displayed as being a closure instead of identifying the real listener, making the events panel and the timeline integration useless for them (quite all listeners are lazy-loaded in Symfony)

@stof do you think it is worth fixing #12007 in Symfony or is this only relevant for Drupal?

Well, it might be worth improving this in Symfony itself, but it must be done in a way which does not break the debugging capabilities

From an architectural POV, do you think it would be legit to reassign the responsibility of providing information about the listeners to the actual event dispatcher? I.e. introduce a public method ContainerAwareEventManager::getListenerInfo().

Another option would be to introduce a protected method EventManager::getCallable($listener) which can be overridden from the container aware dispatcher and then use that in doDispatch to turn the listener into a callable.

Would it be possible to change the return value of EventDispatcherInterface::getListeners() from array to Traversable?

@znerol adding getListenerInfo() would not work, because it would be usable only if it is in the EventDispatcherInterface itself, and it cannot for BC reasons.

Maybe we could have a special invokable proxy in the EventDispatcher component, which would be used by the ContainerAwareDispatcher (rather than using a closure), and could be known by the TraceableDispatcher to describe the inner callable instead

So you are proposing something which is working like:

$callable = new LazyServiceMethodProxy($container, 'service.id', 'method_name');
$callable('some', 'params');

This looks like it could be useful outside of the event dispatcher component. Does something like this exist already?

@znerol given that it needs to live either in the DI or EventDispatcher components to be usable here, and that it is not that useful in DI IMO (using proxy for services themselves is much more likely to be useful as the case where you inject a callable built with a method on a service is not that common), I would keep it in EventDispatcher, alongside the ContainerAwareDispatcher.

and I would create an interface implemented by this class and supported by the TraceableDispatcher:

interface IntrospectableCallable
{
    /**
     * @return callable
     */ 
    public function getInnerCallable(); 
}

Did some wall-time benchmarking (gist) and the results indicate that this version of the ContainerAwareEventDispatcher has a slightly better runtime than the one in HEAD. This mostly affects subsequent invocations of an event handler. The performance improvement on the first call to dispatch is within the margin of error.

A repetition of the experiment in #12007 did result in the same performance improvement for the Drupal request event chain.

I also manually verified that the events are shown properly in the symfony debug toolbar.

your implementation also triggers the lazy-loading of the service when tracing events.

This is obvious, the code is very explicit about that fact.

But instead of providing an interface usable by other implementations of a lazy-loading dispatcher, it is tied to the Symfony container.

The LazyServiceListener is used internally by the ContainerAwareEventDispatcher and both of them depend on the Symfony ContainerInterface. It is impossible to swap out the container with another implementation and still continue to use the ContainerAwareEventDispatcher, but as far as I can tell this is not different in the master branch. Another implementation of the lazy-loading dispatcher operating on something different than the Symfony container would need to bring in its own helper class anyway.

It indeed seems odd that the TraceableEventDispatcher has knowledge about the LazyServiceListener. But let me stress that also the getInnerCallable() method would only ever be useful in the context of debugging. Therefore if we'd implement the latter, we'd end up with debug-code in production with potential negative side-effects.

@znerol the goal of making TraceableEventDispatcher depend on an interface rather than the LazyServiceListener is that could be possible to have a different lazy dispatcher using its own implementation of this interface and beign supported by the TraceableEventDispatcher directly, without having to edit it. Your current implementation would force replacing the TraceableEventDispatcher if you bring your own lazy-loading dispatcher (or getting useless tracing).

And I'm not saying that the IntrospectableListener should be introspected for something else than debugging. But having the interface for it would be cleaner (and it would avoid having to make TraceableEventDispatcher aware of the way LazyServiceListener makes the listener lazy, which is the case currently).

@stof I'd like to understand the real-world consequences of this issue and I'm now trying to find alternative event dispatcher implementations which may be affected by this change. If you know of any, please link them here.

@znerol I don't know of any such implementation, but someone could do one in the future (at some point, there was a PR to make such a dispatcher implementation based on Pimple for Silex, but it has not been merged). Having a design allowing such extension in the component will be great (and it does not have any drawback compared to your current implementation supporting the LazyServiceListener in the TraceableEventListener, while making the code cleaner, so this would be a win)

Ok, this is the situation: I pointed out the drawback of @stof suggestion and he rightfully criticizes the ugliness in TraceableEventListener which this patch introduces. I assume there is enough information in the comments for someone else to chip in and express his/hers opinion such that we can eventually decide on how to proceed.

To summarize: In my point of view introducing a public LazyServiceListener::getInnerCallable() is unacceptable because such a method would have exactly the side-effect I'm trying to eliminate as part of this patch, i.e. potentially preliminary service instantiation. Even more so because its only purpose would be to simplify debug code and maintain automatic compatibility for potential third-party lazy dispatcher implementations which are not using the Symfony container but something different and at the same time desire to use the TraceableEventListener.

I don't see why introducing LazyServiceListener::getInnerCallable() could be unacceptable. When will be used this method? maybe internally in the __invoke() method and in the TraceableEventDispatcher (so during debugging) nowhere else, right? Off course you can use it yourself... but does it really matters?

Moreover if we don't, we a patch in the TraceableEventDispatcher that needs a specific comment to explain the meaning of what we can consider as a hack.
By the way I don't like the new strong dependency to the Symfony Container but I can live with it. But, for me, it's part of the LazyServiceListener class logic and that's why it looks weird in the TraceableEventDispatcher and needs a specific comment.

I discovered another aspect about this change which should be taken into account when deciding on how to proceed. With the patch the addListenerService() and addSubscriberService() call gets heavier than before because it now needs to instantiate a helper class. My benchmark shows that the time to construct the event dispatcher service is roughly doubled by this patch.

@znerol The question is: is it significant? I mean, double time seems like a lot, but if we are talking about a very small number, it might be insignificant.

In my point of view introducing a public LazyServiceListener::getInnerCallable() is unacceptable because such a method would have exactly the side-effect I'm trying to eliminate as part of this patch, i.e. potentially preliminary service instantiation.

Your own implementation of the TraceableDispatcher integration also suffers from this side-effect. My proposal does not introduce the side-effect. It makes the existing behavior of this PR cleaner (and more powerful for other integrations).
The fact that calling getInnerCallable triggers the lazy-loading of the listener and so should be avoided in production code whenever possible is something which should go in the documentation of the method.

@fabpot Depends on the number of subscribers. With a growing number even the time necessary to construct the service in the current implementation can grow rather big. I've benchmarked that and compared it to a method where all service ids would be simply injectied as an array into the constructor of the event dispatcher. The performance gain is impressive, however it suffers from the same problems outlined by @stof in #12069. Therefore it is likely that Drupal will use a custom implementation of the ContainerAwareEventDispatcher

It is possible that the benchmark is flawed, it has not been confirmed by anybody yet.

I close this, turns out that Drupal will very likely get its own version of the event dispatcher. There is no point in solving the lazy loading issue here if that degrades service construction performance even more.

znerol, how is Drupal using its own dispatcher better than improving Symfony's? It sounds like fabpot is open to it. There's no need to close this PR, even if we did commit an alternate implementation to Drupal.

@znerol Can you elaborate. I'm more than willing to make things faster for everyone. Instead of creating your own event dispatcher, is't there a way to make things better for everyone? I thought we were close to an acceptable solution with this PR.

It looks like the main problem is the myriad of function calls executed upon instantiation of the event manager service. The benchmarks are over here: https://www.drupal.org/node/1972300#comment-9216069

@Nicofuma tried a similar optimization here #12069 but it turned out that this would violate the expectation that changed priorities should be reflected without the need of recompiling the container.

This pull request here is even worse when it comes to instantiation performance and hence we'd need to try something different anyway. Therefore let's take the discussion to #12007