-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Optimize calling a WebAssembly function #2757
Merged
Merged
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
fitzgen
approved these changes
Mar 23, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
This commit implements a few optimizations, mainly inlining, that should improve the performance of calling a WebAssembly function. This code path can be quite hot depending on the embedding case and we hadn't really put much effort into optimizing the nitty gritty. The predominant optimization here is adding `#[inline]` to trivial functions so performance is improved without having to compile with LTO. Another optimization is to call `lazy_per_thread_init` when traps are initialized per-thread (when a `Store` is created) rather than each time a function is called. The next optimization is to change the unwind reason in the `CallThreadState` to `MaybeUninit` to avoid extra checks in the default case about whether we need to drop its variants (since in the happy path we never need to drop it). The final optimization is to optimize out a few checks when `async` support is disabled for a small speed boost. In a small benchmark where wasmtime calls a simple wasm function my macOS computer dropped from 110ns to 86ns overhead, a 20% decrease. The macOS overhead is still largely dominated by the global lock acquisition and hash table management for traps right now, but I suspect the Linux overhead is much better (should be on the order of ~30 or so ns). We still have a long way to go to compete with SpiderMonkey which, in testing, seem to have ~6ns overhead in calling the same wasm function on my computer.
github-actions
bot
added
the
wasmtime:api
Related to the API of the `wasmtime` crate itself
label
Mar 23, 2021
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 |
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
Mar 23, 2021
This commit is an extension of bytecodealliance#2757 where the goal is to optimize entry into WebAssembly. Currently wasmtime has two stack-based cleanups when entering wasm, one for the externref activation table and another for stack limits getting reset. This commit fuses these two cleanups together into one and moves some code around which enables less captures for fewer closures and such to speed up calls in to wasm a bit more. Overall this drops the execution time from 88ns to 80ns locally for me. This also updates the atomic orderings when updating the stack limit from `SeqCst` to `Relaxed`. While `SeqCst` is a reasonable starting point the usage here should be safe to use `Relaxed` since we're not using the atomics to actually protect any memory, it's simply receiving signals from other threads.
alexcrichton
added a commit
that referenced
this pull request
Mar 24, 2021
* Combine stack-based cleanups for faster wasm calls This commit is an extension of #2757 where the goal is to optimize entry into WebAssembly. Currently wasmtime has two stack-based cleanups when entering wasm, one for the externref activation table and another for stack limits getting reset. This commit fuses these two cleanups together into one and moves some code around which enables less captures for fewer closures and such to speed up calls in to wasm a bit more. Overall this drops the execution time from 88ns to 80ns locally for me. This also updates the atomic orderings when updating the stack limit from `SeqCst` to `Relaxed`. While `SeqCst` is a reasonable starting point the usage here should be safe to use `Relaxed` since we're not using the atomics to actually protect any memory, it's simply receiving signals from other threads. * Determine whether a pc is wasm via a global map The macOS implementation of traps recently changed to using mach ports for handlers instead of signal handlers. This means that a previously relied upon invariant, each thread fixes its own trap, was broken. The macOS implementation worked around this by maintaining a global map from thread id to thread local information, however, to solve the problem. This global map is quite slow though. It involves taking a lock and updating a hash map on all calls into WebAssembly. In my local testing this accounts for >70% of the overhead of calling into WebAssembly on macOS. Naturally it'd be great to remove this! This commit fixes this issue and removes the global lock/map that is updated on all calls into WebAssembly. The fix is to maintain a global map of wasm modules and their trap addresses in the `wasmtime` crate. Doing so is relatively simple since we're already tracking this information at the `Store` level. Once we've got a global map then the macOS implementation can use this from a foreign thread and everything works out. Locally this brings the overhead, on macOS specifically, of calling into wasm from 80ns to ~20ns. * Fix compiles * Review comments
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
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.
This commit implements a few optimizations, mainly inlining, that should
improve the performance of calling a WebAssembly function. This code
path can be quite hot depending on the embedding case and we hadn't
really put much effort into optimizing the nitty gritty.
The predominant optimization here is adding
#[inline]
to trivialfunctions so performance is improved without having to compile with LTO.
Another optimization is to call
lazy_per_thread_init
when traps areinitialized per-thread (when a
Store
is created) rather than each timea function is called. The next optimization is to change the unwind
reason in the
CallThreadState
toMaybeUninit
to avoid extra checksin the default case about whether we need to drop its variants (since in
the happy path we never need to drop it). The final optimization is to
optimize out a few checks when
async
support is disabled for a smallspeed boost.
In a small benchmark where wasmtime calls a simple wasm function my
macOS computer dropped from 110ns to 86ns overhead, a 20% decrease. The
macOS overhead is still largely dominated by the global lock acquisition
and hash table management for traps right now, but I suspect the Linux
overhead is much better (should be on the order of ~30 or so ns).
We still have a long way to go to compete with SpiderMonkey which, in
testing, seem to have ~6ns overhead in calling the same wasm function on
my computer.