events: refactor to use more primordials by aduh95 · Pull Request #36304 · nodejs/node · GitHub

Checklist

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

This seems to cause some performance regressions:

events/ee-add-remove.js n=1000000                                ***    -20.86 %       ±2.52% ±3.37%  ±4.41%
events/ee-once.js argc=0 n=20000000                              ***     -8.22 %       ±3.13% ±4.17%  ±5.43%
events/ee-once.js argc=1 n=20000000                              ***     -7.36 %       ±2.12% ±2.83%  ±3.68%
events/ee-once.js argc=4 n=20000000                              ***     -6.46 %       ±2.22% ±2.97%  ±3.88%
events/eventtarget.js listeners=10 n=1000000                     ***    -10.51 %       ±5.93% ±7.91% ±10.33%
events/eventtarget.js listeners=5 n=1000000                      ***     -9.26 %       ±4.76% ±6.35%  ±8.30%

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/727/ (queued, will 404 until it starts)

Perf regression is gone for EventTarget, but still here for EventEmitter:

                                                          confidence improvement accuracy (*)   (**)   (***)
events/ee-add-remove.js n=1000000                                ***    -23.23 %       ±2.38% ±3.19%  ±4.21%
events/ee-emit.js listeners=10 argc=4 n=2000000                    *     -2.29 %       ±2.07% ±2.78%  ±3.68%
events/ee-once.js argc=0 n=20000000                              ***     -7.31 %       ±2.24% ±3.01%  ±3.97%
events/ee-once.js argc=1 n=20000000                              ***     -7.74 %       ±3.02% ±4.02%  ±5.24%
events/ee-once.js argc=4 n=20000000                              ***     -6.01 %       ±2.38% ±3.17%  ±4.14%
events/ee-once.js argc=5 n=20000000                              ***     -7.03 %       ±3.08% ±4.10%  ±5.35%

I'll try to investigate this further.

Co-authored-by: Darshan Sen <raisinten@gmail.com>

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/762/

@RaisinTen it seems it didn't have much impact at all:

                                                          confidence improvement accuracy (*)   (**)   (***)
events/ee-add-remove.js n=1000000                                ***    -23.25 %       ±2.52% ±3.36%  ±4.39%
events/ee-once.js argc=0 n=20000000                              ***     -7.67 %       ±2.05% ±2.74%  ±3.61%
events/ee-once.js argc=1 n=20000000                               **     -5.72 %       ±3.92% ±5.28%  ±6.98%
events/ee-once.js argc=4 n=20000000                              ***     -7.81 %       ±2.13% ±2.86%  ±3.76%
events/ee-once.js argc=5 n=20000000                              ***     -7.59 %       ±2.09% ±2.80%  ±3.67%

Details

                                                          confidence improvement accuracy (*)   (**)   (***)
events/ee-add-remove.js n=1000000                                ***    -23.25 %       ±2.52% ±3.36%  ±4.39%
events/ee-emit.js listeners=10 argc=0 n=2000000                          -0.20 %       ±1.36% ±1.80%  ±2.35%
events/ee-emit.js listeners=10 argc=10 n=2000000                          0.16 %       ±1.21% ±1.62%  ±2.11%
events/ee-emit.js listeners=10 argc=2 n=2000000                           0.09 %       ±1.03% ±1.37%  ±1.79%
events/ee-emit.js listeners=10 argc=4 n=2000000                           0.18 %       ±2.15% ±2.87%  ±3.74%
events/ee-emit.js listeners=1 argc=0 n=2000000                           -1.34 %       ±3.99% ±5.31%  ±6.92%
events/ee-emit.js listeners=1 argc=10 n=2000000                           3.31 %       ±5.12% ±6.82%  ±8.87%
events/ee-emit.js listeners=1 argc=2 n=2000000                           -2.13 %       ±3.91% ±5.20%  ±6.78%
events/ee-emit.js listeners=1 argc=4 n=2000000                            0.93 %       ±3.73% ±4.97%  ±6.47%
events/ee-emit.js listeners=5 argc=0 n=2000000                            0.09 %       ±2.06% ±2.74%  ±3.56%
events/ee-emit.js listeners=5 argc=10 n=2000000                           0.53 %       ±1.87% ±2.49%  ±3.24%
events/ee-emit.js listeners=5 argc=2 n=2000000                            0.66 %       ±1.61% ±2.15%  ±2.80%
events/ee-emit.js listeners=5 argc=4 n=2000000                           -0.64 %       ±2.31% ±3.07%  ±4.01%
events/ee-listener-count-on-prototype.js n=50000000                      -0.24 %       ±1.87% ±2.50%  ±3.27%
events/ee-listeners.js raw='false' listeners=50 n=5000000                 0.31 %       ±1.90% ±2.53%  ±3.30%
events/ee-listeners.js raw='false' listeners=5 n=5000000                  0.42 %       ±3.97% ±5.29%  ±6.89%
events/ee-listeners.js raw='true' listeners=50 n=5000000                  0.87 %       ±1.63% ±2.17%  ±2.83%
events/ee-listeners.js raw='true' listeners=5 n=5000000                  -1.04 %       ±3.23% ±4.31%  ±5.62%
events/ee-once.js argc=0 n=20000000                              ***     -7.67 %       ±2.05% ±2.74%  ±3.61%
events/ee-once.js argc=1 n=20000000                               **     -5.72 %       ±3.92% ±5.28%  ±6.98%
events/ee-once.js argc=4 n=20000000                              ***     -7.81 %       ±2.13% ±2.86%  ±3.76%
events/ee-once.js argc=5 n=20000000                              ***     -7.59 %       ±2.09% ±2.80%  ±3.67%
events/eventtarget.js listeners=10 n=1000000                             -1.50 %       ±3.20% ±4.27%  ±5.58%
events/eventtarget.js listeners=1 n=1000000                               0.46 %       ±6.48% ±8.62% ±11.23%
events/eventtarget.js listeners=5 n=1000000                              -1.25 %       ±4.33% ±5.78%  ±7.57%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 25 comparisons, you can thus
expect the following amount of false-positive results:
 1.25 false positives, when considering a   5% risk acceptance (*, **, ***),
 0.25 false positives, when considering a   1% risk acceptance (**, ***),
 0.03 false positives, when considering a 0.1% risk acceptance (***)

@aduh95 hmm, I just found what I missed:

