events: optimize various functions #601

mscdex · 2015-01-25T03:53:57Z

Cache events and listeners objects where possible and loop over
Object.keys() instead of using for..in. These changes alone give
~60-65% improvement in the ee-add-remove benchmark.

The swapping out of the util type checking functions with inline
checking gives another ~5-10% improvement.

quantizor · 2015-01-26T08:35:33Z

lib/events.js

+    // adding it to the listeners, first emit "newListener".
+    if (events.newListener)
+      this.emit('newListener', type,
+                typeof listener.listener === 'function' ?


listener.listener feels awkward, can the initial naming of this be tweaked at all?

Maybe, I just left it as it was before. What do you suggest?

@mscdex Can you put braces around the consequent?

quantizor · 2015-01-26T08:40:05Z

General comment: There's a lot of !(events = this._events) patterns in the code you introduced that are significantly different than what you're replacing. Not sure if there is a formal code style for this sort of expression, but it feels kind of tricky and it may be better to isolate the assignment to its own line rather than capturing the output inline as a side effect.

micnic · 2015-01-26T08:55:28Z

lib/events.js

+  if (!(events = this._events))
+    events = this._events = {};
+  else {
+    existing = events[type];


at this line events is undefined, isn't it?

events variable can be initiated with

var events = this._events;

and here can be used like this:

if (!events) events = this._events = {}; existing = events[type];

No, it is defined there. I have the separate else there because of the code following this line (events.newListener can't possibly exist if this._events isn't set, so there's no need to check in that case).

oh, I see, I was misled by the line nr. 137, was expecting of an equality check, actually using an assignment in an if statement is a bad practice. Would you mind to try the code that I proposed in my first comment and check the performance impact of the changes made?

mscdex · 2015-01-26T13:35:19Z

There's a lot of !(events = this._events) patterns in the code you introduced that are significantly different than what you're replacing.

I'm not sure what you mean by "significantly different?" The checks are still the same checks, except cached versions are used where possible (in addition to inline assignment).

Fishrock123 · 2015-01-28T00:36:18Z

Benchmarks do look good. (+ Tests pass.)

Before:

events/ee-add-remove.js n=250000: 337186.61793

After:

events/ee-add-remove.js n=250000: 529259.95116

Fishrock123 · 2015-01-28T00:37:27Z

lib/events.js

@@ -47,7 +47,7 @@ EventEmitter.prototype.setMaxListeners = function setMaxListeners(n) {
 };

 function $getMaxListeners(that) {
-  if (util.isUndefined(that._maxListeners))
+  if (that._maxListeners === void 0)


undefined is not re-definable in ES5. Is there a good reason to use void 0 over it?

It's what util.isUndefined() was using.

+0.1 for undefined.

+1 for undefined

Fishrock123 · 2015-01-28T01:36:28Z

Mostly LGTM, but I'd like to see other potential comments.

micnic · 2015-01-28T08:08:10Z

For me it also looks good, except the variable assignments inside if statements like @yaycmyk said, they are misleading while reading the code, the cached values should be assigned somewhere before the if statement as I proposed in this discussion

bnoordhuis · 2015-01-28T11:07:15Z

lib/events.js

        return this;
      }

      // emit removeListener for all listeners on all events
      if (arguments.length === 0) {
-        for (key in this._events) {
+        var keys = Object.keys(events);


I suppose Object.keys() could have a negative impact on EventEmitter objects with many events because of the array it creates (instead of iterating over an object like for..in does.) Probably an uncommon case but it might be good to capture it in a benchmark.

I suppose we could manually keep track of the length of this._events and use some length value as the cut-off between using Object.keys() and using a for-in?

This is based on false assumption, V8 for-in eagerly creates an array of the properties before the loop body is even entered. In optimized case this array is cached but that's not the case for events anyway.

So basically you never want to use a for-in, even in the case where you need prototype properties you are better off doing it manually with Object.keys and Object.getPrototypeOf (which could be turned into a function getInheritedKeys to make it fast idiom everywhere)

V8 for-in eagerly creates an array of the properties before the loop body is even entered.

That's only the case for "uncommon" objects, like object proxies or objects with interceptors. The objects that EventEmitter creates are simple objects with an enum cache that for..in iterates over.

That's only the case for "uncommon" objects, like object proxies or objects with interceptors. The objects that EventEmitter creates are simple objects with an enum cache that for..in iterates over.

A normalized object is also an uncommon object which this._events definitely is as there is a delete call on it literally 18 lines below.

so, maybe we should consider getting reed of deletes ?

@vkurchatkin That was tried in the past in the middle of v0.10 (unfortunately) which caused memory leaks (especially if you keep using uniquely named events).

That's not the point, this._events is being treated like a hash table - if it's not normalized by delete it will be soon normalized by adding enough event names on it (16 the last time I checked was the limit on the heuristic). Just forget for-in forever and be happy, you know that Object.keys uses the enum cache as well, right?

you know that Object.keys uses the enum cache as well, right?

It still creates a new array every time though.

What you say about delete obj.key and for..in is true for Crankshaft but not TurboFan. I'm not sure what we should be optimizing for. Crankshaft is still the default but it's effectively in maintenance mode. TurboFan is going to replace it some day but that may still be far off. Or it may be next month, I don't know. :-(

Object normalization is not related to crankshaft and you cannot do a simple map check with normalized objects to see if a property was deleted during an iteration of the loop so I am sure for-in will always be very slow for normalized objects unless it's very simple body that can be analyzed to never delete properties but I am not holding my breath at all.

bnoordhuis · 2015-01-28T11:08:10Z

Some comments but I like the general thrust.

mscdex · 2015-01-28T23:23:07Z

Ok I have made suggested changes.

I also went ahead and optimized some of the other functions. The changes to EventEmitter.listenerCount() show ~14% improvement and the changes to emitter.listeners() show ~195% improvement for 10 listeners.

I benchmarked listenerCount() and listeners() using (new) benchmarks based on ee-add-remove (same number of listeners, same number of iterations, etc.).

All tests still pass.

EDIT: Ok, it seems now that for some reason if the array is large enough (~>=50 elements in my testing), array.slice() somehow ends up becoming faster than a manual copy. I'm looking into that.

mscdex · 2015-01-29T01:31:11Z

Ok I made a tweak to the emitter.listeners() implementation that only does the manual array copy for small numbers of listeners (<50 currently). For >=50 listeners it switches to array.slice().

Fishrock123 · 2015-01-29T02:01:09Z

Hmm, the new changes are slightly slower on the ee-add-remove benchmark.

Unpatched:

events/ee-add-remove.js n=250000: 337186.61793

Previous patch:

events/ee-add-remove.js n=250000: 529259.95116

Current patch:

events/ee-add-remove.js n=250000: 441635.76191

Fishrock123 · 2015-01-29T02:01:56Z

@mscdex where are you getting your numbers from? it may be prudent to write additional benchmarks for these. :)

mscdex · 2015-01-29T02:28:08Z

@Fishrock123 I did write additional benchmarks to test the other functions, but was going to submit them in a separate PR. However I did not change ee-add-remove.

The results for me are the same. Not a whole lot changed in the add/remove functions anyway, merely style changes.

quantizor · 2015-01-29T06:27:12Z

lib/events.js

-  if (!this._events)
-    this._events = {};
+  events = this._events;
+  if (!events)


always include braces for conditionals please

nope, one line body is ok without braces

mscdex · 2015-01-29T17:25:10Z

I've also now made some improvements to emit() that result in a 3x speedup for fast cases and a slight bump for the slow case for multiple handlers (single handler cases still perform the same):

Benchmark code:

var Benchmark = require('benchmark');
var ee = new (require('events').EventEmitter)();

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

var suite = new Benchmark.Suite();
suite.add('emitter#emit', function() {
  // tested code
}, { minSamples: 100 });
suite.on('cycle', function(event) {
  console.log(String(event.target));
});
suite.run({ async: false });

0 args test

ee.emit('dummy');

Results:

Before
- emitter#emit x 1,891,788 ops/sec ±0.17% (197 runs sampled)
After
- emitter#emit x 5,827,473 ops/sec ±0.18% (198 runs sampled)

1 arg test

ee.emit('dummy', 1);

Results:

Before
- emitter#emit x 1,566,929 ops/sec ±0.41% (193 runs sampled)
After
- emitter#emit x 4,017,494 ops/sec ±0.18% (197 runs sampled)

2 args test

ee.emit('dummy', 1, false);

Results:

Before
- emitter#emit x 1,481,542 ops/sec ±0.30% (195 runs sampled)
After
- emitter#emit x 3,863,464 ops/sec ±0.12% (197 runs sampled)

3 args test

ee.emit('dummy', 1, false, null);

Results:

Before
- emitter#emit x 1,435,269 ops/sec ±0.23% (195 runs sampled)
After
- emitter#emit x 3,958,400 ops/sec ±0.16% (197 runs sampled)

4 args test

(this hits the slow case code)

ee.emit('dummy', 1, false, null, 'foo');

Results:

Before
- emitter#emit x 1,370,338 ops/sec ±0.14% (198 runs sampled)
After
- emitter#emit x 1,589,312 ops/sec ±0.13% (198 runs sampled)

tjconcept · 2015-01-31T16:08:11Z

If performance is a priority in iojs/io.js, I think @petkaantonov is the guy. He has done some amazing work by replacing node core stuff with faster (as in 100x) user land versions.
I think he got put off by the attitude towards contributors in joyent/node (petkaantonov/urlparser#10 (comment)), so he might wanna give it another shot here? 😃

mscdex · 2015-02-01T00:31:22Z

@tjconcept There is already #643 for incorporating his url module.

Anyway, I think these changes are a good start.

mscdex · 2015-02-01T23:35:12Z

@bnoordhuis rebased.

bnoordhuis · 2015-02-02T21:50:38Z

@mscdex It seems to break parallel/test-event-emitter-add-listeners and message/stdin_messages:

=== release test-event-emitter-add-listeners ===
Path: parallel/test-event-emitter-add-listeners
newListener: hello
newListener: foo
start
hello
assert.js:87
  throw new assert.AssertionError({
        ^
AssertionError: [ [Function: listen1] ] deepEqual [ [Function: listen2], [Function: listen1] ]
    at Object.<anonymous> (/home/bnoordhuis/src/v1.x/test/parallel/test-event-emitter-add-listeners.js:67:8)
    at Module._compile (module.js:446:26)
    at Object.Module._extensions..js (module.js:464:10)
    at Module.load (module.js:341:32)
    at Function.Module._load (module.js:296:12)
    at Function.Module.runMain (module.js:487:10)
    at startup (node.js:111:16)
    at node.js:799:3
Command: out/Release/iojs /home/bnoordhuis/src/v1.x/test/parallel/test-event-emitter-add-listeners.js
[01:42|%  90|+ 717|-   1]: release stdin_messages length differs.
# elided, very chatty

mscdex · 2015-02-02T21:53:12Z

That's strange ... I did a clean make test yesterday after rebasing and didn't see any errors. I will do more digging tonight.

bnoordhuis · 2015-02-02T21:58:49Z

I landed PR #687 earlier today. It's possible that it's interacting badly with this PR.

mscdex · 2015-02-03T04:44:50Z

Ok, tests now pass again :-)

Fishrock123 · 2015-02-05T20:10:35Z

lib/events.js

@@ -55,22 +55,67 @@ EventEmitter.prototype.getMaxListeners = function getMaxListeners() {
  return $getMaxListeners(this);
 };

+function emitNone(handler, isFn, self) {


a comment above these might be nice.

Fishrock123 · 2015-02-05T20:12:04Z

ee-add-remove consistently sees an 80+% gain with this patch.

events/ee-add-remove.js n=250000: ./out/Release/iojs: 505750 iojs: 278020 . 81.91%

LGTM. @trevnorris Can you take a look since you were doing #533?

(Side-note: we need some emit() benchmarks.)

mscdex · 2015-02-05T20:19:02Z

RE: additional benchmarks, here are some that I created during my testing FWIW.

Fishrock123 · 2015-02-05T20:20:51Z

Mind PR-ing those soon? :)

mscdex · 2015-02-05T20:27:16Z

@Fishrock123 Done: #730.

Cache events and listeners objects where possible and loop over Object.keys() instead of using for..in. These changes alone give ~60-65% improvement in the ee-add-remove benchmark. The changes to EventEmitter.listenerCount() gives ~14% improvement and changes to emitter.listeners() gives significant improvements for <50 listeners (~195% improvement for 10 listeners). The changes to emitter.emit() gives 3x speedup for the fast cases with multiple handlers and a minor speedup for the slow case with multiple handlers. The swapping out of the util.is* type checking functions with inline checks gives another ~5-10% improvement.

trevnorris · 2015-02-06T19:52:50Z

Haven't taken the time to run the tests myself, but the code LGTM.

bnoordhuis · 2015-02-06T20:20:40Z

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/153/

Fishrock123 · 2015-02-06T21:26:53Z

Some results of the new benchmarks added in 847b9d2 with the iterations in #746

events/ee-add-remove.js n=250000: ./out/Release/iojs: 471160 iojs: 257310 ............ 83.11%
events/ee-emit-multi-args.js n=2000000: ./out/Release/iojs: 4326800 iojs: 1509300 ... 186.68%
events/ee-emit.js n=2000000: ./out/Release/iojs: 6289100 iojs: 1864600 .............. 237.28%
events/ee-listener-count.js n=50000000: ./out/Release/iojs: 234610000 iojs: 229210000 . 2.36%
events/ee-listeners-many.js n=5000000: ./out/Release/iojs: 4197000 iojs: 4659400 ..... -9.92%
events/ee-listeners.js n=5000000: ./out/Release/iojs: 18773000 iojs: 7006500 ........ 167.94%

(I frequently am getting a 5-10% regression on that ee-listeners-many.js benchmark)

mscdex · 2015-02-06T22:42:04Z

Yeah, I wasn't quite sure about the heuristics that I used for ee.listeners(), but it seemed to work well on the system I tested on. Suggestions are welcome :-)

bnoordhuis · 2015-02-08T03:22:29Z

I'm willing to chalk up the events/ee-listeners-many regression as a fluke. The benchmark seems to do the same amount of work with and without this pull request applied: memory usage is about the same, output of --trace_opt --trace_deopt --trace_osr --trace_gc is the same. Everything looks healthy, really.

@Fishrock123 @trevnorris Thoughts?

bnoordhuis · 2015-02-09T16:41:21Z

LGTM, for the record. Can I get one more @iojs/collaborators LGTM?

evanlucas · 2015-02-09T16:46:37Z

LGTM

Cache events and listeners objects where possible and loop over Object.keys() instead of using for..in. These changes alone give ~60-65% improvement in the ee-add-remove benchmark. The changes to EventEmitter.listenerCount() gives ~14% improvement and changes to emitter.listeners() gives significant improvements for <50 listeners (~195% improvement for 10 listeners). The changes to emitter.emit() gives 3x speedup for the fast cases with multiple handlers and a minor speedup for the slow case with multiple handlers. The swapping out of the util.is* type checking functions with inline checks gives another ~5-10% improvement. PR-URL: #601 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com>

bnoordhuis · 2015-02-09T16:51:40Z

Thanks Brian, landed in b677b84.

quantizor reviewed Jan 26, 2015
View reviewed changes

micnic reviewed Jan 26, 2015
View reviewed changes

micnic mentioned this pull request Jan 26, 2015

replace util.isType() with typeof #607

Closed

Fishrock123 reviewed Jan 28, 2015
View reviewed changes

bnoordhuis reviewed Jan 28, 2015
View reviewed changes

mscdex force-pushed the perf-ee-add-remove branch from a13ac82 to f93f44a Compare January 28, 2015 23:20

mscdex force-pushed the perf-ee-add-remove branch from f93f44a to d292fd2 Compare January 29, 2015 01:29

mscdex changed the title ~~events: optimize adding and removing listeners~~ events: optimize various functions Jan 29, 2015

brendanashworth mentioned this pull request Jan 29, 2015

lib: reduce util.is*() usage #647

Closed

quantizor reviewed Jan 29, 2015
View reviewed changes

mscdex force-pushed the perf-ee-add-remove branch from d292fd2 to 05da7a7 Compare January 29, 2015 17:23

mscdex force-pushed the perf-ee-add-remove branch from 4107f84 to faf4841 Compare February 3, 2015 04:44

Fishrock123 reviewed Feb 5, 2015
View reviewed changes

mscdex force-pushed the perf-ee-add-remove branch from faf4841 to bf8d55c Compare February 5, 2015 20:35

brendanashworth mentioned this pull request Feb 6, 2015

benchmark: add more EventEmitter benchmarks #730

Closed

trevnorris mentioned this pull request Feb 6, 2015

WIP events: optimize emit() #533

Closed

mscdex mentioned this pull request Feb 9, 2015

Release proposal: v1.2.0 #768

Closed

bnoordhuis closed this Feb 9, 2015

Fishrock123 added this to the 1.2.0 milestone Feb 9, 2015

narqo mentioned this pull request Mar 29, 2015

Next level! nodules/terror#25

Closed

Gudahtt mentioned this pull request Feb 12, 2020

Migrate to TypeScript MetaMask/safe-event-emitter#1

Merged

8 tasks

juanarbol mentioned this pull request Dec 13, 2023

Removing a signal handler from a process newListener event handler doesn't work correctly #51016

Closed

events: optimize various functions #601

events: optimize various functions #601

Conversation

mscdex commented Jan 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quantizor commented Jan 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jan 26, 2015

Fishrock123 commented Jan 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Jan 28, 2015

micnic commented Jan 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Jan 28, 2015

mscdex commented Jan 28, 2015

mscdex commented Jan 29, 2015

Fishrock123 commented Jan 29, 2015

Fishrock123 commented Jan 29, 2015

mscdex commented Jan 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jan 29, 2015

0 args test

1 arg test

2 args test

3 args test

4 args test

tjconcept commented Jan 31, 2015

mscdex commented Feb 1, 2015

mscdex commented Feb 1, 2015

bnoordhuis commented Feb 2, 2015

mscdex commented Feb 2, 2015

bnoordhuis commented Feb 2, 2015

mscdex commented Feb 3, 2015

Choose a reason for hiding this comment

Fishrock123 commented Feb 5, 2015

mscdex commented Feb 5, 2015

Fishrock123 commented Feb 5, 2015

mscdex commented Feb 5, 2015

trevnorris commented Feb 6, 2015

bnoordhuis commented Feb 6, 2015

Fishrock123 commented Feb 6, 2015

mscdex commented Feb 6, 2015

bnoordhuis commented Feb 8, 2015

bnoordhuis commented Feb 9, 2015

evanlucas commented Feb 9, 2015

bnoordhuis commented Feb 9, 2015