wasmtime / PR #7943 Wasmtime: Finish support for the type... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7943 Wasmtime: Finish support for the type...

Wasmtime GitHub notifications bot (Feb 15 2024 at 00:30):

fitzgen requested alexcrichton for a review on PR #7943.

Wasmtime GitHub notifications bot (Feb 15 2024 at 00:30):

fitzgen requested wasmtime-core-reviewers for a review on PR #7943.

Wasmtime GitHub notifications bot (Feb 15 2024 at 00:30):

fitzgen opened PR #7943 from fitzgen:canonicalize-typed-function-references to bytecodealliance:main:

While we supported the function references proposal inside Wasm, we didn't support it on the "outside" in the Wasmtime embedder APIs. So much of the work here is exposing typed function references, and their type system updates, in the embedder API. These changes include:

ValType::FuncRef and ValType::ExternRef are gone, replaced with the introduction of the RefType and HeapType types and a ValType::Ref(RefType) variant.

ValType and FuncType no longer implement Eq and PartialEq. Instead there are ValType::matches and FuncType::matches methods which check directional subtyping. I also added ValType::eq and FuncType::eq static methods for the rare case where someone needs to check precise equality, but that is almost never actually the case, 99.99% of the time you want to check subtyping.

There are also public Val::matches_ty predicates for checking if a value is an instance of a type, as well as internal helpers like Val::ensure_matches_ty that return a formatted error if the value does not match the given type. These helpers are used throughout Wasmtime internals now.

There is now a dedicated wasmtime::Ref type that represents reference values. Table operations have been updated to take and return Refs rather than Vals.

Furthermore, this commit also includes type registry changes to correctly manage lifetimes of types that reference other types. This wasn't previously an issue because the only thing that could reference types that reference other types was a Wasm module that added all the types that could reference each other at the same time and removed them all at the same time. But now that the previously discussed work to expose these things in the embedder API is done, type lifetime management in the registry becomes a little trickier because the embedder might grab a reference to a type that references another type, and then unload the Wasm module that originally defined that type, but then the user should still be able use that type and the other types it transtively references. Before, we were refcounting individual registry entries. Now, we still are refcounting individual entries, but now we are also accounting for type-to-type references and adding a new type to the registry will increment the refcounts of each of the types that it references, and removing a type from the registry will decrement the refcounts of each of the types it references, and then recursively (logically, not literally) remove any types whose refcount has now reached zero.

Additionally, this PR adds support for subtyping to Func::typed- and Func::wrap-style APIs. For result types, you can always use a supertype of the WebAssembly function's actual declared return type in Func::typed. And for param types, you can always use a subtype of the Wasm function's actual declared param type. Doing these things essentially erases information but is always correct. But additionally, for functions which take a reference to a concrete type as a parameter, you can also use the concrete type's supertype. Consider a WebAssembly function that takes a reference to a function with a concrete type: (ref null <func type index>). In this scenario, there is no static wasmtime::Foo Rust type that corresponds to that particular Wasm-defined concrete reference type because Wasm modules are loaded dynamically at runtime. You could do f.typed::<Option<NoFunc>, ()>(), and while that is correctly typed and valid, it is often overly restrictive. The only value you could call the resulting typed function with is the null function reference, but we'd like to call it with non-null function references that happen to be of the correct type. Therefore, f.typed<Option<Func>, ()>() is also allowed in this case, even though Option<Func> represents (ref null func) which is the supertype, not subtype, of (ref null <func type index>). This does imply some minimal dynamic type checks in this case, but it is supported for better ergonomics, to enable passing non-null references into the function.

We can investigate whether it is possible to use generic type parameters and combinators to define Rust types that precisely match concrete reference types in future, follow-up pull requests. But for now, we've made things usable, at least.

