-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Default all async support to std::future
#1741
Conversation
5b3ca90
to
33bfe08
Compare
Since async/await won't be on stable until 1.39 in november, perhaps we should aim to do the breaking bump then? |
33bfe08
to
c716412
Compare
I could go either way on that I think. I do think that we'll want to use In that sense if we were to publish/merge this now it means that local testing of this requires nightly (not that big of a deal) but the latest versions of I'm curious if others using these crates have thoughts on whether it'd be nice to land sooner rather than later given all this, I could personally go either way. |
Can we have support for That's a lot more idiomatic than using |
I haven't done a super thorough review, but it looks like you took the 0.1 code and ported it to 0.3. I think it would be better to go the other way around: take the 0.3 code and add atomic support to it. I had made a lot of improvements to the 0.3 code, which this PR gets rid of. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! -- I'd be in favor of merging this sooner rather than later (as I feel I say every time this comes up, haha)
I think we definitely should! I added enough support for that to tests, but I'd like to work a bit more yeah to get that working as well for futures. I think it may be best to file an issue for that and track in a follow-up PR though. (or do here if I or anyone else gets time!)
Oh oops, sorry didn't mean to lose all that! Can you elaborate a bit on what optimizations the 0.3 implementation had we want to be sure to preserve? |
|
I took a look at the multi-threading code:
Can we punt the multithreading implementation to a future PR while we work out these issues? |
(In the interest of getting this merged, we can merge this as-is and I can send a follow-up PR which adds back in the optimizations) |
Out of curiosity, was there data showing that the optimizations were necessary? I could imagine that hundreds/thousands of promises is probably worse than one, but I'm not sure if that practically really comes up that often.
This is fundamentally required because futures produce
That's correct, and intended. (it's also a fallout of the previous point). This is useful when your computation is, for example, being produced on a foreign rayon thread. When the final notification comes in that a value is ready some small work is done to convert it to a
That sounds good, I checked in with @fitzgen and I think the procedure here is going to be:
|
c716412
to
05cbd44
Compare
This commit defaults all crates in-tree to use `std::future` by default and none of them support the crates.io `futures` 0.1 crate any more. This is a breaking change for `wasm-bindgen-futures` and `wasm-bindgen-test` so they've both received a major version bump to reflect the new defaults. Historical versions of these crates should continue to work if necessary, but they won't receive any more maintenance after this is merged. The movement here liberally uses `async`/`await` to remove the need for using any combinators on the `Future` trait. As a result many of the crates now rely on a much more recent version of the compiler, especially to run tests. The `wasm-bindgen-futures` crate was updated to remove all of its futures-related dependencies and purely use `std::future`, hopefully improving its compatibility by not having any version compat considerations over time. The implementations of the executors here are relatively simple and only delve slightly into the `RawWaker` business since there are no other stable APIs in `std::task` for wrapping these. This commit also adds support for: #[wasm_bindgen_test] async fn foo() { // ... } where previously you needed to pass `(async)` now that's inferred because it's an `async fn`. Closes rustwasm#1558 Closes rustwasm#1695
05cbd44
to
9455bf8
Compare
Yes. It's common for dominator to spawn hundreds (or even thousands) of Futures simultaneously, and the situation is even worse with animations (because hundreds of Futures can run on every frame).
Ah, okay, I see. I had wrongly assumed that it was a proper multithreading implementation (like |
Nice! If you've got time to look into reapplying the optimization that'd be great, otherwise I can try to find time as well to reimplement it. |
I already did, I'll send the PR tomorrow. |
Thanks!
#1754 now! |
This commit defaults all crates in-tree to use
std::future
by defaultand none of them support the crates.io
futures
0.1 crate any more.This is a breaking change for
wasm-bindgen-futures
andwasm-bindgen-test
so they've both received a major version bump toreflect the new defaults. Historical versions of these crates should
continue to work if necessary, but they won't receive any more
maintenance after this is merged.
The movement here liberally uses
async
/await
to remove the need forusing any combinators on the
Future
trait. As a result many of thecrates now rely on a much more recent version of the compiler,
especially to run tests.
The
wasm-bindgen-futures
crate was updated to remove all of itsfutures-related dependencies and purely use
std::future
, hopefullyimproving its compatibility by not having any version compat
considerations over time. The implementations of the executors here are
relatively simple and only delve slightly into the
RawWaker
businesssince there are no other stable APIs in
std::task
for wrapping these.This commit also adds support for:
where previously you needed to pass
(async)
now that's inferredbecause it's an
async fn
.Closes #1558
Closes #1695