Change wasm-to-host trampolines to take the values_vec size - #4192
Merged
alexcrichton merged 2 commits intoJun 1, 2022
Conversation
This commit changes the ABI of wasm-to-host trampolines, which are only used right now for functions created with `Func::new`, to pass along the size of the `values_vec` argument. Previously the trampoline simply received `*mut ValRaw` and assumed that it was the appropriate size. By receiving a size as well we can thread through `&mut [ValRaw]` internally instead of `*mut ValRaw`. The original motivation for this is that I'm planning to leverage these trampolines for the component model for host-defined functions. Out of an abundance of caution of making sure that everything lines up I wanted to be able to write down asserts about the size received at runtime compared to the size expected. This overall led me to the desire to thread this size parameter through on the assumption that it would not impact performance all that much. I ran two benchmarks locally from the `call.rs` benchmark and got: * `sync/no-hook/wasm-to-host - nop - unchecked` - no change * `sync/no-hook/wasm-to-host - nop-params-and-results - unchecked` - 5% slower This is what I roughly expected in that if nothing actually reads the new parameter (e.g. no arguments) then threading through the parameter is effectively otherwise free. Otherwise though accesses to the `ValRaw` storage is now bounds-checked internally in Wasmtime instead of assuming it's valid, leading to the 5% slowdown (~9.6ns to ~10.3ns). If this becomes a peformance bottleneck for a particular use case then we should be fine to remove the bounds checking here or otherwise only bounds check in debug mode, otherwise I plan on leaving this as-is. Of particular note this also changes the C API for `*_unchecked` functions where the C callback now receives the size of the array as well.
Subscribe to Label Actioncc @peterhuene DetailsThis issue or pull request has been labeled: "wasmtime:api", "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
fitzgen
approved these changes
May 31, 2022
This file contains hidden or 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
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 changes the ABI of wasm-to-host trampolines, which are
only used right now for functions created with
Func::new, to passalong the size of the
values_vecargument. Previously the trampolinesimply received
*mut ValRawand assumed that it was the appropriatesize. By receiving a size as well we can thread through
&mut [ValRaw]internally instead of
*mut ValRaw.The original motivation for this is that I'm planning to leverage these
trampolines for the component model for host-defined functions. Out of
an abundance of caution of making sure that everything lines up I wanted
to be able to write down asserts about the size received at runtime
compared to the size expected. This overall led me to the desire to
thread this size parameter through on the assumption that it would not
impact performance all that much.
I ran two benchmarks locally from the
call.rsbenchmark and got:sync/no-hook/wasm-to-host - nop - unchecked- no changesync/no-hook/wasm-to-host - nop-params-and-results - unchecked- 5%slower
This is what I roughly expected in that if nothing actually reads the
new parameter (e.g. no arguments) then threading through the parameter
is effectively otherwise free. Otherwise though accesses to the
ValRawstorage is now bounds-checked internally in Wasmtime instead of assuming
it's valid, leading to the 5% slowdown (~9.6ns to ~10.3ns). If this
becomes a peformance bottleneck for a particular use case then we should
be fine to remove the bounds checking here or otherwise only bounds
check in debug mode, otherwise I plan on leaving this as-is.
Of particular note this also changes the C API for
*_uncheckedfunctions where the C callback now receives the size of the array as
well.