timers: do less work in insert by apapirovski · Pull Request #27345 · nodejs/node · GitHub

Most of the code in insert is only applicable to scheduling non-timers or re-scheduling timers. We can skip most of it in the case of setTimeout, setInterval & setUnrefTimeout.

Second commit fixes a long-standing bug with refresh in cases where it's triggered after the timer already ran its course. Prior to this the refreshed timer would end up always being unrefed, even when it was previously refed.

Checklist

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

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3323

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

@Fishrock123 I've updated with some significant changes in the second commit. There was a bug with refresh that is now fixed and there's a test case provided. (Found it because I was trying to think through and address your comment re: replacing the schedule call in interval branch.)

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

Benchmark with the latest code: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/356/

Results with any significant change:

                                                             confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='clear' n=5000000                          ***     19.14 %       ±2.12%  ±2.80%  ±3.60%
timers/immediate.js type='depth' n=5000000                            *      0.83 %       ±0.73%  ±0.96%  ±1.23%
timers/set-immediate-depth-args.js n=5000000                          *     -0.92 %       ±0.78%  ±1.03%  ±1.32%
timers/timers-cancel-unpooled.js direction='end' n=1000000            *     15.14 %      ±12.87% ±16.98% ±21.81%
timers/timers-depth.js n=1000                                         *      0.06 %       ±0.05%  ±0.07%  ±0.09%
timers/timers-insert-unpooled.js direction='end' n=1000000          ***     24.38 %       ±3.03%  ±4.01%  ±5.15%
timers/timers-timeout-nexttick.js n=50000                            **     -4.00 %       ±2.41%  ±3.18%  ±4.08%
timers/timers-timeout-unpooled.js n=1000000                         ***     -4.59 %       ±2.62%  ±3.45%  ±4.44%

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

I'm not a particular fan of how this makes [kRefed] always be behind _destroyed. Seems like we are unnecessarily adding more "timer is alive or not" dependence to another semi-private property.

Is that required to be part of these changes?

@apapirovski thoughts?

@Fishrock123 Your comment is fair. Let's discuss in person at the summit. Didn't have time for the past month to think about this but will probably try to wrap up this PR in the next two days.

@apapirovski good to merge?

@Fishrock123 yeah, should be. I’ll confirm this weekend and do it if so.

Most of the code in insert is only applicable to scheduling non-timers or re-scheduling timers. We can skip most of it in the case of setTimeout, setInterval & setUnrefTimeout.

Expired timers were not being refresh correctly and would always act as unrefed if refresh was called.

CI: https://ci.nodejs.org/job/node-test-pull-request/27640/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/479/

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

CI: https://ci.nodejs.org/job/node-test-pull-request/27658/ ✔️

@apapirovski the latest benchmark results do not indicate a big difference at all and these are the only three benchmarks that seemed to have any significance:

01:16:52  timers/timers-depth.js n=1000                                       ***      0.71 %       ±0.19%  ±0.25%  ±0.32%
01:16:52  timers/timers-insert-unpooled.js direction='end' n=1000000            *     -2.55 %       ±2.40%  ±3.17%  ±4.07%
01:16:52  timers/timers-timeout-pooled.js n=10000000                           **     -5.74 %       ±4.14%  ±5.46%  ±7.02%

I think this change aims more to reduce potential edge-cases than desiring a perf impact?

Aside form an extra function closure the only thing I can think of is the coalescing boolean checks...

Should this be squashed into one commit or two? If two, which commit should the fixup commit go with?

Should this be squashed into one commit or two? If two, which commit should the fixup commit go with?

@apapirovski ^^^^^^^^

Most of the code in insert is only applicable to scheduling non-timers or re-scheduling timers. We can skip most of it in the case of setTimeout, setInterval & setUnrefTimeout. PR-URL: #27345 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Expired timers were not being refresh correctly and would always act as unrefed if refresh was called. PR-URL: #27345 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

@Trott the fixup is for the second commit. I went ahead and landed it in two commits.

Landed in 0b89761, 1fab8a9 🎉