Threadsafe futures #1514

ibaryshnikov · 2019-05-04T14:07:11Z

Changes:

Added extern function wait_async, which does the same as Atomics::waitAsync
Removed impl Notify for Package, also removed Send + Sync markers for Package
Added Waker struct instead, which implements Notify
Used Waker instead of Package to notify the executor

crates/js-sys/src/lib.rs

crates/futures/src/lib.rs

alexcrichton

I think the general structure that this is going to want to take is that spawning a future creates two main allocations. One is a Rc (or something like that) which holds the future itself. Another is an Arc which has some shared state. When creating a waker the Arc is what's shared, and its notify implementation will probably look like:

if !self.notified.swap(true) {
    // run an atomic notify at the address of `&self.notified`
}

In the poll loop I think it would look roughly like:

let promise = wait_async_on(&self.notified);
while self.notified.swap(true) {
    // do the poll
}
promise.then(&poll_again);

crates/futures/src/polyfill.js

ibaryshnikov · 2019-05-06T23:11:10Z

Does it mean that the state with Polling | Notified | Waiting will be removed, and there will be only true or false?

alexcrichton · 2019-05-08T14:40:45Z

Oh what I gisted was just a straw man, it may be that two bits are still needed rather than one for information exchange.

ibaryshnikov · 2019-05-19T19:17:04Z

@alexcrichton the pr is not tested yet, I pushed changes to check the design.
There are several places to check: first, I removed impl Notify for Package. Then, instead of &me, there is &me.waker passed to poll_future_notify.
And I moved code from impl Notify for Package to fn poll_again
Inside fn poll_again Promise::resolve was replaced with wait_async
Did I get your idea right?

alexcrichton

This looks along the right lines yeah! There's probably a lot of devils in the details, but I think the general direction here is where we want to go.

crates/futures/src/lib.rs

ibaryshnikov · 2019-05-26T22:00:44Z

@alexcrichton I think I hit the wall here: #1525
There's a need to either make js snippets work with node target or replace the snippet with polyfill with the wasm equivalent. What do you think is better?

alexcrichton · 2019-05-28T14:55:40Z

Yeah it may be some time before snippets work on Node, so it's probably best to not block this on that. Perhaps the polyfill could be implemented in Rust though?

alexcrichton

Some great progress!

crates/futures/src/polyfill.rs

crates/futures/src/polyfill.js

crates/futures/src/polyfill.rs

crates/futures/src/lib.rs

crates/futures/src/polyfill.rs

crates/futures/src/atomics.rs

crates/futures/src/lib.rs

crates/futures/src/atomics.rs

ibaryshnikov · 2019-06-17T18:21:16Z

@alexcrichton thank you for a great review! I fixed most of the places, but still have some questions, like usage of AtomicI32, or simplifying Waker struct. Btw, it looks working, and after increasing default timeout from 0 to 10ms the performance looks fine. So it's probably a time to start making changes to raytrace_parallel example itself?

crates/futures/src/lib.rs

crates/futures/src/polyfill.rs

crates/futures/src/atomics.rs

crates/futures/src/polyfill.rs

alexcrichton

This is some comments on the polyfill at least, I'll try to get to the futures implementation later this week!

ibaryshnikov · 2019-06-28T08:49:31Z

@alexcrichton I fixed most of the places, but still have some where I'm not sure, like what to do with closures, and the contents of the Waker struct, which is currently

struct Waker {
    value: AtomicI32,
    notified: AtomicBool,
}

alexcrichton

Sorry for the delays, travel got the best of me! But yes the notified field can be removed and just value needs to be there, and we can use flags in value to indicate whether polling needs to happen and such.

crates/futures/src/atomics.rs

…tes after review

ibaryshnikov · 2019-07-16T22:55:23Z

Updated to the latest master (I hope I didn't miss anything). Removed notified field, there's only value now inside Waker.
Can we summarize, what's left to do in this pr? I'm kind of lost in the comments.

Not implemented yet, and the one there doesn't work with atomics! (we'll get around to this soon-ish)

Use the atomics support now implemented!

* Remove now-unneeded `State` enum * Remove timeout argument from polyfill since we don't need it * Call `Atomics.waitAsync` if it's available instead of using our polyfill * Remove some extraneous dead code from the polyfill * Add a `val: i32` argument to the polyfill * Simplify the flow of futures with `Package` since `waitAsync` handles all the heavy lifting for us. * Remove `Arc<Package>` and just use `Package` * Remove `RefCell` from inside of `Package` now that it is no longer needed.

Turns out it's the exact same for both before and after atomics, so let's use the same definition!

* Use "legacy" instead of "stable" since `futures 0.1` is quicly becoming "legacy" * Rename "atomics" to "legacy_atomics" to leave room for the libstd-based futures atomics version. * Rename "polyfill" to "wait_async_polyfill" to specify what it's polyfilling.

alexcrichton · 2019-07-18T21:04:11Z

Ok thanks for the update @ibaryshnikov!

I've added a few more commits which cleaned up a few things (good chunks of the old future_to_promise implementation aren't necessary when using waitAsync) and reorganized some files a bit. I also hooked up the parallel raytrace example and can confirm it's all plumbed together and at least rudimentarily working.

If CI is green and you're good with this I think it's good to merge!

ibaryshnikov · 2019-07-19T00:44:41Z

@alexcrichton I'm good with this. Thanks for moving this forvard! And thanks for your comments, I've learned a lot from this pr!)

alexcrichton · 2019-07-19T14:38:01Z

Ok let's merge then, thanks so much again @ibaryshnikov!

maxammann · 2022-08-30T12:31:00Z

I think I found a usecase which is incomatible with this PR: #3048

ibaryshnikov commented May 4, 2019

View reviewed changes

crates/js-sys/src/lib.rs Outdated Show resolved Hide resolved

ibaryshnikov commented May 4, 2019

View reviewed changes

crates/futures/src/lib.rs Outdated Show resolved Hide resolved

alexcrichton reviewed May 6, 2019

View reviewed changes

crates/futures/src/polyfill.js Outdated Show resolved Hide resolved

alexcrichton reviewed May 22, 2019

View reviewed changes

crates/futures/src/lib.rs Outdated Show resolved Hide resolved

alexcrichton reviewed Jun 11, 2019

View reviewed changes

crates/futures/src/lib.rs Outdated Show resolved Hide resolved

alexcrichton reviewed Jun 11, 2019

View reviewed changes

crates/futures/src/polyfill.rs Outdated Show resolved Hide resolved

ibaryshnikov commented Jun 17, 2019

View reviewed changes

crates/futures/src/atomics.rs Outdated Show resolved Hide resolved

ibaryshnikov commented Jun 17, 2019

View reviewed changes

crates/futures/src/atomics.rs Outdated Show resolved Hide resolved

ibaryshnikov commented Jun 17, 2019

View reviewed changes

crates/futures/src/lib.rs Outdated Show resolved Hide resolved

ibaryshnikov commented Jun 17, 2019

View reviewed changes

crates/futures/src/atomics.rs Outdated Show resolved Hide resolved

alexcrichton reviewed Jun 18, 2019

View reviewed changes

alexcrichton reviewed Jul 10, 2019

View reviewed changes

crates/futures/src/atomics.rs Outdated Show resolved Hide resolved

ibaryshnikov and others added 9 commits July 17, 2019 00:48

added polyfill for Atomics.wait and used it to notify futures

d1d3021

typo fixed in Atomics docs

c01575c

added polyfill implementation in rust

2fdfe79

moved threadsafe futures behind a flag

e466e1a

moved threadsafe futures implementation to a separate file, made upda…

16c6bdc

…tes after review

placed web-sys dependency behind a feature flag in wasm-bindgen-futures

06c783d

updated default timeout and wait_async signature in wasm-bindgen-futures

221dc73

moved lib.rs to stable.rs in wasm-bindgen-futures, updated during review

6ab1a49

added documentation comment for a stable version of wasm-bindgen-futures

cbaa1d3

ibaryshnikov added 2 commits July 17, 2019 01:24

updated to the latest master

45d2c7c

removed AtomicBool from Waker struct

02be369

alexcrichton added 5 commits July 18, 2019 10:13

Emit a compiler error with futures 0.3 and atomics

d122bbc

Not implemented yet, and the one there doesn't work with atomics! (we'll get around to this soon-ish)

Update parallel raytrace example to use futures

9f77f8d

Use the atomics support now implemented!

Deduplicate JsFuture definitions

d590a9e

Turns out it's the exact same for both before and after atomics, so let's use the same definition!

Rename some files

c8451d6

* Use "legacy" instead of "stable" since `futures 0.1` is quicly becoming "legacy" * Rename "atomics" to "legacy_atomics" to leave room for the libstd-based futures atomics version. * Rename "polyfill" to "wait_async_polyfill" to specify what it's polyfilling.

alexcrichton changed the title ~~[WIP] Threadsafe futures~~ Threadsafe futures Jul 18, 2019

alexcrichton added 2 commits July 18, 2019 14:04

Remove a debugging statement

be294c8

Shared more betwee legacy with/without atomics

b13f757

alexcrichton merged commit efacd8b into rustwasm:master Jul 19, 2019

maxammann mentioned this pull request Aug 30, 2022

Atomics.waitAsync() polyfill does not work in Safari's WebWorkers #3048

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Threadsafe futures #1514

Threadsafe futures #1514

ibaryshnikov commented May 4, 2019

alexcrichton left a comment

ibaryshnikov commented May 6, 2019

alexcrichton commented May 8, 2019

ibaryshnikov commented May 19, 2019

alexcrichton left a comment

ibaryshnikov commented May 26, 2019

alexcrichton commented May 28, 2019

alexcrichton left a comment

ibaryshnikov commented Jun 17, 2019

alexcrichton left a comment

ibaryshnikov commented Jun 28, 2019

alexcrichton left a comment

ibaryshnikov commented Jul 16, 2019

alexcrichton commented Jul 18, 2019

ibaryshnikov commented Jul 19, 2019

alexcrichton commented Jul 19, 2019

maxammann commented Aug 30, 2022

Threadsafe futures #1514

Threadsafe futures #1514

Conversation

ibaryshnikov commented May 4, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

ibaryshnikov commented May 6, 2019

alexcrichton commented May 8, 2019

ibaryshnikov commented May 19, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

ibaryshnikov commented May 26, 2019

alexcrichton commented May 28, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

ibaryshnikov commented Jun 17, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

ibaryshnikov commented Jun 28, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

ibaryshnikov commented Jul 16, 2019

alexcrichton commented Jul 18, 2019

ibaryshnikov commented Jul 19, 2019

alexcrichton commented Jul 19, 2019

maxammann commented Aug 30, 2022