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.

Sign up for GitHub

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

events: several perf related adjustments #16212

Closed
wants to merge 6 commits into from
Closed

events: several perf related adjustments #16212

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
adabed3
events: stricter prop & variable checks for perf
apapirovski Oct 15, 2017
75571fb
events: remove unnecessary console instantiation
apapirovski Oct 15, 2017
00698a6
events: return values directly in listeners
apapirovski Oct 15, 2017
40a7e23
events: use spread function param in emit
apapirovski Oct 15, 2017
0585047
events: onceWrapper apply directly with arguments
apapirovski Oct 15, 2017
05c065f
!fixup events: properly check args.length
apapirovski Oct 20, 2017
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
109 changes: 46 additions & 63 deletions lib/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,6 @@ Object.defineProperty(EventEmitter, 'defaultMaxListeners', {
return defaultMaxListeners;
},
set: function(arg) {
// force global console to be compiled.
// see https://github.com/nodejs/node/issues/4467
console;
// check whether the input is a positive number (whose value is zero or
// greater and not a NaN).
if (typeof arg !== 'number' || arg < 0 || arg !== arg) {
Expand All @@ -77,7 +74,8 @@ EventEmitter.init = function() {
}
}

if (!this._events || this._events === Object.getPrototypeOf(this)._events) {
if (this._events === undefined ||
this._events === Object.getPrototypeOf(this)._events) {
this._events = Object.create(null);
this._eventsCount = 0;
}
Expand Down Expand Up @@ -163,24 +161,21 @@ function emitMany(handler, isFn, self, args) {
}
}

EventEmitter.prototype.emit = function emit(type) {
var er, handler, len, args, i, events, domain;
var needDomainExit = false;
var doError = (type === 'error');
EventEmitter.prototype.emit = function emit(type, ...args) {
Copy link
Member

Choose a reason for hiding this comment

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

@bmeurer what do you think? As far as I understand, this is related to the escape analysis system. Do we need V8 62 to have the perf benefit?

Copy link
Member

Choose a reason for hiding this comment

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

The escape analysis is responsible for avoiding allocations of either arguments object or rest parameters. If escape analysis is off, every invocation of this function will always have to allocate arguments or rest parameters, even if it's not used.

So, yes, for code like this, independent of whether you use arguments or rest parameters, escape analysis usually helps performance-wise.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the latest on Escape Analysis in 8.x LTS? I know there was an issue opened discussing it but can't tell by looking at it what was decided, if anything yet.

let doError = (type === 'error');

events = this._events;
if (events)
doError = (doError && events.error == null);
const events = this._events;
if (events !== undefined)
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes like this really warrant a CITGM run.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I completely agree. 👍 I tried to flag it in my description of the PR but it's a bit of a wall of text.

doError = (doError && events.error === undefined);
else if (!doError)
return false;

domain = this.domain;
const domain = this.domain;

// If there is no 'error' event listener then throw.
if (doError) {
if (arguments.length > 1)
er = arguments[1];
if (domain) {
let er = args[0];
Copy link
Member

Choose a reason for hiding this comment

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

This is bad in case args is of length zero.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, thanks for catching it! That was never meant to make it into the PR because it's about 10x slower than checking length first.

Copy link
Member

Choose a reason for hiding this comment

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

@bmeurer wouldn't it be a good idea to improve this in v8? It is somewhat weird that it is best to check for the length first instead of having v8 doing this automatically.

Copy link
Member

Choose a reason for hiding this comment

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

Reading out of bounds is tricky in JS and requires some machinery. But yes we should probably fix this, esp. since jQuery and Co. already do that and don't want to give up on that practice. Can you file a bug on the V8 tracker pointing to this example?

cc @mathiasbynens

if (domain !== null && domain !== undefined) {
if (!er) {
const errors = lazyErrors();
er = new errors.Error('ERR_UNHANDLED_ERROR');
Expand All @@ -203,37 +198,32 @@ EventEmitter.prototype.emit = function emit(type) {
return false;
}

handler = events[type];
const handler = events[type];

if (!handler)
if (handler === undefined)
return false;

if (domain && this !== process) {
let needDomainExit = false;
if (domain !== null && domain !== undefined && this !== process) {
domain.enter();
needDomainExit = true;
}

var isFn = typeof handler === 'function';
len = arguments.length;
switch (len) {
// fast cases
case 1:
const isFn = typeof handler === 'function';
switch (args.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait if we're using rest args, while the unwinding?

Copy link
Member Author

@apapirovski apapirovski Oct 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's still faster to have these unwound cases than just using .apply(target, args). The main speed up is that we no longer need to create a new array in the slow case and manually copy arguments. (The fast cases are also slightly faster when using args rather than arguments.)

I tested probably 6 different permutations of this, this one came out consistently the fastest. I had some that were faster for the more than 4 args case but slower for the fast cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you gist those permutations?
I'm writing a benchmark so that we can track V8 optimizations.

BTW: I'm also seeing faster results for unwound calls, but <10% differance

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the difference isn't massive but I'm not comfortable having a performance regression. Will put together a gist this weekend if I can recall exactly what it was I tested.

case 0:
emitNone(handler, isFn, this);
break;
case 1:
emitOne(handler, isFn, this, args[0]);
break;
case 2:
emitOne(handler, isFn, this, arguments[1]);
emitTwo(handler, isFn, this, args[0], args[1]);
break;
case 3:
emitTwo(handler, isFn, this, arguments[1], arguments[2]);
break;
case 4:
emitThree(handler, isFn, this, arguments[1], arguments[2], arguments[3]);
emitThree(handler, isFn, this, args[0], args[1], args[2]);
break;
// slower
default:
args = new Array(len - 1);
for (i = 1; i < len; i++)
args[i - 1] = arguments[i];
emitMany(handler, isFn, this, args);
}

Expand All @@ -254,13 +244,13 @@ function _addListener(target, type, listener, prepend) {
}

events = target._events;
if (!events) {
if (events === undefined) {
events = target._events = Object.create(null);
target._eventsCount = 0;
} else {
// To avoid recursion in the case that type === "newListener"! Before
// adding it to the listeners, first emit "newListener".
if (events.newListener) {
if (events.newListener !== undefined) {
target.emit('newListener', type,
listener.listener ? listener.listener : listener);

Expand All @@ -271,7 +261,7 @@ function _addListener(target, type, listener, prepend) {
existing = events[type];
}

if (!existing) {
if (existing === undefined) {
// Optimize the case of one listener. Don't need the extra array object.
existing = events[type] = listener;
++target._eventsCount;
Expand Down Expand Up @@ -337,10 +327,7 @@ function onceWrapper() {
return this.listener.call(this.target, arguments[0], arguments[1],
arguments[2]);
default:
const args = new Array(arguments.length);
for (var i = 0; i < args.length; ++i)
args[i] = arguments[i];
this.listener.apply(this.target, args);
this.listener.apply(this.target, arguments);
}
}
}
Expand Down Expand Up @@ -385,11 +372,11 @@ EventEmitter.prototype.removeListener =
}

events = this._events;
if (!events)
if (events === undefined)
return this;

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

if (list === listener || list.listener === listener) {
Expand Down Expand Up @@ -422,7 +409,7 @@ EventEmitter.prototype.removeListener =
if (list.length === 1)
events[type] = list[0];

if (events.removeListener)
if (events.removeListener !== undefined)
this.emit('removeListener', type, originalListener || listener);
}

Expand All @@ -434,15 +421,15 @@ EventEmitter.prototype.removeAllListeners =
var listeners, events, i;

events = this._events;
if (!events)
if (events === undefined)
return this;

// not listening for removeListener, no need to emit
if (!events.removeListener) {
if (events.removeListener === undefined) {
if (arguments.length === 0) {
this._events = Object.create(null);
this._eventsCount = 0;
} else if (events[type]) {
} else if (events[type] !== undefined) {
if (--this._eventsCount === 0)
this._events = Object.create(null);
else
Expand Down Expand Up @@ -470,7 +457,7 @@ EventEmitter.prototype.removeAllListeners =

if (typeof listeners === 'function') {
this.removeListener(type, listeners);
} else if (listeners) {
} else if (listeners !== undefined) {
// LIFO order
for (i = listeners.length - 1; i >= 0; i--) {
this.removeListener(type, listeners[i]);
Expand All @@ -481,23 +468,19 @@ EventEmitter.prototype.removeAllListeners =
};

EventEmitter.prototype.listeners = function listeners(type) {
var evlistener;
var ret;
var events = this._events;
const events = this._events;

if (!events)
ret = [];
else {
evlistener = events[type];
if (!evlistener)
ret = [];
else if (typeof evlistener === 'function')
ret = [evlistener.listener || evlistener];
else
ret = unwrapListeners(evlistener);
}
if (events === undefined)
return [];

return ret;
const evlistener = events[type];
if (evlistener === undefined)
return [];

if (typeof evlistener === 'function')
return [evlistener.listener || evlistener];

return unwrapListeners(evlistener);
};

EventEmitter.listenerCount = function(emitter, type) {
Expand All @@ -512,12 +495,12 @@ EventEmitter.prototype.listenerCount = listenerCount;
function listenerCount(type) {
const events = this._events;

if (events) {
if (events !== undefined) {
const evlistener = events[type];

if (typeof evlistener === 'function') {
return 1;
} else if (evlistener) {
} else if (evlistener !== undefined) {
return evlistener.length;
}
}
Expand Down