Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: refactor to use more primordials #36304

Closed
wants to merge 13 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 28, 2020

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

@nodejs-github-bot 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 aduh95 added the needs-benchmark-ci PR that need a benchmark CI run. label Nov 28, 2020
mscdex
mscdex previously requested changes Nov 29, 2020
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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%

lib/events.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor Author

aduh95 commented Nov 29, 2020

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

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 29, 2020

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.

@aduh95 aduh95 marked this pull request as draft November 29, 2020 16:02
lib/events.js Outdated Show resolved Hide resolved
lib/events.js Outdated Show resolved Hide resolved
lib/events.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 17, 2020

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 17, 2020

@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%
                                                          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 (***)

@RaisinTen
Copy link
Contributor

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

lib/events.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 18, 2020

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%
                                                          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.

@RaisinTen
Copy link
Contributor

Should we prefer safety over performance here?

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Dec 18, 2020

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

@ExE-Boss
Copy link
Contributor

A possible solution might be to implement SafeArray.

@aduh95 aduh95 marked this pull request as ready for review December 21, 2020 20:16
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 21, 2020

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Dec 22, 2020

(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.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 22, 2020

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 22, 2020

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

                                                           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%

@aduh95 aduh95 requested a review from mscdex December 22, 2020 19:09
@aduh95 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 Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2020
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2020
@github-actions
Copy link
Contributor

Landed in 88fb8e4...997f2fc

@github-actions github-actions bot closed this Dec 25, 2020
nodejs-github-bot pushed a commit that referenced this pull request Dec 25, 2020
@aduh95 aduh95 deleted the events-primordials branch December 25, 2020 18:44
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
targos pushed a commit that referenced this pull request May 16, 2021
targos pushed a commit that referenced this pull request Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. review wanted PRs that need reviews. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants