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: Limit max value of yield_every #2527

Merged
merged 1 commit into from Dec 18, 2021

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Nov 30, 2021

Fixes #2526

The initial version of the #2333 did the same thing (tokio-rs/tokio#3493 (comment)), but at the time I thought it was unnecessary and removed it.

before

n: 10000, time: 315ms
n: 20000, time: 1235ms
n: 40000, time: 4753ms
n: 80000, time: 18912ms
(timeout)

after

n: 10000, time: 29ms
n: 20000, time: 55ms
n: 40000, time: 104ms
n: 80000, time: 202ms
n: 160000, time: 392ms

A little slower than the version using unconstrained, but a little faster than the version using tokio::spawn, on my machine.

with unconstrained

n: 10000, time: 27ms
n: 20000, time: 51ms
n: 40000, time: 93ms
n: 80000, time: 179ms
n: 160000, time: 361ms

with tokio::spawn

n: 10000, time: 50ms
n: 20000, time: 100ms
n: 40000, time: 168ms
n: 80000, time: 340ms
n: 160000, time: 741ms

fyi @jonhoo

@taiki-e taiki-e added A-stream 0.3-backport: pending labels Nov 30, 2021
@taiki-e taiki-e force-pushed the taiki-e/futures_unordered branch from 9afc0f3 to ebf3681 Compare Nov 30, 2021
@psarna
Copy link

psarna commented Nov 30, 2021

Sounds very reasonable to have a constant cap on the number of polls in order to avoid quadratic behavior in Tokio (and I guess also other runtimes that implement preemption). Thanks! Crossing my fingers that it get accepted and backported.

@pkolaczk
Copy link

pkolaczk commented Dec 1, 2021

Is there any branch with changes backported to 0.3? I tried to use it but I couldn't make it work with tokio IntervalStream:

error[E0599]: the method `map` exists for struct `IntervalStream`, but its trait bounds were not satisfied
  --> src/main.rs:90:22
   |
90 |                     .map(|_| deadline.next())
   |                      ^^^ method cannot be called on `IntervalStream` due to unsatisfied trait bounds
   |
  ::: /home/pkolaczk/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-stream-0.1.8/src/wrappers/interval.rs:12:1
   |
12 | pub struct IntervalStream {
   | -------------------------
   | |
   | doesn't satisfy `IntervalStream: FutureExt`
   | doesn't satisfy `IntervalStream: futures::Future`
   | doesn't satisfy `IntervalStream: futures::StreamExt`
   | doesn't satisfy `IntervalStream: futures::Stream`
   | doesn't satisfy `IntervalStream: std::iter::Iterator`
   |
   = note: the following trait bounds were not satisfied:
           `IntervalStream: futures::Future`
           which is required by `IntervalStream: FutureExt`
           `IntervalStream: futures::Stream`
           which is required by `IntervalStream: futures::StreamExt`
           `&IntervalStream: futures::Future`
           which is required by `&IntervalStream: FutureExt`
           `&IntervalStream: futures::Stream`
           which is required by `&IntervalStream: futures::StreamExt`
           `&mut IntervalStream: futures::Future`
           which is required by `&mut IntervalStream: FutureExt`
           `&mut IntervalStream: futures::Stream`
           which is required by `&mut IntervalStream: futures::StreamExt`
           `IntervalStream: std::iter::Iterator`
           which is required by `&mut IntervalStream: std::iter::Iterator`
   = help: items from traits can only be used if the trait is in scope
help: the following traits are implemented but not in scope; perhaps add a `use` for one of them:
   |
1  | use std::iter::Iterator;
   |
1  | use futures::FutureExt;
   |
1  | use futures::StreamExt;
   |
1  | use futures_util::future::future::FutureExt;
   |
     and 5 other candidates

I do have use futures::StreamExt in my code...

jonhoo
jonhoo approved these changes Dec 1, 2021
Copy link
Contributor

@jonhoo jonhoo left a comment

I'd personally make this a const the same way it was in the pre-#2333 implementation, and include a bit more docs (you can probably copy-paste that too from the old code), but otherwise 👍 for this change.

@taiki-e taiki-e force-pushed the taiki-e/futures_unordered branch from ebf3681 to d3747e6 Compare Dec 18, 2021
@taiki-e
Copy link
Member Author

taiki-e commented Dec 18, 2021

@jonhoo Thanks for the review! I've re-added the constant along with docs that was removed in #2333, and added docs about #2333 and this issue.

@taiki-e taiki-e merged commit afff1ce into master Dec 18, 2021
22 checks passed
@taiki-e taiki-e deleted the taiki-e/futures_unordered branch Dec 18, 2021
This was referenced Dec 18, 2021
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending labels Dec 18, 2021
@taiki-e
Copy link
Member Author

taiki-e commented Dec 18, 2021

Published in 0.3.19.

@@ -340,7 +340,7 @@ fn polled_only_once_at_most_per_iteration() {

let mut tasks = FuturesUnordered::from_iter(vec![F::default(); 33]);
assert!(tasks.poll_next_unpin(cx).is_pending());
assert_eq!(33, tasks.iter().filter(|f| f.polled).count());
assert_eq!(32, tasks.iter().filter(|f| f.polled).count());
Copy link
Member

@nagisa nagisa Jan 12, 2022

Choose a reason for hiding this comment

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

Shouldn't this test be adjusted to ensure that eventually all of the contained futures are polled?

Copy link
Member Author

@taiki-e taiki-e Jan 13, 2022

Choose a reason for hiding this comment

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

The purpose of the test is to make sure that the same future is not polled multiple times in a single poll_next call (#2333), so I think it's fine as is.

fn polled_only_once_at_most_per_iteration() {

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.

5 participants