timers: use Maps for internal duration pooling

[Fishrock123]

The ES benchmarks for maps show it fairly clearly as being faster, and I recall from other places that Maps are now supposed to be faster than Object Maps. As such, this PR switches timers' duration pooling to use Maps.

I did some initial benchmarking using the existing timers benchmarks and those from #10925 which came up with some interesting results, but my machine was under other load at the time, so these are inconclusive:

                                                    improvement confidence      p.value
 timers/timers-breadth.js thousands=500                 -5.31 %        *** 4.071485e-06
 timers/timers-cancel-pooled.js thousands=500           -2.85 %         ** 1.209156e-03
 timers/timers-cancel-unpooled.js thousands=100       1507.75 %        *** 1.664058e-42
 timers/timers-depth.js thousands=1                      0.03 %            8.551970e-01
 timers/timers-insert-pooled.js thousands=500           -4.03 %        *** 1.004463e-06
 timers/timers-insert-unpooled.js thousands=100        -28.91 %        *** 4.867811e-38

(This patch was made live during https://www.twitch.tv/videos/117984923 if you'd like to see me working on this in retrospect. :P)

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)

timers

[jasnell]

/cc @mscdex

[mscdex]

IIRC there was some uncertainty surrounding the results of the Map benchmarks some time ago which suggested that they may not actually be as fast as the results were showing. I do not recall the specifics or where the discussion was had, but I'm guessing some thread here on github.

I'm concerned about the performance regressions in the other benchmark configurations. I will take a look at this ...

[lpinca]

@mscdex maybe #7581.

[Fishrock123]

Interesting. Maybe @fhinkel knows more about the average performance of ES Maps?

@mscdex while you are at it, would you mind to double check my new timers benchmarks (#10925)?

[Fishrock123]

cc also @misterdjules

[joyeecheung]

The last time I checked the performance of Map compared to objects depends greatly on the pattern of key insertions. Map performs better when the you do use it as a hash table (have a lot of different keys, or use it with different keys/ordering), but worse if the key patterns are fixed.

FWIW I don't think the patterns in -unpooled benchmarks(putting timers with timeout going from 0 to iterations) are very typical in user land though? Maybe we can add a few cases that only do a certain value of timeout.

EDIT: should be certain arrays of timeouts, like exponential ones.

[Fishrock123]

FWIW I don't think the patterns in -unpooled benchmarks(putting timers with timeout going from 0 to iterations) are very typical in user land though? Maybe we can add a few cases that only do a certain value of timeout.

Correct. They are not typical.

They represent the worst-case scenario. It is important to have that comparison so that we can see how apps in that case could perform.

EDIT: should be certain arrays of timeouts, like exponential ones.

Pardon?

---

I will note that this change does generally err on the side of improving a worse-case scenario rather than a best-case.

[joyeecheung]

@Fishrock123 By exponential ones I mean timeouts of [1, 2, 4, 8, 16, 32 ... 2 ** n](multiplied by 1000 probably), that's a pretty typical usage for retrying things.

[Fishrock123]

@joyeecheung Ah, understood. I'm not sure how that would make any difference to the internals compared to what the current (new) benchmarks stress though.

[Fishrock123]

cc @misterdjules Do you have any thoughts on this one?

[mscdex]

I can more-or-less confirm the benchmark results in the OP. I'm not sure this change is worth it, especially with the perf hit in timers-insert-unpooled.js.

Has anyone looked into alternatives such as hierarchical timer wheels (e.g. especially this implementation) or something else?

[Fishrock123]

I can more-or-less confirm the benchmark results in the OP. I'm not sure this change is worth it, especially with the perf hit in timers-insert-unpooled.js.

I think we should aim to do this if possible, but at the current time I'd prefer to keep this open and try to track why that is when V8 benchmarks appeared to show Maps as faster. (@nodejs/v8)

---

Has anyone looked into alternatives such as hierarchical timer wheels (e.g. especially this implementation) or something else?

Yes, quite a lot at the JS layer... As far as I can tell it would be less efficient than what we already have and far more complex.

Also, libuv recently-ish stalled moving to timer wheels, see libuv/libuv#823 (comment) and the linked document on why linux core moved away from timer wheels: https://www.kernel.org/doc/Documentation/timers/hrtimers.txt ...

Edit: I'd like to add that our average performance is, from what I can tell, better than anything I've seen documented elsewhere, and that our worst-case is also pretty good. (Both of which are O(1 + O(log m)) IIRC, where m is the number of distinct duration pools)

[mscdex]

linux core moved away from timer wheels

I read this earlier, which seems to suggest they didn't move away from them, but instead "reinvented" their existing implementation. It looks like this new reinvention eventually made it into the kernel.

Even so, I think the tickless timer wheel implementation I linked to previously is interesting and may help (at least design-wise if that particular implementation isn't used as-is) with any perf hit incurred by the need of a continually recurring timer.

[Fishrock123]

Read through that. Perhaps we could maintain a separate timers impl for that (although I'd prefer not to). The approach seams reasonable for I/O timeouts but I think it would be irresponsible for us to make all user timers use that as beyond a second or so (my estimate, they gave an example into the 17 minute range) the accuracy really starts to drop off and I don't think we could expect user applications to expect that kind of accuracy loss.

[Fishrock123]

I guess this PR is stalled unless ES maps get sufficiently fast.

[jasnell]

It shouldn't take too terribly long. Map performance is looking rather good under --ignition

[BridgeAR]

I benchmarked this change with 6.0 and it does not look like this has improved at all and the original benefit is gone. I ran this with only 10 runs but I think it is clear that it this is not a improvement.

Please note that delete got somewhat optimization in turbofan and that Map#set is more expensive then adding a property to a plain object.

                                                    improvement confidence      p.value
 timers/timers-breadth.js thousands=500                -10.03 %         ** 3.170429e-03
 timers/timers-cancel-pooled.js thousands=500           -3.05 %            2.629574e-01
 timers/timers-cancel-unpooled.js thousands=100          1.85 %            6.489744e-01
 timers/timers-depth.js thousands=1                      0.30 %            3.145621e-01
 timers/timers-insert-pooled.js thousands=500           -3.45 %            1.032163e-01
 timers/timers-insert-unpooled.js thousands=100         -8.28 %            2.918228e-01
 timers/timers-timeout-pooled.js thousands=500         -28.79 %        *** 8.739314e-05

@Fishrock123 I think this should be closed?

[BridgeAR]

Closing due to long inactivity and my current benchmark results.