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
Contributor

@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
@ibraheemdev
Copy link
Contributor Author

ibraheemdev commented May 7, 2021

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 A-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 and removed 0.3-backport: pending labels May 14, 2021
@ibraheemdev
Copy link
Contributor Author

ibraheemdev commented May 23, 2021

 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)

futures-util/src/future/join_all.rs Outdated Show resolved Hide resolved
@taiki-e taiki-e added S-waiting-on-author and removed S-waiting-on-author labels Jun 16, 2021
@taiki-e taiki-e removed the S-waiting-on-author label Jul 29, 2021
Copy link
Member

@taiki-e taiki-e left a comment

LGTM. Thanks @ibraheemdev!

@taiki-e taiki-e merged commit b57e698 into rust-lang:master Jul 29, 2021
22 checks passed
@taiki-e taiki-e added the 0.3-backport: pending label Jul 29, 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 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
Contributor Author

ibraheemdev commented Sep 1, 2021

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.

3 participants