timers: refactor timer list processing

[apapirovski]

(The first commit is from #18579. I will rebase once that PR lands. Rebased.)

Currently timer handles have their processing function stored at kOnTimeout (which is an index = 0). This isn't actually completely efficient, even if the function is declared on the prototype, as it requires replacing when unref is called and un-assigning it when clearing timeouts. It also requires a property lookup in C++ which is a tad slow.

Instead, it's possible to pass in a function from JS to C++ at startup, similar to how Immediates work currently, which is then called from C++ and that function then decides what to do further (that is, whether to call listOnTimeout or unrefdHandle).

Locally, this change yields a roughly 10% improvement on the newly introduced benchmark for unpooled execution.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

benchmark, timers

[apapirovski]

CI: https://ci.nodejs.org/job/node-test-pull-request/12956/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/115/ (will start shortly when 114 completes)

[apapirovski]

Benchmark results:

 timers/timers-breadth.js thousands=5000                     -1.37 %       ±2.96%   ±3.91%   ±5.02%
 timers/timers-cancel-pooled.js millions=5           ***   1587.57 %      ±88.31% ±116.88% ±150.93%
 timers/timers-cancel-unpooled.js millions=1         ***    106.69 %       ±7.78%  ±10.27%  ±13.20%
 timers/timers-depth.js thousands=1                   **      0.11 %       ±0.07%   ±0.09%   ±0.11%
 timers/timers-insert-pooled.js millions=5                    1.17 %       ±2.19%   ±2.89%   ±3.71%
 timers/timers-insert-unpooled.js millions=1                 -2.54 %       ±2.87%   ±3.79%   ±4.87%
 timers/timers-timeout-pooled.js millions=10                  2.63 %       ±5.60%   ±7.38%   ±9.48%
 timers/timers-timeout-unpooled.js millions=1        ***     20.32 %       ±4.07%   ±5.37%   ±6.91%

Nice bump on the timers-timeout-unpooled benchmark. The cancel bumps are from the first commit, which is from another PR.

[apapirovski]

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

[BridgeAR]

LGTM

[BridgeAR]

I personally do not like the "millions" and would actually like to replace those with e.g. n over time but that might only be my view on it. It is at least for me easier to read how many operations got executed that way instead of having to see that the output is actually also in millions.

[apapirovski]

Thanks for the feedback. Changed over to n = 1e6.

[BridgeAR]

I personally feel like it would also be better to move the start above the loop to include that time in the benchmark as I would say the creation time is important as well. But I guess you explicitly wanted to only measure the execution time?

[apapirovski]

Yeah, we have other tests that check the creation time. This way there's less noise in assessing the execution time.

[apapirovski]

Hi @nodejs/collaborators — could you please review this timers PR? It's been a little while and I would like to get some reviews so it can be improved or land. Thanks.

[mcollina]

LGTM

[apapirovski]

Thanks everyone. Landed in 0f9efef

[MylesBorins]

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

[MylesBorins]

Opting to not land on v8.x, please lmk if we should reconsider