use faster timers

[ronag]

Another major bottleneck I found is the constant refreshing of timers, in particular the body timeout timer which is refresh every time we get a packet.

Our current use of timers is that we have a very few timers which we update frequently where high accuracy is not required. This PR tries to implement that.

[ronag]

@mcollina This breaks all the tests using fake timers... thoughts on what to do about that?

[mcollina]

What benefits is this giving us?

I would prefer this timer to be attached to the agent, so we can actually stop them. If this is significantly faster, why aren't we briging that optimization to Node.js itself?

[ronag]

What benefits is this giving us?

In high throughput scenarios with lots of small packets we are constantly calling refresh on node timers which is much more expensive than what this PR does.

I would prefer this timer to be attached to the agent, so we can actually stop them. If this is significantly faster, why aren't we briging that optimization to Node.js itself?

I don't follow... Node has a more generic timer implementation which works for other less nisch use cases.

[ronag]

@mcollina PTAL

[ronag]

Note that the timers in this PR are much less accurate than Node core timers, which I believe is fine for our use case.

[mcollina]

Have you got a benchmark before/after?

[ronag]

Before:

[bench:run] │ undici - stream │ 1 │ 7383.29 req/sec │ ± 0.00 % │ + 395.85 % │

After:

[bench:run] │ undici - stream │ 1 │ 7683.40 req/sec │ ± 0.00 % │ + 358.60 % │

I'm a little sceptical about our benchmark accuracy...

[ronag]

I haven't looked into connectionsCheckingInterval so much. But would an alternative be the other way around? Go from a c++ version to an inaccurate but fast js version?

[ShogunPanda]

Well, I guess you could try and measure the impact.

For you to consider: connectionsCheckingInterval is a mechanism that every x seconds check the entire pool of opened inbound connections comparing (in the same "inaccurate" way you did here) if the timeout as gone off.
He's written in C++ (leveraging C sorted sets and thus able to easily bail out early in the loop) but I think it can be easily adapted in JS.

The only pain is to maintain the list of opened/active connections (especially when we think about KeepAlive)

[ronag]

I see. I wouldn't mind using that per se. I don't know how that would practically work in undici which is developed outside of core.

[ronag]

CI looks ok 👍

[mcollina]

lgtm

[mcollina]

CI is still very red :(

[ronag]

I will fix that

[ronag]

Seems ok now. Same as usual with windows on node 19.

[KhafraDev]

v12 CI failed on this test

     # Subtest: keep-alive threshold
        1..2
        ok 1 - should not error
        not ok 2 - (unnamed test)
          ---
          at:
            line: 217
            column: 13
            file: test/client-keep-alive.js
            type: Timeout
            method: _onTimeout
          stack: |
            Timeout._onTimeout (test/client-keep-alive.js:217:13)
          source: |2
                    const timeout = setTimeout(() => {
                      t.fail()
            ------------^
                    }, 2e3)
                    client.on('disconnect', () => {
          ...
        
        # failed 1 of 2 tests
    not ok 6 - keep-alive threshold # time=2017.2ms