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

domain: allow concurrent user-land impl #33013

Closed
wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 23, 2020

Picking up #26326

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added domain Issues and PRs related to the domain subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. events Issues and PRs related to the events subsystem / EventEmitter. labels Apr 23, 2020
@Trott
Copy link
Member Author

Trott commented Apr 23, 2020

A chunk of code that was deleted in the original pull request is restored here so that test/parallel/test-domain-emit-error-handler-stack.js will pass. Is that the right approach, or is the thing to do to delete/update the test as its behavior should change?

@misterdjules @vdeturckheim @addaleax @devsnek

@Trott
Copy link
Member Author

Trott commented Apr 23, 2020

Remaining to be done:

@mscdex
Copy link
Contributor

mscdex commented Apr 23, 2020

This produces some significant performance regressions:

                                                    confidence improvement accuracy (*)   (**)  (***)
events/ee-emit.js listeners=10 argc=0 n=2000000            ***    -25.42 %       ±1.07% ±1.43% ±1.88%
events/ee-emit.js listeners=10 argc=10 n=2000000           ***    -17.68 %       ±0.89% ±1.18% ±1.53%
events/ee-emit.js listeners=10 argc=2 n=2000000            ***    -14.36 %       ±2.09% ±2.81% ±3.71%
events/ee-emit.js listeners=10 argc=4 n=2000000            ***    -14.72 %       ±1.41% ±1.89% ±2.47%
events/ee-emit.js listeners=1 argc=0 n=2000000             ***    -19.82 %       ±3.42% ±4.56% ±5.94%
events/ee-emit.js listeners=1 argc=10 n=2000000            ***    -34.73 %       ±2.36% ±3.15% ±4.12%
events/ee-emit.js listeners=1 argc=2 n=2000000             ***    -32.23 %       ±3.10% ±4.14% ±5.43%
events/ee-emit.js listeners=1 argc=4 n=2000000             ***    -27.68 %       ±2.74% ±3.66% ±4.79%
events/ee-emit.js listeners=5 argc=0 n=2000000             ***    -14.22 %       ±1.69% ±2.25% ±2.94%
events/ee-emit.js listeners=5 argc=10 n=2000000            ***    -20.74 %       ±1.79% ±2.40% ±3.15%
events/ee-emit.js listeners=5 argc=2 n=2000000             ***    -13.06 %       ±1.46% ±1.95% ±2.54%
events/ee-emit.js listeners=5 argc=4 n=2000000             ***    -14.43 %       ±2.13% ±2.85% ±3.72%
events/ee-once.js argc=4 n=20000000                        ***     -4.64 %       ±1.85% ±2.48% ±3.25%
events/ee-once.js argc=5 n=20000000                        ***     -4.46 %       ±1.60% ±2.13% ±2.78%

Can we find a way to avoid these first?

@addaleax
Copy link
Member

I’m surprised that the impact is that large, but yes, we should definitely address that first…

@Trott
Copy link
Member Author

Trott commented Aug 7, 2020

