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

runtime: avoid unnecessary polling of block_on future #3582

Merged
merged 7 commits into from
Mar 16, 2021

Conversation

zaharidichev
Copy link
Contributor

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.

Fix: tokio-rs#3475

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Copy link
Contributor

@Darksonn Darksonn left a 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.

Comment on lines +76 to +77
// indicates whether the blocked on thread was woken
woken: AtomicBool,
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Member

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.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Mar 5, 2021
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Comment on lines 11 to 17
#[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));
Copy link
Contributor

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>.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Comment on lines 13 to 14
use std::sync::atomic::AtomicBool;
use std::sync::atomic::Ordering::{Acquire, Release};
Copy link
Contributor

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>
@carllerche
Copy link
Member

I reviewed the PR. I believe the AtomicBool is in the correct location as concurrent calls to block_on have some coordination ensuring only a single call gets to that AtomicBool.

However, I do believe there is a race condition.

  • Task is notified by A.
  • Task wakes up
  • Task is notified by B.
  • Task sets bool to false w/ Release ordering.
  • Task executes w/o "acquiring" the memory from B.
  • Task doesn't park and loops again to "consume" the wakeup from B.
  • Task skips polling as bool is `false.

First, this should be verified w/ loom.

The fix would be to ensure that memory is "acquired" when setting the bool to false. This is probably a question of using swap w/ AcqRel.

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
@zaharidichev
Copy link
Contributor Author

@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?

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
@carllerche
Copy link
Member

@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 true then notify the task. The block_on task should be able to observe (in potentially multiple iteration) both bools being set to true.

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>
Copy link
Member

@carllerche carllerche left a 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.

Copy link
Contributor

@Darksonn Darksonn left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants