bytecodealliance / bytecodealliance/wasmtime
Consider integrating `ecmascript_atomics` crate
- Dominant language
- Rust
- Stars
- 18.6k
- Forks
- 1.8k
- Avg merge
- 1d 19h
- Merged PRs (30d)
- 121
Description
Awhile back I [started a discussion](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Writing.20a.20WebAssembly.20interpreter.20in.20Rust.20for.20wasm.20threads/with/541643681) on the Rust Zulip about implementing the wasm threads proposal in Pulley. The conclusion at the time was that (a) this isn't possible in safe Rust, (b) it should be possible with inline assembly, and (c) we weren't going to pursue it for Pulley due to the portability goal of Pulley in the meantime. The consequence of this in Wasmtime is [this line of code](https://github.com/bytecodealliance/wasmtime/blob/df21758b250366717d56ae9b972860de4e3d71bd/crates/wasmtime/src/config.rs#L2095) and a notable lack of anything atomic in Pulley's interpreter.
In the meantime the discussion on the Zulip issue continued on and resulted in publishing a crate named [`ecmascript_atomics`](https://crates.io/crates/ecmascript_atomics) which is targeted at this exact use case. This crate looks to be drawn from SpiderMonkey's implementation of the same. There are two reasons I think we'll want to integrate this crate:
* The primary one is the resolution of https://github.com/bytecodealliance/wasmtime/issues/4203. For example [this memcpy](https://github.com/bytecodealliance/wasmtime/blob/df21758b250366717d56ae9b972860de4e3d71bd/crates/wasmtime/src/runtime/vm/instance.rs#L1031), entirely unrelated to Pulley, is almost surely "technically not sound" given Rust's memory model. Functions like [`RacySlice::copy_from_slice`](https://docs.rs/ecmascript_atomics/latest/ecmascript_atomics/struct.RacySlice.html#method.copy_from_slice), however, look tailor made for this use case.
* Integrating this crate into Pulley would enable implementing the wasm threads proposal. Integrating this would require adding atomic instructions to Pulley and additionally updating **all** reads and writes to use this crate. Ordinary reads/writes would use `Unordered` and atomics would use `SeqCst`.
The main barrier to adopting this crate is portability at this time. As of the time of this writing it has support for x86, x86_64, aarch64, and arm. This notably does not include platforms Cranelift supports such as s390x and riscv64 and does not include "theoretically all platforms Rust compiles to" for Pulley. Nevertheless I think this would be worthwhile to integrate into Wasmtime as so:
* [ ] Add a wrapper around this crate to `wasmtime-internal-math` in-tree (probably renaming this to maybe `wasmtime-internal-core` in the process). This would use `ecmascript_atomics` on supported platforms and some emulated fallback on unsupported platforms (e.g. the unsound `std::ptr::copy` for a memcpy-based move).
* [ ] Update Pulley's support of the wasm threads proposal to be architecture-specific. For example it would support the proposal on platforms that `ecmascript_atomics` supports, but it would continue to not have support on other platforms (e.g. Pulley powerpc would not have support for the atomics proposal)
* [ ] Update Pulley's reads/writes of today to use `Unordered` atomic reads/writes
* [ ] Add atomic instructions, probably in the "extended" opcode namespace for Pulley, and equivalent lowerings to Cranelift to support generating atomics.
With all that in place we could still benefit from this crate, despite the lack of portability. While it probably isn't too hard to add s390x/riscv64 the lack of portability in this approach is pretty fundamental. I don't think that should block us from integrating it at all, however, but conditionally disabling threads in Pulley would be a reasonable consequence that reflects an accurate depiction of support.
Contributor guide
Assessment
This issue has not been assessed yet.