-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
runtime: avoid unnecessary polling of block_on future #3582
Conversation
Fix: tokio-rs#3475 Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see a test that counts the number of calls to poll
.
// indicates whether the blocked on thread was woken | ||
woken: AtomicBool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we have multiple calls to block_on
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Darksonn the field is set to false in Spawner::waker_ref
, Wouldn't that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you implement the loom test I have mentioned below, that will answer this question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is additional synchronization that ensures only one concurrent blocker hits this bool.
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
#[test] | ||
fn block_on_num_polls() { | ||
loom::model(|| { | ||
let rt = runtime::Builder::new_current_thread().build().unwrap(); | ||
|
||
let (tx, rx) = oneshot::channel(); | ||
let num_polls = Arc::new(AtomicUsize::new(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loom tests are not really interesting when there's only one thread, however I'd like to see a test that spawns a few threads, doing the spawn
/block_on
from below in each thread, but each instance of the test performed on the same runtime shared with Arc<Runtime>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also make sense to have just the above test without threading, but it should be a non-loom test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, thanks a lot.
tokio/src/runtime/basic_scheduler.rs
Outdated
use std::sync::atomic::AtomicBool; | ||
use std::sync::atomic::Ordering::{Acquire, Release}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use crate::loom::sync::atomic::AtomicBool
here?
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
I reviewed the PR. I believe the However, I do believe there is a race condition.
First, this should be verified w/ loom. The fix would be to ensure that memory is "acquired" when setting the bool to |
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
@carllerche that makes sense. I guess I am just having trouble coming up with a valid test for that. If that is the case shouln't this test deadlock? |
It isn't a question of deadlock, it's a question of synchronizing memory. So, to reproduce with loom, you need to create two atomics (atomic bool initialized as false fine). In two separate threads, set each bool to This probably will require a block_on poll_fn: block_on(poll_fn(|cx| {
if bool1.load(Acquire) && bool2.load(Acquire) {
Ready(())
} else {
Pending
}
}); |
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks 👍. I will let @Darksonn have a last glance and merge if it looks good to her.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me.
Motivation
As explained in #3475, futures passed to block_on get polled when subtasks become ready.
Solution
Introduce a
was_woken
field in the waker to track when the future should be polled.