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

Use proper timers in sleepAsync. #8209

Closed
wants to merge 1 commit into from
Closed

Conversation

dom96
Copy link
Contributor

@dom96 dom96 commented Jul 5, 2018

I'm surprised this wasn't done before. This should fix all problems associated with sleepAsync and high CPU usage.

Fixes #7886.

Fixes #7758 (remember that `poll` waits for 500ms, change the
             timeout to something like 5s and you'll get
             1 poll call for that code)

Fixes #6929.

Fixes #3909.
@dom96 dom96 force-pushed the sleepAsync-use-timers branch from df22afb to 29bf869 Compare July 5, 2018 16:06
@Araq
Copy link
Member

Araq commented Jul 6, 2018

Unfortunately that means async timers consume a file handle. This means for an HTTP connection you use 2 FDs where previously only 1 was required.

@dom96
Copy link
Contributor Author

dom96 commented Jul 6, 2018

Yes, but this is a worthy trade-off.

@yglukhov
Copy link
Member

yglukhov commented Jul 6, 2018

This may be only useful for user-implemented runloops. Otherwise it's extremely harmful, and a showstopper for high load servers.

EDIT: Not high load servers, but the code making heavy use of sleepAsync.

@yglukhov
Copy link
Member

yglukhov commented Jul 6, 2018

I'd suggest making this a default (but configurable) option of the dispatcher.

@dom96
Copy link
Contributor Author

dom96 commented Jul 6, 2018

For servers where this is a bottleneck I think that the developers would be better off implementing their own timing functionality. This change removes a lot of relatively complex code and I would prefer to simply remove it rather than keep it as an option.

@yglukhov
Copy link
Member

yglukhov commented Jul 6, 2018

"implementing their own" mindset doesn't play well with code reuse. E.g. I've implemented my own timers. Now I can't use third-party libs using sleepAsync.

@dom96
Copy link
Contributor Author

dom96 commented Jul 8, 2018

Can you give a real-world example where these pseudo-timers would be necessary and the new timers would be a big problem?

@Araq
Copy link
Member

Araq commented Jul 9, 2018

@dom96 please add timer support to your httpbeast library and measure both the old timers and your patch.

@dom96
Copy link
Contributor Author

dom96 commented Jul 9, 2018

@Araq The only thing I would use timers for in httpbeast is to refresh the current server time (sadly, the HTTP spec mandates that it is sent with each request...). I'm already using the OS timers for that. I can change it to use sleepAsync pre-this-PR if that's what you mean?

@Araq
Copy link
Member

Araq commented Jul 9, 2018

Huh? Every HTTP connection can time-out. Don't you need timers for that?

@dom96
Copy link
Contributor Author

dom96 commented Jul 9, 2018

Huh? Every HTTP connection can time-out. Don't you need timers for that?

Nope. Although I suppose I should add those, ugh.

@yglukhov
Copy link
Member

yglukhov commented Jul 9, 2018

Let's say there's a high load HTTP server that proxies a request to another server (not necessarily HTTP), and has to return a timeout error page in case of timeout. Likely it will use asyncSleep per every request. asyncSleep consuming an FD will effectively reduce the number of concurrently handled connections by factor of 2. Not to mention that your asyncSleep introduces a syscall, which is not performance friendly.

dom96 added a commit that referenced this pull request Aug 22, 2018
@dom96 dom96 closed this Aug 22, 2018
Araq pushed a commit that referenced this pull request Aug 23, 2018
Araq pushed a commit that referenced this pull request Aug 23, 2018
@narimiran narimiran deleted the sleepAsync-use-timers branch October 11, 2018 06:53
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

Successfully merging this pull request may close these issues.

3 participants