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

apapirovski · 2017-11-26T15:22:42Z

This is a bit broad in scope but all related so hard to separate into independent PRs. There are two commits though to make reviewing easier.

1st commit:

No longer copy array of listeners on emit but rather set a flag and only copy if that same array is modified while the emit is happening.
No longer use delete or Object.create(null) to reset the dictionary object, instead just set to undefined and modify the rest of the codebase to account for this change.
A few other assorted changes to improve performance.

2nd commit:

Introduce new wrappedListeners method which allows the user to retrieve the actual functions stored within _events (such as the once wrappers). We were reaching into _events for this within our own code and it's also one of the most common reasons to reach into _events by user code.
Remove remaining reaches into _events from C++

CitGM came back clean for these changes. Also, here are the results from the Benchmark CI:

events/ee-add-remove.js listeners=1 events=0 n=5000000     88.44 %        *** 3.644977e-199
events/ee-add-remove.js listeners=1 events=5 n=5000000    121.70 %        *** 9.927899e-110
events/ee-add-remove.js listeners=5 events=0 n=5000000     14.42 %        ***  6.703542e-82
events/ee-add-remove.js listeners=5 events=5 n=5000000     22.55 %        *** 8.566324e-104
events/ee-emit.js listeners=10 argc=0 n=20000000           36.31 %        *** 1.895656e-133
events/ee-emit.js listeners=10 argc=10 n=20000000          18.74 %        *** 1.108759e-125
events/ee-emit.js listeners=10 argc=2 n=20000000           20.78 %        *** 1.598679e-116
events/ee-emit.js listeners=10 argc=4 n=20000000           23.10 %        *** 3.275036e-110
events/ee-emit.js listeners=1 argc=0 n=20000000             3.46 %        ***  3.607081e-44
events/ee-emit.js listeners=1 argc=10 n=20000000            2.31 %        ***  1.616438e-12
events/ee-emit.js listeners=1 argc=2 n=20000000             0.63 %             9.366437e-02
events/ee-emit.js listeners=1 argc=4 n=20000000             1.04 %        ***  1.090454e-05
events/ee-emit.js listeners=5 argc=0 n=20000000            36.88 %        *** 5.504055e-157
events/ee-emit.js listeners=5 argc=10 n=20000000           19.54 %        *** 1.898596e-105
events/ee-emit.js listeners=5 argc=2 n=20000000            22.44 %        *** 3.811312e-108
events/ee-emit.js listeners=5 argc=4 n=20000000            25.42 %        *** 6.045287e-119
events/ee-emit-multi.js listeners=10 n=20000000            19.98 %        *** 1.808712e-160
events/ee-emit-multi.js listeners=1 n=20000000              1.46 %        ***  8.833914e-07
events/ee-emit-multi.js listeners=5 n=20000000             22.94 %        *** 1.293491e-115
events/ee-event-names.js n=1000000                        -29.22 %        *** 2.200089e-125
events/ee-listener-count-on-prototype.js n=50000000        -0.09 %             9.372100e-01
events/ee-listeners.js n=50000000                           0.22 %             5.623445e-01
events/ee-listeners-many.js n=10000000                     -0.37 %             4.721418e-01
events/ee-once.js listeners=1 n=5000000                    53.51 %        *** 3.023736e-140
events/ee-once.js listeners=5 n=5000000                     2.83 %        ***  1.692646e-08

Refs: #17074

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

domain, events, process

apapirovski · 2017-11-26T15:23:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11728/

jasnell

Generally looks good. Couple of nits

jasnell · 2017-11-26T16:01:53Z

doc/api/events.md

 -->
- `eventName` {any}
+* `eventName` {any} The name of the event.
+* `unwrap` {boolean} When set to true, will return the original listeners


Not generally a fan of Boolean args in public APIs. Perhaps having a separate unwrappedListeners() method would work?

I think it would need to be wrappedListeners or something similar. Will think on it.

If you want an option argument, I'd prefer to be explicit and call it unwrapOnceListeners (which could also be a string or object instead of a boolean).

However, from a performance point of view new methods for separate input types should always be a good idea.

This should be addressed now, instead we have a wrappedListeners method on EventEmitter.

jasnell · 2017-11-26T16:04:14Z

lib/events.js

