wasi-threads: an initial implementation #5484
Conversation
Subscribe to Label Actioncc @kubkon
This issue or pull request has been labeled: "wasi"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
abrown
force-pushed
the
wasi-threads-revisited
branch
2 times, most recently
from
61be0fa
to
2ba9f11
Compare
abrown
force-pushed
the
wasi-threads-revisited
branch
from
2ba9f11
to
f75d006
Compare
|
Oh one thing as well, it looks like this test may not be running on CI since it requires explicit opt-in to run. That being said including the threads feature in the build by default I think is reasonable since it doesn't pull in any extra build requirements and still requires a flag to enable at runtime. |
That sounds good, but out of an abundance of caution, can we merge as-is, then make the feature default? I want to get a few days of no-issue CI builds and general usage. Also, I want to take another pass on the testing side, improving the one existing test and possibly including some other tests that others have been working on. |
|
Sure yeah, would you prefer to not test at all on CI while manual testing is done? |
|
I'm fine with manual testing for now (like, a week?) of the wasi-threads parts--they are pretty straight-forward. The parts I'm concerned about getting some usage of are the changes to wasi-common. |
There was a problem hiding this comment.
We discussed this PR during last week's Wasmtime meeting, and I had a call with @abrown about it on Friday. Based on these conversations, I support merging (almost, see below) as-is.
The feature is disabled by default, and needs the --experimental-wasi-threads flag to be passed to be enabled. Additionally, it implements an active proposal in the WASI standardization process. This combination clears the bar for Wasmtime's criteria for shipping content-visible features.
As for the changes themselves, the one request I have is to change the documentation to include wasi-threads in the list of supported proposals.
I do want to emphasize that merging this PR doesn't represent commitment by the Wasmtime project to ship wasi-threads to production, enabled by default. As documented, that will require the wasi-threads proposal itself reaching stage 4 in the standardization process.
I'm highlighting this because there are open questions around how to specify and implement wasi-threads in terms of the Component Model. More specifically, the Component Model requires all APIs to be fully virtualizable, and to uphold full encapsulation between separate components. Based on conversations with @abrown, @alexcrichton, @sunfishcode, and @lukewagner, I understand that there are candidate solutions for these problems, but none that are known to work as of yet.
Vetting one of these candidate solutions to a point where there is sufficient shared optimism about its viability would be an important next step.
This patch adds interior mutability to the WasiCtx Table and the Table elements. Major pain points: * `File` only needs `RwLock<cap_std::fs::File>` to implement `File::set_fdflags()` on Windows, because of [1] * Because `File` needs a `RwLock` and `RwLock*Guard` cannot be hold across an `.await`, The `async` from `async fn num_ready_bytes(&self)` had to be removed * Because `File` needs a `RwLock` and `RwLock*Guard` cannot be dereferenced in `pollable`, the signature of `fn pollable(&self) -> Option<rustix::fd::BorrowedFd>` changed to `fn pollable(&self) -> Option<Arc<dyn AsFd + '_>>` [1] https://github.com/bytecodealliance/system-interface/blob/da238e324e752033f315f09c082ad9ce35d42696/src/fs/fd_flags.rs#L210-L217 Signed-off-by: Harald Hoyer <[email protected]>
This change is a first step toward implementing `wasi-threads` in
Wasmtime. We may find that it has some missing pieces, but the core
functionality is there: when `wasi::thread_spawn` is called by a running
WebAssembly module, a function named `wasi_thread_start` is found in the
module's exports and called in a new instance. The shared memory of the
original instance is reused in the new instance.
This new WASI proposal is in its early stages and details are still
being hashed out in the [spec] and [wasi-libc] repositories. Due to its
experimental state, the `wasi-threads` functionality is hidden behind
both a compile-time and runtime flag: one must build with `--features
wasi-threads` but also run the Wasmtime CLI with `--wasm-features
threads` and `--wasi-modules experimental-wasi-threads`. One can
experiment with `wasi-threads` by running:
```console
$ cargo run --features wasi-threads -- \
--wasm-features threads --wasi-modules experimental-wasi-threads \
<a threads-enabled module>
```
Threads-enabled Wasm modules are not yet easy to build. Hopefully this
is resolved soon, but in the meantime see the use of
`THREAD_MODEL=posix` in the [wasi-libc] repository for some clues on
what is necessary. Wiggle complicates things by requiring the Wasm
memory to be exported with a certain name and `wasi-threads` also
expects that memory to be imported; this build-time obstacle can be
overcome with the `--import-memory --export-memory` flags only available
in the latest Clang tree. Due to all of this, the included tests are
written directly in WAT--run these with:
```console
$ cargo test --features wasi-threads -p wasmtime-cli -- cli_tests
```
[spec]: https://github.com/WebAssembly/wasi-threads
[wasi-libc]: https://github.com/WebAssembly/wasi-libc
This change does not protect the WASI implementations themselves from
concurrent access. This is already complete in previous commits or left
for future commits in certain cases (e.g., wasi-nn).
As is being discussed [elsewhere], either calling `proc_exit` or trapping in any thread should halt execution of all threads. The Wasmtime CLI already has logic for adapting a WebAssembly error code to a code expected in each OS. This change factors out this logic to a new function, `maybe_exit_on_error`, for use within the `wasi-threads` implementation. This will work reasonably well for CLI users of Wasmtime + `wasi-threads`, but embedders will want something better in the future: when a `wasi-threads` threads fails, they may not want their application to exit. Handling this is tricky, because it will require cancelling the threads spawned by the `wasi-threads` implementation, something that is not trivial to do in Rust. With this change, we defer that work until later in order to provide a working implementation of `wasi-threads` for experimentation. [elsewhere]: WebAssembly/wasi-threads#17
In order to make progress with wasi-threads, this change temporarily works around limitations induced by `wasi-common`'s `fd_fdstat_set_flags` to allow `&mut self` use in the implementation. Eventual resolution is tracked in bytecodealliance#5643. This change makes several related helper functions (e.g., `set_fdflags`) take `&mut self` as well. Co-authored-by: Alex Crichton <[email protected]>
Previously, the test simply executed in a loop for some hardcoded number of iterations. This changes uses `wait` and `notify` and atomic operations to keep track of when the spawned threads are done and join on the main thread appropriately.
abrown
force-pushed
the
wasi-threads-revisited
branch
from
e77e4e7
to
f3ece48
Compare
|
Rebased to handle conflicts due to version bumps and #5683. |
|
Note that the |
As @TerrorJack pointed out in bytecodealliance#5484, that PR implements an older name--`thread_spawn`. This change uses the now-official name from the specification--`thread-spawn`.
@TerrorJack, thanks for the heads up. Yes, there are a few details that still need to be cleaned up, like this one. I opened #5748 to resolve this. |
* wasi-threads: fix import name As @TerrorJack pointed out in #5484, that PR implements an older name--`thread_spawn`. This change uses the now-official name from the specification--`thread-spawn`. * fix: update name in test
| ;; A helper function for printing ptr-len strings. | ||
| (func $print (param $ptr i32) (param $len i32) | ||
| (i32.store (i32.const 8) (local.get $len)) | ||
| (i32.store (i32.const 4) (local.get $ptr)) |
There was a problem hiding this comment.
Are you thinking about atomic stores or a lock around the $__wasi_fd_write call? (Recall that wasi-common has some internal locking).
There was a problem hiding this comment.
no. it's about the linear memory used for iovec.
As @yamt points out [here], the `wait`/`notify` pairing used in this manual WAT test was not effective. The `wait` always immediately returned, meaning that the main thread essentially spins until a counter is atomically incremented. This is fine for test correctness, but was not the original intent, which was lost in a refactoring. This change uses the `$i` local to keep track of the counter value we expect to see for the `wait`, so that the `wait`/`notify` pair actually waits as expected. [here]: bytecodealliance#5484 (comment)
Now that Wasmtime has merged bytecodealliance/wasmtime#5484 the language used in the README can be updated.
Now that Wasmtime has merged bytecodealliance/wasmtime#5484 the language used in the README can be updated.
As @yamt points out [here], the `wait`/`notify` pairing used in this manual WAT test was not effective. The `wait` always immediately returned, meaning that the main thread essentially spins until a counter is atomically incremented. This is fine for test correctness, but was not the original intent, which was lost in a refactoring. This change uses the `$i` local to keep track of the counter value we expect to see for the `wait`, so that the `wait`/`notify` pair actually waits as expected. [here]: #5484 (comment)
This PR builds on several other under-review PRs to demonstrate a working wasi-threads implementation in Wasmtime. The first three commits are cherry-picked from the following PRs:
The last two commits build on these to expose the wasi-threads API to WebAssembly modules. Upon building and running Wasmtime with the right flags, one can then run multi-threaded WebAssembly programs:
Due to Cargo feature issues in 05020d4, this PR is not yet expected to pass all CI tasks but, perhaps more interestingly, one can use these changes to run some simple pthread tests over in the wasi-libc repository: WebAssembly/wasi-libc#369.