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

Replace parking with std::thread::park #54

Closed
wants to merge 3 commits into from
Closed

Replace parking with std::thread::park #54

wants to merge 3 commits into from

Conversation

james7132
Copy link

parking adds extra compile time for something that's already available in the std lib. This PR replaces parking with std::thread::park and Thread::unpark when using future::block_on. It has also gotten signfigant platform-specific improvements that parking lacks. This might add one additional thread local lookup when the thread is parked, but if you're parking the thread anyway, I don't think that overhead really matters.

@notgull
Copy link
Member

notgull commented Jun 13, 2022

Doesn't park() add spurious wakeups, which parking avoids? I don't know, I'm still waiting on an answer for smol-rs/parking#7

@sbarral
Copy link

sbarral commented Jul 13, 2022

AFAIU the reason for using parking is not to avoid spurious wakeups but to prevent the risk of deadlock when the future also makes use of park/unpark.
See this issue: rust-lang/futures-rs#2010

@taiki-e
Copy link
Collaborator

taiki-e commented Jul 16, 2022

Yeah, the use of parking should be correct here. Also, parking is enough small and I don't think it would cause an increase in compile time in practice. (Try building with the --timings flag.)

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

Successfully merging this pull request may close these issues.

None yet

4 participants