@mscdex Can you re-run the benchmark CI so I can see what parameters you use / it uses? I'm running locally and getting very different results, which may be correct--different platforms/environments having different benchmarks--but it seems striking.

 events/ee-add-remove.js n=1000000                                         0.93 %       ±3.44%  ±4.58%  ±5.97%
 events/ee-emit.js listeners=1 argc=0 n=2000000                            1.63 %      ±13.38% ±17.81% ±23.20%
 events/ee-emit.js listeners=1 argc=10 n=2000000                          -1.59 %      ±14.54% ±19.35% ±25.20%
 events/ee-emit.js listeners=1 argc=2 n=2000000                           -3.36 %      ±13.41% ±17.84% ±23.22%
 events/ee-emit.js listeners=1 argc=4 n=2000000                            0.36 %      ±13.42% ±17.85% ±23.24%
 events/ee-emit.js listeners=10 argc=0 n=2000000                           0.03 %      ±14.22% ±18.92% ±24.63%
 events/ee-emit.js listeners=10 argc=10 n=2000000                         -0.16 %      ±17.77% ±23.64% ±30.77%
 events/ee-emit.js listeners=10 argc=2 n=2000000                          -4.87 %      ±14.30% ±19.03% ±24.77%
 events/ee-emit.js listeners=10 argc=4 n=2000000                          -1.99 %      ±13.51% ±17.98% ±23.42%
 events/ee-emit.js listeners=5 argc=0 n=2000000                           -7.17 %      ±15.48% ±20.60% ±26.81%
 events/ee-emit.js listeners=5 argc=10 n=2000000                          -9.15 %      ±12.54% ±16.68% ±21.71%
 events/ee-emit.js listeners=5 argc=2 n=2000000                           -4.81 %      ±14.36% ±19.11% ±24.88%
 events/ee-emit.js listeners=5 argc=4 n=2000000                           -8.17 %      ±13.48% ±17.94% ±23.37%
 events/ee-listener-count-on-prototype.js n=50000000                       0.18 %       ±3.31%  ±4.41%  ±5.75%
 events/ee-listeners.js raw='false' listeners=5 n=5000000                 -0.98 %       ±3.63%  ±4.83%  ±6.29%
 events/ee-listeners.js raw='false' listeners=50 n=5000000                 2.08 %       ±3.95%  ±5.25%  ±6.83%
 events/ee-listeners.js raw='true' listeners=5 n=5000000                   0.06 %       ±3.60%  ±4.79%  ±6.25%
 events/ee-listeners.js raw='true' listeners=50 n=5000000           *      8.66 %       ±6.59%  ±8.77% ±11.43%
 events/ee-once.js argc=0 n=20000000                                      -3.79 %      ±11.45% ±15.26% ±19.92%
 events/ee-once.js argc=1 n=20000000                                      -3.11 %       ±7.93% ±10.55% ±13.74%
 events/ee-once.js argc=4 n=20000000                                      -2.75 %       ±8.40% ±11.18% ±14.56%
 events/ee-once.js argc=5 n=20000000                                       2.38 %       ±8.14% ±10.83% ±14.09%
 events/eventtarget.js listeners=1 n=1000000                        *     -6.82 %       ±6.76%  ±9.00% ±11.73%
 events/eventtarget.js listeners=10 n=1000000                              5.46 %       ±6.92%  ±9.21% ±12.01%
 events/eventtarget.js listeners=5 n=1000000                      ***    -17.82 %       ±5.94%  ±7.91% ±10.29%

I also ran this after rebasing against master so it's possible something else changed that caused this to have less impact.

@mmarchini mmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2020
@nodejs-github-bot
Copy link
Collaborator

@mscdex
Copy link
Contributor

mscdex commented Aug 8, 2020

CI results look about the same as before for this PR:

                                                          confidence improvement accuracy (*)   (**)   (***)
events/ee-emit.js listeners=10 argc=0 n=2000000                  ***    -22.05 %       ±1.25% ±1.67%  ±2.18%
events/ee-emit.js listeners=10 argc=10 n=2000000                 ***    -15.45 %       ±2.06% ±2.76%  ±3.62%
events/ee-emit.js listeners=10 argc=2 n=2000000                  ***     -8.32 %       ±1.84% ±2.47%  ±3.26%
events/ee-emit.js listeners=10 argc=4 n=2000000                  ***     -9.01 %       ±0.89% ±1.18%  ±1.54%
events/ee-emit.js listeners=1 argc=0 n=2000000                   ***    -15.22 %       ±5.73% ±7.67% ±10.09%
events/ee-emit.js listeners=1 argc=10 n=2000000                  ***    -31.82 %       ±4.32% ±5.80%  ±7.67%
events/ee-emit.js listeners=1 argc=2 n=2000000                   ***    -26.61 %       ±2.35% ±3.14%  ±4.09%
events/ee-emit.js listeners=1 argc=4 n=2000000                   ***    -30.98 %       ±3.45% ±4.62%  ±6.09%
events/ee-emit.js listeners=5 argc=0 n=2000000                   ***    -15.69 %       ±3.59% ±4.79%  ±6.26%
events/ee-emit.js listeners=5 argc=10 n=2000000                  ***    -26.00 %       ±1.17% ±1.55%  ±2.02%
events/ee-emit.js listeners=5 argc=2 n=2000000                   ***     -8.70 %       ±1.45% ±1.93%  ±2.51%
events/ee-emit.js listeners=5 argc=4 n=2000000                   ***    -12.15 %       ±1.71% ±2.27%  ±2.96%
events/ee-once.js argc=1 n=20000000                              ***     -4.73 %       ±1.82% ±2.42%  ±3.16%
events/ee-once.js argc=4 n=20000000                              ***     -6.53 %       ±2.73% ±3.64%  ±4.75%
events/ee-once.js argc=5 n=20000000                              ***     -5.82 %       ±3.13% ±4.18%  ±5.48%

@Trott
Copy link
Member Author

Trott commented Aug 8, 2020

The parameters for the benchmark CI run, for when that link expires:

image

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott force-pushed the support-userland-domains-bis branch 2 times, most recently from f2ae64b to 6cc7faa Compare March 27, 2021 13:37
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 27, 2021

@mscdex
Copy link
Contributor

mscdex commented Mar 27, 2021

FWIW there are still some significant regressions:

events/ee-emit.jslisteners=10 argc=0 n=2000000       ***    -19.07 %       ±1.58% ±2.10%  ±2.74%
events/ee-emit.jslisteners=10 argc=10 n=2000000      ***    -33.23 %       ±0.88% ±1.17%  ±1.52%
events/ee-emit.jslisteners=10 argc=2 n=2000000       ***    -23.71 %       ±0.76% ±1.02%  ±1.33%
events/ee-emit.jslisteners=10 argc=4 n=2000000       ***    -27.12 %       ±0.86% ±1.14%  ±1.49%
events/ee-emit.jslisteners=1 argc=0 n=2000000        ***    -20.25 %       ±3.37% ±4.50%  ±5.88%
events/ee-emit.jslisteners=1 argc=10 n=2000000       ***    -29.84 %       ±4.91% ±6.56%  ±8.58%
events/ee-emit.jslisteners=1 argc=2 n=2000000        ***    -27.89 %       ±2.60% ±3.46%  ±4.53%
events/ee-emit.jslisteners=1 argc=4 n=2000000        ***    -30.05 %       ±3.50% ±4.65%  ±6.06%
events/ee-emit.jslisteners=5 argc=0 n=2000000        ***    -13.61 %       ±1.67% ±2.23%  ±2.90%
events/ee-emit.jslisteners=5 argc=10 n=2000000       ***    -42.20 %       ±1.41% ±1.89%  ±2.50%
events/ee-emit.jslisteners=5 argc=2 n=2000000        ***    -31.84 %       ±1.39% ±1.85%  ±2.42%
events/ee-emit.jslisteners=5 argc=4 n=2000000        ***    -33.71 %       ±1.52% ±2.03%  ±2.66%
events/ee-once.jsargc=0 n=20000000                   ***     -2.04 %       ±0.93% ±1.23%  ±1.61%
events/ee-once.jsargc=5 n=20000000                   ***     -4.53 %       ±1.56% ±2.09%  ±2.73%

@Trott
Copy link
Member Author

Trott commented Mar 28, 2021

@Trott Trott force-pushed the support-userland-domains-bis branch from 6cc7faa to a45ef63 Compare March 28, 2021 04:42
@Trott
Copy link
Member Author

Trott commented Mar 28, 2021

Whoops, @mscdex's results were from a benchmark CI run and not a local run, so my benchmark CI run was superfluous. Oh well, here are the results anyway:

22:12:45                                                          confidence improvement accuracy (*)   (**)  (***)
22:12:45 events/ee-add-remove.jsn=1000000                                         0.03 %       ±2.21% ±2.94% ±3.83%
22:12:45 events/ee-emit.jslisteners=10 argc=0 n=2000000                  ***    -17.66 %       ±2.83% ±3.81% ±5.04%
22:12:45 events/ee-emit.jslisteners=10 argc=10 n=2000000                 ***    -32.48 %       ±1.21% ±1.62% ±2.12%
22:12:45 events/ee-emit.jslisteners=10 argc=2 n=2000000                  ***    -24.41 %       ±1.02% ±1.36% ±1.77%
22:12:45 events/ee-emit.jslisteners=10 argc=4 n=2000000                  ***    -26.14 %       ±2.73% ±3.67% ±4.84%
22:12:45 events/ee-emit.jslisteners=1 argc=0 n=2000000                   ***    -20.79 %       ±3.38% ±4.52% ±5.93%
22:12:45 events/ee-emit.jslisteners=1 argc=10 n=2000000                  ***    -34.28 %       ±3.47% ±4.64% ±6.09%
22:12:45 events/ee-emit.jslisteners=1 argc=2 n=2000000                   ***    -27.86 %       ±2.94% ±3.92% ±5.10%
22:12:45 events/ee-emit.jslisteners=1 argc=4 n=2000000                   ***    -28.60 %       ±3.69% ±4.92% ±6.43%
22:12:45 events/ee-emit.jslisteners=5 argc=0 n=2000000                   ***    -11.99 %       ±1.83% ±2.44% ±3.17%
22:12:45 events/ee-emit.jslisteners=5 argc=10 n=2000000                  ***    -41.38 %       ±1.29% ±1.73% ±2.27%
22:12:45 events/ee-emit.jslisteners=5 argc=2 n=2000000                   ***    -31.60 %       ±2.03% ±2.73% ±3.60%
22:12:45 events/ee-emit.jslisteners=5 argc=4 n=2000000                   ***    -33.87 %       ±2.17% ±2.91% ±3.84%
22:12:45 events/ee-listener-count-on-prototype.jsn=50000000                       2.23 %       ±4.07% ±5.44% ±7.12%
22:12:45 events/ee-listeners.jsraw='false' listeners=50 n=5000000                -0.15 %       ±2.52% ±3.36% ±4.39%
22:12:45 events/ee-listeners.jsraw='false' listeners=5 n=5000000                  1.06 %       ±1.55% ±2.07% ±2.69%
22:12:45 events/ee-listeners.jsraw='true' listeners=50 n=5000000                 -0.02 %       ±1.82% ±2.43% ±3.16%
22:12:45 events/ee-listeners.jsraw='true' listeners=5 n=5000000                   1.66 %       ±3.92% ±5.22% ±6.82%
22:12:45 events/ee-once.jsargc=0 n=20000000                                      -1.25 %       ±3.90% ±5.19% ±6.76%
22:12:45 events/ee-once.jsargc=1 n=20000000                               **     -4.86 %       ±2.93% ±3.95% ±5.23%
22:12:45 events/ee-once.jsargc=4 n=20000000                              ***     -5.65 %       ±1.54% ±2.06% ±2.68%
22:12:45 events/ee-once.jsargc=5 n=20000000                              ***     -5.21 %       ±0.96% ±1.29% ±1.69%
22:12:45 events/eventtarget.jslisteners=10 n=1000000                              0.02 %       ±1.75% ±2.32% ±3.02%
22:12:45 events/eventtarget.jslisteners=1 n=1000000                              -2.71 %       ±3.96% ±5.33% ±7.06%
22:12:45 events/eventtarget.jslisteners=5 n=1000000                              -0.94 %       ±3.50% ±4.69% ±6.16%
22:12:45 
22:12:45 Be aware that when doing many comparisons the risk of a false-positive
22:12:45 result increases. In this case, there are 25 comparisons, you can thus
22:12:45 expect the following amount of false-positive results:
22:12:45   1.25 false positives, when considering a   5% risk acceptance (*, **, ***),
22:12:45   0.25 false positives, when considering a   1% risk acceptance (**, ***),
22:12:45   0.03 false positives, when considering a 0.1% risk acceptance (***)
22:12:45 ++ mv output280321-054823.csv /w/bnch-comp

Julien Gilli and others added 2 commits March 30, 2021 06:44
Currently, only one domain-lke implementation (the core domain one) can
be used to handle uncaught exceptions or unhandled error events.

This PR aims at making it possible for different domain-like
user-land implementations to be used concurrently (including with the
core domain impl) so that the state of the core domain module (doc
deprecated) does not prevent users of domains from having a
well-maintained domain-like facility.

Ref: nodejs#23348
Co-authored-by: Rich Trott <rtrott@gmail.com>
@Trott Trott force-pushed the support-userland-domains-bis branch from a45ef63 to 110983c Compare March 30, 2021 17:50
@Trott Trott force-pushed the support-userland-domains-bis branch from 8e77095 to f992e36 Compare March 31, 2021 14:24
@Trott
Copy link
Member Author

Trott commented Mar 31, 2021

I'm not seeing any obvious way to improve performance and this isn't something I'm inclined to seek out non-obvious ways to do it. So I'm inclined to close.

@mscdex Do you see anything obvious that might be worth trying to refactor into a more performant shape?

@Trott Trott closed this Mar 31, 2021
@Trott Trott deleted the support-userland-domains-bis branch September 25, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain Issues and PRs related to the domain subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants