lib: refactor Timeout, ImmediateList, TimersList to class

[ttzztztz]

The prototype is old-style, refactor it to the ES6-class.

Special Note:
Line 139-140 was moved to Line 301-302 because class doesn't support hoisting

// Create a single linked list instance only once at startup	
const immediateQueue = new ImmediateList();

[Trott]

I'm going to guess that this will have a significant negative effect on performance, but I would be delighted to be wrong about that. Benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/950/

[ttzztztz]

I'm going to guess that this will have a significant negative effect on performance, but I would be delighted to be wrong about that. Benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/950/

Benchmark result:

16:34:12                                                             confidence improvement accuracy (*)    (**)   (***)
16:34:12 timers/immediate.jstype='breadth1' n=5000000                                0.26 %       ±4.06%  ±5.40%  ±7.03%
16:34:12 timers/immediate.jstype='breadth4' n=5000000                                0.56 %       ±3.69%  ±4.91%  ±6.39%
16:34:12 timers/immediate.jstype='breadth' n=5000000                                 1.41 %       ±3.63%  ±4.83%  ±6.29%
16:34:12 timers/immediate.jstype='clear' n=5000000                                   2.93 %       ±3.01%  ±4.01%  ±5.22%
16:34:12 timers/immediate.jstype='depth1' n=5000000                                  1.30 %       ±2.63%  ±3.52%  ±4.63%
16:34:12 timers/immediate.jstype='depth' n=5000000                                   1.11 %       ±1.19%  ±1.60%  ±2.10%
16:34:12 timers/set-immediate-breadth-args.jsn=5000000                              -0.44 %       ±3.90%  ±5.19%  ±6.77%
16:34:12 timers/set-immediate-breadth.jsn=10000000                                  -1.29 %       ±2.95%  ±3.92%  ±5.11%
16:34:12 timers/set-immediate-depth-args.jsn=5000000                                 0.00 %       ±1.79%  ±2.39%  ±3.11%
16:34:12 timers/timers-breadth-args.jsn=1000000                                      3.38 %       ±6.03%  ±8.03% ±10.48%
16:34:12 timers/timers-breadth.jsn=5000000                                          -0.64 %       ±3.26%  ±4.34%  ±5.65%
16:34:12 timers/timers-cancel-pooled.jsn=5000000                                     3.25 %      ±11.54% ±15.36% ±20.00%
16:34:12 timers/timers-cancel-unpooled.jsdirection='end' n=1000000                   4.24 %       ±9.24% ±12.31% ±16.07%
16:34:12 timers/timers-cancel-unpooled.jsdirection='start' n=1000000                -2.12 %       ±6.23%  ±8.30% ±10.83%
16:34:12 timers/timers-depth.jsn=1000                                                0.46 %       ±0.87%  ±1.16%  ±1.51%
16:34:12 timers/timers-insert-pooled.jsn=5000000                                    -1.83 %       ±3.37%  ±4.50%  ±5.89%
16:34:12 timers/timers-insert-unpooled.jsdirection='end' n=1000000                   0.60 %       ±5.65%  ±7.51%  ±9.78%
16:34:12 timers/timers-insert-unpooled.jsdirection='start' n=1000000                 0.92 %       ±5.33%  ±7.09%  ±9.24%
16:34:12 timers/timers-timeout-nexttick.jsn=50000                                    1.05 %       ±2.07%  ±2.75%  ±3.59%
16:34:12 timers/timers-timeout-nexttick.jsn=5000000                           *      5.99 %       ±4.66%  ±6.24%  ±8.22%
16:34:12 timers/timers-timeout-pooled.jsn=10000000                                   3.69 %       ±7.01%  ±9.32% ±12.14%
16:34:12 timers/timers-timeout-unpooled.jsn=1000000                                 11.44 %      ±12.48% ±16.62% ±21.67%

It looks good, Maybe there is some optimizations from V8 engine.

[Trott]

@nodejs/timers

[nodejs-github-bot]

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

[Trott]

Maybe there is some optimizations from V8 engine.

Yes, and/or maybe none of the changes here affect any hot paths. Regardless, I'm delighted for my speculation on performance impact to have been wrong.

[jasnell]

This LGTM but I'm defensively marking it semver-major. While these objects are undocumented they are exposed to user code and there's a non-zero possibility of the change to ES6 classes breaking someone.

[Fishrock123]

Yeah make sure you smoke test this one

[Trott]

LGTM with clean CITGM as suggested by @Fishrock123

[ttzztztz]

CITGM

What does CITGM mean? I'm a little confused about this abbreviation.

[Fishrock123]

CITGM is the "smoke-test" tool use to test Node.js core changes on npm ecosystem modules to check for potential ecosystem breakage.

[Trott]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2654/

(Don't let all the red/failures scare you. CITGM always fails a lot. We need to compare it to a run against the master branch to see if it is failing more than usual.)

[Trott]

CITGM against master branch for comparison: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2655/

[Trott]

CITGM results look OK.

[nodejs-github-bot]

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

[Trott]

@nodejs/tsc This semver-major-out-of-caution PR is ready to land: Two TSC approvals, green CI, clean CTIGM, open for far more than 48 hours, benchmark showing no performance regression. Out of caution and a general sentiment that a lot can go wrong with timers if we're not careful, I'm pinging the TSC to try to get more reviews. PTAL if you have the time and inclination.

[Trott]

Adding tsc-agenda label just to surface it for more attention if it hasn't gotten any by the next TSC meeting. (If it lands and the PR is closed/merge, the label will have no effect.)

[mcollina]

lgtm

[apapirovski]

@ttzztztz would you like to pursue this further? It looks like there's a conflict with some recent changes to timers but if you can resolve, we likely should be able to get this landed.

[ttzztztz]

@ttzztztz would you like to pursue this further? It looks like there's a conflict with some recent changes to timers but if you can resolve, we likely should be able to get this landed.

OK, please give me some time to work on this. 👍

[ttzztztz]

@apapirovski Done. Please check the code or rerun the CITGM if necessary in order to confirm the correctness of my modification. Thanks

[aduh95]

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

A few (minor) perf regressions

                                                             confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='breadth1' n=5000000                                1.34 %       ±3.15%  ±4.20%  ±5.47%
timers/immediate.js type='breadth4' n=5000000                               -0.07 %       ±3.74%  ±4.98%  ±6.49%
timers/immediate.js type='breadth' n=5000000                          *      4.60 %       ±4.28%  ±5.70%  ±7.45%
timers/immediate.js type='clear' n=5000000                                   0.55 %       ±2.94%  ±3.91%  ±5.10%
timers/immediate.js type='depth1' n=5000000                          **     -1.63 %       ±1.03%  ±1.37%  ±1.80%
timers/immediate.js type='depth' n=5000000                                   0.12 %       ±1.57%  ±2.09%  ±2.72%
timers/set-immediate-breadth-args.js n=5000000                              -0.32 %       ±3.30%  ±4.39%  ±5.72%
timers/set-immediate-breadth.js n=10000000                                  -1.15 %       ±3.14%  ±4.18%  ±5.44%
timers/set-immediate-depth-args.js n=5000000                                -0.21 %       ±0.78%  ±1.04%  ±1.35%
timers/timers-breadth-args.js n=1000000                               *      3.03 %       ±2.78%  ±3.71%  ±4.83%
timers/timers-breadth.js n=5000000                                           0.64 %       ±6.47%  ±8.61% ±11.22%
timers/timers-cancel-pooled.js n=5000000                                    -2.94 %      ±12.15% ±16.16% ±21.04%
timers/timers-cancel-unpooled.js direction='end' n=1000000                  -1.16 %      ±20.38% ±27.13% ±35.33%
timers/timers-cancel-unpooled.js direction='start' n=1000000          *     -8.16 %       ±7.58% ±10.09% ±13.13%
timers/timers-depth.js n=1000                                               -0.03 %       ±0.36%  ±0.48%  ±0.62%
timers/timers-insert-pooled.js n=5000000                                     1.78 %       ±3.96%  ±5.27%  ±6.86%
timers/timers-insert-unpooled.js direction='end' n=1000000                   1.22 %       ±4.17%  ±5.55%  ±7.24%
timers/timers-insert-unpooled.js direction='start' n=1000000         **     -7.40 %       ±5.02%  ±6.72%  ±8.81%
timers/timers-timeout-nexttick.js n=50000                                   -2.40 %       ±5.17%  ±6.91%  ±9.07%
timers/timers-timeout-nexttick.js n=5000000                                 -2.66 %       ±6.62%  ±8.81% ±11.46%
timers/timers-timeout-pooled.js n=10000000                            *     -7.87 %       ±7.73% ±10.29% ±13.40%
timers/timers-timeout-unpooled.js n=1000000                                  2.25 %       ±5.39%  ±7.25%  ±9.61%

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 9256728