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

asyncdispatch.poll() eats cpu; poll is not blocking #6929

Closed
enthus1ast opened this issue Dec 15, 2017 · 9 comments
Closed

asyncdispatch.poll() eats cpu; poll is not blocking #6929

enthus1ast opened this issue Dec 15, 2017 · 9 comments
Labels
Async Everything related to Nim's async Standard Library

Comments

@enthus1ast
Copy link
Contributor

enthus1ast commented Dec 15, 2017

import asyncdispatch
proc foo() {.async.} = 
  while true:
    echo "in foo"
    await sleepAsync(1000)

asyncCheck foo()
while true:
  echo "."
  poll(500)

results in 100% cpu load on one core. Tested on linux

edit: fixed typo

@ghost ghost added the Async Everything related to Nim's async label Dec 15, 2017
@dom96
Copy link
Contributor

dom96 commented Dec 15, 2017

Proves we need to move to proper timers :)

@cheatfate
Copy link
Member

All latest changes to poll() procedure causes this bug... And changing behavior of sleepAsync() to use hardware timers is not a solution for this problem.

@cheatfate
Copy link
Member

cheatfate commented Feb 20, 2018

After some investigation, i have found that this bug is regression, made by somebody who introduced this branch (https://github.com/nim-lang/Nim/blob/devel/lib/pure/asyncdispatch.nim#L1234).
As i mentioned in other issues, this happens just because of policy of rash and untested 'improvements'.

@enthus1ast, remove this line, and everything will be fine.

@dom96, while trying to fix this #4262 via 5bf1643, you have introduced this one.

@dom96
Copy link
Contributor

dom96 commented Feb 21, 2018

Are you sure this is a regression? This line was always in there: https://github.com/nim-lang/Nim/blame/000b8afd26fa16684a116d9afe798ea94df9c270/lib/upcoming/asyncdispatch.nim#L1222

@cheatfate
Copy link
Member

cheatfate commented Feb 21, 2018

@dom96, i'm sure that exactly this check must be removed, but this will enable #4262 again, but #4262 must be fixed in other way. This check is actually disables timeouts processing, when there no descriptors registered.

@enthus1ast
Copy link
Contributor Author

enthus1ast commented Feb 26, 2018

this is the output of the above test with
asyncdispatch.nim#L1234). removed:

.
.
in foo
.
.
.
.
.
.
.
in foo
.
.
in foo
.
.
in foo
.
.
.
.
.
.
.
.
.
.
.
in foo
.
.
in foo
.

@cheatfate
Copy link
Member

@enthus1ast, looks like work as expected, isn't it?

@enthus1ast
Copy link
Contributor Author

enthus1ast commented Feb 26, 2018

@cheatfate it works! (i just have no clue how asyncdispatch works inside, i guess...)

dom96 added a commit that referenced this issue Jul 5, 2018
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 added a commit that referenced this issue Jul 5, 2018
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
Copy link
Contributor

dom96 commented Aug 22, 2018

You were right all along @cheatfate :)

I decided to finally investigate how to fix #7758/#7886 and ended up doing exactly what you suggest here. I can't reproduce #4262 anymore though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Async Everything related to Nim's async Standard Library
Projects
None yet
Development

No branches or pull requests

3 participants