events: several perf related adjustments #16212

apapirovski · 2017-10-15T06:12:50Z

This PR covers a few different perf related changes to EventEmitter. Everything is separated into stand-alone commits. Might be easier to review as such but overall there actually there aren't that many lines being changed so it should be pretty easy to get through.

All performance benchmarks cited below are based on 100 or 200 sets (as necessary to get the right certainty) and are independent of other changes within the PR (so something like the onceWrapper result is strictly for that change and does not include the changes that precede it).

Feedback & reviews greatly appreciated!

events: stricter prop & variable checks for perf

Replace truthy/falsey checks of EventEmitter.prototype._events and EventEmitter.prototype._events[type] with comparisons to undefined for better performance:

events/ee-add-remove.js n=250000    5.30 %    *** 4.260028e-07
events/ee-emit.js n=2000000         4.18 %    *** 1.026649e-05

This has a knock-on effect on modules that use lots of events, e.g.:

http2/headers.js nheaders=0 n=1000  2.60 %    *** 0.000298338

It's possible this could affect user-land code if users are interacting with _events directly. We should strongly consider running CITGM. Maybe this is semver-major? Not sure since it's not documented and leading _ obviously signifies a private prop.

events: remove unnecessary console instantiation

Previously, console had to be compiled in case it was not available but this is no longer necessary - remove it.

Refs: #15111

events: return values directly in listeners

Each conditional branch in EventEmitter.prototype.listeners assigns its return value to a variable ret which is returned at the end. Instead just return from within each branch. This is both clearer and more performant.

events/ee-listeners.js n=5000000      3.65 %        *** 3.359171e-10

events: use spread function param in emit

With recent changes in V8, it is now as performant or faster to use spread parameter within EventEmitter.prototype.emit, especially in cases where looping over arguments is required.

events/ee-emit.js n=2000000               4.40 %    *** 1.505543e-06
events/ee-emit-1-arg.js n=2000000         2.16 %    *** 2.434584e-10
events/ee-emit-2-args.js n=2000000        1.05 %     ** 0.001764852
events/ee-emit-3-args.js n=2000000        2.18 %    *** 3.234954e-08
events/ee-emit-6-args.js n=2000000       17.17 %    *** 1.298702e-103
events/ee-emit-10-args.js n=2000000      17.14 %    *** 1.144958e-97

This has a knock-on effect for modules that use events extensively, such as http2:

http2/headers.js nheaders=0 n=1000        2.10 %    *** 6.792106e-11

events: onceWrapper apply directly with arguments

Due to changes in V8, it's no longer necessary to copy arguments to avoid deopt. Just call apply with arguments. Retains fast cases for 0-3 arguments.

events/ee-once-4-args.js n=20000000     11.58 %      *** 1.310379e-05

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

events

Replace truthy/falsey checks of _events and _events[type] with comparisons to undefined for better performance: events/ee-add-remove.js n=250000 5.30 % *** 4.260028e-07 events/ee-emit.js n=2000000 4.18 % *** 1.026649e-05 This has a knock-on effect on modules that use lots of events, e.g.: http2/headers.js nheaders=0 n=1000 2.60 % *** 0.000298338

Previously, console had to be compiled in case it was not available but this is no longer necessary - remove it. Refs: nodejs#15111

lpinca · 2017-10-15T07:01:57Z

lib/events.js


-  if (events === undefined)
-    ret = [];
+  if (!events)


Why this was changed back to !events?

Complete accident. Will push a fix shortly.

lpinca · 2017-10-15T07:02:12Z

lib/events.js

-    if (evlistener === undefined)
-      ret = [];
+    const evlistener = events[type];
+    if (!evlistener)


Same as above, total accident. Fixed now. Thanks for flagging!

joyeecheung · 2017-10-15T08:30:51Z

I started a benchmark CI job, the 19 one (still in the queue at the moment): https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/

mscdex · 2017-10-15T14:33:49Z

