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

FuturesUnordered: Do not poll the same future twice per iteration #2333

Merged
merged 2 commits into from
Feb 20, 2021

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Feb 7, 2021

Instead of using a constant, use the length of FuturesUnordered to ensures that each future is polled only once at most per iteration. This does not solve the fundamental problem, but should be able to improve the current situation somewhat.

This fixes at least one of the problems reported in tokio-rs/tokio#3493. See that issue for details of the bug.

r? @cramertj @jonhoo
cc #2053

@taiki-e taiki-e added the A-stream Area: futures::stream label Feb 7, 2021
@jonhoo
Copy link
Contributor

jonhoo commented Feb 11, 2021

I think my only concern here is whether the futures are guaranteed to not have their order changed during a single call to FuturesUnordered::poll If that were the case, then we might end up calling (worst case) a single future .len() times if it keeps being moved to the front of the poll list. This is just from memory, but I seem to recall there being a linked list in the implementation with an insert on .wake(), which is where this fear is coming from. If that's not the case, then +1 from me.

It doesn't solve the underlying issue, but does make things at least a bit more sane.

@taiki-e
Copy link
Member Author

taiki-e commented Feb 11, 2021

I think my only concern here is whether the futures are guaranteed to not have their order changed during a single call to FuturesUnordered::poll

Good point.
IIUC, ReadyToRunQueue::dequeue is the only function that can change the order of futures within the range (0..self.len) that acquired at the first of poll_next. And AFAIK this is only called in Stream::poll_next and Drop::drop that take mutable reference. (enqueue adds a task at the last of the queue, so the order doesn't change.)

/// The dequeue function from the 1024cores intrusive MPSC queue algorithm
///
/// Note that this is unsafe as it required mutual exclusion (only one
/// thread can call this) to be guaranteed elsewhere.
pub(super) unsafe fn dequeue(&self) -> Dequeue<Fut> {

// Safety: &mut self guarantees the mutual exclusion `dequeue`
// expects
let task = match unsafe { self.ready_to_run_queue.dequeue() } {

So, my understanding is that the order doesn't change. But I might have missed something.

@taiki-e taiki-e merged commit f273d3b into rust-lang:master Feb 20, 2021
@taiki-e taiki-e deleted the futures-unordered branch February 20, 2021 07:39
@taiki-e taiki-e mentioned this pull request Feb 20, 2021
krallin added a commit to krallin/futures-rs that referenced this pull request Feb 23, 2021
…per iteration

Same as rust-lang#2333. The same issue exists in 0.1, so backporting it there
helps for code that is still using Futures 0.1 in some places.
krallin added a commit to krallin/futures-rs that referenced this pull request Feb 23, 2021
…per iteration

Same as rust-lang#2333. The same issue exists in 0.1, so backporting it there
helps for code that is still using Futures 0.1 in some places.
krallin added a commit to krallin/futures-rs that referenced this pull request Feb 24, 2021
…per iteration

Same as rust-lang#2333. The same issue exists in 0.1, so backporting it there
helps for code that is still using Futures 0.1 in some places.
taiki-e pushed a commit that referenced this pull request Feb 24, 2021
…per iteration (#2358)

Same as #2333. The same issue exists in 0.1, so backporting it there
helps for code that is still using Futures 0.1 in some places.
facebook-github-bot pushed a commit to facebookexperimental/rust-shed that referenced this pull request Mar 4, 2021
Summary:
Those newer versions of Futures have compatibility improvements with Tokio,
notably:

- rust-lang/futures-rs#2333
- rust-lang/futures-rs#2358

Reviewed By: farnz

Differential Revision: D26778794

fbshipit-source-id: 5a9dc002083e5edfa5c614d8d2242e586a93fcf6
facebook-github-bot pushed a commit to facebook/sapling that referenced this pull request Mar 4, 2021
Summary:
Those newer versions of Futures have compatibility improvements with Tokio,
notably:

- rust-lang/futures-rs#2333
- rust-lang/futures-rs#2358

Reviewed By: farnz

Differential Revision: D26778794

fbshipit-source-id: 5a9dc002083e5edfa5c614d8d2242e586a93fcf6
facebook-github-bot pushed a commit to facebook/fbthrift that referenced this pull request Mar 4, 2021
Summary:
Those newer versions of Futures have compatibility improvements with Tokio,
notably:

- rust-lang/futures-rs#2333
- rust-lang/futures-rs#2358

Reviewed By: farnz

Differential Revision: D26778794

fbshipit-source-id: 5a9dc002083e5edfa5c614d8d2242e586a93fcf6
facebook-github-bot pushed a commit to facebookincubator/below that referenced this pull request Mar 4, 2021
Summary:
Those newer versions of Futures have compatibility improvements with Tokio,
notably:

- rust-lang/futures-rs#2333
- rust-lang/futures-rs#2358

Reviewed By: farnz

Differential Revision: D26778794

fbshipit-source-id: 5a9dc002083e5edfa5c614d8d2242e586a93fcf6
facebook-github-bot pushed a commit to facebook/fb303 that referenced this pull request Mar 4, 2021
Summary:
Those newer versions of Futures have compatibility improvements with Tokio,
notably:

- rust-lang/futures-rs#2333
- rust-lang/futures-rs#2358

Reviewed By: farnz

Differential Revision: D26778794

fbshipit-source-id: 5a9dc002083e5edfa5c614d8d2242e586a93fcf6
vkill pushed a commit to bk-rs/fbthrift-git-rs that referenced this pull request Mar 23, 2021
Summary:
Those newer versions of Futures have compatibility improvements with Tokio,
notably:

- rust-lang/futures-rs#2333
- rust-lang/futures-rs#2358

Reviewed By: farnz

Differential Revision: D26778794

fbshipit-source-id: 5a9dc002083e5edfa5c614d8d2242e586a93fcf6
exrook pushed a commit to exrook/futures-rs that referenced this pull request Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants