performance,events: use Map to store events in EventEmitter #21856

lundibundi · 2018-07-17T18:18:58Z

I've had some time to play around with Map as backing storage for EE and the results turned out to be pretty good. I'm not sure if this is an appropriate change so feel free to note this and close.

If this is acceptable then this should probably be a semver-major, even though _events is 'private'.

EE benchmarks

 ➔ dev/node/node cat compare-events-pr.csv | Rscript benchmark/compare.R        59s events-perf :: ● :: ⬡
                                                     confidence improvement accuracy (*)    (**)   (***)
 events/ee-add-remove.js n=1000000                          ***     11.29 %       ±5.75%  ±7.70% ±10.14%
 events/ee-emit.js listeners=1 argc=0 n=2000000             ***     16.70 %       ±8.86% ±11.89% ±15.66%
 events/ee-emit.js listeners=1 argc=10 n=2000000                     8.69 %      ±10.88% ±14.58% ±19.18%
 events/ee-emit.js listeners=1 argc=2 n=2000000                      5.96 %       ±7.76% ±10.41% ±13.73%
 events/ee-emit.js listeners=1 argc=4 n=2000000              **     10.72 %       ±7.08%  ±9.49% ±12.51%
 events/ee-emit.js listeners=10 argc=0 n=2000000                     2.64 %      ±10.67% ±14.30% ±18.80%
 events/ee-emit.js listeners=10 argc=10 n=2000000                    1.67 %       ±7.19%  ±9.63% ±12.67%
 events/ee-emit.js listeners=10 argc=2 n=2000000                    -0.33 %       ±5.85%  ±7.85% ±10.36%
 events/ee-emit.js listeners=10 argc=4 n=2000000                    -4.62 %       ±6.34%  ±8.51% ±11.24%
 events/ee-emit.js listeners=5 argc=0 n=2000000                     -2.17 %       ±9.37% ±12.56% ±16.53%
 events/ee-emit.js listeners=5 argc=10 n=2000000                    -0.05 %       ±8.56% ±11.48% ±15.11%
 events/ee-emit.js listeners=5 argc=2 n=2000000                     -0.02 %       ±8.38% ±11.24% ±14.80%
 events/ee-emit.js listeners=5 argc=4 n=2000000                      4.58 %       ±6.30%  ±8.45% ±11.15%
 events/ee-listener-count-on-prototype.js n=50000000        ***     22.49 %       ±8.90% ±12.00% ±15.95%
 events/ee-listeners-many.js n=5000000                        *      9.11 %       ±7.27%  ±9.80% ±13.02%
 events/ee-listeners.js n=5000000                                   -3.90 %       ±8.99% ±12.04% ±15.84%
 events/ee-once.js n=20000000                                        0.76 %       ±4.47%  ±5.99%  ±7.87%

Also I'd like some help on understanding why is .clear() is so much worse than creating a new Map, I thought that this should be the other way around. (it shows regression of around -25% for ee-once). Comments with _events.clear() are there to show where I intended to use it. I'll remove them later.

Perf with new Map

Perf with map.clear()

Perf of master if needed:

Checklist

make -j4 test (UNIX) passes
benchmarks are included
documentation is changed or added (there is single mention in events.md in the console log of EE)
commit message follows commit guidelines

/cc @nodejs/performance

addaleax · 2018-07-17T18:23:05Z

I love this.

I do think we’ve seen people take a stab at this before – this is quite a breaking change, unfortunately. Do you think it would be possible to store the map behind a Symbol, add a getter/setter pair for _events that returns some kind of Proxy which does The Right Thing™, to minimize breakage? I realize it’s quite a bit of work but it might be worth it.

TimothyGu · 2018-07-17T18:25:59Z

See previous PR in #17074.

Looping in @apapirovski.

benjamingr · 2018-07-17T18:28:06Z

This also means event names don't have to be strings (or symbols) anymore.

benjamingr · 2018-07-17T18:35:52Z

lib/events.js