lib/events.js

-  if (events)
-    doError = (doError && events.error == null);
+  const events = this._events;
+  if (events !== undefined)


Changes like this really warrant a CITGM run.

Yeah, I completely agree. 👍 I tried to flag it in my description of the PR but it's a bit of a wall of text.

mscdex · 2017-10-15T14:36:08Z

Did you benchmark this against master or a node canary branch? I just checked using spread in master the other week for emit() and it was singificantly slower still.

apapirovski · 2017-10-15T15:14:21Z

@mscdex Master. I mean it's slower if we completely switch to using spread (it loses out big time for the 0-3 cases) but this is sort of a hybrid approach.

(When I say completely, I mean getting rid of the separate emit functions and inlining it with spread.)

apapirovski · 2017-10-15T15:20:25Z

Benchmark CI results:

events/ee-add-remove.js n=250000                         3.96 %        *** 3.698348e-19
 events/ee-emit.js n=2000000                              4.00 %        *** 3.956165e-09
 events/ee-emit-multi-args.js n=2000000                   1.17 %        *** 8.071210e-04
 events/ee-listener-count-on-prototype.js n=50000000      9.38 %        *** 7.890460e-04
 events/ee-listeners.js n=5000000                         3.07 %        *** 2.906789e-07
 events/ee-listeners-many.js n=5000000                    5.37 %         ** 4.763790e-03
 events/ee-once.js n=20000000                             3.80 %        *** 3.043686e-13

Keep in mind we don't have a bench for passing emit more than 2 args. The ee-emit-multi-args is just 2 arguments and matches my results above.

mscdex · 2017-10-15T15:24:00Z

Have you tested this on current node v8.x as well? If v8.x does not get V8 6.2 then we want to make sure we don't include a performance regression in that branch.

apapirovski · 2017-10-15T15:24:25Z

@mscdex Can test this shortly. Good point.

apapirovski · 2017-10-15T17:36:13Z

events/ee-emit.js n=2000000      5.84 %        *** 7.900974e-09
events/ee-emit-multi-args.js n=2000000      2.25 %        *** 5.169845e-05
events/ee-emit-6-args.js n=2000000     20.76 %        *** 3.152791e-32

Just a quick test on v8.x to make sure there aren't any regressions. That said, I think the most sensible approach would be to have a separate backport PR & run the benchmark CI there. The first commit in here needs to be backported anyway as it doesn't land cleanly.

BridgeAR · 2017-10-15T17:43:49Z

lib/events.js

    else if (typeof evlistener === 'function')
-      ret = [evlistener.listener || evlistener];
+      return [evlistener.listener || evlistener];
    else


The else conditions become obsolete here due to the return

Speaking of, I think the whole thing can be adjusted to not be an 'else' since the first if returns. I'll force push an adjusted commit.

That is what I implied 😄

Each conditional branch in EventEmitter.prototype.listeners assigns its return value to a variable ret which is returned at the end. Instead just return from within each branch. This is both clearer and more performant. events/ee-listeners.js n=5000000 3.65 % *** 3.359171e-10

With recent changes in V8, it is now as performant or faster to use spread parameter within EventEmitter.prototype.emit, especially in cases where looping over arguments is required. events/ee-emit.js n=2000000 4.40 % *** 1.505543e-06 events/ee-emit-1-arg.js n=2000000 2.16 % *** 2.434584e-10 events/ee-emit-2-args.js n=2000000 1.05 % ** 0.001764852 events/ee-emit-3-args.js n=2000000 2.18 % *** 3.234954e-08 events/ee-emit-6-args.js n=2000000 17.17 % *** 1.298702e-103 events/ee-emit-10-args.js n=2000000 17.14 % *** 1.144958e-97 This has a knock-on effect for modules that use events extensively, such as http2: http2/headers.js nheaders=0 n=1000 2.10 % *** 6.792106e-11

