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.

Sign up for GitHub

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

Jump to bottom

timers: refactor to use rest parameter #35947

Closed
wants to merge 2 commits into from
Closed

timers: refactor to use rest parameter #35947

wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 3, 2020

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

@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Nov 3, 2020
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2020
@lpinca
Copy link
Member

lpinca commented Nov 3, 2020

I remember something like this was proposed a few times but was always rejected due to performance loss. Can you benchmark this?

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

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

@aduh95 aduh95 added the needs-benchmark-ci PR that need a benchmark CI run. label Nov 3, 2020
@mscdex
Copy link
Contributor

mscdex commented Nov 4, 2020

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

                                                             confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='breadth1' n=5000000                       ***    -19.68 %       ±2.91%  ±3.87%  ±5.04%
timers/immediate.js type='breadth4' n=5000000                       ***     23.60 %       ±3.97%  ±5.29%  ±6.90%
timers/immediate.js type='breadth' n=5000000                        ***    -36.60 %       ±3.12%  ±4.17%  ±5.46%
timers/immediate.js type='clear' n=5000000                          ***     25.25 %       ±4.99%  ±6.64%  ±8.64%
timers/immediate.js type='depth1' n=5000000                                 -0.77 %       ±1.44%  ±1.92%  ±2.51%
timers/immediate.js type='depth' n=5000000                                   1.51 %       ±1.60%  ±2.13%  ±2.78%
timers/set-immediate-breadth-args.js n=5000000                      ***    -18.62 %       ±2.80%  ±3.72%  ±4.85%
timers/set-immediate-breadth.js n=10000000                          ***    -25.66 %       ±2.77%  ±3.69%  ±4.82%
timers/set-immediate-depth-args.js n=5000000                         **      3.61 %       ±2.36%  ±3.15%  ±4.14%
timers/timers-breadth-args.js n=1000000                             ***     -9.35 %       ±3.56%  ±4.74%  ±6.20%
timers/timers-breadth.js n=5000000                                  ***      9.50 %       ±4.10%  ±5.46%  ±7.12%
timers/timers-cancel-pooled.js n=5000000                            ***    -31.00 %      ±10.59% ±14.10% ±18.36%
timers/timers-cancel-unpooled.js direction='end' n=1000000                  -5.48 %      ±18.48% ±24.69% ±32.34%
timers/timers-cancel-unpooled.js direction='start' n=1000000                 0.41 %       ±7.48%  ±9.96% ±12.97%
timers/timers-depth.js n=1000                                               -1.08 %       ±1.29%  ±1.72%  ±2.24%
timers/timers-insert-pooled.js n=5000000                            ***    -18.38 %       ±3.47%  ±4.62%  ±6.01%
timers/timers-insert-unpooled.js direction='end' n=1000000            *     -4.42 %       ±3.75%  ±4.99%  ±6.50%
timers/timers-insert-unpooled.js direction='start' n=1000000        ***    -12.92 %       ±2.96%  ±3.95%  ±5.16%
timers/timers-timeout-nexttick.js n=50000                           ***     -4.42 %       ±2.52%  ±3.36%  ±4.38%
timers/timers-timeout-nexttick.js n=5000000                                 -4.69 %       ±7.36%  ±9.79% ±12.75%
timers/timers-timeout-pooled.js n=10000000                            *     -9.96 %       ±8.81% ±11.73% ±15.29%
timers/timers-timeout-unpooled.js n=1000000                         ***    -25.38 %       ±4.03%  ±5.36%  ±6.99%

still too much of a drop to consider using

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 4, 2020 •
edited

I might be reading this wrong, but it seems to be an improvement in some cases. Let me try another approach to get the best of both implementations.

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

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 4, 2020

Here are the results:

                                                             confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='breadth1' n=5000000                       ***     10.62 %       ±4.30%  ±5.73%  ±7.46%
