Skip to content

Commit

Permalink
events: do not keep arrays with a single listener
Browse files Browse the repository at this point in the history
Use the remaining listener directly if the array of listeners has only
one element after running `EventEmitter.prototype.removeListener()`.

Advantages:

- Better memory usage and better performance if no new listeners are
  added for the same event.

Disadvantages:

- A new array must be created if new listeners are added for the same
  event.

PR-URL: nodejs#12043
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
  • Loading branch information
lpinca authored and jasnell committed Apr 4, 2017
1 parent f637703 commit 8d386ed
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 7 deletions.
18 changes: 11 additions & 7 deletions lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ EventEmitter.prototype.removeListener =
} else if (typeof list !== 'function') {
position = -1;

for (i = list.length; i-- > 0;) {
for (i = list.length - 1; i >= 0; i--) {
if (list[i] === listener || list[i].listener === listener) {
originalListener = list[i].listener;
position = i;
Expand All @@ -375,7 +375,6 @@ EventEmitter.prototype.removeListener =
return this;

if (list.length === 1) {
list[0] = undefined;
if (--this._eventsCount === 0) {
this._events = Object.create(null);
return this;
Expand All @@ -384,8 +383,12 @@ EventEmitter.prototype.removeListener =
}
} else if (position === 0) {
list.shift();
if (list.length === 1)
events[type] = list[0];
} else {
spliceOne(list, position);
if (list.length === 1)
events[type] = list[0];
}

if (events.removeListener)
Expand All @@ -397,7 +400,7 @@ EventEmitter.prototype.removeListener =

EventEmitter.prototype.removeAllListeners =
function removeAllListeners(type) {
var listeners, events;
var listeners, events, i;

events = this._events;
if (!events)
Expand All @@ -420,7 +423,8 @@ EventEmitter.prototype.removeAllListeners =
// emit removeListener for all listeners on all events
if (arguments.length === 0) {
var keys = Object.keys(events);
for (var i = 0, key; i < keys.length; ++i) {
var key;
for (i = 0; i < keys.length; ++i) {
key = keys[i];
if (key === 'removeListener') continue;
this.removeAllListeners(key);
Expand All @@ -437,9 +441,9 @@ EventEmitter.prototype.removeAllListeners =
this.removeListener(type, listeners);
} else if (listeners) {
// LIFO order
do {
this.removeListener(type, listeners[listeners.length - 1]);
} while (listeners[0]);
for (i = listeners.length - 1; i >= 0; i--) {
this.removeListener(type, listeners[i]);
}
}

return this;
Expand Down
17 changes: 17 additions & 0 deletions test/parallel/test-event-emitter-remove-listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,3 +157,20 @@ assert.throws(() => {
const e = ee.removeListener('foo', listener);
assert.strictEqual(e, ee);
}

{
const ee = new EventEmitter();

ee.on('foo', listener1);
ee.on('foo', listener2);
assert.deepStrictEqual(ee.listeners('foo'), [listener1, listener2]);

ee.removeListener('foo', listener1);
assert.strictEqual(ee._events.foo, listener2);

ee.on('foo', listener1);
assert.deepStrictEqual(ee.listeners('foo'), [listener2, listener1]);

ee.removeListener('foo', listener1);
assert.strictEqual(ee._events.foo, listener2);
}

0 comments on commit 8d386ed

Please sign in to comment.