EventEmitter (yeah...) Prototypal Events Issue

[skeggse]

This is a "simple" issue, but the need for the event emitter to remain frozen may simply mean it's a tolerable issue.

var Emitter = require('events').EventEmitter;
var emitter = new Emitter();

emitter.emit('valueOf'); // => true

// bad example
Object.defineProperty(Object.prototype, 'tosser', {
  enumerable: false,
  writable: false,
  configurable: false,
  value: function() {
    throw new Error("this wasn't supposed to happen");
  }
});

// not necessarily the expected outcome
emitter.emit('tosser'); // Error

// betterish example
Object.defineProperty(Object.prototype, 'badDay', {
  enumerable: false,
  writable: false,
  configurable: false,
  value: function() {
    // basically, has side effects
    send('money').to('group').where('max(group.hate)');
    // perhaps a moot point because no matter what object prototype methods shouldn't have side effects...
  }
});

// doesn't do what's expected
emitter.emit('badDay'); // => true

In summary, because the event emitter does not handle prototypal methods on the _events object, there could be conflict, and it certainly makes the emit's return method return true where it otherwise shouldn't.

Best-case change given that the API is frozen: set the _events object to Object.create(null). This would, incidentally, slightly reduce the time taken to emit an event because there would be no prototype chain.

Luckily, because _events is a private property, this shouldn't be a breaking change. Even if someone were tampering with the _events object, they should have the knowledge to not depend on its methods.

Thoughts?

[faridnsh]

This has been discussed before: #4366
It will cause huge performance regressions so it's not possible to do so.

[skeggse]

Edit: my bad, didn't fully read #4366.

[skeggse]

I find it odd that the performance of creating event emitters is so critical--wouldn't the performance of adding listeners and emitting events be more important?

[faridnsh]

Per each http request, you at least create two event emitters: one for response and one for request. Most probably you'll have more for piping from request and to response.
I guess the performance adding listeners and emitting events would be same with no prototype: http://jsperf.com/js-object-creation-and-accessing-properties

You can try making the change and seeing how it will perform in the node benchmarks, you get a different result. v8 have changed a lot from a year ago.

[skeggse]

Hmm I was under the false impression a null prototype made more of a difference than that.

However, even if you're creating ten emitters for each request, Amdahl's law implicates that because the creation of the object for the emitter is such a small percentage of the overall performance, it shouldn't really matter.

If you're running through a thousand requests in a second, the time taken to create your two emitters is still less than a millisecond.

I guess the theory is all a moot point if in practice it decreases performance.

[trevnorris]

Amdahl's law is as applicable here as General Relativity is in a black
hole. ;-)

But anyways. So much is constantly going on in core code that no one call
costs much. We have to keep all calls as cheap as possible. If you trace
the number of EE calls I think you'd be surprised how often they're
instantiated.

[skeggse]

Amdahl's law is as applicable here as General Relativity is in a black
hole. ;-)

Touché :)

But anyways. So much is constantly going on in core code that no one call costs much. We have to keep all calls as cheap as possible. If you trace the number of EE calls I think you'd be surprised how often they're instantiated.

Yeah, that's a good point. Assuming two-ish event emitters isn't really foolproof logic.