Why did this get removed?

It has moved here https://github.com/nodejs/node/pull/17403/files/426afc70832c47f18f9afd9f90f35c1e554c8d3a#diff-0b93f5d72b0d34f5b972b661935344c3R415

The only case when this code was reached was when a domain was present, other situation throw.

I should remove the else here

We should change this comment. It serves no purpose now, other than backward compatibility.

Updated!

Nit: typos

// Override EventEmitter methods to make it domain-aware.

Updated!

@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.

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

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).

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

cc @bmeurer

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.

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 :)

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?

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

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

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

@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.

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

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

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).

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).