Introduce a new API that allows notifying that a Store has moved to a new thread #2822
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
26 tasks
github-actions
bot
added
the
wasmtime:api
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasmtime:api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
I wonder if we should be invoking |
|
It's a good point, and in fact it's already done automatically by wasmtime after resuming from a suspended fiber: https://github.com/bytecodealliance/wasmtime/blob/main/crates/runtime/src/traphandlers.rs#L436 Your remark makes me notice that this new API would also create a double standard: the runtime may automatically manage switching to a different thread in one case (async import causing wasm execution resuming on a different thread) when in another case one has to explicitly call the new API. The ubiquity of multithreaded future executors makes it justified in my opinion, but that's open to discussion! |
|
Thanks for this! I agree that it's a little odd that this might automatically happen as part of a future execution but then it doesn't automatically happen otherwise, but I agree that it's probably ok for now. Can you add documentation about this function on the multithreading page and also link the documentation on this function to the multithreading page? |
bnjbvr
force-pushed
the
store_notify_thread_switch
branch
from
f52af76 to
7ec65c7
Compare
|
Done, thanks! |
github-actions
bot
added
the
wasmtime:docs
alexcrichton
approved these changes
Apr 16, 2021
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
… new thread (bytecodealliance#2822) * Introduce a new API that allows notifying that a Store has moved to a new thread * Add backlink to documentation, and mention the new API in the multithreading doc;
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While potentially unsafe, https://docs.wasmtime.dev/examples-rust-multithreading.html#multithreading-without-send suggests that we can move over a Store and all that's attached to it to other threads. To make this a reality, a new API is required to notify the store that it's been moved over, so that it can initialize trap handling on this particular thread. I've made it an unsafe function, this should catch the eye of the users that they wouldn't need to use it in general. Also added a test that crashed before on a Mac.