Finally, this also takes the first baby step towards adding support for the Wasm GC proposal. Right now the only thing that is supported is nofunc references, and this was mainly to make testing function reference subtyping easier. But that does mean that supporting nofunc references entailed also adding a wasmtime::NoFunc type as well as the Config::wasm_gc(enabled) knob. So we officially have an in-progress implementation of Wasm GC in Wasmtime after this PR lands!

Fixes https://github.com/bytecodealliance/wasmtime/issues/6455

Wasmtime GitHub notifications bot (Feb 15 2024 at 01:57):

github-actions[bot] commented on PR #7943:

Subscribe to Label Action

cc @fitzgen, @peterhuene

<details>
This issue or pull request has been labeled: "fuzzing", "wasmtime:api", "wasmtime:c-api", "wasmtime:config"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

peterhuene: wasmtime:api, wasmtime:c-api

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Feb 15 2024 at 01:57):

github-actions[bot] commented on PR #7943:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[ ] If you added a new Config method, you wrote extensive documentation for
it.

<details>

Our documentation should be of the following form:

```text
Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

Example

Optional example here.
```

</details>

[ ] If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

<details>

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

</details>

[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

Wasmtime GitHub notifications bot (Feb 15 2024 at 21:09):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 15 2024 at 21:36):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 15 2024 at 22:25):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton submitted PR review:

This all looks great to me, thanks again for putting all this together! I think it all shaped up really well in the end :+1:

I do still have wishy-washy perf concerns but I don't know how to bottom them out. Failing all that I think it would be good to at least ensure that wasmtime serve doesn't regress performance, but I do think this highlights a blind spot we have for perf where I have a hunch this is going to regress an important use case but I don't know how to discover said use case...

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton submitted PR review:

This all looks great to me, thanks again for putting all this together! I think it all shaped up really well in the end :+1:

I do still have wishy-washy perf concerns but I don't know how to bottom them out. Failing all that I think it would be good to at least ensure that wasmtime serve doesn't regress performance, but I do think this highlights a blind spot we have for perf where I have a hunch this is going to regress an important use case but I don't know how to discover said use case...

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

Given the relative cost of this to malloc in the guest I think it would be best to memoize this somehow to avoid reconstructing this on each time realloc is called

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

How come this rendering changed in this PR? I personally like to ideally have exhaustive matches here because it's nice to see diffs in PRs, but I do realize that they're a pain to update

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

Can you be sure we've got some tests to ensure that the direction of the test here is the same? For example you can store a nofunc val into a funcref global but you can't store a funcref into a nofunc global

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

I'm actually a bit confused by this, how come this fails?

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

These unreachable!s can be replaced with match self.inner {} I think to statically prove this isn't reachable

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

I'm a bit worried about this for a few reasons, but I don't realistically have a way to measure the impact of this change.

The loss of Copy is unfortunate for one but the part I'm more worried about is that this represents "more Arc stuff" on the hot path of calling functions.

Perhaps as at least a small gut-check, could you measure the rps locally of wasmtime serve for a component before/after this change? If it's like 20% slower that's probably something to investigate, but something like 5% may just be noise.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

Would it be worth adding a special case here to test if expected == actual and short-circuit everything else?

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

To confirm, did you add tests asserting success and failure in both these directions for results and params?

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

I was a bit surprised by this in that I thought the directionality of the check would need to go the other direction. I'm not actually sure which way is correct, but can you add a test like the global one above where we assert that a nofunc table can be copied into a funcref table but not the other way around?

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

Mind adding is_v128? It feels left out

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

(same for a number of the errors above this as well)

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

Also to be clear I understand there's no semantic alternative here, we _must_ have type information as part of the call and there's no avoiding that any more. I'm mostly trying to frontload any issues related to perf so we can refactor things early on.

I also think it's ok to refactor things in future PRs if a slowdown is found, mostly just want to be aware.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

(or is this a copy/paste typo from the global test above?)

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:07):

alexcrichton created PR review comment:

This might actually be good docs to include on Val itself?

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:17):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:17):

fitzgen created PR review comment:

