events: Support prototype property name as an event type

[tricknotes]

The prototype property name (such as 'proto') as an event type does not work as my expectation.

> var events = require('events')
> var e = new events.EventEmitter();
> e.on('__proto__', function() {});
> e.emit('__proto__');
TypeError: Object #<Object> has no method 'apply'
    at EventEmitter.emit (events.js:126:20)
    at repl:1:3
    at REPLServer.self.eval (repl.js:109:21)
    at rli.on.self.bufferedCmd (repl.js:258:20)
    at REPLServer.self.eval (repl.js:116:5)
    at Interface.<anonymous> (repl.js:248:12)
    at Interface.EventEmitter.emit (events.js:96:17)
    at Interface._onLine (readline.js:200:10)
    at Interface._line (readline.js:518:8)
    at Interface._ttyWrite (readline.js:736:14)

To support these, use Object.create(null) instead of {}.

[bnoordhuis]

While the PR in itself LGTM, I'm not sure if it's an acceptable change.

The event emitter code is considered a red hot code path and obj = Object.create(null) is 20-30 times slower than obj = {}.

[tricknotes]

I see. It is true that I haven't look around about the performance.
So, what do you feel about the following code?:

obj = {};
obj.__proto__ = null;

This is not a cool code, but it is able you to use prototype property name as an event type without compromising the performance.

[bnoordhuis]

Same issue as Object.create(null), it's about 20 times slower.

[tricknotes]

Hmm, really?
In my local environment, obj.__proto__ = null has the same performance as obj = {}; on initialize.
And, faster on property lookup.

ref: https://gist.github.com/4214426

Please tell me another case that should be checked if it exist.

[AlexeyKupershtokh]

There's a bug:

for (i = 0; i < count; i++)
  obj = {}; obj.__proto__ = null;

[tricknotes]

Thanks @AlexeyKupershtokh.
That's right!

This code is really slower.

[AlexeyKupershtokh]

lol :)
it's even faster to clone a created object using require('node-v8-clone').v8_clone:

clone(a);:
  16143 ms
{}:
  436 ms
Object.create(null):
  24133 ms
obj = {};obj.__proto__ = null:
  19603 ms

[isaacs]

It's nice in theory, but in practice, we cannot do this for performance reasons. I'd be open to a doc patch explaining that __proto__ is not a valid event name.

[isaacs]

Another option worth trying would be to prefix all event names in the object with some character, like "e" or something. I used that approach to make node-lru-cache work with keys named __proto__

[TooTallNate]

Another option worth trying would be to prefix all event names in the object with some character, like "e" or something. I used that approach to make node-lru-cache work with keys named __proto__

+1. This is the "standard" dictionary-in-JS practice as far as I know, and would solve all of these problems.

[isaacs]

Whipped this up to test it out: https://github.com/isaacs/node/compare/ev_proto

Seems like http_simple doesn't get any slower with this. There's a bit of added string juggling, but not too bad.

[ThisIsMissEm]

@isaacs might be nice to use "ev:" instead of just "ev", as to not end up with things like "evview", and instead "ev:view" (event name is "view")

[isaacs]

Landed on b48e303.

might be nice to use "ev:" instead of just "ev", as to not end up with things like "evview", and instead "ev:view" (event name is "view")

Meh. It's internal anyway.

[isaacs]

Turns out that this causes a significant regression with the emit() function. Users will just have to not use events named __proto__ or toString.