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

events: optimize for performance, remove _events use outside of events #17324

Closed
wants to merge 5 commits into from
Closed

events: optimize for performance, remove _events use outside of events #17324

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9cf8c68
events: optimize performance of add, remove, emit
apapirovski Nov 25, 2017
9534a22
events: remove reaches into _events internals
apapirovski Nov 25, 2017
c3670cd
fixup: missing paren in doc
apapirovski Nov 29, 2017
6e3b199
fixup: rename wrappedListeners to rawListeners
apapirovski Dec 1, 2017
145bbc3
fixup: address feedback
apapirovski Dec 2, 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
18 changes: 15 additions & 3 deletions benchmark/events/ee-add-remove.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,30 @@
const common = require('../common.js');
const events = require('events');

const bench = common.createBenchmark(main, { n: [25e4] });
const bench = common.createBenchmark(main, {
n: [5e6],
events: [0, 5],
listeners: [1, 5]
});

function main(conf) {
const n = conf.n | 0;
let n = conf.n | 0;
const eventsCOunt = conf.events | 0;
const listenersCount = conf.listeners | 0;

const ee = new events.EventEmitter();
const listeners = [];

if (listenersCount === 1)
n *= 2;

var k;
for (k = 0; k < 10; k += 1)
for (k = 0; k < listenersCount; k += 1)
listeners.push(function() {});

for (k = 0; k < eventsCOunt; k++)
ee.on(`dummyunused${k}`, () => {});

bench.start();
for (var i = 0; i < n; i += 1) {
const dummy = (i % 2 === 0) ? 'dummy0' : 'dummy1';
Expand Down
36 changes: 36 additions & 0 deletions benchmark/events/ee-emit-multi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
'use strict';
const common = require('../common.js');
const EventEmitter = require('events').EventEmitter;

const bench = common.createBenchmark(main, {
n: [2e7],
listeners: [1, 5, 10],
});

function main(conf) {
var n = conf.n | 0;
const listeners = Math.max(conf.listeners | 0, 1);

const ee = new EventEmitter();

if (listeners === 1)
n *= 5;
else if (listeners === 5)
n *= 2;

for (var k = 0; k < listeners; k += 1) {
ee.on('dummy', function() {});
ee.on(`dummy${k}`, function() {});
}

bench.start();
for (var i = 0; i < n; i += 1) {
if (i % 3 === 0)
ee.emit('dummy', true, 5);
else if (i % 2 === 0)
ee.emit('dummy', true, 5, 10, false);
else
ee.emit('dummy');
}
bench.end(n);
}
13 changes: 10 additions & 3 deletions benchmark/events/ee-emit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,27 @@ const common = require('../common.js');
const EventEmitter = require('events').EventEmitter;

const bench = common.createBenchmark(main, {
n: [2e6],
n: [2e7],
argc: [0, 2, 4, 10],
listeners: [1, 5, 10],
});

function main(conf) {
const n = conf.n | 0;
var n = conf.n | 0;
const argc = conf.argc | 0;
const listeners = Math.max(conf.listeners | 0, 1);

const ee = new EventEmitter();

for (var k = 0; k < listeners; k += 1)
if (listeners === 1)
n *= 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think automagically adjusting the n value is a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, we don't have a ton of options here. The benchmark doesn't run long enough with 1 listener whereas setting a higher default n makes the other ones run way too long.

Copy link
Contributor

@mscdex mscdex Nov 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

That's just a general benchmarking problem, just like any particular n value might cause things to run too fast or too slow on someone else's machine. At some point you will just need to supply your own set of values for each parameter if you have combinations that vary in speed that much.

else if (listeners === 5)
n *= 2;

for (var k = 0; k < listeners; k += 1) {
ee.on('dummy', function() {});
ee.on(`dummy${k}`, function() {});
}

var i;
switch (argc) {
Expand Down
38 changes: 38 additions & 0 deletions benchmark/events/ee-event-names.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';
const common = require('../common.js');
const EventEmitter = require('events').EventEmitter;

const bench = common.createBenchmark(main, {
n: [1e6],
listeners: [1, 10],
symbols: ['true', 'false']
});

function main(conf) {
const n = conf.n | 0;
const listeners = conf.listeners | 0;
const useSymbols = conf.symbols === 'true';

const ee = new EventEmitter();

for (var k = 0; k < listeners; k += 1) {
ee.on(`dummy0${k}`, function() {});
ee.on(`dummy1${k}`, function() {});
ee.on(`dummy2${k}`, function() {});
if (useSymbols)
ee.on(Symbol(`dummy${k}`), function() {});
}

ee.removeAllListeners('dummy01');
ee.removeAllListeners('dummy11');
ee.removeAllListeners('dummy21');
ee.removeAllListeners('dummy06');
ee.removeAllListeners('dummy16');
ee.removeAllListeners('dummy26');

bench.start();
for (var i = 0; i < n; i += 1) {
ee.eventNames();
}
bench.end(n);
}
2 changes: 1 addition & 1 deletion benchmark/events/ee-listeners-many.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
const common = require('../common.js');
const EventEmitter = require('events').EventEmitter;

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

function main(conf) {
const n = conf.n | 0;
Expand Down
2 changes: 1 addition & 1 deletion benchmark/events/ee-listeners.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
const common = require('../common.js');
const EventEmitter = require('events').EventEmitter;

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

function main(conf) {
const n = conf.n | 0;
Expand Down
16 changes: 12 additions & 4 deletions benchmark/events/ee-once.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,27 @@
const common = require('../common.js');
const EventEmitter = require('events').EventEmitter;

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

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

if (listeners === 1)
n *= 2;

const ee = new EventEmitter();

function listener() {}

bench.start();
for (var i = 0; i < n; i += 1) {
for (var i = 0; i < n; ++i) {
const dummy = (i % 2 === 0) ? 'dummy0' : 'dummy1';
ee.once(dummy, listener);
for (var j = 0; j < listeners; ++j)
ee.once(dummy, listener);
ee.emit(dummy);
}
bench.end(n);
Expand Down
9 changes: 9 additions & 0 deletions doc/api/events.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,15 @@ to indicate an unlimited number of listeners.

Returns a reference to the `EventEmitter`, so that calls can be chained.

### emitter.wrappedListeners(eventName)
Copy link
Member

Choose a reason for hiding this comment

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

I kind of feel like something like rawListeners() might be a better name, but this should be okay.

(i.e. I’m not saying it is a better name, just that wrappedListeners() does not immediately tell you what this does … neither does rawListeners(), obviously, but I think it comes closer? 😄 )

<!-- YAML
added: REPLACEME
-->
- `eventName` {any}

Returns a copy of the array of listeners for the event named `eventName`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think saying 'event named' sounds a little confusing. Maybe something like 'event in' instead? Or maybe just 'event'?

Copy link
Member Author

@apapirovski apapirovski Nov 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I can change this but we have another 9 instances of this exact phrasing in this doc.

Copy link
Member

Choose a reason for hiding this comment

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

I think the current wording is fine here and if we do change it, I'd rather do that throughout the doc and in a separate PR for consistency

including any wrappers (such as those created by `.once`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing end parenthesis. Also, maybe link once()?


[`--trace-warnings`]: cli.html#cli_trace_warnings
[`EventEmitter.defaultMaxListeners`]: #events_eventemitter_defaultmaxlisteners
[`domain`]: domain.html
Expand Down
Loading