Stream: git-wasmtime

Topic: wasmtime / PR #10897 Refactor the representation of `Func`

Wasmtime GitHub notifications bot (Jun 02 2025 at 18:34):

alexcrichton requested fitzgen for a review on PR #10897.

Wasmtime GitHub notifications bot (Jun 02 2025 at 18:34):

alexcrichton opened PR #10897 from alexcrichton:refactor-func-representation to bytecodealliance:main:

This commit rewrites the internals of wasmtime::Func. The Stored type is no longer used meaning that it's now free to create a wasmtime::Func at any time. It is effectively a store-tagged NonNull<VMFuncRef>. This required a few internal changes to how functions are passed around:

• Previously the insertion of a wasm_call-less VMFuncRef was deferred until the raw pointer was loaded. Now the insertion happens immediately as soon as the function is placed within a store. This is done to ensure that a Func corresponds to one exact VMFuncRef and that's it, and lazily filled in versions within the store are still lazily filled in but they're more eagerly allocated. This isn't expected to have much of an impact perf-wise since all these lazily allocated functions were already almost guaranteed to get lazily allocated anyway.

• Filling in "holes" in VMFuncRef, notably the wasm_call field, no longer happens lazily when the VMFuncRef is demanded. Instead during instantiation a pass is made to fill in holes with the new module being instantiated (after registration). Additionally when a lazily-allocated VMFuncRef is created the module registry is checked immediately. This means that all store-local VMFuncRef values are either filled in immediately or filled in during instantiation. This notably means that the previous logic in Func::vmimport is now "just" an .unwrap() with a lot of comments saying why the unwrap shouldn't panic.

• To implement this commit a previous optimization for the Func API was removed as well, namely Func::call will become slower after this commit. The Func::call API is a dynamically-typed API which requires run-time type-checking of arguments. Previously a FuncType was loaded into a cache once-per-Func which helped amortize the cost of using Func::call repeatedly. Now, though, there's no natural place to put such a cache since Func no longer has dedicated storage within a Store. Historically this optimization was added for the C API before the *_call_unchecked APIs existed, but nowadays the *_call_unchecked APIs should suffice for performance-critical applications where needed. In the future it might also be possible to have a hash map in the Store of a VMSharedTypeIndex to FuncType which is lazily populated based on calls to Func::call, but that feels a bit overkill nowadays for a possibly rarely-used map.

• The no-longer-necessary RootedHostFunc type is now gone as its unsafety and various contracts are subsumed by other preexisting unsafe blocks.

• The specific drop order between StoreOpaque::store_data and StoreOpaque::rooted_host_funcs is removed. The rooted_host_funcs field now lives in the func_refs field and the FuncKind type, where the destructors came from before, is no more.

• The func_refs field has grown storage locations for a variety of "keep this thing alive as long as the store" related to functions and such.

Closes #10868

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jun 02 2025 at 18:34):

alexcrichton requested wasmtime-core-reviewers for a review on PR #10897.

Wasmtime GitHub notifications bot (Jun 02 2025 at 18:34):

alexcrichton commented on PR #10897:

I'll also note that this is split out of https://github.com/bytecodealliance/wasmtime/pull/10870/commits/b228b9718ab0c7e405ff98f62207d63f0e8484a7

Wasmtime GitHub notifications bot (Jun 02 2025 at 18:36):

alexcrichton edited a comment on PR #10897:

I'll also note that this is split out of https://github.com/bytecodealliance/wasmtime/pull/10870

Wasmtime GitHub notifications bot (Jun 02 2025 at 18:39):

alexcrichton updated PR #10897.

Wasmtime GitHub notifications bot (Jun 02 2025 at 21:19):

fitzgen created PR review comment:

Kind of an aside but should all of these assertions actually live in the wasmtime-c-api crate so that we don't need to have a dummy struct which itself needs to be kept in sync with the struct that we actually care about? We've removed one syncing problem but added another, so we haven't really changed anything. I guess the only assertion that would remain in the wasmtime crate would be the one for the offset of the store field.

Wasmtime GitHub notifications bot (Jun 02 2025 at 21:19):

fitzgen created PR review comment:

/// Attempts to fill the `wasm_call` field of `func_ref` given `modules`
/// registered and returns `true` if the field was filled, `false` otherwise.

Wasmtime GitHub notifications bot (Jun 02 2025 at 21:19):

fitzgen created PR review comment:

Sooooooo nice to get rid of FuncKind

Wasmtime GitHub notifications bot (Jun 02 2025 at 21:19):

fitzgen submitted PR review:

Very nice!!

Wasmtime GitHub notifications bot (Jun 02 2025 at 21:19):

fitzgen created PR review comment:

Just to clarify, since there isn't any assert! here:

                // Note that this is a load-bearing `unwrap` here, but is

Wasmtime GitHub notifications bot (Jun 02 2025 at 23:03):

alexcrichton updated PR #10897.

Wasmtime GitHub notifications bot (Jun 02 2025 at 23:03):

alexcrichton updated PR #10897.

Wasmtime GitHub notifications bot (Jun 02 2025 at 23:04):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 02 2025 at 23:04):

alexcrichton created PR review comment:

Oh I think you might be assuming there's a struct wasmtime_func_t { ... } in Rust in crates/c-api but that doesn't exist. In that sense this struct C is the only C-version-written-in-Rust anywhere in the codebase, and I figured here was a good as place as any to test things out.

Wasmtime GitHub notifications bot (Jun 02 2025 at 23:04):

alexcrichton has enabled auto merge for PR #10897.

Wasmtime GitHub notifications bot (Jun 02 2025 at 23:23):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 02 2025 at 23:23):

fitzgen created PR review comment:

Ah, I was indeed

Wasmtime GitHub notifications bot (Jun 03 2025 at 00:18):

alexcrichton merged PR #10897.

Last updated: Dec 06 2025 at 07:03 UTC