-      } else if (typeof list !== 'function') {
-        position = -1;
+        events[type] = undefined;
+        --this._eventsCount;


Having the -- at the start here just looks odd. Having it at the end may be more readable

Yeah, I'm not sure which way to go here. It matches the rest of the events. Feels more confusing if we go back and forth throughout this code. Maybe just convert all the usage within this file to be postfix (where the result isn't being used)?

I'm okay with having consistency in this file.

It probably looks a lot less weird to you if you're used to writing C++ code :)

mscdex · 2017-11-26T16:31:18Z

Why the large eventNames() regression?

apapirovski · 2017-11-26T17:24:56Z

@mscdex We have to iterate over _events now to find where listeners exist as opposed to just undefined. In practice, this seems less of an issue than the performance of on, removeListener and emit. Even packages that use eventNames (such as ultron) seem to benefit since they usually couple it with removeListener.

apapirovski · 2017-11-26T19:17:14Z

@mscdex Refactored that method and now it iterates over event names without Reflect.ownKeys (using for..in & Object.getOwnPropertySymbols) which is mostly faster than it was previously, with the tiny exception being a really small list of events (less than 10) that includes Symbols. For most use cases there's now no regression.

 events/ee-event-names.js symbols="false" listeners=10 n=1000000      9.47 %        *** 2.556083e-48
 events/ee-event-names.js symbols="false" listeners=1 n=1000000      22.61 %        *** 3.692565e-20
 events/ee-event-names.js symbols="true" listeners=10 n=1000000      10.37 %        *** 2.488306e-95
 events/ee-event-names.js symbols="true" listeners=1 n=1000000      -25.36 %        *** 2.084307e-58

addaleax · 2017-11-26T19:57:08Z

doc/api/events.md

 -->
- `eventName` {any}
+* `eventName` {any} The name of the event.
+* `unwrap` {boolean} When set to true, will return the original listeners


If you want an option argument, I'd prefer to be explicit and call it unwrapOnceListeners (which could also be a string or object instead of a boolean).

However, from a performance point of view new methods for separate input types should always be a good idea.

addaleax · 2017-11-26T19:58:43Z

lib/events.js

-      } else if (typeof list !== 'function') {
-        position = -1;
+        events[type] = undefined;
+        --this._eventsCount;


I'm okay with having consistency in this file.

It probably looks a lot less weird to you if you're used to writing C++ code :)

addaleax · 2017-11-26T19:59:46Z

lib/internal/bootstrap_node.js

        caught = process.domain._errorHandler(er);

-      if (!caught)
+      if (shouldCatch && !caught)


I'm still not sure what exactly shouldCatch entails. I'd like to get #17159 done at some point... maybe we can try to do that before this lands?

This part is slightly different than the work within that PR. It's so we don't need ClearFatalExceptionHandlers which deep reaches into a bunch of JS code (including _events).

The part of this PR that would be made obsolete by #17159 is within lib/domain.js (& a bit within node.cc). I'm guessing this could land and then we can refactor later? Not sure where that work is at though so I'm happy to wait if it's close.

Edit: The main reason I say 'land this' is just because it's strictly better than what's there currently which requires actually inspecting the _events object and looking for error handlers, whereas now that responsibility is mostly in JS land and all we check for is a single flag. Far from ideal but a lot better.

At its call sites, `ClearFatalExceptionHandlers()` was used to make the process crash as soon as possible once an exception occurred, without giving JS land a chance to interfere. `ClearFatalExceptionHandlers()` awkwardly removed the current domain and any `uncaughtException` handlers, whereas a clearer way is to execute the relevant reporting (and `exit()`) code directly. Refs: nodejs#17159 Refs: nodejs#17324

apapirovski · 2017-11-27T00:46:53Z

This will need #17333 and #17159 to land, at which point a significant portion of the 2nd commit will go away.

At its call sites, `ClearFatalExceptionHandlers()` was used to make the process crash as soon as possible once an exception occurred, without giving JS land a chance to interfere. `ClearFatalExceptionHandlers()` awkwardly removed the current domain and any `uncaughtException` handlers, whereas a clearer way is to execute the relevant reporting (and `exit()`) code directly. PR-URL: #17333 Refs: #17159 Refs: #17324 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax · 2017-11-29T15:03:02Z

@apapirovski Just merged the other 2 PRs :)

If more than one handler is bound to an event type, avoid copying it on emit and instead set a flag that regulates whether to make a copy when a new event is added or removed. Do not use delete or Object.create(null) when removing listeners, instead set to undefined and modify other methods to account for this.