timers/immediate.js type='breadth4' n=5000000                                5.30 %       ±5.37%  ±7.18%  ±9.41%
timers/immediate.js type='breadth' n=5000000                        ***    -13.77 %       ±3.52%  ±4.69%  ±6.11%
timers/immediate.js type='clear' n=5000000                                   1.71 %       ±4.38%  ±5.83%  ±7.59%
timers/immediate.js type='depth1' n=5000000                                 -0.47 %       ±2.60%  ±3.46%  ±4.52%
timers/immediate.js type='depth' n=5000000                                  -1.33 %       ±2.64%  ±3.51%  ±4.59%
timers/set-immediate-breadth-args.js n=5000000                        *     -5.21 %       ±4.78%  ±6.38%  ±8.35%
timers/set-immediate-breadth.js n=10000000                          ***     10.83 %       ±3.22%  ±4.29%  ±5.60%
timers/set-immediate-depth-args.js n=5000000                                 0.37 %       ±2.84%  ±3.78%  ±4.92%
timers/timers-breadth-args.js n=1000000                                      0.95 %       ±4.34%  ±5.80%  ±7.62%
timers/timers-breadth.js n=5000000                                   **     -6.01 %       ±3.97%  ±5.29%  ±6.89%
timers/timers-cancel-pooled.js n=5000000                                     3.07 %      ±12.39% ±16.53% ±21.61%
timers/timers-cancel-unpooled.js direction='end' n=1000000                  18.89 %      ±24.82% ±33.03% ±43.03%
timers/timers-cancel-unpooled.js direction='start' n=1000000                -2.25 %       ±8.49% ±11.31% ±14.73%
timers/timers-depth.js n=1000                                                0.67 %       ±1.33%  ±1.77%  ±2.31%
timers/timers-insert-pooled.js n=5000000                                     2.26 %       ±4.09%  ±5.44%  ±7.08%
timers/timers-insert-unpooled.js direction='end' n=1000000                  -2.34 %       ±3.26%  ±4.34%  ±5.66%
timers/timers-insert-unpooled.js direction='start' n=1000000                 0.62 %       ±4.94%  ±6.57%  ±8.56%
timers/timers-timeout-nexttick.js n=50000                                   -1.88 %       ±2.46%  ±3.27%  ±4.26%
timers/timers-timeout-nexttick.js n=5000000                                  2.20 %       ±6.33%  ±8.42% ±10.96%
timers/timers-timeout-pooled.js n=10000000                                   6.06 %       ±8.68% ±11.57% ±15.10%
timers/timers-timeout-unpooled.js n=1000000                           *      2.42 %       ±2.29%  ±3.07%  ±4.04%
` `

@mscdex
Copy link
Contributor

mscdex commented Nov 5, 2020

That looks better, but I'm still -1 on this because I don't think it's worthwhile in this case to trade one improvement for a regression in another. If there were reliably no regressions at all, then I would be fine with that. It is interesting that those two different setImmediate() breadth benchmarks are complete opposites though.

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 5, 2020

Fair enough. Let me try yet another approach that doesn't use the rest operator.

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

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 5, 2020 

                                                               confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='breadth1' n=5000000                               -1.11 %       ±3.34%  ±4.45%  ±5.79%
timers/immediate.js type='breadth4' n=5000000                       ***     23.02 %       ±4.56%  ±6.08%  ±7.92%
timers/immediate.js type='breadth' n=5000000                                -1.20 %       ±3.36%  ±4.48%  ±5.86%
timers/immediate.js type='clear' n=5000000                          ***     17.84 %       ±5.58%  ±7.43%  ±9.69%
timers/immediate.js type='depth1' n=5000000                                  0.03 %       ±2.17%  ±2.89%  ±3.76%
timers/immediate.js type='depth' n=5000000                                   2.36 %       ±2.45%  ±3.28%  ±4.31%
timers/set-immediate-breadth-args.js n=5000000                       **     -6.01 %       ±3.80%  ±5.07%  ±6.62%
timers/set-immediate-breadth.js n=10000000                                   0.04 %       ±3.33%  ±4.43%  ±5.77%
timers/set-immediate-depth-args.js n=5000000                        ***     -3.78 %       ±1.94%  ±2.59%  ±3.38%
timers/timers-breadth-args.js n=1000000                             ***    -22.43 %       ±3.32%  ±4.42%  ±5.76%
timers/timers-breadth.js n=5000000                                          -1.81 %       ±4.83%  ±6.43%  ±8.37%
timers/timers-cancel-pooled.js n=5000000                                     9.96 %      ±12.00% ±15.96% ±20.78%
timers/timers-cancel-unpooled.js direction='end' n=1000000            *    -18.30 %      ±18.08% ±24.07% ±31.37%
timers/timers-cancel-unpooled.js direction='start' n=1000000                 6.42 %       ±8.41% ±11.19% ±14.58%
timers/timers-depth.js n=1000                                               -0.82 %       ±1.16%  ±1.54%  ±2.01%
timers/timers-insert-pooled.js n=5000000                                    -1.28 %       ±3.10%  ±4.13%  ±5.38%
timers/timers-insert-unpooled.js direction='end' n=1000000                  -1.94 %       ±4.03%  ±5.37%  ±7.02%
timers/timers-insert-unpooled.js direction='start' n=1000000                -0.65 %       ±4.32%  ±5.75%  ±7.49%
timers/timers-timeout-nexttick.js n=50000                                    0.29 %       ±1.75%  ±2.33%  ±3.04%
timers/timers-timeout-nexttick.js n=5000000                                 -2.53 %       ±7.23%  ±9.62% ±12.52%
timers/timers-timeout-pooled.js n=10000000                                   6.46 %       ±9.52% ±12.67% ±16.50%
timers/timers-timeout-unpooled.js n=1000000                           *     -2.92 %       ±2.46%  ±3.30%  ±4.35%

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 5, 2020 •
edited

