Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
events: improve events performance#50422
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
rluvaton commented Oct 27, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
4c02bdb to bb44f6fCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bb44f6f to 249c2b8Comparenodejs-github-bot commented Oct 27, 2023
ronag commented Oct 27, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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? |
ronag commented Oct 27, 2023
I'm actually not that interested in ee-listen-unique and ee-once. |
rluvaton commented Oct 27, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
|
rluvaton commented Oct 27, 2023
you can see the results when removing |
rluvaton commented Oct 27, 2023
This is concerning: |
benjamingr left a comment
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 ._eventsis incorrect since if someone sets a value on it it won't get reflected in the emitter.
ronag commented Oct 27, 2023
What happens if you remove that hint? Sorry it's a bit unclear what your last benchmark results are from. |
rluvaton commented Oct 27, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 |
ronag commented Oct 27, 2023
Looks great to me! |
ronag commented Oct 27, 2023
Also what about something like the following for streams? |
rluvaton commented Oct 27, 2023
Do you think it's representative, I think the reason someone put |
rluvaton commented Oct 27, 2023
I thought about that, but since I changed to Map I did not do it |
benjamingr commented Oct 27, 2023
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 |
lpinca commented Oct 27, 2023
rluvaton commented Oct 29, 2023
Supersede by #50428 |
Benchmarks
Change
_eventsto be aMap:commit: 249c2b8
Notes:
EventEmitterclass 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
Maptodos:_eventsand use[kEvents]instead_eventsso need to migrate as wellchange_eventsto be without__proto__: nullchange
_eventsto be without__proto__: nullCommit: 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