I was going to try do that initially, but then I noticed that Options is Copy, and I didn't feel like going down the rabbit hole of seeing how much work it would be to undo that. Do you have a sense? Or a suggested place to cache it somewhere else?

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:19):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:19):

fitzgen created PR review comment:

Could always just throw it somewhere in the store I guess... Is that the best approach?

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:22):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:22):

fitzgen created PR review comment:

Global types are invariant with their content type if they are mutable, ie must match precisely and can't be a subtype or supertype. If the global isn't mutable, then you can instantiate a module that needs a g with any g' <: g. I did write tests for these cases.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:23):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:23):

fitzgen created PR review comment:

Ah but I only wrote tests for instantiating wasm modules with existing globals, not the global value initializer in the embedder API. Will do that.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:25):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:25):

fitzgen created PR review comment:

Similarly, I only wrote linker tests here, and the embedder API does indeed have bugs. Will add some more tests.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:27):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:27):

fitzgen created PR review comment:

Oh I forgot to talk about this in the commit message. I got annoyed that the traps.rs tests don't pass if you have RUST_BACKTRACE=1 set, so I loosened the assertions from being exact to contains(..) calls about the relevant parts.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:29):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:29):

fitzgen created PR review comment:

Ah this part is a copy-paste from the global test above, yeah.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:31):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:31):

fitzgen created PR review comment:

Yep! See typed_concrete_{param,result} in tests/all/func.rs.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:45):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:45):

fitzgen created PR review comment:

We could remove the dynamic downcasting checks from the typed function API and instead explore (in a follow up PR) implementing WasmTy for TypedFunc<P, R> such that you could do
// succeeds if typeof(f) <: (func (param i64) (result (ref $t)))
// where $t = (type (func (param funcref) (result (ref null $u))))
// and $u = (type (func (result i32)))
let f = instance.get_typed_func::<
    i64,
    TypedFunc<
        Option<Func>,
        Option<TypedFunc<(), i32>>,
    >,
>(&store)?;
Chris was suggesting this to me yesterday, and I think it should be doable. It would allow us to completely remove dynamic checks for calling concrete function references, the way Func::typed works today.

What do you think of that?

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:48):

alexcrichton created PR review comment:

At a high level what I'd expect is:

At Component compilation time the ModuleTypesBuilder gets the realloc signature added, no matter whether it was already there or not. This is saved in the component compilation artifacts.

At Component creation time this local interned index is turned into a engine-interned index and saved in a Component

Upon instantiation this engine-interned index makes its way into Instance which I think is referenced from functions, either that or the index is copied into functions/options/etc

The call_raw function is updated to take an index instead of an object, ... somehow ... (unsure about this) ... and then avoids reifying the type each time. Either that or I suppose the FuncType could be stored somewhere in Component and make its way into Instance/FuncData.

Or something like that.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:49):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:49):

fitzgen created PR review comment:

I'll move that into the matches method so every caller bebnefits from it.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:50):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:50):

alexcrichton created PR review comment:

Oh I mean more the val and ty here, e.g. the initial value to the global. Not globals-into-instances but values-into-globals, and basically testing the matrix of "I can pass a supertype" or "I can't pass a supertype" depending on the direction

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:51):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:51):

alexcrichton created PR review comment:

Mind splitting this part out to a separate PR?

Also, you're on nightly, right? I think that's a feature of anyhow where it auto-captures on nightly using unstable APIs but it's all turned off on stable/beta (e.g. I don't remember running into this before).

Either way agreed we should fix, but if you're ok I'd like to split out

Wasmtime GitHub notifications bot (Feb 16 2024 at 00:57):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 16 2024 at 01:15):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 01:15):

fitzgen created PR review comment:

https://github.com/bytecodealliance/wasmtime/pull/7950

Wasmtime GitHub notifications bot (Feb 16 2024 at 01:23):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 16 2024 at 01:23):

fitzgen created PR review comment:

(will rebase this PR after that merges)

