Skip to content
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

Wasmtime: Finish support for the typed function references proposal #7943

Merged

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Feb 15, 2024

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 #6455

@fitzgen fitzgen requested a review from a team as a code owner February 15, 2024 00:30
@github-actions github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. wasmtime:config Issues related to the configuration of Wasmtime labels Feb 15, 2024
Copy link

Subscribe to Label Action

cc @fitzgen, @peterhuene

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 .github/subscribe-to-label.json configuration file.

Learn more.

Copy link

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.

    Our documentation should be of the following form:

    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.
    
  • If you added a new Config method, or modified an existing one, you
    ensured that this configuration is exercised by the fuzz targets.

    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 (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 for more 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.


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

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

Learn more.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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...

crates/wasmtime/src/runtime/component/func/options.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/externals/global.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/externals/table.rs Outdated Show resolved Hide resolved
tests/all/traps.rs Outdated Show resolved Hide resolved
tests/all/linker.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/func/typed.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/func/typed.rs Show resolved Hide resolved
crates/wasmtime/src/runtime/types.rs Show resolved Hide resolved
crates/wasmtime/src/runtime/types/matching.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/values.rs Outdated Show resolved Hide resolved
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 `Ref`s rather
  than `Val`s.

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 bytecodealliance#6455
Ever since `FuncType`'s internal `RegisteredType` holds onto its own `Engine`,
we don't need these anymore.

Still useful to keep the `Engine` parameter around for the `ensure_matches`
methods because that can be used to check correct store/engine usage for
embedders.
@fitzgen fitzgen force-pushed the canonicalize-typed-function-references branch from 8f67710 to fdf1889 Compare February 16, 2024 18:39
@alexcrichton
Copy link
Member

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.

@fitzgen
Copy link
Member Author

fitzgen commented Feb 16, 2024

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!

@fitzgen
Copy link
Member Author

fitzgen commented Feb 16, 2024

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

#7953

@fitzgen fitzgen requested a review from a team as a code owner February 16, 2024 23:44
@alexcrichton alexcrichton added this pull request to the merge queue Feb 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 20, 2024
@fitzgen fitzgen added this pull request to the merge queue Feb 20, 2024
Merged via the queue into bytecodealliance:main with commit ff93bce Feb 20, 2024
43 checks passed
@fitzgen fitzgen deleted the canonicalize-typed-function-references branch February 20, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. wasmtime:config Issues related to the configuration of Wasmtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Function references unresolved issues
2 participants