It seems there might be a sweet-spot. Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/682/console

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 5, 2020 

                                                             confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='breadth1' n=5000000                                2.04 %       ±3.51%  ±4.67%  ±6.08%
timers/immediate.js type='breadth4' n=5000000                       ***     32.00 %       ±4.58%  ±6.12%  ±8.00%
timers/immediate.js type='breadth' n=5000000                                -3.70 %       ±4.33%  ±5.77%  ±7.53%
timers/immediate.js type='clear' n=5000000                          ***     24.72 %       ±4.60%  ±6.12%  ±7.97%
timers/immediate.js type='depth1' n=5000000                                  1.90 %       ±1.92%  ±2.57%  ±3.37%
timers/immediate.js type='depth' n=5000000                                   1.67 %       ±2.01%  ±2.68%  ±3.50%
timers/set-immediate-breadth-args.js n=5000000                      ***     -6.59 %       ±3.45%  ±4.59%  ±5.99%
timers/set-immediate-breadth.js n=10000000                                  -0.95 %       ±2.55%  ±3.40%  ±4.42%
timers/set-immediate-depth-args.js n=5000000                                -2.09 %       ±2.29%  ±3.06%  ±4.01%
timers/timers-breadth-args.js n=1000000                                      0.73 %       ±4.69%  ±6.30%  ±8.30%
timers/timers-breadth.js n=5000000                                          -3.12 %       ±3.99%  ±5.32%  ±6.92%
timers/timers-cancel-pooled.js n=5000000                                    -1.23 %      ±11.21% ±14.91% ±19.41%
timers/timers-cancel-unpooled.js direction='end' n=1000000                  -0.85 %      ±22.24% ±29.62% ±38.60%
timers/timers-cancel-unpooled.js direction='start' n=1000000                 3.19 %       ±7.81% ±10.39% ±13.52%
timers/timers-depth.js n=1000                                                0.33 %       ±1.33%  ±1.78%  ±2.32%
timers/timers-insert-pooled.js n=5000000                                    -0.83 %       ±3.79%  ±5.06%  ±6.62%
timers/timers-insert-unpooled.js direction='end' n=1000000            *     -3.66 %       ±3.59%  ±4.78%  ±6.22%
timers/timers-insert-unpooled.js direction='start' n=1000000                 0.00 %       ±5.12%  ±6.85%  ±8.99%
timers/timers-timeout-nexttick.js n=50000                                   -1.35 %       ±2.32%  ±3.09%  ±4.02%
timers/timers-timeout-nexttick.js n=5000000                                 -1.64 %       ±6.10%  ±8.12% ±10.58%
timers/timers-timeout-pooled.js n=10000000                                   0.44 %       ±9.09% ±12.10% ±15.76%
timers/timers-timeout-unpooled.js n=1000000                                  1.00 %       ±5.47%  ±7.29%  ±9.49%

@mscdex
Copy link
Contributor

mscdex commented Nov 6, 2020

Using Array's slice() on arguments has traditionally incurred a performance hit (which is why we used a loop previously), which seems to still be the case judging by the most recent benchmark results?

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 6, 2020

I can see positive improvements, look at line 3 and 5:

timers/immediate.js type='breadth4' n=5000000                       ***     32.00 %
timers/immediate.js type='clear' n=5000000                          ***     24.72 %

and regression at line 8:

timers/set-immediate-breadth-args.js n=5000000                      ***     -6.59 %

It seems to me the improvement outweighs the regression, but maybe I'm reading this wrong.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 14, 2020 •
edited

I've ran another benchmark CI; some lines look better, probably thanks to #36221, but not all figures are positive still:

                                                             confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='breadth4' n=5000000                       ***     13.62 %       ±4.32%  ±5.75%  ±7.49%