if (result !== undefined && result !== null) {

This guarded the addCatch calls. You were right, it didn't get called because ()=>{} returns undefined.

How did u fix the performance issue in EventTarget?

I was thinking about reverting the ArrayPrototypeShift substitution. It wasn't there in the file previously and it's the only primordial that gets used that most in both on and removeListener.

Did u ever notice primordials causing any performance issues before? I wonder what the cause might be.

This reverts commit e3d0bb1.

This reverts commit 940bd12.

I was thinking about reverting the ArrayPrototypeShift substitution. It wasn't there in the file previously and it's the only primordial that gets used that most in both on and removeListener.

It seems to indeed have a significant impact on events/ee-add-remove.js:

                                                          confidence improvement accuracy (*)   (**)  (***)
events/ee-add-remove.js n=1000000                                ***     -5.98 %       ±2.71% ±3.62% ±4.73%
events/ee-once.js argc=0 n=20000000                              ***     -8.87 %       ±1.88% ±2.51% ±3.26%
events/ee-once.js argc=1 n=20000000                              ***     -9.81 %       ±1.36% ±1.81% ±2.36%
events/ee-once.js argc=4 n=20000000                              ***    -11.64 %       ±2.24% ±2.98% ±3.88%
events/ee-once.js argc=5 n=20000000                              ***     -7.59 %       ±2.45% ±3.28% ±4.33%

Details

                                                          confidence improvement accuracy (*)   (**)  (***)
events/ee-add-remove.js n=1000000                                ***     -5.98 %       ±2.71% ±3.62% ±4.73%
events/ee-emit.js listeners=10 argc=0 n=2000000                          -0.80 %       ±1.28% ±1.70% ±2.21%
events/ee-emit.js listeners=10 argc=10 n=2000000                   *     -1.37 %       ±1.15% ±1.54% ±2.00%
events/ee-emit.js listeners=10 argc=2 n=2000000                           1.48 %       ±1.67% ±2.24% ±2.96%
events/ee-emit.js listeners=10 argc=4 n=2000000                          -1.10 %       ±2.05% ±2.73% ±3.57%
events/ee-emit.js listeners=1 argc=0 n=2000000                            0.09 %       ±4.18% ±5.58% ±7.28%
events/ee-emit.js listeners=1 argc=10 n=2000000                    *     -3.58 %       ±3.20% ±4.27% ±5.59%
events/ee-emit.js listeners=1 argc=2 n=2000000                            0.84 %       ±4.00% ±5.33% ±6.94%
events/ee-emit.js listeners=1 argc=4 n=2000000                           -3.26 %       ±3.83% ±5.10% ±6.65%
events/ee-emit.js listeners=5 argc=0 n=2000000                           -0.27 %       ±2.09% ±2.79% ±3.65%
events/ee-emit.js listeners=5 argc=10 n=2000000                           0.15 %       ±1.73% ±2.30% ±3.00%
events/ee-emit.js listeners=5 argc=2 n=2000000                     *     -2.12 %       ±1.81% ±2.40% ±3.13%
events/ee-emit.js listeners=5 argc=4 n=2000000                           -0.74 %       ±1.47% ±1.96% ±2.56%
events/ee-listener-count-on-prototype.js n=50000000                       0.18 %       ±5.37% ±7.15% ±9.30%
events/ee-listeners.js raw='false' listeners=50 n=5000000                 1.69 %       ±2.14% ±2.86% ±3.76%
events/ee-listeners.js raw='false' listeners=5 n=5000000                  0.17 %       ±1.99% ±2.65% ±3.46%
events/ee-listeners.js raw='true' listeners=50 n=5000000                  2.52 %       ±2.74% ±3.65% ±4.75%
events/ee-listeners.js raw='true' listeners=5 n=5000000                  -1.29 %       ±3.12% ±4.15% ±5.41%
events/ee-once.js argc=0 n=20000000                              ***     -8.87 %       ±1.88% ±2.51% ±3.26%
events/ee-once.js argc=1 n=20000000                              ***     -9.81 %       ±1.36% ±1.81% ±2.36%
events/ee-once.js argc=4 n=20000000                              ***    -11.64 %       ±2.24% ±2.98% ±3.88%
events/ee-once.js argc=5 n=20000000                              ***     -7.59 %       ±2.45% ±3.28% ±4.33%
events/eventtarget.js listeners=10 n=1000000                             -2.43 %       ±3.43% ±4.61% ±6.10%
events/eventtarget.js listeners=1 n=1000000                               1.04 %       ±3.63% ±4.87% ±6.40%
events/eventtarget.js listeners=5 n=1000000                               3.40 %       ±5.11% ±6.83% ±8.94%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 25 comparisons, you can thus
expect the following amount of false-positive results:
 1.25 false positives, when considering a   5% risk acceptance (*, **, ***),
 0.25 false positives, when considering a   1% risk acceptance (**, ***),
 0.03 false positives, when considering a 0.1% risk acceptance (***)

How did u fix the performance issue in EventTarget?

By replacing ReflectApply calls with FunctionPrototypeCall mostly.

Should we prefer safety over performance here?

Well, Array.prototype.shift and Array.prototype.unshift have to also move all the items in the array, in addition to adding or removing the items, whereas Array.prototype.push and Array.prototype.pop only have to add or remove items.

In fact, Array.prototype.shift and Array.prototype.unshift have O(n+m) and O(m) complexity respectively (n = number of added items; m = number of existing items in the array), whereas Array.prototype.push and Array.prototype.pop have O(n) and O(1) complexity respectively.

That said, there probably isn’t much that we can do here, given that Array.prototype.unshift is necessary to support prepending listeners:

node/lib/events.js

Lines 446 to 455 in df98f27

    
           if (typeof existing === 'function') { 
        
             // Adding the second element, need to change to array. 
        
             existing = events[type] = 
        
               prepend ? [listener, existing] : [existing, listener]; 
        
             // If we've already got an array, just append. 
        
           } else if (prepend) { 
        
             ArrayPrototypeUnshift(existing, listener); 
        
           } else { 
        
             ArrayPrototypePush(existing, listener); 
        
           }

A possible solution might be to implement SafeArray.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/781/

(E.G.: what would .map() return? An Array or a SafeArray?)

Well, Array.prototype.map(…) calls ArraySpeciesCreate, which calls new this.constructor[Symbol.species](this.length), and the [Symbol.species] getters on the built‑in constructors return this (e.g.: get Array[Symbol.species]()).

In other words, calling .map(…) on a SafeArray would return a SafeArray, since SafeArray.prototype.constructor points to SafeArray.

Refs: nodejs#36304 (comment) Refs: nodejs#36565

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/788/

@mscdex I removed the changes that were causing perf regressions, PTAL

Details

                                                           confidence improvement accuracy (*)   (**)  (***)
 events/ee-add-remove.js n=1000000                                         0.42 %       ±1.90% ±2.52% ±3.28%
 events/ee-emit.js listeners=10 argc=0 n=2000000                           0.28 %       ±2.74% ±3.65% ±4.76%
 events/ee-emit.js listeners=10 argc=10 n=2000000                         -0.14 %       ±1.47% ±1.97% ±2.58%
 events/ee-emit.js listeners=10 argc=2 n=2000000                           0.73 %       ±0.89% ±1.18% ±1.53%
 events/ee-emit.js listeners=10 argc=4 n=2000000                          -2.61 %       ±4.60% ±6.20% ±8.21%
 events/ee-emit.js listeners=1 argc=0 n=2000000                           -0.66 %       ±4.87% ±6.48% ±8.44%
 events/ee-emit.js listeners=1 argc=10 n=2000000                          -0.80 %       ±3.21% ±4.28% ±5.58%
 events/ee-emit.js listeners=1 argc=2 n=2000000                            2.58 %       ±4.10% ±5.46% ±7.10%
 events/ee-emit.js listeners=1 argc=4 n=2000000                            1.73 %       ±3.75% ±4.99% ±6.49%
 events/ee-emit.js listeners=5 argc=0 n=2000000                           -0.85 %       ±2.34% ±3.12% ±4.07%
 events/ee-emit.js listeners=5 argc=10 n=2000000                           1.03 %       ±2.37% ±3.16% ±4.12%
 events/ee-emit.js listeners=5 argc=2 n=2000000                            1.49 %       ±3.36% ±4.51% ±5.93%
 events/ee-emit.js listeners=5 argc=4 n=2000000                            0.35 %       ±1.72% ±2.29% ±2.99%
 events/ee-listener-count-on-prototype.js n=50000000                      -0.44 %       ±3.97% ±5.34% ±7.06%
 events/ee-listeners.js raw='false' listeners=50 n=5000000                -0.59 %       ±1.33% ±1.77% ±2.31%
 events/ee-listeners.js raw='false' listeners=5 n=5000000                 -0.23 %       ±1.47% ±1.95% ±2.54%
 events/ee-listeners.js raw='true' listeners=50 n=5000000                 -0.26 %       ±1.19% ±1.59% ±2.07%
 events/ee-listeners.js raw='true' listeners=5 n=5000000                  -0.89 %       ±4.13% ±5.50% ±7.17%
 events/ee-once.js argc=0 n=20000000                                      -1.17 %       ±2.70% ±3.61% ±4.74%
 events/ee-once.js argc=1 n=20000000                                       1.63 %       ±2.49% ±3.33% ±4.36%
 events/ee-once.js argc=4 n=20000000                                       0.20 %       ±1.64% ±2.19% ±2.87%
 events/ee-once.js argc=5 n=20000000                                      -0.86 %       ±1.88% ±2.51% ±3.29%
 events/eventtarget.js listeners=10 n=1000000                             -1.11 %       ±3.24% ±4.34% ±5.70%
 events/eventtarget.js listeners=1 n=1000000                              -0.31 %       ±5.77% ±7.67% ±9.99%
 events/eventtarget.js listeners=5 n=1000000                               3.13 %       ±5.12% ±6.89% ±9.11%

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

Landed in 88fb8e4...997f2fc

PR-URL: #36304 Reviewed-By: Rich Trott <rtrott@gmail.com>

lib: add `SafeArray` to `primordials` and use it for `FreeList` Refs: nodejs#36304 (comment) Refs: nodejs#36565 Signed-off-by: ExE Boss <ExE-Boss@code.ExE-Boss.tech>

nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Nov 28, 2020

aduh95 added the needs-benchmark-ci PR that need a benchmark CI run. label Nov 28, 2020

mscdex previously requested changes Nov 29, 2020

View reviewed changes

targos reviewed Nov 29, 2020

View reviewed changes

Comment thread lib/events.js Outdated

aduh95 marked this pull request as draft November 29, 2020 16:02

targos reviewed Nov 29, 2020

View reviewed changes

Comment thread lib/events.js Outdated

This was referenced Dec 14, 2020

events: refactor to use more primordials #36517

Closed

lib: refactor events.js #36528

Closed

RaisinTen reviewed Dec 17, 2020

View reviewed changes

Comment thread lib/events.js Outdated

RaisinTen reviewed Dec 17, 2020

View reviewed changes

Comment thread lib/events.js Outdated

aduh95 and others added 2 commits December 17, 2020 16:12

events: refactor to use more primordials

5e8dfd7

fixup! events: refactor to use more primordials

4355063

Co-authored-by: Darshan Sen <raisinten@gmail.com>

aduh95 force-pushed the events-primordials branch from 085a66d to 4355063 Compare December 17, 2020 15:14

fixup! events: refactor to use more primordials

69ff9a3

aduh95 added 5 commits December 18, 2020 01:02

fixup! events: refactor to use more primordials

e3d0bb1

fixup! events: refactor to use more primordials

940bd12

fixup! events: refactor to use more primordials

7a158a2

This reverts commit e3d0bb1.

fixup! events: refactor to use more primordials

9863b29

This reverts commit 940bd12.

fixup! events: refactor to use more primordials

df98f27

RaisinTen reviewed Dec 18, 2020

View reviewed changes

Comment thread lib/events.js Outdated

aduh95 added 2 commits December 21, 2020 21:18

fixup! events: refactor to use more primordials

19239d1

fixup! events: refactor to use more primordials

3d16ad8

ExE-Boss added a commit to ExE-Boss/node that referenced this pull request Dec 22, 2020

lib: add SafeArray to primordials and use it for FreeList

283189f

Refs: nodejs#36304 (comment) Refs: nodejs#36565

ExE-Boss mentioned this pull request Dec 22, 2020

lib: add SafeArray to primordials and use it for FreeList #36600

Open

3 tasks

fixup! events: refactor to use more primordials

8d5b676

aduh95 requested a review from mscdex December 22, 2020 19:09

fixup! events: refactor to use more primordials

27d8c94

aduh95 added review wanted PRs that need reviews. and removed needs-benchmark-ci PR that need a benchmark CI run. labels Dec 22, 2020

Trott approved these changes Dec 25, 2020

View reviewed changes

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2020

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2020

github-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2020

github-actions Bot closed this Dec 25, 2020

nodejs-github-bot pushed a commit that referenced this pull request Dec 25, 2020

events: refactor to use more primordials

997f2fc

PR-URL: #36304 Reviewed-By: Rich Trott <rtrott@gmail.com>

aduh95 deleted the events-primordials branch December 25, 2020 18:44

danielleadams pushed a commit that referenced this pull request Jan 12, 2021

events: refactor to use more primordials

82393ae

PR-URL: #36304 Reviewed-By: Rich Trott <rtrott@gmail.com>

danielleadams mentioned this pull request Jan 12, 2021

v15.6.0 proposal #36889

Merged

targos pushed a commit that referenced this pull request May 16, 2021

events: refactor to use more primordials

c260d98

PR-URL: #36304 Reviewed-By: Rich Trott <rtrott@gmail.com>

targos mentioned this pull request Jun 6, 2021

v14.17.1 release proposal #38948

Merged

targos pushed a commit that referenced this pull request Jun 11, 2021

events: refactor to use more primordials

4fdcbae

PR-URL: #36304 Reviewed-By: Rich Trott <rtrott@gmail.com>

events: refactor to use more primordials by aduh95 · Pull Request #36304 · nodejs/node · GitHub

Uh oh!

Uh oh!

events: refactor to use more primordials - #36304

events: refactor to use more primordials#36304aduh95 wants to merge 13 commits intonodejs:masternodejs/node:masterfrom aduh95:events-primordialsaduh95/node:events-primordialsCopy head branch name to clipboard

Conversation

aduh95 commented Nov 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mscdex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aduh95 commented Nov 29, 2020

Uh oh!

aduh95 commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduh95 commented Dec 17, 2020

Uh oh!

aduh95 commented Dec 17, 2020

Uh oh!

RaisinTen commented Dec 17, 2020

Uh oh!

Uh oh!

aduh95 commented Dec 18, 2020

Uh oh!

RaisinTen commented Dec 18, 2020

Uh oh!

ExE-Boss commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ExE-Boss commented Dec 18, 2020

Uh oh!

aduh95 commented Dec 21, 2020

Uh oh!

ExE-Boss commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Dec 22, 2020

Uh oh!

aduh95 commented Dec 22, 2020

Uh oh!

nodejs-github-bot commented Dec 25, 2020

Uh oh!

github-actions Bot commented Dec 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

events: refactor to use more primordials#36304
aduh95 wants to merge 13 commits into
nodejs:masterfrom
aduh95:events-primordials

aduh95 commented Nov 28, 2020 •
edited
Loading

aduh95 commented Nov 29, 2020 •
edited
Loading

ExE-Boss commented Dec 18, 2020 •
edited
Loading

ExE-Boss commented Dec 22, 2020 •
edited
Loading