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

Jump to bottom

events: remove emit micro-optimizations #16869

Closed
wants to merge 3 commits into from
Closed

events: remove emit micro-optimizations #16869

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b659275
events: remove emit micro-optimizations
apapirovski Nov 7, 2017
8b95f82
[fixup] more flexible emit benchmark
apapirovski Nov 8, 2017
7ddf389
[fixup] do not bind or apply args
apapirovski Nov 8, 2017
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
0 benchmark/events/ee-emit-multi-args.js → benchmark/events/ee-emit-2-args.js
View file
Open in desktop
File renamed without changes.
20 changes: 20 additions & 0 deletions benchmark/events/ee-emit-4-args.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict';
const common = require('../common.js');
const EventEmitter = require('events').EventEmitter;

const bench = common.createBenchmark(main, { n: [2e6] });

function main(conf) {
const n = conf.n | 0;

const ee = new EventEmitter();

for (var k = 0; k < 10; k += 1)
Copy link
Contributor

@mathiasbynens mathiasbynens Nov 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Coding style question: why not let? (Not just here but throughout the patch)

Copy link
Member

Choose a reason for hiding this comment

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

its much slower because it creates a closure for the variable in each loop iteration

Copy link
Member Author

Choose a reason for hiding this comment

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

@mathiasbynens We have a lint rule re: this so that's the main reason. I think @TimothyGu tried to change it recently and feedback from @bmeurer was that we shouldn't quite yet.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the relevant PR with more conversation: #15648

ee.on('dummy', function() {});

bench.start();
for (var i = 0; i < n; i += 1) {
ee.emit('dummy', 5, true, 'something', false);
}
bench.end(n);
}
96 changes: 9 additions & 87 deletions lib/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,63 +105,6 @@ EventEmitter.prototype.getMaxListeners = function getMaxListeners() {
return $getMaxListeners(this);
};

// These standalone emit* functions are used to optimize calling of event
// handlers for fast cases because emit() itself often has a variable number of
// arguments and can be deoptimized because of that. These functions always have
// the same number of arguments and thus do not get deoptimized, so the code
// inside them can execute faster.
function emitNone(handler, isFn, self) {
if (isFn)
handler.call(self);
else {
var len = handler.length;
var listeners = arrayClone(handler, len);
for (var i = 0; i < len; ++i)
listeners[i].call(self);
}
}
function emitOne(handler, isFn, self, arg1) {
if (isFn)
handler.call(self, arg1);
else {
var len = handler.length;
var listeners = arrayClone(handler, len);
for (var i = 0; i < len; ++i)
listeners[i].call(self, arg1);
}
}
function emitTwo(handler, isFn, self, arg1, arg2) {
if (isFn)
handler.call(self, arg1, arg2);
else {
var len = handler.length;
var listeners = arrayClone(handler, len);
for (var i = 0; i < len; ++i)
listeners[i].call(self, arg1, arg2);
}
}
function emitThree(handler, isFn, self, arg1, arg2, arg3) {
if (isFn)
handler.call(self, arg1, arg2, arg3);
else {
var len = handler.length;
var listeners = arrayClone(handler, len);
for (var i = 0; i < len; ++i)
listeners[i].call(self, arg1, arg2, arg3);
}
}

function emitMany(handler, isFn, self, args) {
if (isFn)
handler.apply(self, args);
else {
var len = handler.length;
var listeners = arrayClone(handler, len);
for (var i = 0; i < len; ++i)
listeners[i].apply(self, args);
}
}

EventEmitter.prototype.emit = function emit(type, ...args) {
let doError = (type === 'error');

Expand Down Expand Up @@ -212,22 +155,13 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
needDomainExit = true;
}

const isFn = typeof handler === 'function';
switch (args.length) {
case 0:
emitNone(handler, isFn, this);
break;
case 1:
emitOne(handler, isFn, this, args[0]);
break;
case 2:
emitTwo(handler, isFn, this, args[0], args[1]);
break;
case 3:
emitThree(handler, isFn, this, args[0], args[1], args[2]);
break;
default:
emitMany(handler, isFn, this, args);
if (typeof handler === 'function') {
handler.apply(this, args);
} else {
const len = handler.length;
const listeners = arrayClone(handler, len);
Copy link
Member

@bmeurer bmeurer Nov 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Can you leave a TODO here to consider switching to Array.prototype.slice once V8 6.4 lands in Node? Or even to evaluate the idea of avoiding the defensive copy on emit and rather making sure that the handler itself is never mutated?

for (var i = 0; i < len; ++i)
listeners[i].apply(this, args);
}

if (needDomainExit)
Expand Down Expand Up @@ -313,23 +247,11 @@ EventEmitter.prototype.prependListener =
return _addListener(this, type, listener, true);
};

function onceWrapper() {
function onceWrapper(...args) {
if (!this.fired) {
this.target.removeListener(this.type, this.wrapFn);
this.fired = true;
switch (arguments.length) {
case 0:
return this.listener.call(this.target);
case 1:
return this.listener.call(this.target, arguments[0]);
case 2:
return this.listener.call(this.target, arguments[0], arguments[1]);
case 3:
return this.listener.call(this.target, arguments[0], arguments[1],
arguments[2]);
default:
this.listener.apply(this.target, arguments);
}
this.listener.apply(this.target, args);
}
}

Expand Down
8 changes: 4 additions & 4 deletions test/message/stdin_messages.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ SyntaxError: Strict mode code may not include a with statement
at Module._compile (module.js:*:*)
at evalScript (bootstrap_node.js:*:*)
at Socket.<anonymous> (bootstrap_node.js:*:*)
at emitNone (events.js:*:*)
at Socket.emit (events.js:*:*)
at endReadableNT (_stream_readable.js:*:*)
at _combinedTickCallback (internal/process/next_tick.js:*:*)
at process._tickCallback (internal/process/next_tick.js:*:*)
42
42
[stdin]:1
Expand All @@ -27,9 +27,9 @@ Error: hello
at Module._compile (module.js:*:*)
at evalScript (bootstrap_node.js:*:*)
at Socket.<anonymous> (bootstrap_node.js:*:*)
at emitNone (events.js:*:*)
at Socket.emit (events.js:*:*)
at endReadableNT (_stream_readable.js:*:*)
at _combinedTickCallback (internal/process/next_tick.js:*:*)
[stdin]:1
throw new Error("hello")
^
Expand All @@ -42,9 +42,9 @@ Error: hello
at Module._compile (module.js:*:*)
at evalScript (bootstrap_node.js:*:*)
at Socket.<anonymous> (bootstrap_node.js:*:*)
at emitNone (events.js:*:*)
at Socket.emit (events.js:*:*)
at endReadableNT (_stream_readable.js:*:*)
at _combinedTickCallback (internal/process/next_tick.js:*:*)
100
[stdin]:1
var x = 100; y = x;
Expand All @@ -58,9 +58,9 @@ ReferenceError: y is not defined
at Module._compile (module.js:*:*)
at evalScript (bootstrap_node.js:*:*)
at Socket.<anonymous> (bootstrap_node.js:*:*)
at emitNone (events.js:*:*)
at Socket.emit (events.js:*:*)
at endReadableNT (_stream_readable.js:*:*)
at _combinedTickCallback (internal/process/next_tick.js:*:*)

[stdin]:1
var ______________________________________________; throw 10
Expand Down