timers/immediate.js type='clear' n=5000000                          ***     24.20 %       ±5.51%  ±7.34%  ±9.58%
timers/set-immediate-breadth-args.js n=5000000                      ***     29.73 %       ±5.60%  ±7.47%  ±9.77%
timers/timers-breadth.js n=5000000                                   **      4.46 %       ±3.10%  ±4.12%  ±5.36%
timers/timers-insert-pooled.js n=5000000                             **     -3.62 %       ±2.68%  ±3.57%  ±4.64%
timers/timers-timeout-nexttick.js n=5000000                           *      5.05 %       ±4.87%  ±6.54%  ±8.62%

                                                             confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='breadth1' n=5000000                               -1.12 %       ±3.84%  ±5.11%  ±6.66%
timers/immediate.js type='breadth4' n=5000000                       ***     13.62 %       ±4.32%  ±5.75%  ±7.49%
timers/immediate.js type='breadth' n=5000000                                 0.97 %       ±3.31%  ±4.41%  ±5.73%
timers/immediate.js type='clear' n=5000000                          ***     24.20 %       ±5.51%  ±7.34%  ±9.58%
timers/immediate.js type='depth1' n=5000000                                  0.71 %       ±1.28%  ±1.71%  ±2.22%
timers/immediate.js type='depth' n=5000000                                   1.16 %       ±1.96%  ±2.62%  ±3.44%
timers/set-immediate-breadth-args.js n=5000000                      ***     29.73 %       ±5.60%  ±7.47%  ±9.77%
timers/set-immediate-breadth.js n=10000000                                   2.67 %       ±3.51%  ±4.67%  ±6.09%
timers/set-immediate-depth-args.js n=5000000                                -1.09 %       ±2.44%  ±3.24%  ±4.22%
timers/timers-breadth-args.js n=1000000                                     -4.92 %       ±5.35%  ±7.14%  ±9.32%
timers/timers-breadth.js n=5000000                                   **      4.46 %       ±3.10%  ±4.12%  ±5.36%
timers/timers-cancel-pooled.js n=5000000                                     4.76 %      ±11.81% ±15.73% ±20.48%
timers/timers-cancel-unpooled.js direction='end' n=1000000                   5.14 %      ±11.25% ±14.98% ±19.53%
timers/timers-cancel-unpooled.js direction='start' n=1000000                -2.23 %       ±5.69%  ±7.59%  ±9.92%
timers/timers-depth.js n=1000                                               -0.07 %       ±1.08%  ±1.44%  ±1.88%
timers/timers-insert-pooled.js n=5000000                             **     -3.62 %       ±2.68%  ±3.57%  ±4.64%
timers/timers-insert-unpooled.js direction='end' n=1000000                   1.85 %       ±4.83%  ±6.43%  ±8.38%
timers/timers-insert-unpooled.js direction='start' n=1000000                 1.50 %       ±3.76%  ±5.00%  ±6.50%
timers/timers-timeout-nexttick.js n=50000                                   -0.77 %       ±2.20%  ±2.92%  ±3.80%
timers/timers-timeout-nexttick.js n=5000000                           *      5.05 %       ±4.87%  ±6.54%  ±8.62%
timers/timers-timeout-pooled.js n=10000000                                   1.09 %       ±8.56% ±11.39% ±14.84%
timers/timers-timeout-unpooled.js n=1000000                                 -6.05 %      ±11.17% ±14.87% ±19.37%

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

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 14, 2020

@mscdex Should I close this?

@targos
Copy link
Member

targos commented Dec 14, 2020

Those results look acceptable to me

aduh95
aduh95 commented Dec 14, 2020
View reviewed changes
lib/timers.js Outdated
// Extend array dynamically, makes .apply run much faster in v6.0.0
args[i - 2] = arguments[i];
}
args = [arg1, arg2, ...arg3];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing to consider is that this syntax relies on Array.prototype[Symbol.iterator] and %ArrayIteratorPrototype%.next, both user-mutable. Maybe it's just a bad idea after all, and we should keep the current implementation.

@aduh95 aduh95 added the blocked PRs that are blocked by other issues or PRs. label Dec 16, 2020
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 27, 2021

Let's not do that, the current implementation is fine.

@aduh95 aduh95 closed this Apr 27, 2021
@aduh95 aduh95 deleted the timers-rest-parameters branch April 27, 2021 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. needs-benchmark-ci PR that need a benchmark CI run. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aduh95 @lpinca @nodejs-github-bot @mscdex @targos @jasnell