Wasmtime GitHub notifications bot (Feb 16 2024 at 01:23):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 16:25):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 16 2024 at 16:52):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 16:52):

fitzgen created PR review comment:

Hm so not all realloc callers have a full wasmtime::component::Instance, a bunch just have a pointer to a wasmtime_runtime::ComponentInstance, and the latter can't (directly) hold a wasmtime::FuncType because of the artificial crate boundary between wasmtime and wasmtime_runtime. We could stuff it in a Box<Any> inside wasmtime_runtime::ComponentInstance but that is pretty unsatisfactory.

Alternatively, if we remove the dynamic type checks style of Func::typed that I added in this PR, then we don't need a FuncType for TypedFunc::call_raw anymore, which means we don't need realloc's FuncType here anymore, and TypedFunc and Options can even remain Copy. That does, however, mean we need an alternative way of doing typed function calls for functions that use concrete reference types, and the only alternative that I can think of at the moment (other than just not suppporting them, which is super unsatisfactory) is making TypedFunc implement WasmTy, as discussed below.

Wasmtime GitHub notifications bot (Feb 16 2024 at 16:57):

fitzgen edited PR review comment.

Wasmtime GitHub notifications bot (Feb 16 2024 at 17:06):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 17:06):

alexcrichton created PR review comment:

Oh now that's clever, I really like that!

Would that remove the need for this field though? I thought that this was used additionally as part of call_raw where the type is needed to know what the params/results are?

Wasmtime GitHub notifications bot (Feb 16 2024 at 17:49):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2024 at 17:49):

alexcrichton created PR review comment:

To close the loop here Nick and I talked more about this here

Wasmtime GitHub notifications bot (Feb 16 2024 at 18:25):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 16 2024 at 18:35):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 16 2024 at 18:39):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 16 2024 at 22:46):

alexcrichton commented on PR #7943:

I talked a bunch with @fitzgen this afternoon about this PR where we were investigating perf related to this as well. We had a reproducible 3% slowdown in rps in the 17.0.1 release vs this PR and ~5% (to be confirmed in a second bit Nick) in the call microbenchmarks. In the rps perf we couldn't find anything related to this PR that was significantly worrisome and the delta included more than just this PR as it was the 17.0.1 release vs this PR.

Given all that while there may still be some perf issues lurking here that we need to improve on we aren't really in a position to articulate what they are and actually take action on them. With that in mind I'm ok landing this and we can continue to investigate after-the-fact and refactor as necessary if anything crops up.

Wasmtime GitHub notifications bot (Feb 16 2024 at 23:06):

fitzgen commented on PR #7943:

We had a reproducible ... ~5% [slowdown] in the call microbenchmarks.

Actually, it seems like with a handful of tweaks related to this PR and a bunch that are not related to this PR, the call benchmark is actually even better than it used to be now:
sync/no-hook/core - host-to-wasm - typed - nop-params-and-results
                        time:   [25.909 ns 26.294 ns 26.952 ns]
                        change: [-19.268% -17.214% -14.973%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  3 (3.00%) high mild
  6 (6.00%) high severe
I'm going to split out the perf improvements that are unrelated to this PR into their own PR. Once that's done, I think this will be good to go!

Wasmtime GitHub notifications bot (Feb 16 2024 at 23:18):

fitzgen commented on PR #7943:

I'm going to split out the perf improvements that are unrelated to this PR into their own PR.

https://github.com/bytecodealliance/wasmtime/pull/7953

Wasmtime GitHub notifications bot (Feb 16 2024 at 23:44):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 16 2024 at 23:44):

fitzgen requested wasmtime-default-reviewers for a review on PR #7943.

Wasmtime GitHub notifications bot (Feb 20 2024 at 15:55):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 20 2024 at 18:00):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 20 2024 at 19:58):

fitzgen updated PR #7943.

Wasmtime GitHub notifications bot (Feb 20 2024 at 21:14):

fitzgen merged PR #7943.

Last updated: Nov 22 2024 at 17:03 UTC