Skip to content
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

Document guidance around multithreading and Wasmtime #2812

Merged
merged 2 commits into from
Apr 7, 2021

Conversation

alexcrichton
Copy link
Member

This commit writes a page of documentation for the Wasmtime book to
serve as guidance for embedders looking to add multithreading with
Wasmtime support. As always with any safe Rust API this reading is
optional because you can't mis-use Wasmtime without unsafe, but I'm
hoping that this documentation can serve as a point of reference for
folks who want to add multithreading but are confused/annoyed that
Wasmtime's types do not implement the Send and Sync traits.

Closes #793

This commit writes a page of documentation for the Wasmtime book to
serve as guidance for embedders looking to add multithreading with
Wasmtime support. As always with any safe Rust API this reading is
optional because you can't mis-use Wasmtime without `unsafe`, but I'm
hoping that this documentation can serve as a point of reference for
folks who want to add multithreading but are confused/annoyed that
Wasmtime's types do not implement the `Send` and `Sync` traits.

Closes bytecodealliance#793
@github-actions github-actions bot added the wasmtime:docs Issues related to Wasmtime's documentation label Apr 7, 2021
Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just minor nits!

@alexcrichton alexcrichton merged commit e43d940 into bytecodealliance:main Apr 7, 2021
@alexcrichton alexcrichton deleted the doc-multithreading branch April 7, 2021 21:34
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Apr 28, 2021
Platforms Wasmtime supports may have per-thread initialization that
needs to run before WebAssembly. For example Unix needs to setup a
sigaltstack and macOS needs to set up mach ports. In bytecodealliance#2757 this
per-thread setup was moved out of the invocation of a wasm function,
relying on the lack of Send for Store to initialize the thread at Store
creation time and never worry about it later.

This conflicted with [wasmtime's desired multithreading
story](bytecodealliance#2812) so a new
[`Store::notify_switched_thread` was
added](bytecodealliance#2822) to
explicitly indicate a Store has moved to another thread (if it unsafely
did so).

It turns out though that it's not always easy to determine when a
`Store` moves to a new thread. For example the Go bindings for Wasmtime
are generally unaware when a goroutine switches OS threads. This led to
bytecodealliance/wasmtime-go#74 where a SIGILL
was left uncaught, making it appear that traps aren't working properly.

This commit revisits the decision in bytecodealliance#2757 and moves per-thread
initialization back into the path of calling into WebAssembly. This is
differently from before, though, where there's still only one TLS access
on the path of calling into WebAssembly, unlike before where it was a
separate access. This allows us to get the speed benefits of bytecodealliance#2757 as
well as the flexibility benefits of not having to explicitly move a
store between threads.

With this new ability this commit deletes the recently added
`Store::notify_switched_thread` method since it's no longer necessary.
alexcrichton added a commit that referenced this pull request Apr 28, 2021
* Bring back per-thread lazy initialization

Platforms Wasmtime supports may have per-thread initialization that
needs to run before WebAssembly. For example Unix needs to setup a
sigaltstack and macOS needs to set up mach ports. In #2757 this
per-thread setup was moved out of the invocation of a wasm function,
relying on the lack of Send for Store to initialize the thread at Store
creation time and never worry about it later.

This conflicted with [wasmtime's desired multithreading
story](#2812) so a new
[`Store::notify_switched_thread` was
added](#2822) to
explicitly indicate a Store has moved to another thread (if it unsafely
did so).

It turns out though that it's not always easy to determine when a
`Store` moves to a new thread. For example the Go bindings for Wasmtime
are generally unaware when a goroutine switches OS threads. This led to
bytecodealliance/wasmtime-go#74 where a SIGILL
was left uncaught, making it appear that traps aren't working properly.

This commit revisits the decision in #2757 and moves per-thread
initialization back into the path of calling into WebAssembly. This is
differently from before, though, where there's still only one TLS access
on the path of calling into WebAssembly, unlike before where it was a
separate access. This allows us to get the speed benefits of #2757 as
well as the flexibility benefits of not having to explicitly move a
store between threads.

With this new ability this commit deletes the recently added
`Store::notify_switched_thread` method since it's no longer necessary.

* Fix a test compiling
mchesser pushed a commit to mchesser/wasmtime that referenced this pull request May 24, 2021
* Bring back per-thread lazy initialization

Platforms Wasmtime supports may have per-thread initialization that
needs to run before WebAssembly. For example Unix needs to setup a
sigaltstack and macOS needs to set up mach ports. In bytecodealliance#2757 this
per-thread setup was moved out of the invocation of a wasm function,
relying on the lack of Send for Store to initialize the thread at Store
creation time and never worry about it later.

This conflicted with [wasmtime's desired multithreading
story](bytecodealliance#2812) so a new
[`Store::notify_switched_thread` was
added](bytecodealliance#2822) to
explicitly indicate a Store has moved to another thread (if it unsafely
did so).

It turns out though that it's not always easy to determine when a
`Store` moves to a new thread. For example the Go bindings for Wasmtime
are generally unaware when a goroutine switches OS threads. This led to
bytecodealliance/wasmtime-go#74 where a SIGILL
was left uncaught, making it appear that traps aren't working properly.

This commit revisits the decision in bytecodealliance#2757 and moves per-thread
initialization back into the path of calling into WebAssembly. This is
differently from before, though, where there's still only one TLS access
on the path of calling into WebAssembly, unlike before where it was a
separate access. This allows us to get the speed benefits of bytecodealliance#2757 as
well as the flexibility benefits of not having to explicitly move a
store between threads.

With this new ability this commit deletes the recently added
`Store::notify_switched_thread` method since it's no longer necessary.

* Fix a test compiling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:docs Issues related to Wasmtime's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reconciling wasmtime::Instance and Send
3 participants