Due to changes in V8 in 6.0 & 6.1, it's no longer necessary to copy arguments to avoid deopt. Just call .apply with arguments. Retains fast cases for 0-3 arguments. events/ee-once-4-args.js n=20000000 11.58 % *** 1.310379e-05

apapirovski · 2017-10-15T18:09:33Z

Replaced all else & else if branches with if in EventEmitter.prototype.listeners since they all return.

apapirovski · 2017-10-16T18:26:23Z

Could somebody please start a CITGM for this? Want to make sure the whole world doesn't burn because of the first commit that changes _events checks to compare to undefined.

lpinca · 2017-10-16T18:34:04Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1012/

@apapirovski you should be able to it yourself.

apapirovski · 2017-10-16T20:30:01Z

@lpinca not quite there yet, not until Wednesday :) Thanks for starting it!

apapirovski · 2017-10-16T20:39:05Z

No clue how to interpret the results. No obvious red flags but some of these failures seem to be a regular occurrence and with others I can't tell.

lpinca · 2017-10-17T05:51:15Z

Yes, honestly I also don't know how to interpret CITGM results. Not sure what are the expected failures nowadays.
cc: @refack

Due to changes in V8 in 6.0 & 6.1, it's no longer necessary to copy arguments to avoid deopt. Just call .apply with arguments. Retains fast cases for 0-3 arguments. events/ee-once-4-args.js n=20000000 11.58 % *** 1.310379e-05 PR-URL: nodejs#16212 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

apapirovski · 2017-10-21T13:32:20Z

Landed in f61cc15, c8d4ff1, fd166a8, d5fb789 and fe13e00

apapirovski · 2017-10-21T18:18:21Z

@jasnell I would love to see this land in 9.0.0 if at all possible so it can begin baking immediately. Thanks!

MylesBorins · 2017-10-23T22:21:45Z

This does not land cleanly in 8.x

It would need to be manually backported. As it seems this perf improvement needs some time to bake we may want to wait for it to live in 9.x for a bit before backporting

mcollina · 2017-10-26T08:05:53Z

@MylesBorins this needs to be benchmarked again when landed on 8. It likely needs V8 6.2.

apapirovski · 2017-10-26T08:17:54Z

@mcarter @MylesBorins This has been tested against v8.x already as @mscdex requested it. Most of the perf improvements were nearly identical.

targos · 2017-10-26T08:21:51Z

Actually I don't think it was tested on V8 6.2. Master still was on V8 6.1 when you published the benchmark numbers

mcollina · 2017-10-26T08:24:40Z

@targos I think we had escape analysis enabled on master at that point

Replace truthy/falsey checks of _events and _events[type] with comparisons to undefined for better performance: events/ee-add-remove.js n=250000 5.30 % *** 4.260028e-07 events/ee-emit.js n=2000000 4.18 % *** 1.026649e-05 This has a knock-on effect on modules that use lots of events, e.g.: http2/headers.js nheaders=0 n=1000 2.60 % *** 0.000298338 PR-URL: nodejs/node#16212 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Previously, console had to be compiled in case it was not available but this is no longer necessary - remove it. PR-URL: nodejs/node#16212 Refs: nodejs/node#15111 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Each conditional branch in EventEmitter.prototype.listeners assigns its return value to a variable ret which is returned at the end. Instead just return from within each branch. This is both clearer and more performant. events/ee-listeners.js n=5000000 3.65 % *** 3.359171e-10 PR-URL: nodejs/node#16212 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

