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: remove domain handling from events to domain #17403

Closed
wants to merge 1 commit into from
Closed

events: remove domain handling from events to domain #17403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 46 additions & 5 deletions lib/domain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,6 @@ const EventEmitter = require('events');
const errors = require('internal/errors');
const { createHook } = require('async_hooks');

// communicate with events module, but don't require that
// module to have to load this one, since this module has
// a few side effects.
EventEmitter.usingDomains = true;

// overwrite process.domain with a getter/setter that will allow for more
// effective optimizations
var _domain = [null];
Expand Down Expand Up @@ -387,3 +382,49 @@ Domain.prototype.bind = function(cb) {

return runBound;
};

// Override EventEmitter methods to make it domain-aware.
EventEmitter.usingDomains = true;

const eventInit = EventEmitter.init;
EventEmitter.init = function() {
this.domain = null;
if (exports.active && !(this instanceof exports.Domain)) {
this.domain = exports.active;
}

return eventInit.call(this);
};

const eventEmit = EventEmitter.prototype.emit;
EventEmitter.prototype.emit = function emit(...args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@vdeturckheim Would you mind changing this to emit() and then using arguments everywhere? I realize performance is not that big of a deal but this will actually make a substantial difference.

Copy link
Member

@benjamingr benjamingr Dec 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@apapirovski are you sure it's faster? (In recent V8)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm certain because I checked it very recently. ...args is only faster when arguments requires slicing or manually copying in situations like emit(type, ...args).

Copy link
Member

@benjamingr benjamingr Dec 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@apapirovski very interesting can you share a benchmark showing that? I'm pretty sure ...args is optimized.

cc @bmeurer

Copy link
Contributor

Choose a reason for hiding this comment

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

It has been optimized but in practical usage it still seems to perform worse. I've run into this working on nextTick, events and timers.

> function fn(...args) {
...   return Reflect.apply(Math.max, Math, args);
... }
> 
> function fn2() {
...   return Reflect.apply(Math.max, Math, arguments);
... }
> console.time('test'); for (var i = 0; i < 1e8; i++) { fn(1, 2, 3, 4); } console.timeEnd('test');
test: 279.502ms
> console.time('test'); for (var i = 0; i < 1e8; i++) { fn2(1, 2, 3, 4); } console.timeEnd('test');
test: 284.195ms

The basic benchmark shows it being optimized. As soon as I've tried using it in practice where there's more than one layer to the usage, arguments seems to win out.

Copy link
Contributor

@apapirovski apapirovski Dec 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

btw @benjamingr I decided to finally split test all the possible variations. Here are my findings:

function fn(...args) {
  cb(...args);
}

is the fastest for that particular purpose, faster than using arguments (about 3-5% faster). But then there's:

function fn(something, ...args) {
  cb(...args);
}

seems to depend on specific usage of args within fn. For example, I've found that in setTimeout and setImmediate we incur about 20-25% performance penalty compared to using manual copying of arguments.

On the flipside, it performs as expected within EventEmitter.prototype.emit... and is, in fact, yet again faster than using arguments. There definitely seem to be some edge cases around this behaviour that I don't quite understand.

I don't know if this helps with your plan to bug V8 devs regarding this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible this might be related to whether args is being stored somewhere or just passed through to another function, or whether args is accessed at all in the body?

const domain = this.domain;
if (domain === null || domain === undefined || this === process) {
return eventEmit.apply(this, args);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use Reflect.apply here (and everywhere else) to avoid any potentially overridden apply methods?

Copy link
Member

Choose a reason for hiding this comment

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

Why would we use Reflect.apply here but not in the regular event emitter code?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a PR introducing it there too, it's just currently blocked on some other stuff.

Copy link
Member

Choose a reason for hiding this comment

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

@apapirovski sounds good - although a little defensive (if someone overrides Function.prototype.apply or apply on a function object - they other didn't mean to or they know exactly what they're doing - in both cases I'm not sure Node.js should guard against it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely should. The usage of .apply is an implementation detail and should not be affected by monkey-patching.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't Reflect.apply have the same "issue"? It is writable too.

Copy link
Contributor

@apapirovski apapirovski Dec 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Of course. But it's a step better because we're no longer relying on a method of a user provided function. At least Reflect.apply allows us consistent behaviour between all the different functions that are going to be called, instead of relying on the potentially unique apply of each specific callback.

Strictly speaking, using some sort of an internal way of invoking functions brings us closer to the spec too (e.g. for timers).

Copy link
Contributor

@apapirovski apapirovski Dec 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Also, to elaborate, this has become less of an issue lately but it was potentially even more problematic in the past where certain functions were called as callback(arguments[0]) and others callback.apply(undefined, arguments).

}

const type = args[0];
// edge case: if there is a domain and an existing non error object is given,
// it should not be errorized
// see test/parallel/test-event-emitter-no-error-provided-to-error-event.js
if (type === 'error' && args.length > 1 && args[1] &&
!(args[1] instanceof Error)) {
domain.emit('error', args[1]);
return false;
}

domain.enter();
try {
return eventEmit.apply(this, args);
} catch (er) {
if (typeof er === 'object' && er !== null) {
er.domainEmitter = this;
er.domain = domain;
er.domainThrown = false;
}
domain.emit('error', er);
return false;
} finally {
domain.exit();
}
};
48 changes: 6 additions & 42 deletions lib/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

'use strict';

var domain;
var spliceOne;

function EventEmitter() {
Expand All @@ -32,9 +31,6 @@ module.exports = EventEmitter;
// Backwards-compat with node 0.10.x
EventEmitter.EventEmitter = EventEmitter;

EventEmitter.usingDomains = false;

EventEmitter.prototype.domain = undefined;
EventEmitter.prototype._events = undefined;
EventEmitter.prototype._maxListeners = undefined;

Expand Down Expand Up @@ -66,14 +62,6 @@ Object.defineProperty(EventEmitter, 'defaultMaxListeners', {
});

EventEmitter.init = function() {
this.domain = null;
if (EventEmitter.usingDomains) {
// if there is an active domain, then attach to it.
domain = domain || require('domain');
if (domain.active && !(this instanceof domain.Domain)) {
this.domain = domain.active;
}
}

if (this._events === undefined ||
this._events === Object.getPrototypeOf(this)._events) {
Expand Down Expand Up @@ -114,47 +102,26 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
else if (!doError)
return false;

const domain = this.domain;

// If there is no 'error' event listener then throw.
if (doError) {
let er;
if (args.length > 0)
er = args[0];
if (domain !== null && domain !== undefined) {
if (!er) {
const errors = lazyErrors();
er = new errors.Error('ERR_UNHANDLED_ERROR');
}
if (typeof er === 'object' && er !== null) {
er.domainEmitter = this;
er.domain = domain;
er.domainThrown = false;
}
domain.emit('error', er);
} else if (er instanceof Error) {
if (er instanceof Error) {
throw er; // Unhandled 'error' event
} else {
// At least give some kind of context to the user
const errors = lazyErrors();
const err = new errors.Error('ERR_UNHANDLED_ERROR', er);
err.context = er;
throw err;
}
return false;
// At least give some kind of context to the user
const errors = lazyErrors();
const err = new errors.Error('ERR_UNHANDLED_ERROR', er);
err.context = er;
throw err;
}

const handler = events[type];

if (handler === undefined)
return false;

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

if (typeof handler === 'function') {
handler.apply(this, args);
} else {
Expand All @@ -164,9 +131,6 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
listeners[i].apply(this, args);
}

if (needDomainExit)
domain.exit();

return true;
};

Expand Down