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

Revert "process: add getActiveResourcesInfo()" #41298

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Dec 23, 2021

This reverts commit 0d9f3bd.

There still appears to be some unexpected perf regressions in #41231
as shown by #41231 (comment). I'll run some benchmarks on this PR
to verify if those results are actually true because locally I got a ~20%
slow down for timers/timers-insert-pooled.js even after reverting.

Benchmark CI for timers: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1074/

                                                             confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='breadth1' n=5000000                         *     -3.78 %       ±3.43%  ±4.57%  ±5.95%
timers/immediate.js type='breadth4' n=5000000                         *      4.19 %       ±3.50%  ±4.67%  ±6.10%
timers/immediate.js type='breadth' n=5000000                        ***     31.68 %       ±4.42%  ±5.89%  ±7.67%
timers/immediate.js type='clear' n=5000000                          ***    279.90 %       ±9.45% ±12.73% ±16.90%
timers/immediate.js type='depth1' n=5000000                         ***     47.28 %       ±1.87%  ±2.50%  ±3.28%
timers/immediate.js type='depth' n=5000000                          ***     48.86 %       ±2.09%  ±2.78%  ±3.61%
timers/set-immediate-breadth-args.js n=5000000                              -2.60 %       ±3.89%  ±5.18%  ±6.75%
timers/set-immediate-breadth.js n=10000000                                   3.03 %       ±3.16%  ±4.21%  ±5.48%
timers/set-immediate-depth-args.js n=5000000                        ***     43.89 %       ±1.88%  ±2.51%  ±3.27%
timers/timers-breadth-args.js n=1000000                             ***    -12.25 %       ±5.33%  ±7.15%  ±9.43%
timers/timers-breadth.js n=5000000                                          -3.36 %       ±4.90%  ±6.55%  ±8.58%
timers/timers-cancel-pooled.js n=5000000                            ***    129.33 %      ±18.62% ±24.94% ±32.80%
timers/timers-cancel-unpooled.js direction='end' n=1000000            *     18.29 %      ±15.22% ±20.26% ±26.39%
timers/timers-cancel-unpooled.js direction='start' n=1000000        ***     17.86 %       ±8.82% ±11.77% ±15.39%
timers/timers-depth.js n=1000                                               -0.59 %       ±0.60%  ±0.80%  ±1.06%
timers/timers-insert-pooled.js n=5000000                            ***    -18.66 %       ±4.23%  ±5.62%  ±7.32%
timers/timers-insert-unpooled.js direction='end' n=1000000          ***     10.97 %       ±4.52%  ±6.02%  ±7.83%
timers/timers-insert-unpooled.js direction='start' n=1000000        ***     10.07 %       ±5.03%  ±6.69%  ±8.71%
timers/timers-timeout-nexttick.js n=50000                           ***     14.97 %       ±3.22%  ±4.28%  ±5.57%
timers/timers-timeout-nexttick.js n=5000000                         ***     61.77 %       ±8.38% ±11.17% ±14.57%
timers/timers-timeout-pooled.js n=10000000                          ***     80.16 %      ±14.66% ±19.69% ±26.00%
timers/timers-timeout-unpooled.js n=1000000                         ***     70.64 %       ±9.77% ±13.03% ±17.02%

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Dec 23, 2021
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

cc @nodejs/tsc

@mscdex
Copy link
Contributor

mscdex commented Dec 23, 2021

It could very well be some strange interaction with the V8 optimizer.

@RaisinTen
Copy link
Contributor Author

I've updated the PR description with the benchmark results and they look very similar to the results of #41231. Since the other PR fixes #41219 by preserving process.getActiveResourcesInfo(), it would be better if we close this PR and go ahead with the changes submitted in #41231.

@RaisinTen RaisinTen closed this Dec 23, 2021
@RaisinTen RaisinTen deleted the revert-process-getActiveResourcesInfo branch December 23, 2021 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants