Skip to content

[EventDispatcher] Performance of TraceableEventDispatcher::getNotCalledListeners() #43658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
umulmrum opened this issue Oct 22, 2021 · 4 comments · Fixed by #45184
Closed

[EventDispatcher] Performance of TraceableEventDispatcher::getNotCalledListeners() #43658

umulmrum opened this issue Oct 22, 2021 · 4 comments · Fixed by #45184
Labels
EventDispatcher Help wanted Issues and PRs which are looking for volunteers to complete them. Performance WebProfilerBundle

Comments

@umulmrum
Copy link
Contributor

Description
I analyzed performance issues in my project in the dev environment when I saw that TraceableEventDispatcher::getNotCalledListeners() is using up quite a bit of CPU. This is because all event listeners are instantiated which means that their dependencies are also loaded which in my case ultimately leads to the instantiation of multiple Doctrine repositories and therefore lots of annotation loading (which will also be tracked by the profiler, leading to even worse performance). When removing the call to getNotCalledListeners(), cache usage drops by 75% and wall time by 0.4s/30% which is significant even if it is "only" the dev environment. There were ~100 uncalled listeners.

So my question is if others experience the same problem in a real-world project. If yes, I suggest to improve this, e.g. by doing one of the following:

  • Avoid instantiating all listeners/subscribers, as we are only interested in names here (although I don't have an idea on how to do this).
  • Add a possibility to opt out of this feature
  • Remove the feature. I'm not sure what the rationale behind it is. The profiler already shows called events, and when wondering why a specific event listener was not called, the command debug:event-dispatcher can tell if it is registered correctly. So I suppose it might be expendable, but could of course err.

Some time ago @javiereguiluz also asked to improve performance here, see #31970, but it wasn't pursued any further.

Thanks for considering (I'm glad to help on the easier solutions if requested)!
@xabbuh
Copy link
Member

xabbuh commented Oct 28, 2021

Could you create a small application that allows to actually work with something and trying different approaches to improve performance?

@xabbuh
Copy link
Member

xabbuh commented Nov 5, 2021

@umulmrum Do you have any news for us?

@umulmrum
Copy link
Contributor Author

umulmrum commented Nov 8, 2021

@xabbuh Unfortunately I haven't found the time yet - I hope to get started this week (but am also happy if someone else jumps in).

@nicolas-grekas
Copy link
Member

We could find a way to access EventDispatcher::$listeners from TraceableEventDispatcher. It could be by reflection to me since this is all in the same component. Then, when a listener in that list is a lazy callable (those \is_array($listener) && isset($listener[0]) && $listener[0] instanceof \Closure && 2 >= \count($listener) checks in EventDispatcher), we won't need to resolve that lazy callable. Instead, we know that it's not been called, and we just need a way to get it's name. Since the lazy callable is going to be hard to inspect (to be confirmed), we could compute that name at compile time and put it in $listener[2] (see previous sentence.)

I'm putting a Help Wanted label on this issue in case someone would like to give it a try. I'd be happy to review/help.

@nicolas-grekas nicolas-grekas added Help wanted Issues and PRs which are looking for volunteers to complete them. and removed Status: Waiting feedback labels Nov 10, 2021
@Jean-Beru Jean-Beru mentioned this issue Jan 26, 2022
Merged
nicolas-grekas added a commit that referenced this issue Feb 18, 2022
@nicolas-grekas
minor #45184 [DependencyInjection][EventDispatcher] Avoid instantiati…
2d723e1 
…ng not called listeners when tracing (Jean-Beru)

This PR was squashed before being merged into the 6.1 branch.

Discussion
----------

[DependencyInjection][EventDispatcher] Avoid instantiating not called listeners when tracing

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #43658
| License       | MIT
| Doc PR        |

This PR removes useless listeners instantiation when `TraceableEventDispatcher::getNotCalledListeners()` is called.

Even if this PR is in WIP, I created it to allow comments from `@nicolas`-grekas (and anyone interested in of course).

Commits
-------

d4e60e9 [DependencyInjection][EventDispatcher] Avoid instantiating not called listeners when tracing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
EventDispatcher Help wanted Issues and PRs which are looking for volunteers to complete them. Performance WebProfilerBundle
Projects
None yet
Milestone
No milestone
6 participants
@javiereguiluz @nicolas-grekas @stof @xabbuh @umulmrum @carsonbot