Skip to content
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

EventEmitter: use a Map to store event listeners #1785

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions lib/_stream_readable.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ module.exports = Readable;
Readable.ReadableState = ReadableState;

const EE = require('events').EventEmitter;
const EEEvents = require('internal/symbols').EEEvents;
const Stream = require('stream');
const Buffer = require('buffer').Buffer;
const util = require('util');
Expand Down Expand Up @@ -542,12 +543,13 @@ Readable.prototype.pipe = function(dest, pipeOpts) {
}
// 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)
const events = dest[EEEvents];
if (!events || !events.has('error'))
dest.on('error', onerror);
else if (Array.isArray(dest._events.error))
dest._events.error.unshift(onerror);
else if (Array.isArray(events.get('error')))
events.get('error').unshift(onerror);
else
dest._events.error = [onerror, dest._events.error];
events.set('error', [onerror, events.get('error')]);


// Both close and finish should trigger unpipe, but only once.
Expand Down
106 changes: 52 additions & 54 deletions lib/events.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

const EEEvents = require('internal/symbols').EEEvents;

var domain;

function EventEmitter() {
Expand All @@ -13,7 +15,7 @@ EventEmitter.EventEmitter = EventEmitter;
EventEmitter.usingDomains = false;

EventEmitter.prototype.domain = undefined;
EventEmitter.prototype._events = undefined;
EventEmitter.prototype[EEEvents] = undefined;
EventEmitter.prototype._maxListeners = undefined;

// By default EventEmitters will print a warning if more than 10 listeners are
Expand All @@ -30,10 +32,9 @@ EventEmitter.init = function() {
}
}

if (!this._events || this._events === Object.getPrototypeOf(this)._events) {
this._events = {};
this._eventsCount = 0;
}
if (!this[EEEvents] ||
this[EEEvents] === Object.getPrototypeOf(this)[EEEvents])
this[EEEvents] = new Map();

