events: optimize adding and removing of listeners more #785
Conversation
This keeps in line with how things are done for the fast path and *might* even provide a *slight* performance increase.
It seems to offer a significant improvement to more than just ee-add-remove. The ee-listener-count and ee-listeners-many seem to vary a little bit though. |
|
Huh... I thought I checked the other benchmarks also... |
|
@evanlucas You must be testing against a version of iojs that is pre-b677b844fc1. I tested relative to the current head of v1.x and ee-add-remove is the only one that really shows an improvement. |
|
Ah, I had thought for some reason I was up to date on v1.x. Perhaps not. :] |
|
|
@Fishrock123 Yeah |
| m = $getMaxListeners(this); | ||
| if (m && m > 0 && existing.length > m) { | ||
| existing.warned = true; | ||
| console.error('(node) warning: possible EventEmitter memory ' + |
There was a problem hiding this comment.
Should this be io.js instead of node now?
There was a problem hiding this comment.
shrug I didn't change anything in that whole block, just wrapped it all within an else {}.
mscdex
force-pushed
the
perf-events-part2
branch
from
39599f9
to
9763ffc
Compare
|
Code updated with suggested changes. |
| else | ||
| return false; | ||
| } else | ||
| doError = (type === 'error' && !events.error); |
There was a problem hiding this comment.
This can be made a little flatter and more readable by swapping the consequent and the alternate. Suggestion:
if (events)
doError = (type === 'error' && events.error != null);
else if (type === 'error')
doError = true;
else
return false;You can condense it but I'll leave it up to you to decide what's more readable.
doError = (type === 'error');
if (events)
doError = (doError && events.error == null);
else if (!doError)
return false;These optimizations result in >2x speedup in the ee-add-remove benchmark: * Don't mutate array.length when removing the last listener for an event * Don't bother checking max listeners if listeners isn't an array * Don't call delete when removing the last event in _events, just re-assign a new object instead
mscdex
force-pushed
the
perf-events-part2
branch
from
9763ffc
to
a29f3d3
Compare
|
Updated again with suggested changes. |
This keeps in line with how things are done for the fast path and *might* even provide a *slight* performance increase. PR-URL: #785 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com>
These optimizations result in >2x speedup in the ee-add-remove benchmark: * Don't mutate array.length when removing the last listener for an event * Don't bother checking max listeners if listeners isn't an array * Don't call delete when removing the last event in _events, just re-assign a new object instead PR-URL: #785 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com>
|
Thanks Brian, landed in 630f636...7061669. |
|
// This is a brutally ugly hack to make sure that our error handler
// is attached before any userland ones. NEVER DO THIS.
if (!dest._events || !dest._events.error)
dest.on('error', onerror);
else if (Array.isArray(dest._events.error))
dest._events.error.unshift(onerror);
else
dest._events.error = [onerror, dest._events.error];Edit: Sorry, I was wrong, it's fine =). |
These optimizations result in >2x speedup in the ee-add-remove
benchmark:
an event
re-assign a new object instead