Skip to content

Conversation

RaisinTen
Copy link
Member

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

5 participants