With recent changes in V8, it is now as performant or faster to use spread parameter within EventEmitter.prototype.emit, especially in cases where looping over arguments is required. events/ee-emit.js n=2000000 4.40 % *** 1.505543e-06 events/ee-emit-1-arg.js n=2000000 2.16 % *** 2.434584e-10 events/ee-emit-2-args.js n=2000000 1.05 % ** 0.001764852 events/ee-emit-3-args.js n=2000000 2.18 % *** 3.234954e-08 events/ee-emit-6-args.js n=2000000 17.17 % *** 1.298702e-103 events/ee-emit-10-args.js n=2000000 17.14 % *** 1.144958e-97 This has a knock-on effect for modules that use events extensively, such as http2: http2/headers.js nheaders=0 n=1000 2.10 % *** 6.792106e-11 PR-URL: nodejs/node#16212 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Due to changes in V8 in 6.0 & 6.1, it's no longer necessary to copy arguments to avoid deopt. Just call .apply with arguments. Retains fast cases for 0-3 arguments. events/ee-once-4-args.js n=20000000 11.58 % *** 1.310379e-05 PR-URL: nodejs/node#16212 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Replace truthy/falsey checks of _events and _events[type] with comparisons to undefined for better performance: events/ee-add-remove.js n=250000 5.30 % *** 4.260028e-07 events/ee-emit.js n=2000000 4.18 % *** 1.026649e-05 This has a knock-on effect on modules that use lots of events, e.g.: http2/headers.js nheaders=0 n=1000 2.60 % *** 0.000298338 PR-URL: nodejs/node#16212 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Previously, console had to be compiled in case it was not available but this is no longer necessary - remove it. PR-URL: nodejs/node#16212 Refs: nodejs/node#15111 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Each conditional branch in EventEmitter.prototype.listeners assigns its return value to a variable ret which is returned at the end. Instead just return from within each branch. This is both clearer and more performant. events/ee-listeners.js n=5000000 3.65 % *** 3.359171e-10 PR-URL: nodejs/node#16212 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

With recent changes in V8, it is now as performant or faster to use spread parameter within EventEmitter.prototype.emit, especially in cases where looping over arguments is required. events/ee-emit.js n=2000000 4.40 % *** 1.505543e-06 events/ee-emit-1-arg.js n=2000000 2.16 % *** 2.434584e-10 events/ee-emit-2-args.js n=2000000 1.05 % ** 0.001764852 events/ee-emit-3-args.js n=2000000 2.18 % *** 3.234954e-08 events/ee-emit-6-args.js n=2000000 17.17 % *** 1.298702e-103 events/ee-emit-10-args.js n=2000000 17.14 % *** 1.144958e-97 This has a knock-on effect for modules that use events extensively, such as http2: http2/headers.js nheaders=0 n=1000 2.10 % *** 6.792106e-11 PR-URL: nodejs/node#16212 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Due to changes in V8 in 6.0 & 6.1, it's no longer necessary to copy arguments to avoid deopt. Just call .apply with arguments. Retains fast cases for 0-3 arguments. events/ee-once-4-args.js n=20000000 11.58 % *** 1.310379e-05 PR-URL: nodejs/node#16212 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

MylesBorins · 2018-08-17T19:08:23Z

Quick ping to see if we want to include this in v8.x

jasnell · 2018-08-17T19:22:24Z

At this point I don't see a backport to 8.x as being critical

apapirovski added 2 commits October 14, 2017 20:49

events: remove unnecessary console instantiation

75571fb

Previously, console had to be compiled in case it was not available but this is no longer necessary - remove it. Refs: nodejs#15111

nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Oct 15, 2017

lpinca reviewed Oct 15, 2017

View reviewed changes

apapirovski force-pushed the patch-events-strict-prop-checks branch from f1b2966 to d16c788 Compare October 15, 2017 07:06

lpinca approved these changes Oct 15, 2017

View reviewed changes

mscdex reviewed Oct 15, 2017

View reviewed changes

mscdex added performance Issues and PRs related to the performance of Node.js. dont-land-on-v4.x labels Oct 15, 2017

BridgeAR approved these changes Oct 15, 2017

View reviewed changes

apapirovski added 3 commits October 15, 2017 14:07

events: onceWrapper apply directly with arguments

0585047

