Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider a timeout heap #6

Closed
Raynos opened this issue May 25, 2020 · 4 comments
Closed

Consider a timeout heap #6

Raynos opened this issue May 25, 2020 · 4 comments

Comments

@Raynos
Copy link

Raynos commented May 25, 2020

We implemented a timeout Heap datastructure at uber ( https://github.com/uber/tchannel-node/blob/master/time_heap.js ).

It supports a minTimeout function or number which we use like

    // for processing operation timeouts
    this.timeHeap = this.options.timeHeap || new TimeHeap({
        timers: this.timers,
        // TODO: do we still need/want fuzzing?
        minTimeout: fuzzedMinTimeout
    });

    function fuzzedMinTimeout() {
        var fuzz = self.options.timeoutFuzz;
        if (fuzz) {
            fuzz = Math.floor(fuzz * (self.random() - 0.5));
        }
        return self.options.timeoutCheckInterval + fuzz;
    }

Using a heap is better then an array or linked list for nuanced reasons I don't quite understand.

But what is better then setTimeout is to say

"we will set a min timeout for 100ms and if a lot of "timers" expire in this minTimeout window then we will fire their callback function with upto 100ms delay.

This basically means that if you have 10,000 sockets you are garantueed a maximum of 10 setTimeout calls on the libuv event loop every 10 seconds no matter what load there is.

If you have a 5s or 30s timeout on sockets then an extra 100ms delay on the socket.destroy() is not a big deal in our usecase.

Also timeoutFuzz is a super awesome feature to avoid "thundering herd" problems, if you restart all your servers at the same time they might all set a timeout to expire exactly 10s in the future, by adding 50ms fuzz to either side you can spread out a "thundering reconnection herd" over a 100ms window in time instead of a 0ms window in time.

@mcollina
Copy link
Owner

Would you like to send a PR?

Overall this module is way less useful as setTimeout became really fast recently thanks to improvement in Node.js, so this is hardly needed at all in its current form. If there is something that we can use to improve its performance still, I'd be happy to get a PR.

@Raynos
Copy link
Author

Raynos commented May 25, 2020

For timeouts on sockets that are ~ 1 minute it's probably fine.

We build an RPC framework and were pushing 1000 QPS with timeouts of 100ms by default under the motto of "Your P99 should be 100ms or go home". Trashing the event loop with 1000 timers of 100ms each was not a happy time :(

After reading more of the README I think our use cases are very different.

@mcollina
Copy link
Owner

Trashing the event loop with 1000 timers of 100ms each was not a happy time :(

Since Node 11, the timers would have been deduplicated and you'd get only 10.
Essentially a similar technique of what you are mentioning got implemented in core.
See nodejs/node#20555 and nodejs/node#20894

@Raynos
Copy link
Author

Raynos commented May 25, 2020

I never got the chance to upgrade the version of node and check out the performance at scale 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants