timers: refactor to use rest parameter

[aduh95]

Checklist

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

[lpinca]

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

[nodejs-github-bot]

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

[mscdex]

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]

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]

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]

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]

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]

                                                               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]

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]

                                                             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]

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]

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]

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%

Details

                                                             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]

@mscdex Should I close this?

[targos]

Those results look acceptable to me

[aduh95]

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]

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