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

reset woken of outer future after polled #4157

Merged
merged 2 commits into from
Oct 12, 2021
Merged

Conversation

suikammd
Copy link
Contributor

@suikammd suikammd commented Oct 7, 2021

Motivation

Current implementation does not reset woken flag after polling outer future. This results in outer future being polled again and again which is unnecessary when outer future not ready.

Solution

Swap woken flag to false at the start of loop.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime R-loom Run loom tests on this PR labels Oct 7, 2021
@Darksonn Darksonn requested a review from zaharidichev October 7, 2021 16:23
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 don't really understand why it wasn't done like this in #3582.

tokio/src/runtime/basic_scheduler.rs Show resolved Hide resolved
Copy link
Contributor

@zaharidichev zaharidichev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks correct. You think there is a test you can write for this?

@suikammd
Copy link
Contributor Author

suikammd commented Oct 8, 2021

It seems not easy to test. If I want to test this, I have to change many fields or structs' visibility. :)

@Darksonn
Copy link
Contributor

Darksonn commented Oct 8, 2021

The other PR had a test that a certain future was not polled more than three times. It seems like a test like that could work here without changing the visibility of anything.

@suikammd
Copy link
Contributor Author

suikammd commented Oct 8, 2021

The other PR had a test that a certain future was not polled more than three times. It seems like a test like that could work here without changing the visibility of anything.

What I want to test is woken flag stay false after the outer future was polled.

@Darksonn
Copy link
Contributor

Darksonn commented Oct 8, 2021

You could test that checking whether it gets polled when some other spawned future gets polled, no?

@github-actions github-actions bot removed the R-loom Run loom tests on this PR label Oct 9, 2021
@suikammd suikammd force-pushed the master branch 10 times, most recently from 78ff17f to 8f46f0e Compare October 9, 2021 06:19
@suikammd
Copy link
Contributor Author

suikammd commented Oct 9, 2021

That looks correct. You think there is a test you can write for this?

cc @Darksonn @zaharidichev please review the new added test

@Darksonn Darksonn added the R-loom Run loom tests on this PR label Oct 10, 2021
// expect ResetFuture.pending_cnt = 1
// 2. recv message from channel, ResetFuture returns Ready immediately.
// We expect ResetFuture.pending_cnt = 0
future::ready(()).await;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing you want tokio::task::yield_now here? Awaiting a future::ready doesn't actually yield.

@@ -31,6 +31,40 @@ fn assert_at_most_num_polls(rt: Arc<Runtime>, at_most_polls: usize) {
assert!(polls <= at_most_polls);
}

fn assert_no_unnecessary_poll(rt: 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.

You don't need to wrap the runtime in an Arc as its used here.

@github-actions github-actions bot removed the R-loom Run loom tests on this PR label Oct 10, 2021
@Darksonn
Copy link
Contributor

Please don't force push the PR when you make changes. It's a lot harder to keep track of what changed since I last looked when you do that.

@Darksonn Darksonn added the R-loom Run loom tests on this PR label Oct 10, 2021
@suikammd
Copy link
Contributor Author

Please don't force push the PR when you make changes. It's a lot harder to keep track of what changed since I last looked when you do that.

I see, I was worried about too many commits.

@Darksonn
Copy link
Contributor

We squash your PR on merge, so that's not a concern.

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 R-loom Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants