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

Use futuresordered in join_all #2412

Merged
merged 9 commits into from Jul 29, 2021
Merged

Conversation

ibraheemdev
Copy link
Member

@ibraheemdev ibraheemdev commented May 6, 2021

Switch to FuturesOrdered in join_all when dealing with large amounts of futures. We will probably need benchmarks to determine what SMALL is.

Closes #2201

@ibraheemdev ibraheemdev requested a review from taiki-e as a code owner May 6, 2021 21:33
@ibraheemdev
Copy link
Member Author

I think CI is failing because FuturesUnordered is feature gated under cfg_target_has_atomic?

@taiki-e
Copy link
Member

taiki-e commented May 7, 2021

I think CI is failing because FuturesUnordered is feature gated under cfg_target_has_atomic?

Yeah, I think you probably have to use #[cfg(not(futures_no_atomic_cas))] on JoinAllKind::Big. (In other words, if atomic CAS is not available, always use JoinAllKind::Small.)

@taiki-e taiki-e added 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. A-stream Area: futures::stream labels May 7, 2021
@taiki-e
Copy link
Member

taiki-e commented May 10, 2021

I think CI is failing because FuturesUnordered is feature gated under cfg_target_has_atomic?

Yeah, I think you probably have to use #[cfg(not(futures_no_atomic_cas))] on JoinAllKind::Big. (In other words, if atomic CAS is not available, always use JoinAllKind::Small.)

Btw, at the time this PR was opened, it was cfg_target_has_atomic, but now it is futures_no_atomic_cas after #2400 has been merged.

@taiki-e taiki-e added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author and removed 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels May 14, 2021
@ibraheemdev
Copy link
Member Author

 error[E0599]: no variant named `Big` found for enum `JoinAllKind<F>`
  --> futures-util/src/future/join_all.rs:45:9
   |
37 | / pin_project_lite::pin_project! {
38 | |     #[project = JoinAllKindProj]
39 | |     pub enum JoinAllKind<F>
40 | |     where
...  |
45 | |         Big  { #[pin] fut: Collect<FuturesOrdered<F>, Vec<F::Output>> },
   | |         ^^^ variant not found in `JoinAllKind<F>`
46 | |     }
47 | | }
   | |_- variant `Big` not found here

Is this a pin-project-lite problem?

@taiki-e
Copy link
Member

taiki-e commented May 23, 2021

Yeah, that is pin-project-lite's limitation: taiki-e/pin-project-lite#3 (comment)

@taiki-e taiki-e added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author labels Jun 16, 2021
@taiki-e taiki-e removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label Jul 29, 2021
Copy link
Member

@taiki-e taiki-e 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 @ibraheemdev!

@taiki-e taiki-e merged commit b57e698 into rust-lang:master Jul 29, 2021
@taiki-e taiki-e added the 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. label Jul 29, 2021
taiki-e pushed a commit that referenced this pull request Aug 28, 2021
@taiki-e taiki-e mentioned this pull request Aug 28, 2021
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Aug 28, 2021
@stuhood
Copy link
Contributor

stuhood commented Sep 1, 2021

Hey folks! Thanks for the optimization. Were any benchmarks performed for this? Also, would the same optimization apply to https://github.com/rust-lang/futures-rs/blob/dd019055ef5bf4309f15db934407e202caf52e14/futures-util/src/future/try_join_all.rs ? It seems to have the same shape.

@ibraheemdev
Copy link
Member Author

Good point, try_join_all could use the same optimization. Note that this is not a complete fix, if you pass a large iterator without a size hint, the issue is still there. No, no benchmarks were performed, but the issue is quite apparent, although SMALL could probably be tweaked.

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.

Shall quadratic complexity of join_all be documented?
3 participants