-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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.
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
events: improve events performance #50422
Conversation
4c02bdb
to
bb44f6f
Compare
bb44f6f
to
249c2b8
Compare
This is probably going to break someone somewhere. That being said, maybe there is a way to minimize it by returning a Also, I'm surprised this is so much faster. Often the same events are used (e.g. for streams), so I would assume that V8 uses a shape rather than a map behind the scenes, which should be faster than a Map instance. @benjamingr wdyt? |
I'm actually not that interested in ee-listen-unique and ee-once. |
|
you can see the results when removing |
This is concerning:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neat but I don't understand how it's faster since objects in dictionary mode and maps have basically the same performance.
Also the "polyfill" for ._events
is incorrect since if someone sets a value on it it won't get reflected in the emitter.
What happens if you remove that hint? Sorry it's a bit unclear what your last benchmark results are from. |
I know I still have a todo there to convert to Proxy so it support everything
Maybe because it by default go to the dict mode?
the benchmarks are from the linked commit change
|
Looks great to me! |
Also what about something like the following for streams?
|
Do you think it's representative, I think the reason someone put |
I thought about that, but since I changed to Map I did not do it |
Eh, this makes one use case slower and another one faster (changing to no proto: null), making "EventEmitter events shape specific to common use cases for streams" could be worth it, with benchmarks |
Supersede by #50428 |
Benchmarks
Change
_events
to be aMap
:commit: 249c2b8
Notes:
EventEmitter
class functions (cc @ronag )Benchmark URL
I run it on streams as well to see the impact as this was my original intent: url
Change to
Map
todos:_events
and use[kEvents]
instead_events
so need to migrate as wellchange_events
to be without__proto__: null
change
_events
to be without__proto__: null
Commit: eb29e97
I think the performance without proto is not fully representative as the event name can be anything so the dictionary mode is not used here
Benchmark URL