events: optimize eventEmitter.once()

[mscdex]

This basically removes the extra closure previously created by
once() and instead uses an object to denote a one-time handler.
The result is ~340% performance increase when adding one-time
handlers, ~48% performance increase when emitting events with
mixed persistent and one-time handlers, and ~10% performance
increase when emitting with multiple arguments with mixed handlers,
all with negligible negative performance impact to other EventEmitter
functionality.

Also, removeListener() was optimized a bit by eliminating handler
arrays of length 1 so that the leftover handler is set on
_events[type].

The benchmarks from #912 were used when measuring the changes of this PR.

[chrisdickinson]

I think this might pose a problem – Readable and Duplex streams coming out of pegged versions of readable-stream on npm won't have this logic, and so won't preserve the once-ness of existing error handlers after these revisions.

For example:

var Readable = require('readable-stream').Readable;
var rs = new Readable;

var Writable = require('stream').Writable;
var ws = new Writable;
writable.once('error', function() {
  // important code to only perform once
});
rs.pipe(ws);

[mscdex]

Yep, that'd be a problem. Gotta start somewhere though :-)

[mscdex]

@chrisdickinson If we simultaneously merge this same change into iojs/readable-stream everything should still work for both node.js and io.js right?

[brendanashworth]

Reiterating on this, is this PR still in the discussion stage?

[mscdex]

@brendanashworth Since this modifies streams code, there might be a potential issue with module authors who have pegged their readable-stream dependencies to a specific version.

[ChALkeR]

@mscdex As I understand this, this PR breaks unattaching and reattaching an event that should be fired once. That is used, for example, by https://github.com/timoxley/overshadow-listeners/blob/master/index.js.

The problem here is that EventEmitter.prototype.listeners returns different results before and after this PR. Testcase:

const EventEmmiter = require('events');
var ee = new EventEmmiter();
ee.once('foo', function() {});
console.log(ee.listeners('foo')[0].toString());

To be compatible, EventEmitter.prototype.listeners should use old-style wraps around «once» callbacks. Note: you should also double-check EventEmitter.prototype.removeListener after that.

If not corrected, this would be a semver-major.

[ChALkeR]

Another testcase:

const EventEmmiter = require('events');
var ee = new EventEmmiter();

ee.once('foo', function() { console.log('foo!'); });
var bar = ee.listeners('foo')[0];
ee.removeAllListeners('foo');
ee.on('foo', bar);

ee.emit('foo');
ee.emit('foo');

[mscdex]

@ChALkeR Yes that would be backwards-incompatible for that particular use case. However IMHO the new behavior is probably what most people would expect. All the user knows is that they added a function to be called once and if they then copied that function and added it to be called more than once, you would expect it to be called more than once.

[ChALkeR]

But that still makes it a semver-major.

[mscdex]

@ChALkeR Right, I'm not disagreeing there.

[ChALkeR]

And the new implementation returns exactly the same callback for

const EventEmmiter = require('events');
var ee = new EventEmmiter();
ee.once('foo', function() {});
var callback = ee.listeners('foo')[0];

and

const EventEmmiter = require('events');
var ee = new EventEmmiter();
ee.on('foo', function() {});
var callback = ee.listeners('foo')[0];

Now there is no way to distinguish «once» and normal callbacks when listing the listeners.
I wouldn't say that this is expected. Though I wouldn't say that current #914 (comment) result is expected too.

Maybe there should a new API that properly distinguishes «once» and normal callbacks.
For example:

• EventEmmiter.prototype.listenersRegular returns only regular («on») callbacks,
• EventEmmiter.prototype.listenersOnce returns only «once» callbacks,
• EventEmmiter.prototype.listeners keeps compatibility by wrapping «once» callbacks.

This could work. Though EventEmitter.prototype.removeListener might need a closer look and could also cause a semver-major.

[mscdex]

@ChALkeR Why would you need to distinguish between one-time event handlers and not when receiving the current list of event handlers for an event?

[ChALkeR]

@mscdex For example, https://github.com/timoxley/overshadow-listeners/

[mscdex]

@ChALkeR I already saw that. IMHO that's inappropriately relying on an implementation detail.

I could also argue that your original test case does not make sense (without knowing EventEmitter internals) when run without the changes in this PR. "I added my listener with on(), why isn't it firing more than once?"

[ChALkeR]

@mscdex Are you talking about #914 (comment)? If yes, I already mentioned in #914 (comment).

[Fishrock123]

@mscdex did you still want to do this or can we close it?

[mscdex]

@Fishrock123 I think the main problem is the npm modules that are pegged at particular versions of readable-stream. We'd need those modules to either manually update the pegged version (once the readable-stream changes would be incorporated simultaneously with node) or to change their version specifier (e.g. to use semver's ^) so that the new changes would be picked up automatically.