this._maxListeners = this._maxListeners || undefined;
};
Expand Down Expand Up @@ -119,9 +120,9 @@ EventEmitter.prototype.emit = function emit(type) {
var needDomainExit = false;
var doError = (type === 'error');

events = this._events;
events = this[EEEvents];
if (events)
doError = (doError && events.error == null);
doError = (doError && !events.has('error'));
else if (!doError)
return false;

Expand All @@ -148,7 +149,7 @@ EventEmitter.prototype.emit = function emit(type) {
return false;
}

handler = events[type];
handler = events.get(type);

if (!handler)
return false;
Expand Down Expand Up @@ -196,32 +197,31 @@ EventEmitter.prototype.addListener = function addListener(type, listener) {
if (typeof listener !== 'function')
throw new TypeError('listener must be a function');

events = this._events;
events = this[EEEvents];
if (!events) {
events = this._events = {};
this._eventsCount = 0;
events = this[EEEvents] = new Map();
} else {
// To avoid recursion in the case that type === "newListener"! Before
// adding it to the listeners, first emit "newListener".
if (events.newListener) {
if (events.has('newListener')) {
this.emit('newListener', type,
listener.listener ? listener.listener : listener);

// Re-assign `events` because a newListener handler could have caused the
// this._events to be assigned to a new object
events = this._events;
// this[EEEvents] to be assigned to a new object
events = this[EEEvents];
}
existing = events[type];
existing = events.get(type);
}

if (!existing) {
// Optimize the case of one listener. Don't need the extra array object.
existing = events[type] = listener;
++this._eventsCount;
events.set(type, listener);
} else {
if (typeof existing === 'function') {
// Adding the second element, need to change to array.
existing = events[type] = [existing, listener];
existing = [existing, listener];
events.set(type, existing);
} else {
// If we've already got an array, just append.
existing.push(listener);
Expand Down Expand Up @@ -267,30 +267,26 @@ EventEmitter.prototype.once = function once(type, listener) {
return this;
};

// emits a 'removeListener' event iff the listener was removed
// emits a 'removeListener' event if the listener was removed
EventEmitter.prototype.removeListener =
function removeListener(type, listener) {
var list, events, position, i;

if (typeof listener !== 'function')
throw new TypeError('listener must be a function');

events = this._events;
events = this[EEEvents];
if (!events)
return this;

list = events[type];
list = events.get(type);
if (!list)
return this;

if (list === listener || (list.listener && list.listener === listener)) {
if (--this._eventsCount === 0)
this._events = {};
else {
delete events[type];
if (events.removeListener)
this.emit('removeListener', type, listener);
}
events.delete(type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about performance differences with this particular change. Is it faster to keep _eventsCount around and create a new Map()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no significant performance difference if I revert ca75072. I tested with this._events.clear() and this._events = new Map()

if (events.has('removeListener'))
this.emit('removeListener', type, listener);
} else if (typeof list !== 'function') {
position = -1;

Expand All @@ -307,17 +303,12 @@ EventEmitter.prototype.removeListener =

if (list.length === 1) {
list[0] = undefined;
if (--this._eventsCount === 0) {
this._events = {};
return this;
} else {
delete events[type];
}
events.delete(type);
} else {
spliceOne(list, position);
}

if (events.removeListener)
if (events.has('removeListener'))
this.emit('removeListener', type, listener);
}

Expand All @@ -328,39 +319,32 @@ EventEmitter.prototype.removeAllListeners =
function removeAllListeners(type) {
var listeners, events;

events = this._events;
events = this[EEEvents];
if (!events)
return this;

// not listening for removeListener, no need to emit
if (!events.removeListener) {
if (!events.has('removeListener')) {
if (arguments.length === 0) {
this._events = {};
this._eventsCount = 0;
} else if (events[type]) {
if (--this._eventsCount === 0)
this._events = {};
else
delete events[type];
events.clear();
} else {
events.delete(type);
}
return this;
}

// 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) {
key = keys[i];
for (var key of events.keys()) {
if (key === 'removeListener') continue;
this.removeAllListeners(key);
}
this.removeAllListeners('removeListener');
this._events = {};
this._eventsCount = 0;
events.clear();
return this;
}

listeners = events[type];
listeners = events.get(type);

if (typeof listeners === 'function') {
this.removeListener(type, listeners);
Expand All @@ -377,12 +361,12 @@ EventEmitter.prototype.removeAllListeners =
EventEmitter.prototype.listeners = function listeners(type) {
var evlistener;
var ret;
var events = this._events;
var events = this[EEEvents];

if (!events)
ret = [];
else {
evlistener = events[type];
evlistener = events.get(type);
if (!evlistener)
ret = [];
else if (typeof evlistener === 'function')
Expand All @@ -399,10 +383,10 @@ EventEmitter.listenerCount = function(emitter, type) {
};

EventEmitter.prototype.listenerCount = function listenerCount(type) {
const events = this._events;
const events = this[EEEvents];

if (events) {
const evlistener = events[type];
const evlistener = events.get(type);

if (typeof evlistener === 'function') {
return 1;
Expand All @@ -414,6 +398,20 @@ EventEmitter.prototype.listenerCount = function listenerCount(type) {
return 0;
};

Object.defineProperty(EventEmitter.prototype, '_events', {
get() {
const thisEvents = this[EEEvents];
if (!thisEvents)
return;

const events = {};
for (var event of thisEvents) {
events[event[0]] = event[1];
}
return events;
}
});

// About 1.5x faster than the two-arg version of Array#splice().
function spliceOne(list, index) {
for (var i = index, k = i + 1, n = list.length; k < n; i += 1, k += 1)
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/symbols.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
'use strict';

exports.EEEvents = Symbol('EventEmitter events');
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
'lib/internal/child_process.js',
'lib/internal/freelist.js',
'lib/internal/socket_list.js',
'lib/internal/symbols.js',
'lib/internal/repl.js',
'lib/internal/util.js',
],
Expand Down
1 change: 0 additions & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ namespace node {
V(env_string, "env") \
V(errno_string, "errno") \
V(error_string, "error") \
V(events_string, "_events") \
V(exec_argv_string, "execArgv") \
V(exec_path_string, "execPath") \
V(exiting_string, "_exiting") \
Expand Down
3 changes: 0 additions & 3 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2925,9 +2925,6 @@ void SetupProcessObject(Environment* env,
env->SetMethod(process, "_setupNextTick", SetupNextTick);
env->SetMethod(process, "_setupPromises", SetupPromises);
env->SetMethod(process, "_setupDomainUse", SetupDomainUse);

// pre-set _events object for faster emit checks
process->Set(env->events_string(), Object::New(env->isolate()));
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? Isn't there a way to create a Map from C++?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. I couldn't find anything related to maps in the v8 API and I know that the Map constructor is written in JS but there may be a way to call it from C++

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the NativeWeakMap but I can't find the regular Map either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be possible with v8 4.5: v8/v8@395fa8b

Expand Down
30 changes: 16 additions & 14 deletions test/parallel/test-event-emitter-check-listener-leaks.js
Original file line number Diff line number Diff line change
@@ -1,40 +1,42 @@
'use strict';
// Flags: --expose-internals
var common = require('../common');
var assert = require('assert');
var events = require('events');
const EEEvents = require('internal/symbols').EEEvents;

var e = new events.EventEmitter();

// default
for (var i = 0; i < 10; i++) {
e.on('default', function() {});
}
assert.ok(!e._events['default'].hasOwnProperty('warned'));
assert.ok(!e[EEEvents].get('default').hasOwnProperty('warned'));
e.on('default', function() {});
assert.ok(e._events['default'].warned);
assert.ok(e[EEEvents].get('default').warned);

// specific
e.setMaxListeners(5);
for (var i = 0; i < 5; i++) {
e.on('specific', function() {});
}
assert.ok(!e._events['specific'].hasOwnProperty('warned'));
assert.ok(!e[EEEvents].get('specific').hasOwnProperty('warned'));
e.on('specific', function() {});
assert.ok(e._events['specific'].warned);
assert.ok(e[EEEvents].get('specific').warned);

// only one
e.setMaxListeners(1);
e.on('only one', function() {});
assert.ok(!e._events['only one'].hasOwnProperty('warned'));
assert.ok(!e[EEEvents].get('only one').hasOwnProperty('warned'));
e.on('only one', function() {});
assert.ok(e._events['only one'].hasOwnProperty('warned'));
assert.ok(e[EEEvents].get('only one').hasOwnProperty('warned'));

// unlimited
e.setMaxListeners(0);
for (var i = 0; i < 1000; i++) {
e.on('unlimited', function() {});
}
assert.ok(!e._events['unlimited'].hasOwnProperty('warned'));
assert.ok(!e[EEEvents].get('unlimited').hasOwnProperty('warned'));

// process-wide
events.EventEmitter.defaultMaxListeners = 42;
Expand All @@ -43,25 +45,25 @@ e = new events.EventEmitter();
for (var i = 0; i < 42; ++i) {
e.on('fortytwo', function() {});
}
assert.ok(!e._events['fortytwo'].hasOwnProperty('warned'));
assert.ok(!e[EEEvents].get('fortytwo').hasOwnProperty('warned'));
e.on('fortytwo', function() {});
assert.ok(e._events['fortytwo'].hasOwnProperty('warned'));
delete e._events['fortytwo'].warned;
assert.ok(e[EEEvents].get('fortytwo').hasOwnProperty('warned'));
delete e[EEEvents].get('fortytwo').warned;

events.EventEmitter.defaultMaxListeners = 44;
e.on('fortytwo', function() {});
assert.ok(!e._events['fortytwo'].hasOwnProperty('warned'));
assert.ok(!e[EEEvents].get('fortytwo').hasOwnProperty('warned'));
e.on('fortytwo', function() {});
assert.ok(e._events['fortytwo'].hasOwnProperty('warned'));
assert.ok(e[EEEvents].get('fortytwo').hasOwnProperty('warned'));

// but _maxListeners still has precedence over defaultMaxListeners
events.EventEmitter.defaultMaxListeners = 42;
e = new events.EventEmitter();
e.setMaxListeners(1);
e.on('uno', function() {});
assert.ok(!e._events['uno'].hasOwnProperty('warned'));
assert.ok(!e[EEEvents].get('uno').hasOwnProperty('warned'));
e.on('uno', function() {});
assert.ok(e._events['uno'].hasOwnProperty('warned'));
assert.ok(e[EEEvents].get('uno').hasOwnProperty('warned'));

// chainable
assert.strictEqual(e, e.setMaxListeners(1));
Loading