events: optimize once() and removeListener()#64373
Conversation
Use a closure instead of a bound state object for the once() wrapper, keep the events object shape in removeListener() by storing undefined instead of deleting keys or reallocating the object, and outline the cold unhandled 'error' path from emit() and the max-listeners warning from _addListener() so both fit within V8 inlining budgets. ee-once +25%, ee-add-remove +12-22%, ee-listen-unique +7-15%, streams/pipe +13.9%. Signed-off-by: Matteo Collina <hello@matteocollina.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64373 +/- ##
==========================================
- Coverage 90.25% 90.24% -0.01%
==========================================
Files 741 741
Lines 240990 241196 +206
Branches 45402 45431 +29
==========================================
+ Hits 217502 217666 +164
- Misses 15066 15086 +20
- Partials 8422 8444 +22
🚀 New features to boost your workflow:
|
| configurable: true, | ||
| }); | ||
| } catch { | ||
| // Continue regardless of error. |
There was a problem hiding this comment.
Can you expand this to explain why we're ignoring the error?
There was a problem hiding this comment.
I know this isn't new but it's always bugged me that's it's not explained :-)
There was a problem hiding this comment.
In the lines above we are trying to expand the stacktrace. If we can't for whatever reasons, we don't hide the original error.
There was a problem hiding this comment.
@jasnell do you want me to update the comment?
|
Landed in 384d7a4 |
| // the `events` object in the same shape and avoids both a `delete` | ||
| // (which would put the object into dictionary mode) and allocating | ||
| // a fresh object when the last listener is removed. | ||
| events[type] = undefined; |
There was a problem hiding this comment.
This can cause memory leaks when using unstructured event names, I.e random ids etc...
Restore shape mode for preallocated EventEmitters while deleting dynamic names from ordinary EventEmitters. Keeping every removed name as an undefined property causes unbounded memory growth when event names are unstructured. Refs: nodejs#64373 Signed-off-by: Matteo Collina <hello@matteocollina.com>
Restore shape mode for preallocated EventEmitters while deleting dynamic names from ordinary EventEmitters. Keeping every removed name as an undefined property causes unbounded memory growth when event names are unstructured. Refs: #64373 Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #64475 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Optimize the EventEmitter hot paths:
once()wrapper as a closure instead ofbind()+ a state object_eventsobject shape inremoveListener(): storeundefinedinstead ofdelete-ing keys or reallocating the object (eventNames()now filters, matchingStream.prototype.eventNames); removes the now-unusedkShapeModeerrorpath fromemit()(439 → 278 bytecodes) and the max-listeners warning from_addListener()(405 → 288) so both fit within the V8 inlining budgetsbenchmark/compare.js --runs 30before/after all changes:events
streams