events: several perf related adjustments by apapirovski · Pull Request #16212 · nodejs/node · GitHub

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

Why this was changed back to !events?

Complete accident. Will push a fix shortly.

Ditto.

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

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/

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.

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.

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

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.

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.

@mscdex Can test this shortly. Good point.

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.

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

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

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.

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

@apapirovski you should be able to it yourself.

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

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.

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 <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Landed in f61cc15, c8d4ff1, fd166a8, d5fb789 and fe13e00

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

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

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

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

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

@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 <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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

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

events: several perf related adjustments by apapirovski · Pull Request #16212 · nodejs/node · GitHub

Uh oh!

Uh oh!

events: several perf related adjustments - #16212

events: several perf related adjustments#16212apapirovski wants to merge 6 commits intonodejs:masternodejs/node:masterfrom apapirovski:patch-events-strict-prop-checksapapirovski/node:patch-events-strict-prop-checksCopy head branch name to clipboard

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)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Oct 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Oct 15, 2017

Uh oh!

apapirovski commented Oct 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Oct 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Oct 15, 2017

Uh oh!

apapirovski commented Oct 15, 2017

Uh oh!

apapirovski commented Oct 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Oct 15, 2017

Uh oh!

apapirovski commented Oct 16, 2017

Uh oh!

lpinca commented Oct 16, 2017

Uh oh!

apapirovski commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Oct 16, 2017

Uh oh!

lpinca commented Oct 17, 2017

Uh oh!

apapirovski commented Oct 21, 2017

Uh oh!

apapirovski commented Oct 21, 2017

Uh oh!

MylesBorins commented Oct 23, 2017

Uh oh!

mcollina commented Oct 26, 2017

Uh oh!

apapirovski commented Oct 26, 2017

Uh oh!

targos commented Oct 26, 2017

Uh oh!

mcollina commented Oct 26, 2017

Uh oh!

MylesBorins commented Aug 17, 2018

Uh oh!

jasnell commented Aug 17, 2018

events: several perf related adjustments#16212
apapirovski wants to merge 6 commits into
nodejs:masterfrom
apapirovski:patch-events-strict-prop-checks

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