Due to changes in V8 in 6.0 & 6.1, it's no longer necessary to copy arguments to avoid deopt. Just call .apply with arguments. Retains fast cases for 0-3 arguments. events/ee-once-4-args.js n=20000000 11.58 % *** 1.310379e-05

apapirovski force-pushed the patch-events-strict-prop-checks branch from d16c788 to 0585047 Compare October 15, 2017 18:07

apapirovski requested a review from mscdex October 18, 2017 21:18

apapirovski closed this Oct 21, 2017

apapirovski deleted the patch-events-strict-prop-checks branch October 21, 2017 13:46

apapirovski added this to the 9.0.0 milestone Oct 21, 2017

MylesBorins added the backported-to-v8.x label Oct 23, 2017

MylesBorins added backport-requested-v8.x and removed backported-to-v8.x labels Oct 23, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

MylesBorins added dont-land-on-v8.x and removed backport-requested-v8.x baking-for-lts PRs that need to wait before landing in a LTS release. labels Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: several perf related adjustments #16212

events: several perf related adjustments #16212

apapirovski commented Oct 15, 2017

lpinca Oct 15, 2017

apapirovski Oct 15, 2017

lpinca Oct 15, 2017

apapirovski Oct 15, 2017

joyeecheung commented Oct 15, 2017

mscdex Oct 15, 2017

apapirovski Oct 15, 2017

mscdex commented Oct 15, 2017

apapirovski commented Oct 15, 2017 •

edited

Loading

apapirovski commented Oct 15, 2017 •

edited

Loading

mscdex commented Oct 15, 2017

apapirovski commented Oct 15, 2017

apapirovski commented Oct 15, 2017 •

edited

Loading

BridgeAR Oct 15, 2017

apapirovski Oct 15, 2017

BridgeAR Oct 15, 2017

apapirovski commented Oct 15, 2017

apapirovski commented Oct 16, 2017

lpinca commented Oct 16, 2017

apapirovski commented Oct 16, 2017 •

edited

Loading

apapirovski commented Oct 16, 2017

lpinca commented Oct 17, 2017

apapirovski commented Oct 21, 2017

apapirovski commented Oct 21, 2017

MylesBorins commented Oct 23, 2017

mcollina commented Oct 26, 2017

apapirovski commented Oct 26, 2017

targos commented Oct 26, 2017

mcollina commented Oct 26, 2017

MylesBorins commented Aug 17, 2018

jasnell commented Aug 17, 2018

events: several perf related adjustments #16212

events: several perf related adjustments #16212

Conversation

apapirovski commented Oct 15, 2017

events: stricter prop & variable checks for perf

events: remove unnecessary console instantiation

events: return values directly in listeners

events: use spread function param in emit

events: onceWrapper apply directly with arguments

Checklist

Affected core subsystem(s)

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

joyeecheung commented Oct 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Oct 15, 2017

apapirovski commented Oct 15, 2017 • edited Loading

apapirovski commented Oct 15, 2017 • edited Loading

mscdex commented Oct 15, 2017

apapirovski commented Oct 15, 2017

apapirovski commented Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Oct 15, 2017

apapirovski commented Oct 16, 2017

lpinca commented Oct 16, 2017

apapirovski commented Oct 16, 2017 • edited Loading

apapirovski commented Oct 16, 2017

lpinca commented Oct 17, 2017

apapirovski commented Oct 21, 2017

apapirovski commented Oct 21, 2017

MylesBorins commented Oct 23, 2017

mcollina commented Oct 26, 2017

apapirovski commented Oct 26, 2017

targos commented Oct 26, 2017

mcollina commented Oct 26, 2017

MylesBorins commented Aug 17, 2018

jasnell commented Aug 17, 2018

apapirovski commented Oct 15, 2017 •

edited

Loading

apapirovski commented Oct 15, 2017 •

edited

Loading

apapirovski commented Oct 15, 2017 •

edited

Loading

apapirovski commented Oct 16, 2017 •

edited

Loading