-          if (events.removeListener)
+        if (--this._eventsCount === 0) {
+          // this._events.clear();
+          this._events = new Map();


What happens if we remove the if and always delete ?

I've tried that initially but there is a significant regression for ee-once if we do that, seems that either Map creation is cheaper or there is some missed optimization. Maybe someone can take a look.

➔ dev/node/node cat compare-events-pr-1.csv | Rscript benchmark/compare.R confidence improvement accuracy (*) (**) (***) events/ee-add-remove.js n=1000000 3.38 % ±4.91% ±6.73% ±9.16% events/ee-emit.js listeners=1 argc=0 n=2000000 -0.73 % ±13.90% ±19.27% ±26.76% events/ee-emit.js listeners=1 argc=10 n=2000000 *** 15.68 % ±7.91% ±10.91% ±15.04% events/ee-emit.js listeners=1 argc=2 n=2000000 8.14 % ±9.82% ±13.45% ±18.34% events/ee-emit.js listeners=1 argc=4 n=2000000 9.24 % ±9.50% ±13.11% ±18.05% events/ee-emit.js listeners=10 argc=0 n=2000000 -0.54 % ±10.02% ±13.75% ±18.80% events/ee-emit.js listeners=10 argc=10 n=2000000 1.89 % ±8.26% ±11.33% ±15.48% events/ee-emit.js listeners=10 argc=2 n=2000000 -2.79 % ±11.40% ±15.66% ±21.44% events/ee-emit.js listeners=10 argc=4 n=2000000 2.88 % ±6.85% ±9.38% ±12.78% events/ee-emit.js listeners=5 argc=0 n=2000000 0.59 % ±11.30% ±15.49% ±21.11% events/ee-emit.js listeners=5 argc=10 n=2000000 2.33 % ±9.27% ±12.72% ±17.36% events/ee-emit.js listeners=5 argc=2 n=2000000 0.10 % ±11.37% ±15.61% ±21.33% events/ee-emit.js listeners=5 argc=4 n=2000000 -2.23 % ±9.85% ±13.50% ±18.41% events/ee-listener-count-on-prototype.js n=50000000 *** 31.36 % ±8.61% ±12.06% ±17.04% events/ee-listeners-many.js n=5000000 0.69 % ±12.55% ±17.44% ±24.30% events/ee-listeners.js n=5000000 -2.08 % ±8.51% ±11.78% ±16.31% events/ee-once.js n=20000000 *** -26.48 % ±5.40% ±7.42% ±10.16%

diff:

@ events.js:323 @ EventEmitter.prototype.removeListener = return this; if (list === listener || list.listener === listener) { - if (--this._eventsCount === 0) { - // this._events.clear(); - this._events = new Map(); - } else { - events.delete(type); - if (events.has('removeListener')) - this.emit('removeListener', type, list.listener || listener); - } + --this._eventsCount; + events.delete(type); + if (events.has('removeListener')) + this.emit('removeListener', type, list.listener || listener); } else if (typeof list !== 'function') { position = -1; @ events.js:376 @ EventEmitter.prototype.removeAllListeners = this._events = new Map(); this._eventsCount = 0; } else if (events.has(type)) { - if (--this._eventsCount === 0) - this._events = new Map(); - // this._events.clear(); - else - events.delete(type); + --this._eventsCount; + events.delete(type); } return this; }

Prof:

prof json file (github doesn't like .json, so I renamed it to .txt)
v8-clean-remove.txt

lundibundi · 2018-07-17T18:38:12Z

@addaleax That's what I initially thought to do I just wanted to gather feedback before I implement it to avoid needless work 👍.
Though could you elaborate on where Symbol'ed map should be stored in EE? I thought of maybe replacing _events with es6 Proxy and storing the map as something like _events_raw.

mcollina · 2018-07-17T18:49:24Z

_events is used everywhere in place of prependListener. See the hack we have to do in readable-stream to support older Node.js releases: nodejs/readable-stream@25ec4c4.

I would store things in Symbol('events') and expose _events as a proxy.

I exposed my opinion on this approach in #17074 (comment), which is:

backport the change in https://github.com/nodejs/readable-stream/blob/d6c391d7101946c8b8b97914fc46fd3322c450d1/lib/_stream_readable.js#L89-L101 to readable-stream@1.0.x and readable-stream@1.1.x.
release those lines, and check with NPM the download numbers by version.
~~deprecate all the previous readable-stream releases, this will annoy users.~~ This is already in effect due to the security checks, some of the older lines were impossible to update for the Buffer constructor vulns.
after point 1-4, check if the usage of the offending versions goes down, merit of semver and the deprecation warning.

readable-stream is by far the main offender for this, and the last time I checked (1 year ago) the download numbers of the unpatched versions of readable-stream were still in the millions. I can ask npm for some more data.

jasnell · 2018-07-17T18:56:03Z

FWIW, I've even come across (extremely not good) code that replaces _events entirely. It's horrible and it sucks. A change like this is significantly breaking, even if it is highly desirable (which it is). We have to be very careful on this.

jasnell · 2018-07-17T19:02:29Z

One approach, that does not absolutely ensure backwards compat but also isn't as bad, would be:

Change _events into a getter/setter, such that, by default, get _events() returns a proxy object that wraps the Symbol('events') (as suggested by @mcollina).

For set _events(), the passed in object is used to overwrite the Map entirely.

A challenge with this approach is that the following would not be true unless we retained a reference to bad_code and did some magic in the proxy.

const bad_code = { a: () => { /* handle event */ } }
const ee = new EventEmitter()
ee._events = bad_code
ee._events === bad_code  /// false!
ee._events == bad_code   /// false!

lundibundi · 2018-07-17T19:24:19Z

@jasnell Do I understand it correctly that this proxy will then be used in events.js code itself? imo this will make this PR improvements negligible. Otherwise suggested setter implementation will probably be not possible.
I'm +1 on @mcollina approach with this and don't think we should go as far as supporting overwriting _events.

I'll try to implement a proxy now.

Also, it would be great if someone familiar with V8 could take a look at #21856 (comment).

mscdex · 2018-07-17T20:07:35Z

Hrmmm, the benchmark results in CI show something different. The only benchmark the two sets of results seem to mostly "agree" on is events/ee-listener-count-on-prototype.js.

lundibundi · 2018-07-17T20:21:47Z

@mscdex Hmm, I'll try to rebuild both versions and run them again. Maybe recent commits in master changed something or there was something outdated in my build (I've also started using ccache so maybe it was the problem).

Edit:
@mscdex well, I'm still getting results somewhat higher than CI, that's strange. Though indeed it seems my node-master was outdated.

                                                     confidence improvement accuracy (*)    (**)   (***)
 events/ee-add-remove.js n=1000000                          ***      7.55 %       ±3.48%  ±4.64%  ±6.04%
 events/ee-emit.js listeners=1 argc=0 n=2000000                      1.41 %       ±6.11%  ±8.13% ±10.59%
 events/ee-emit.js listeners=1 argc=10 n=2000000             **      7.43 %       ±5.32%  ±7.08%  ±9.23%
 events/ee-emit.js listeners=1 argc=2 n=2000000               *      7.06 %       ±5.72%  ±7.61%  ±9.92%
 events/ee-emit.js listeners=1 argc=4 n=2000000                      5.38 %       ±5.90%  ±7.86% ±10.24%
 events/ee-emit.js listeners=10 argc=0 n=2000000                    -1.95 %       ±5.02%  ±6.69%  ±8.71%
 events/ee-emit.js listeners=10 argc=10 n=2000000                    0.09 %       ±4.82%  ±6.42%  ±8.36%
 events/ee-emit.js listeners=10 argc=2 n=2000000                    -2.25 %       ±4.76%  ±6.33%  ±8.24%
 events/ee-emit.js listeners=10 argc=4 n=2000000                    -3.14 %       ±4.40%  ±5.86%  ±7.63%
 events/ee-emit.js listeners=5 argc=0 n=2000000                     -0.36 %       ±5.39%  ±7.18%  ±9.34%
 events/ee-emit.js listeners=5 argc=10 n=2000000                     2.22 %       ±4.87%  ±6.49%  ±8.46%
 events/ee-emit.js listeners=5 argc=2 n=2000000                     -2.86 %       ±5.27%  ±7.02%  ±9.13%
 events/ee-emit.js listeners=5 argc=4 n=2000000                     -1.36 %       ±4.70%  ±6.26%  ±8.14%
 events/ee-listener-count-on-prototype.js n=50000000        ***     19.95 %       ±6.68%  ±8.94% ±11.76%
 events/ee-listeners-many.js n=5000000                              11.56 %      ±16.35% ±21.75% ±28.32%
 events/ee-listeners.js n=5000000                                    2.96 %      ±14.46% ±19.24% ±25.05%
 events/ee-once.js n=20000000                                       -1.90 %       ±5.88%  ±7.83% ±10.19%

apapirovski · 2018-07-18T02:33:50Z

So I've done this before, including a proxy for _events without which this could never land. The problem is that perf looks good until you add more events and then it's worse. I'm -1 on this happening for the forseeable future.

apapirovski · 2018-07-18T02:38:46Z

The PR linked above already did all the hard work for the proxy btw. https://github.com/nodejs/node/pull/17074/files

lundibundi · 2018-07-18T09:40:16Z

@apapirovski I don't see anything related to proxy in the diff of #17074. Did you maybe use this branch for another PR and removed the changes?

Also, I've added your benchmarks, though I'm not sure if there is a need to change n for different listenerCount, does it really improve the statistical result?

For my current proxy defined with defineProperty (I want to change the way I store proxy slightly before I push) passes all tests without my commit to modify tests (I only changed test-benchmark-events to include new parameters) and has the following benchmark results

 ➔ dev/node/node cat compare-events-pr-proxy.csv | Rscript benchmark/compare.R
                                                        confidence improvement accuracy (*)    (**)   (***)
 events/ee-add-remove.js listeners=1 events=0 n=5000000                 2.28 %       ±4.73%  ±6.33%  ±8.31%
 events/ee-add-remove.js listeners=1 events=5 n=5000000          *     -5.43 %       ±4.68%  ±6.23%  ±8.11%
 events/ee-add-remove.js listeners=5 events=0 n=5000000          *      5.17 %       ±4.63%  ±6.17%  ±8.07%
 events/ee-add-remove.js listeners=5 events=5 n=5000000                -1.75 %       ±3.96%  ±5.27%  ±6.87%
 events/ee-emit-multi.js listeners=1 n=20000000                        -1.33 %       ±3.74%  ±5.00%  ±6.56%
 events/ee-emit-multi.js listeners=10 n=20000000                       -0.35 %       ±2.81%  ±3.75%  ±4.89%
 events/ee-emit-multi.js listeners=5 n=20000000                        -2.50 %       ±3.03%  ±4.03%  ±5.25%
 events/ee-emit.js listeners=1 argc=0 n=2000000                ***     13.72 %       ±3.83%  ±5.10%  ±6.65%
 events/ee-emit.js listeners=1 argc=10 n=2000000               ***     11.57 %       ±3.66%  ±4.87%  ±6.34%
 events/ee-emit.js listeners=1 argc=2 n=2000000                ***     12.42 %       ±3.71%  ±4.94%  ±6.42%
 events/ee-emit.js listeners=1 argc=4 n=2000000                ***     11.55 %       ±4.73%  ±6.30%  ±8.20%
 events/ee-emit.js listeners=10 argc=0 n=2000000                        1.68 %       ±4.95%  ±6.58%  ±8.57%
 events/ee-emit.js listeners=10 argc=10 n=2000000                       1.01 %       ±3.82%  ±5.08%  ±6.61%
 events/ee-emit.js listeners=10 argc=2 n=2000000                       -1.08 %       ±4.32%  ±5.75%  ±7.48%
 events/ee-emit.js listeners=10 argc=4 n=2000000                       -0.95 %       ±3.70%  ±4.92%  ±6.40%
 events/ee-emit.js listeners=5 argc=0 n=2000000                         1.64 %       ±4.86%  ±6.46%  ±8.41%
 events/ee-emit.js listeners=5 argc=10 n=2000000                        1.37 %       ±4.11%  ±5.46%  ±7.11%
 events/ee-emit.js listeners=5 argc=2 n=2000000                        -1.29 %       ±3.71%  ±4.93%  ±6.42%
 events/ee-emit.js listeners=5 argc=4 n=2000000                         1.47 %       ±3.64%  ±4.84%  ±6.30%
 events/ee-event-names.js n=1000000                            ***    -20.95 %       ±2.40%  ±3.21%  ±4.20%
 events/ee-listener-count-on-prototype.js n=50000000           ***     25.46 %       ±6.20%  ±8.28% ±10.86%
 events/ee-listeners-many.js n=5000000                                  5.71 %       ±7.84% ±10.43% ±13.58%
 events/ee-listeners.js n=5000000                              ***      4.63 %       ±2.35%  ±3.13%  ±4.08%
 events/ee-once.js listenerCount=1 n=5000000                           -4.62 %       ±5.34%  ±7.10%  ±9.25%
 events/ee-once.js listenerCount=5 n=5000000                   ***      9.69 %       ±4.27%  ±5.68%  ±7.40%

I haven't had time to investigate the issue with ee-event-names but I'll take a look soon.

apapirovski · 2018-07-18T15:15:53Z

Yeah, it's in a different branch. I wouldn't recommend spending more time on this given the benchmark results above. There's not much point to making a breaking change like this without significant improvement on the most significant benchmark (ee-add-remove).

https://github.com/apapirovski/node/blob/73c104d47b6ca32e677e2666b408f1a781820841/lib/events.js

apapirovski

The benchmark results aren't good enough — in fact there are as many regressions as improvements — to break most of the ecosystem.

BridgeAR · 2018-07-18T15:28:33Z

I tend to agree with @apapirovski. I think the benefit does not outweigh the negative side.

lundibundi · 2018-07-18T18:03:53Z

@apapirovski well, I've been playing around with replacing delete with set(key, undefined) and here are the results:

                                                                      confidence improvement accuracy (*)   (**)   (***)
 events/ee-add-remove.js listenerCount=1 staleEventsCount=0 n=5000000                -0.94 %       ±6.22% ±8.27% ±10.78%
 events/ee-add-remove.js listenerCount=1 staleEventsCount=5 n=5000000        ***     43.41 %       ±6.74% ±8.98% ±11.71%
 events/ee-add-remove.js listenerCount=5 staleEventsCount=0 n=5000000                 1.62 %       ±6.57% ±8.74% ±11.39%
 events/ee-add-remove.js listenerCount=5 staleEventsCount=5 n=5000000          *      6.04 %       ±4.79% ±6.37%  ±8.29%
 events/ee-emit-multi.js listenerCount=1 n=20000000                                   1.27 %       ±3.29% ±4.40%  ±5.77%
 events/ee-emit-multi.js listenerCount=10 n=20000000                                 -1.35 %       ±1.72% ±2.29%  ±2.99%
 events/ee-emit-multi.js listenerCount=5 n=20000000                          ***     -3.63 %       ±1.48% ±1.98%  ±2.59%
 events/ee-emit.js listenerCount=1 argc=0 n=2000000                          ***      9.01 %       ±2.35% ±3.13%  ±4.08%
 events/ee-emit.js listenerCount=1 argc=10 n=2000000                         ***     11.72 %       ±3.12% ±4.15%  ±5.41%
 events/ee-emit.js listenerCount=1 argc=2 n=2000000                          ***     14.31 %       ±3.21% ±4.27%  ±5.55%
 events/ee-emit.js listenerCount=1 argc=4 n=2000000                          ***     13.78 %       ±2.63% ±3.51%  ±4.57%
 events/ee-emit.js listenerCount=10 argc=0 n=2000000                           *      3.16 %       ±2.69% ±3.60%  ±4.73%
 events/ee-emit.js listenerCount=10 argc=10 n=2000000                                 1.63 %       ±1.71% ±2.28%  ±2.98%
 events/ee-emit.js listenerCount=10 argc=2 n=2000000                                 -0.84 %       ±1.65% ±2.21%  ±2.89%
 events/ee-emit.js listenerCount=10 argc=4 n=2000000                                  0.65 %       ±1.62% ±2.16%  ±2.83%
 events/ee-emit.js listenerCount=5 argc=0 n=2000000                                   0.53 %       ±2.99% ±3.98%  ±5.19%
 events/ee-emit.js listenerCount=5 argc=10 n=2000000                                  0.66 %       ±2.20% ±2.93%  ±3.82%
 events/ee-emit.js listenerCount=5 argc=2 n=2000000                                  -2.03 %       ±2.29% ±3.06%  ±4.00%
 events/ee-emit.js listenerCount=5 argc=4 n=2000000                                   0.09 %       ±1.97% ±2.62%  ±3.42%
 events/ee-event-names.js n=1000000                                          ***    -50.48 %       ±3.25% ±4.33%  ±5.67%
 events/ee-listener-count-on-prototype.js n=50000000                         ***     25.40 %       ±5.33% ±7.18%  ±9.52%
 events/ee-listeners-many.js n=10000000                                      ***      5.63 %       ±1.61% ±2.15%  ±2.80%
 events/ee-listeners.js n=5000000                                                    -1.93 %       ±3.93% ±5.24%  ±6.83%
 events/ee-once.js listenerCount=1 n=5000000                                  **     -6.68 %       ±4.76% ±6.33%  ±8.24%

(eventNames now actually filters all undefineds so this regression is obvious)

There seems to be some problem with map.delete in V8. I've been trying to investigate this but to no avail yet. In this comment #21856 (comment), If I understand it correctly you can see that in constantly tries to Map Shrink and replace with new underlying storage upon delete which seems wrong.

Well, as I said in the first post, I'm okay with closing this one if it doesn't bring much merit.

lpinca · 2018-07-18T19:45:08Z

set(key, undefined) creates a leak which is the reason why we don't use _events[key] = undefined;.

lundibundi · 2018-07-18T21:38:25Z

@lpinca Yeah, I'm aware, I wanted to check it compared to map.delete because it seems that it's doing too much work.
Edit: currently I am also skeptical about this one because of the issues with map.delete as there is no other way (that I know of) to properly delete items from a map.

lundibundi · 2018-07-22T12:30:33Z

Could someone please run the benchmarks?
As I was close to finishing this, I've pushed my changes at least for the sake of history later.
I've added a simple V8 fix of making delete not shrink unless the capacity of the Map is more than 16 (capacity is #buckets * 2), at least that's similar to what v8 does for WeakMap https://github.com/nodejs/node/blob/master/deps/v8/src/builtins/builtins-collections-gen.cc#L2337, I not sure about this change at all as I haven't really looked into the v8 codebase but it seemed simple enough to try out. (I'm aware that v8 patches should be submitted upstream usually, I just wanted to check the performance with it not only on my machine).

benjamingr · 2018-07-22T14:21:16Z

Could someone please run the benchmarks?

Sure: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/215/

lundibundi · 2018-07-22T19:03:05Z

Well they indeed doesn't look very good even with the V8 patch:

                                                                      confidence improvement accuracy (*)    (**)   (***)
 events/ee-add-remove.js listenerCount=1 staleEventsCount=0 n=5000000                -0.11 %       ±3.96%  ±5.27%  ±6.86%
 events/ee-add-remove.js listenerCount=1 staleEventsCount=5 n=5000000                -1.32 %       ±3.53%  ±4.71%  ±6.17%
 events/ee-add-remove.js listenerCount=5 staleEventsCount=0 n=5000000        ***     -3.49 %       ±1.90%  ±2.53%  ±3.29%
 events/ee-add-remove.js listenerCount=5 staleEventsCount=5 n=5000000                -1.92 %       ±3.98%  ±5.35%  ±7.06%
 events/ee-emit.js listenerCount=10 argc=0 n=2000000                                  0.17 %       ±1.21%  ±1.61%  ±2.09%
 events/ee-emit.js listenerCount=10 argc=10 n=2000000                        ***      3.57 %       ±1.32%  ±1.75%  ±2.29%
 events/ee-emit.js listenerCount=10 argc=2 n=2000000                                 -0.42 %       ±3.24%  ±4.35%  ±5.75%
 events/ee-emit.js listenerCount=10 argc=4 n=2000000                                  0.66 %       ±1.11%  ±1.47%  ±1.92%
 events/ee-emit.js listenerCount=1 argc=0 n=2000000                                  -0.37 %       ±5.74%  ±7.64%  ±9.94%
 events/ee-emit.js listenerCount=1 argc=10 n=2000000                                  3.53 %       ±4.36%  ±5.85%  ±7.69%
 events/ee-emit.js listenerCount=1 argc=2 n=2000000                                  -5.22 %       ±5.83%  ±7.80% ±10.24%
 events/ee-emit.js listenerCount=1 argc=4 n=2000000                                  -3.83 %       ±4.30%  ±5.73%  ±7.46%
 events/ee-emit.js listenerCount=5 argc=0 n=2000000                                   0.56 %       ±2.48%  ±3.31%  ±4.30%
 events/ee-emit.js listenerCount=5 argc=10 n=2000000                          **      2.70 %       ±1.70%  ±2.26%  ±2.95%
 events/ee-emit.js listenerCount=5 argc=2 n=2000000                                  -2.07 %       ±2.91%  ±3.89%  ±5.10%
 events/ee-emit.js listenerCount=5 argc=4 n=2000000                                   0.00 %       ±2.13%  ±2.84%  ±3.71%
 events/ee-emit-multi.js listenerCount=10 n=20000000                                 -0.86 %       ±1.46%  ±1.95%  ±2.54%
 events/ee-emit-multi.js listenerCount=1 n=20000000                           **     -6.32 %       ±4.59%  ±6.13%  ±8.02%
 events/ee-emit-multi.js listenerCount=5 n=20000000                           **     -3.54 %       ±2.10%  ±2.80%  ±3.64%
 events/ee-event-names.js n=1000000                                          ***    940.20 %      ±17.09% ±23.03% ±30.57%
 events/ee-listener-count-on-prototype.js n=50000000                         ***     15.24 %       ±5.28%  ±7.03%  ±9.15%
 events/ee-listeners.js n=5000000                                              *     -2.91 %       ±2.74%  ±3.65%  ±4.76%
 events/ee-listeners-many.js n=10000000                                      ***      4.31 %       ±1.54%  ±2.05%  ±2.67%
 events/ee-once.js listenerCount=10 n=5000000                                         0.63 %       ±1.81%  ±2.41%  ±3.14%
 events/ee-once.js listenerCount=1 n=5000000                                 ***     13.29 %       ±4.08%  ±5.45%  ±7.13%
 events/ee-once.js listenerCount=5 n=5000000                                   *      2.56 %       ±1.92%  ±2.57%  ±3.39%

But at least no severe regressions, I don't think these results are 'good enough' but maybe there are indeed some bugs in v8 (or our specific usage of the map prevents some optimizations) that keep this from performing better, as in my simple Map/object benchmarks Map was usually smth like 1.4-2 times better in any use case without my patch (only 1 element map performed just slightly better (1.1-1.2) than object) and my patch fixed this use-case of one element map.

simonkcleung · 2018-07-23T07:02:46Z

Don't know why the target of proxy is the emitter, rather than the new Map object.
If you want to inspect the emitter._events or set the prototype of emitter._events, you are actually inspecting/setting the emitter object, which is misleading.

lundibundi · 2018-07-23T09:06:17Z

@simonkcleung emitter._events is just a plug to not break those that use this object too badly, it shouldn't be used at all. As for the setPrototypeOf I'll probably disallow it by returning false from the appropriate method. Also, what do you mean by 'inspect'?

simonkcleung · 2018-07-23T11:00:28Z

@lundibundi I mean other operations of the proxy handler object are missing, e.g. something in emitter._events ? ... : ... or simply console.log(emitter._events). It wouldn't be a problem if the target is the new Map object.

Using setPrototypeOf let you add many event listeners at one go, or something like default action of an event emitter. If it was disallowed, it should raise an error.

addaleax

Basically LGTM

addaleax · 2018-07-23T13:07:32Z

lib/events.js

+    let proxy = this[kEventsProxy];
+    if (proxy === undefined && this[kEvents] !== undefined)
+      proxy = new Proxy(this, proxyEventsHandler);
+    return proxy;


I think you want to set this[kEventsProxy] here as well, right?

addaleax · 2018-07-23T13:09:47Z

lib/events.js

+    if (!value || Array.isArray(value) && value.length === 0) {
+      this[kEvents] = new Map();
+      this._eventsCount = 0;
+    }


Are we okay with making this a no-op if value is a "real" events object?

This looks wrong. Should value not be an object with properties? I expect it to look something like:

if (value) { this[kEvents] = new Map(Object.entries(value)); this._eventsCount = this[kEvents].size; }

addaleax · 2018-07-23T13:12:02Z

lib/events.js

+        var keys = new Array(len);
+        i = 0;
+        for (var value of events.keys())
+          keys[i++] = value;


Is this much faster than var keys = [...events.keys()];? I'm not sure that this would be particularly hot code

I just ran a benchmark against this and it does indeed seem to be much better the way it is. I open an issue about it for the V8 team.

https://bugs.chromium.org/p/v8/issues/detail?id=7973

mcollina · 2018-07-23T13:28:09Z

What is events/ee-emit-multi.js verifying that events/ee-emit.js does not?

I'm a bit concerned about that regression.

IMHO we should check how this impacts HTTP and Stream. They are both heavy users of EE, and if it does not regress there we should be grand.

apapirovski · 2018-07-23T14:00:26Z

@addaleax @mcollina why would we land something that does not improve performance? Honestly confused here. As much as a lot of work went into this, it unfortunately doesn't bring anything to the table. There are more regressions than improvements.

mcollina · 2018-07-23T14:05:03Z

I’m not LGTM this just yet. I want to understand how this impacts other parts of the codebase.

I’m specifically interested in the improvements in the once flow.

apapirovski · 2018-07-23T14:16:22Z

I’m specifically interested in the improvements in the once flow.

It's a bad benchmark mostly.

As for, ee-emit-multi tests that performance with multiple events doesn't regress. Our benchmarks mostly test a single event type with 1 or more listeners. (It could also be improved. It's something I quickly threw together last time.)

Streams benchmark https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/216/

apapirovski · 2018-07-23T14:43:12Z

Anyway, I'm going to remain -1 on this because Maps have exponentially decreasing performance as their size grows. Here's a result from ee-once that uses an EventEmitter that already has 100 events attached:

                                             confidence improvement accuracy (*)   (**)   (***)
 events/ee-once.js listenerCount=1 n=5000000        ***    -68.67 %       ±2.42% ±3.28%  ±4.40%
 events/ee-once.js listenerCount=5 n=5000000        ***    -34.84 %       ±5.83% ±8.08% ±11.19%

BridgeAR

@nodejs/v8 PTAL if the v8 change actually makes sense or not.

I personally am -0.5 on this as the numbers do not really show much profit. @lundibundi thanks a lot for doing this though since it is good to see current numbers with different approaches.

BridgeAR · 2018-07-23T13:51:32Z

lib/events.js

+        var keys = new Array(len);
+        i = 0;
+        for (var value of events.keys())
+          keys[i++] = value;


I just ran a benchmark against this and it does indeed seem to be much better the way it is. I open an issue about it for the V8 team.

BridgeAR · 2018-07-23T13:52:52Z

lib/events.js

-          else
-            delete events[type];
+        } else if (events.delete(type)) {
+          --this._eventsCount;


I think _eventsCount is now obsolete if we just use events.size instead.

BridgeAR · 2018-07-23T13:58:04Z

lib/events.js

+};
+
+Object.defineProperty(EventEmitter.prototype, '_events', {
+  enumerable: true,


It should likely also be configurable.

BridgeAR · 2018-07-23T14:07:14Z

lib/events.js

+    if (!value || Array.isArray(value) && value.length === 0) {
+      this[kEvents] = new Map();
+      this._eventsCount = 0;
+    }


This looks wrong. Should value not be an object with properties? I expect it to look something like:

if (value) { this[kEvents] = new Map(Object.entries(value)); this._eventsCount = this[kEvents].size; }

BridgeAR · 2018-07-23T14:10:11Z

benchmark/events/ee-add-remove.js

@@ -2,16 +2,23 @@
 const common = require('../common.js');
 const events = require('events');

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


Do we really have to push up the iterations here?

BridgeAR · 2018-07-23T14:10:51Z

benchmark/events/ee-emit-multi.js

+const EventEmitter = require('events').EventEmitter;
+
+const bench = common.createBenchmark(main, {
+  n: [2e7],


This seems to be a pretty high iteration count.

BridgeAR · 2018-07-23T14:11:19Z

benchmark/events/ee-listeners-many.js

@@ -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] });


Do we have to increase the iterations here?

apapirovski · 2018-07-23T14:54:06Z

And this is the ee-add-remove result when using 20 events, instead of 5.

                                                                       confidence improvement accuracy (*)   (**)  (***)
 events/ee-add-remove.js listenerCount=1 staleEventsCount=0 n=5000000         ***     -8.70 %       ±3.47% ±4.67% ±6.17%
 events/ee-add-remove.js listenerCount=1 staleEventsCount=20 n=5000000        ***    -15.89 %       ±4.64% ±6.21% ±8.15%

And here's 100 events:

                                                                        confidence improvement accuracy (*)    (**)   (***)
 events/ee-add-remove.js listenerCount=1 staleEventsCount=100 n=5000000        ***    -72.74 %      ±14.14% ±19.59% ±27.20%

mscdex

-1 on this as well. I think this shows that Map is still not really a viable alternative.

lundibundi · 2018-07-23T17:39:19Z

@apapirovski thanks, I didn't check if the regression was increasing with event size.
Well, this is indeed not appropriate and I guess we should wait for probably 'another Map' as this one clearly performs even worse that Object. Though, I looked at prof for master and v8 actually makes our _events a Dictionary backed by HashTable which is somewhat of an ancestor of OrderedHashTable which is a Map. It seems that restrictions placed on a Map by the standart are too much I guess.
Though I'm still interested in the Stream benchmark.

Edit: at least that's what I understood from this perf:

BridgeAR · 2018-07-25T12:18:52Z

@lundibundi thanks a lot for all your work that you put into this! Since it seems like the change itself regresses performance in some cases while improving it in others without showing a significant gain in general, I am going to close this.

nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Jul 17, 2018

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 17, 2018

benjamingr reviewed Jul 17, 2018

View reviewed changes

apapirovski requested changes Jul 18, 2018

View reviewed changes

ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Jul 18, 2018

lundibundi added 3 commits July 22, 2018 15:11

events: use Map in events.js to store _events

246c7ca

fixup! implement proxy for each EE, add benchmarks

a3af552

try V8 fix of limiting Map shrink until capacity >= 16

e694796

lundibundi force-pushed the events-perf branch from 73eac9a to e694796 Compare July 22, 2018 12:17

addaleax reviewed Jul 23, 2018

View reviewed changes

BridgeAR reviewed Jul 23, 2018

View reviewed changes

mscdex suggested changes Jul 23, 2018

View reviewed changes

BridgeAR closed this Jul 25, 2018

apapirovski mentioned this pull request Dec 29, 2021

lib: faster event listening #41309

Closed

performance,events: use Map to store events in EventEmitter #21856

performance,events: use Map to store events in EventEmitter #21856

Conversation

lundibundi commented Jul 17, 2018

Checklist

addaleax commented Jul 17, 2018

TimothyGu commented Jul 17, 2018 • edited Loading

benjamingr commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi commented Jul 17, 2018

mcollina commented Jul 17, 2018

jasnell commented Jul 17, 2018

jasnell commented Jul 17, 2018

lundibundi commented Jul 17, 2018

mscdex commented Jul 17, 2018 • edited Loading

lundibundi commented Jul 17, 2018 • edited Loading

apapirovski commented Jul 18, 2018

apapirovski commented Jul 18, 2018

lundibundi commented Jul 18, 2018 • edited Loading

apapirovski commented Jul 18, 2018

apapirovski left a comment • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Jul 18, 2018

lundibundi commented Jul 18, 2018 • edited Loading

lpinca commented Jul 18, 2018

lundibundi commented Jul 18, 2018 • edited Loading

lundibundi commented Jul 22, 2018

benjamingr commented Jul 22, 2018

lundibundi commented Jul 22, 2018 • edited Loading

simonkcleung commented Jul 23, 2018

lundibundi commented Jul 23, 2018 • edited Loading

simonkcleung commented Jul 23, 2018 • edited Loading

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

mcollina commented Jul 23, 2018

apapirovski commented Jul 23, 2018

mcollina commented Jul 23, 2018

apapirovski commented Jul 23, 2018

apapirovski commented Jul 23, 2018 • edited Loading

BridgeAR 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 commented Jul 23, 2018 • edited Loading

mscdex left a comment

Choose a reason for hiding this comment

lundibundi commented Jul 23, 2018 • edited Loading

BridgeAR commented Jul 25, 2018

TimothyGu commented Jul 17, 2018 •

edited

Loading

mscdex commented Jul 17, 2018 •

edited

Loading

lundibundi commented Jul 17, 2018 •

edited

Loading

lundibundi commented Jul 18, 2018 •

edited

Loading

apapirovski left a comment •

edited

Loading

lundibundi commented Jul 18, 2018 •

edited

Loading

lundibundi commented Jul 18, 2018 •

edited

Loading

lundibundi commented Jul 22, 2018 •

edited

Loading

lundibundi commented Jul 23, 2018 •

edited

Loading

simonkcleung commented Jul 23, 2018 •

edited

Loading

apapirovski commented Jul 23, 2018 •

edited

Loading

apapirovski commented Jul 23, 2018 •

edited

Loading

lundibundi commented Jul 23, 2018 •

edited

Loading