Stream: git-wasmtime

Topic: wasmtime / PR #4192 Change wasm-to-host trampolines to ta...

Wasmtime GitHub notifications bot (May 27 2022 at 20:10):

alexcrichton opened PR #4192 from change-wasm-to-host-abi to main:

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.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (May 27 2022 at 20:31):

alexcrichton updated PR #4192 from change-wasm-to-host-abi to main.

Wasmtime GitHub notifications bot (May 27 2022 at 20:31):

alexcrichton requested fitzgen for a review on PR #4192.

Wasmtime GitHub notifications bot (May 31 2022 at 21:37):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 01 2022 at 14:05):

alexcrichton merged PR #4192.

Last updated: Nov 22 2024 at 16:03 UTC