Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

Merged
merged 1 commit into from Sep 5, 2019

Conversation

@alexcrichton
Copy link
Contributor

alexcrichton commented Aug 28, 2019

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 #1558
Closes #1695

@fitzgen

This comment has been minimized.

Copy link
Member

fitzgen commented Aug 28, 2019

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.

Since async/await won't be on stable until 1.39 in november, perhaps we should aim to do the breaking bump then?

@alexcrichton alexcrichton force-pushed the alexcrichton:std-futures branch from 33bfe08 to c716412 Aug 28, 2019
@alexcrichton

This comment has been minimized.

Copy link
Contributor Author

alexcrichton commented Aug 28, 2019

I could go either way on that I think. I do think that we'll want to use async/await in crates like wasm-bindgen-futures and wasm-bindgen-test, although it's technically possible that we don't have to. The current series of crates on crates.io should continue to work with the futures crate and whatever the latest version of wasm-bindgen is published, though.

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 wasm-bindgen-{test,futures} would not compile on stable Rust (but would compile on nightly), but the current versions on crates.io would continue to work.

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.

@Pauan

This comment has been minimized.

Copy link
Member

Pauan commented Aug 28, 2019

Can we have support for async fn foo() -> Result<(), JsValue> (or better yet, Result<(), js_sys::Error>)?

That's a lot more idiomatic than using unwrap.

@Pauan

This comment has been minimized.

Copy link
Member

Pauan commented Aug 29, 2019

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.

Copy link

yoshuawuyts left a comment

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)

@alexcrichton

This comment has been minimized.

Copy link
Contributor Author

alexcrichton commented Sep 3, 2019

@Pauan

Can we have support for async fn foo() -> Result<(), JsValue>

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!)

I had made a lot of improvements to the 0.3 code, which this PR gets rid of.

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?

@Pauan

This comment has been minimized.

Copy link
Member

Pauan commented Sep 4, 2019

Can you elaborate a bit on what optimizations the 0.3 implementation had we want to be sure to preserve?

  • future_to_promise should be implemented using spawn_local, not the other way around.

  • Rather than creating a Promise per Task per wakeup, it should generate 1 cached Promise for all Tasks.

@Pauan

This comment has been minimized.

Copy link
Member

Pauan commented Sep 4, 2019

I took a look at the multi-threading code:

  • future_to_promise accepts non-Send Futures, which seems wrong.

  • It doesn't seem to actually execute the Future on multiple threads, the only thing that happens on other threads is the wakeup (is that actually useful?)

Can we punt the multithreading implementation to a future PR while we work out these issues?

@Pauan

This comment has been minimized.

Copy link
Member

Pauan commented Sep 4, 2019

(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)

@alexcrichton

This comment has been minimized.

Copy link
Contributor Author

alexcrichton commented Sep 5, 2019

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.

future_to_promise accepts non-Send Futures, which seems wrong.

This is fundamentally required because futures produce JsValue which isn't Send. The returned value, Promise, is also pinned to exactly one thread since it can't leave the context of a JS thread. Basically the implementation is just ensuring that we can source events happening from any thread (any wakeup anywhere), but the final value is pinned to a particular thread.

It doesn't seem to actually execute the Future on multiple threads, the only thing that happens on other threads is the wakeup (is that actually useful?)

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 JsValue and then it's resolved on a JS thread.

(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)

That sounds good, I checked in with @fitzgen and I think the procedure here is going to be:

  • We'll merge this now
  • We'll publish new versions when necessary
  • If necessary to backport changes to the older versions we'll make a new branch and maintain them there. As soon as async/await hits stable I think we'll drop support for the current crates.io tracks of wasm-bindgen-{test,futures}
@alexcrichton alexcrichton force-pushed the alexcrichton:std-futures branch from c716412 to 05cbd44 Sep 5, 2019
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 #1558
Closes #1695
@alexcrichton alexcrichton force-pushed the alexcrichton:std-futures branch from 05cbd44 to 9455bf8 Sep 5, 2019
@alexcrichton alexcrichton merged commit 3c887c4 into rustwasm:master Sep 5, 2019
0 of 19 checks passed
0 of 19 checks passed
rustwasm.wasm-bindgen Build #20190905.2 failed
Details
rustwasm.wasm-bindgen (Build almost all examples) Build almost all examples failed
Details
rustwasm.wasm-bindgen (Build benchmarks) Build benchmarks failed
Details
rustwasm.wasm-bindgen (Build raytrace examples) Build raytrace examples failed
Details
rustwasm.wasm-bindgen (Dist Darwin binary) Dist Darwin binary failed
Details
rustwasm.wasm-bindgen (Dist Linux binary) Dist Linux binary failed
Details
rustwasm.wasm-bindgen (Dist Windows binary) Dist Windows binary failed
Details
rustwasm.wasm-bindgen (Doc - build the API documentation) Doc - build the API documentation failed
Details
rustwasm.wasm-bindgen (Doc - build the book) Doc - build the book failed
Details
rustwasm.wasm-bindgen (Run UI tests) Run UI tests failed
Details
rustwasm.wasm-bindgen (Run js-sys crate tests) Run js-sys crate tests failed
Details
rustwasm.wasm-bindgen (Run wasm-bindgen crate tests (Windows)) Run wasm-bindgen crate tests (Windows) failed
Details
rustwasm.wasm-bindgen (Run wasm-bindgen crate tests (nightly)) Run wasm-bindgen crate tests (nightly) failed
Details
rustwasm.wasm-bindgen (Run wasm-bindgen crate tests (unix)) Run wasm-bindgen crate tests (unix) failed
Details
rustwasm.wasm-bindgen (Run wasm-bindgen-cli crate tests) Run wasm-bindgen-cli crate tests failed
Details
rustwasm.wasm-bindgen (Run wasm-bindgen-wasm-interpreter tests) Run wasm-bindgen-wasm-interpreter tests failed
Details
rustwasm.wasm-bindgen (Run wasm-bindgen-webidl crate tests) Run wasm-bindgen-webidl crate tests failed
Details
rustwasm.wasm-bindgen (Run web-sys crate tests) Run web-sys crate tests failed
Details
rustwasm.wasm-bindgen (Test TypeScript output of wasm-bindgen) Test TypeScript output of wasm-bindgen failed
Details
@alexcrichton alexcrichton deleted the alexcrichton:std-futures branch Sep 5, 2019
@Pauan

This comment has been minimized.

Copy link
Member

Pauan commented Sep 5, 2019

Out of curiosity, was there data showing that the optimizations were necessary?

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).

Basically the implementation is just ensuring that we can source events happening from any thread (any wakeup anywhere), but the final value is pinned to a particular thread.

This is useful when your computation is, for example, being produced on a foreign rayon thread.

Ah, okay, I see. I had wrongly assumed that it was a proper multithreading implementation (like futures::executor::ThreadPool).

@alexcrichton

This comment has been minimized.

Copy link
Contributor Author

alexcrichton commented Sep 5, 2019

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).

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.

@Pauan

This comment has been minimized.

Copy link
Member

Pauan commented Sep 5, 2019

If you've got time to look into reapplying the optimization that'd be great

I already did, I'll send the PR tomorrow.

@alexcrichton

This comment has been minimized.

Copy link
Contributor Author

alexcrichton commented Sep 5, 2019

I already did, I'll send the PR tomorrow.

Thanks!

Can we have support for async fn foo() -> Result<(), JsValue>

#1754 now!

@tomaka tomaka mentioned this pull request Sep 19, 2019
0 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.