Refactor lib & src code to eliminate all deep reaches into the internal _events dictionary object, instead use available APIs and add an extra method to EventEmitter: wrappedListeners.

apapirovski · 2017-11-29T15:15:47Z

Updated. PTAL everyone.

CI: https://ci.nodejs.org/job/node-test-pull-request/11804/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1119/

mscdex · 2017-11-29T15:41:52Z

benchmark/events/ee-emit.js

  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;


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

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.

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.

mscdex · 2017-11-29T15:43:11Z

doc/api/events.md

+- `eventName` {any}
+
+Returns a copy of the array of listeners for the event named `eventName`,
+including any wrappers (such as those created by `.once`.


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

mscdex · 2017-11-29T15:44:27Z

doc/api/events.md

+-->
+- `eventName` {any}
+
+Returns a copy of the array of listeners for the event named `eventName`,


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

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

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

mscdex · 2017-11-29T15:45:56Z

lib/events.js

@@ -36,6 +36,7 @@ EventEmitter.usingDomains = false;

 EventEmitter.prototype.domain = undefined;
 EventEmitter.prototype._events = undefined;
+EventEmitter.prototype._eventsCount = 0;


You might check the performance on this, but I remember at least back with Crankshaft, putting properties like this (that are likely to change in every EventEmitter instance) on the prototype could slow things down.

It's definitely fine now from the testing I did. (I ran benchmarks with & without.)

mscdex · 2017-11-29T15:53:16Z

lib/events.js

+  }
+
+  // Check for listener leak
+  const m = $getMaxListeners(target);


Did you benchmark this particular change? I would think that checking list.warned is less expensive than calling out to $getMaxListeners() every time, when .warned === true?

Accessing .warned or any other named key on an Array is really expensive from the testing I've done. Also, even if it wasn't, it's better to optimize for the case where warned === false since that's going to be more common.

(It's been a while but as I can recall, this change alone made roughly 2% difference for cases where warned === false. This was before I added all the major changes that account for the 20-120% difference.)

apapirovski · 2017-12-01T14:33:47Z

@addaleax & @TimothyGu would you consider reviewing this please? You had some feedback for the previous iteration of this in #17074

Thanks!

addaleax

It might take a bit to review this but I’ll try to get to it :)

addaleax · 2017-11-29T19:14:05Z

doc/api/events.md

@@ -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)


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? 😄 )

addaleax

Changes to lib, src, test LGTM

addaleax · 2017-12-01T20:37:41Z

doc/api/events.md

+-->
+- `eventName` {any}
+
+Returns a copy of the array of listeners for the event named `eventName`,


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

addaleax · 2017-12-01T21:21:43Z

lib/events.js

-        if (position === 0)
+        if (list.length === 2)
+          events[type] = list[position ? 0 : 1];
+        else if (list.emitting) {


Tiny nit: Can you add {} for the if as well if there is one for the else part as well?

addaleax · 2017-12-01T21:23:10Z

lib/events.js

          this.removeListener(type, listeners[i]);
        }
      }

      return this;
    };

-EventEmitter.prototype.listeners = function listeners(type) {
-  const events = this._events;
+function _listeners(target, type, unwrap) {


I wonder if it would make sense to turn this into a closure where unwrap can be constant-folded, like:

function makeListeners(unwrap) { return function listeners(type) { // Code of _listeners with `this` as `target` } } EventEmitter.prototype.listeners = makeListeners(true); EventEmitter.prototype.rawListeners = makeListeners(false);

This was just to mimic how we do it elsewhere in this file, namely addListener and prependListener but yeah, this could also work. I suppose the suggested version does close over one variable whereas what I'm guessing happens with the current implementation is that V8 inlines _listeners?

No clue what's preferable. Maybe just stick with consistency for now?

If there is no perf difference, then yes, just stick with this :)

addaleax · 2017-12-01T21:26:01Z

lib/events.js

+  }
+
+  // We must have Symbols to fill in
+  if (j < count) {


Tiny nit: It would make sense to me if the comment was inside the if since it refers to what must be true if the condition is fulfilled... does that make sense?

mscdex · 2017-12-03T08:10:58Z

lib/events.js

-        }
-      } else if (typeof list !== 'function') {
-        position = -1;
+        events[type] = undefined;


This will cause memory leaks.

Yep, I originally ran a modified version of our ee-add-remove benchmark but the 25e4 iterations are not very helpful in testing this. It also regresses the performance once the size of the dictionary object grows (although that seems to not be an issue in later versions of V8, the memory still is).

Mostly I think I was just too optimistic re: user input into the EventEmitter being mostly constant. Probably not a safe assumption.

mscdex · 2017-12-03T08:11:24Z

lib/events.js

-            this._events = Object.create(null);
-          else
-            delete events[type];
+          events[type] = undefined;


This will also cause memory leaks.

At its call sites, `ClearFatalExceptionHandlers()` was used to make the process crash as soon as possible once an exception occurred, without giving JS land a chance to interfere. `ClearFatalExceptionHandlers()` awkwardly removed the current domain and any `uncaughtException` handlers, whereas a clearer way is to execute the relevant reporting (and `exit()`) code directly. PR-URL: #17333 Refs: #17159 Refs: #17324 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

apapirovski requested review from mscdex and addaleax November 26, 2017 15:22

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 26, 2017

jasnell approved these changes Nov 26, 2017

View reviewed changes

apapirovski force-pushed the patch-ee-perf branch 2 times, most recently from 6aeb42a to 73e0d13 Compare November 26, 2017 18:49

addaleax reviewed Nov 26, 2017

View reviewed changes

apapirovski force-pushed the patch-ee-perf branch from 73e0d13 to f0483bd Compare November 26, 2017 20:19

apapirovski added dont-land-on-v4.x labels Nov 26, 2017

addaleax mentioned this pull request Nov 27, 2017

src: remove ClearFatalExceptionHandlers() #17333

Closed

2 tasks

addaleax added the dont-land-on-v8.x label Nov 29, 2017

apapirovski force-pushed the patch-ee-perf branch from f0483bd to 6d230e3 Compare November 29, 2017 15:11

events: remove reaches into _events internals

9534a22

Refactor lib & src code to eliminate all deep reaches into the internal _events dictionary object, instead use available APIs and add an extra method to EventEmitter: wrappedListeners.

apapirovski force-pushed the patch-ee-perf branch from 6d230e3 to 9534a22 Compare November 29, 2017 15:14

mscdex reviewed Nov 29, 2017

View reviewed changes

fixup: missing paren in doc

c3670cd

apapirovski requested review from addaleax and TimothyGu December 1, 2017 14:33

addaleax reviewed Dec 1, 2017

View reviewed changes

fixup: rename wrappedListeners to rawListeners

6e3b199

addaleax approved these changes Dec 1, 2017

View reviewed changes

fixup: address feedback

145bbc3

apapirovski removed the domain Issues and PRs related to the domain subsystem. label Dec 2, 2017

apapirovski mentioned this pull request Dec 2, 2017

timers: minor refinements to TimersList & stricter undefined/null checks #17429

Closed

2 tasks

mscdex reviewed Dec 3, 2017

View reviewed changes

apapirovski added the blocked PRs that are blocked by other issues or PRs. label Dec 3, 2017

apapirovski mentioned this pull request Dec 3, 2017

events: remove reaches into _events internals #17440

Closed

4 tasks

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

apapirovski closed this Dec 24, 2017

		@@ -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)

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

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

Conversation

apapirovski commented Nov 26, 2017 • edited

Checklist

Affected core subsystem(s)

apapirovski commented Nov 26, 2017

jasnell left a 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

apapirovski Nov 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Nov 26, 2017

apapirovski commented Nov 26, 2017 • edited

apapirovski commented Nov 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Nov 26, 2017 • edited

Choose a reason for hiding this comment

apapirovski commented Nov 27, 2017

addaleax commented Nov 29, 2017

apapirovski commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Nov 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Nov 29, 2017 • edited

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

mscdex Nov 29, 2017 • edited

Choose a reason for hiding this comment

apapirovski Nov 29, 2017 • edited

Choose a reason for hiding this comment

apapirovski commented Dec 1, 2017

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax left a 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

apapirovski Dec 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Nov 26, 2017 •

edited

apapirovski Nov 26, 2017 •

edited

apapirovski commented Nov 26, 2017 •

edited

apapirovski Nov 26, 2017 •

edited

mscdex Nov 29, 2017 •

edited

apapirovski Nov 29, 2017 •

edited

mscdex Nov 29, 2017 •

edited

apapirovski Nov 29, 2017 •

edited

apapirovski Dec 3, 2017 •

edited