Stream: git-wasmtime

Topic: wasmtime / PR #6691 Implement component model resources i...

Wasmtime GitHub notifications bot (Jul 05 2023 at 18:38):

alexcrichton opened PR #6691 from alexcrichton:resources to bytecodealliance:main:

This PR is a work-in-progress of implementing the resource type of the component model in Wasmtime. My goal at this time is to implement everything necessary to get *.wast and embedder API tests working. This has notable omissions which are deferred to a future PR:

• No support in component::bindgen!
• No support for component-to-component resources (fact compiler)

Remaining items:

• [ ] destructors
• [ ] (borrow $T)
• [ ] probably more tests
• [ ] rewrite this OP with more words
• [ ] fill out all /// TODO documentation
• [ ] Land and publish wasm-tools changes

Wasmtime GitHub notifications bot (Jul 05 2023 at 22:00):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jul 05 2023 at 22:00):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jul 05 2023 at 22:00):

sunfishcode created PR review comment:

This is probably not a huge optimization, but these two instructions to compute the new rep value can be emitted into call_func_ref_block instead so that they're not executed if there is no destructor.

Wasmtime GitHub notifications bot (Jul 05 2023 at 22:00):

sunfishcode created PR review comment:

I'm curious why this code is checking for the function pointer being null, instead of testing the least significant bit of the value returned by resource_drop. Do we ever have a Some value with a null destructor pointer?

Wasmtime GitHub notifications bot (Jul 05 2023 at 22:33):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 05 2023 at 22:35):

alexcrichton created PR review comment:

Ah they end up being two different conditions. First is that resource.drop can be used to drop either own or borrow values, so the first check is basically "which did I drop?". The second check is "ok is there actually a destructor?". So a return value of 0 from the intrinsic means "you dropped a borrow do nothing else". Otherwise the return value is (rep << 1) | 1 where the destructor, if specified, is invoked with rep.

As I type this out though I realize that whether or not the destructor is specified is actually static so there's actually no need for a runtime check of the funcref, all that needs to be checked is the return value from the drop intrinsic to see if it's an own/borrow.

Wasmtime GitHub notifications bot (Jul 05 2023 at 22:45):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 05 2023 at 23:18):

alexcrichton edited PR #6691:

This PR is a work-in-progress of implementing the resource type of the component model in Wasmtime. My goal at this time is to implement everything necessary to get *.wast and embedder API tests working. This has notable omissions which are deferred to a future PR:

• No support in component::bindgen!
• No support for component-to-component resources (fact compiler)

Remaining items:

• [x] destructors
• [ ] (borrow $T)
• [ ] probably more tests
• [ ] rewrite this OP with more words
• [ ] fill out all /// TODO documentation
• [ ] Land and publish wasm-tools changes

Wasmtime GitHub notifications bot (Jul 06 2023 at 04:43):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 06 2023 at 15:28):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 06 2023 at 17:46):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 06 2023 at 18:59):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 06 2023 at 20:50):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2023 at 20:50):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2023 at 20:50):

sunfishcode created PR review comment:

Can this use NonNull::cast?

Wasmtime GitHub notifications bot (Jul 06 2023 at 20:50):

sunfishcode created PR review comment:

This comment should be updated to reflect its meaning.

Wasmtime GitHub notifications bot (Jul 06 2023 at 20:50):

sunfishcode created PR review comment:

This comment about the null check is now out of date; it'd be good to mention what you said earlier about the type being known statically.

Wasmtime GitHub notifications bot (Jul 10 2023 at 20:30):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 10 2023 at 20:32):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 10 2023 at 20:34):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 10 2023 at 20:40):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 10 2023 at 21:14):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 10 2023 at 21:53):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 11 2023 at 16:30):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 11 2023 at 17:43):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 13 2023 at 15:26):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 13 2023 at 18:03):

alexcrichton edited PR #6691:

This PR is a work-in-progress of implementing the resource type of the component model in Wasmtime. My goal at this time is to implement everything necessary to get *.wast and embedder API tests working. This has notable omissions which are deferred to a future PR:

• No support in component::bindgen!
• No support for component-to-component resources (fact compiler)

Remaining items:

• [x] destructors
• [x] (borrow $T)
• [ ] probably more tests
• [ ] rewrite this OP with more words
• [ ] fill out all /// TODO documentation
• [ ] Land and publish wasm-tools changes

Wasmtime GitHub notifications bot (Jul 13 2023 at 18:03):

alexcrichton edited PR #6691:

This PR is a work-in-progress of implementing the resource type of the component model in Wasmtime. My goal at this time is to implement everything necessary to get *.wast and embedder API tests working. This has notable omissions which are deferred to a future PR:

• No support in component::bindgen!
• No support for component-to-component resources (fact compiler)

Remaining items:

• [x] destructors
• [x] (borrow $T)
• [x] probably more tests
• [ ] rewrite this OP with more words
• [ ] fill out all /// TODO documentation
• [ ] Land and publish wasm-tools changes

Wasmtime GitHub notifications bot (Jul 13 2023 at 18:03):

alexcrichton edited PR #6691:

This PR is a work-in-progress of implementing the resource type of the component model in Wasmtime. My goal at this time is to implement everything necessary to get *.wast and embedder API tests working. This has notable omissions which are deferred to a future PR:

• No support in component::bindgen!
• No support for component-to-component resources (fact compiler)

Remaining items:

• [x] destructors
• [x] (borrow $T)
• [x] probably more tests
• [ ] rewrite this OP with more words
• [ ] fill out all /// TODO documentation
• [x] Land and publish wasm-tools changes

Wasmtime GitHub notifications bot (Jul 13 2023 at 18:03):

alexcrichton edited PR #6691:

This PR is a work-in-progress of implementing the resource type of the component model in Wasmtime. My goal at this time is to implement everything necessary to get *.wast and embedder API tests working. This has notable omissions which are deferred to a future PR:

• No support in component::bindgen!
• No support for component-to-component resources (fact compiler)

Remaining items:

• [x] destructors
• [x] (borrow $T)
• [x] probably more tests
• [ ] rewrite this OP with more words
• [ ] fill out all /// TODO documentation
• [x] Land and publish wasm-tools changes

Closes #6583

Wasmtime GitHub notifications bot (Jul 13 2023 at 18:40):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 13 2023 at 20:58):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 14 2023 at 16:51):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 14 2023 at 16:51):

alexcrichton edited PR #6691:

This PR is a work-in-progress of implementing the resource type of the component model in Wasmtime. My goal at this time is to implement everything necessary to get *.wast and embedder API tests working. This has notable omissions which are deferred to a future PR:

• No support in component::bindgen!
• No support for component-to-component resources (fact compiler)

Remaining items:

• [x] destructors
• [x] (borrow $T)
• [x] probably more tests
• [ ] rewrite this OP with more words
• [x] fill out all /// TODO documentation
• [x] Land and publish wasm-tools changes

Closes #6583

Wasmtime GitHub notifications bot (Jul 14 2023 at 16:57):

alexcrichton edited PR #6691:

This PR is an implementation of the resource datatype in the component model as specified upstream. The goal of this PR is to get lots of the low-level infrastructure for resources sorted out, but not 100% of the story as there's still remaining work. Features implemented in this PR are:

• Validation, translation, and compilation of resources now works
• Implementation of all resource-related intrinsics such as resource.{rep,new,drop}
• Updates to the compiled image of components as necessary for intrinsics
• Implementation of runtime state of resources, aka tables, in component instances
• Updates to type-checking and type-matching to take resources into account
• Implementation of an embedding API for host-defined resources
• Implementation of an embedding API for guest-defined resources
• An initial suite of test which showcase basic behavior of resources and the capabilities of the embedding API

This is intended to be a solid implementation for all the "internals" of resources throughout Wasmtime. It's expected that all further work will be much easier, less invasive, and not so large a scale. Or at least that's the hope. Note though that this is not a 100% complete story for resources in Wasmtime. For example it's still not possible to take a WIT off the shelf with resources and use that with Wasmtime. Key missing features in Wasmtime related to resources are:

• https://github.com/bytecodealliance/wasmtime/issues/6722
• https://github.com/bytecodealliance/wasmtime/issues/6696
• https://github.com/bytecodealliance/wasmtime/issues/6724

This work is a prerequisite for the above, though, and the hope additionally is that the above can all start to progress in parallel with this work as a basis. Basically getting enough in that everything is no longer bottlenecked on me but it's possible to start building out from here.

Closes #6583

Wasmtime GitHub notifications bot (Jul 14 2023 at 16:57):

alexcrichton requested fitzgen for a review on PR #6691.

Wasmtime GitHub notifications bot (Jul 14 2023 at 16:57):

alexcrichton has marked PR #6691 as ready for review.

Wasmtime GitHub notifications bot (Jul 14 2023 at 16:57):

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

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen submitted PR review:

r=me with nitpicks below

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

This isn't just for transcodes, right? Outdated comment?

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

It would be nice to have an index newtype for libcalls.

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen submitted PR review:

r=me with nitpicks below

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

Can this be read only?

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

Copy paste comment needs to be inverted.

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

This as well?

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

Can we emit an assertion in the generated code for this when cfg(debug_assertions)?

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

Can we make this branch unreachable!()?

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

    /// definition which should refer to a resource itself.

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

    /// Unlike `resource_id_to_table_index` this is required to be eagerly

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

Sometimes "wasmparser" has backticks and sometimes it doesn't in this big doc comment. We should be consistent. I personally prefer backticks but this is obviously not a big deal.

(Thanks, btw, for writing this nice big doc comment!)

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

//! and then from then on purely talk about table indices. Each component

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

Copy-paste doc comment is misleading

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

Non-resource types are structural, not nominal, right?

//! such as `list`, `record`, and `string` are considered "structural" where two
//! types are considered equal if their components are equal. For example `(list
//! $a)` and `(list $b)` are the same if `$a` and `$b` are the same. Resources,

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:40):

fitzgen created PR review comment:

//! Given the subtlety here the goal is to lean on `wasmparser` as much as

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:41):

fitzgen created PR review comment:

        // signature for `resource.drop` is mentioned somewhere, and if the
        // wasm-to-native trampoline for `resource.drop` hasn't been created yet
        // then insert that here. This is possibly required by destruction of

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:41):

fitzgen created PR review comment:

Not sure I am 100% following so take this with a grain of salt, but can this just check whether ret.resource_drop_wasm_to_native_trampoline.is_some() instead of maintaining a hash set if all compile keys?

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:41):

fitzgen created PR review comment:

If a component takes two resource type imports, and is instantiated from the host with the same resource for both imports, and we get the ResourceType for each of those imports from that component, will both of those result in the same ResourceType? That is, even though the compiled component cannot know they are the same, at the Wasmtime API level when we do reflection on the instantiated component, do we see through everything and determine they are the same? I think we should, and I think we should test this if we don't already.

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:41):

fitzgen created PR review comment:

Kind of annoying we have to thread this through everything specially, when we worked so hard to avoid that elsewhere. Can we perhaps give it it a known compile key or something, so we can just look it up in the various output maps or whatever? Rather than add a special field to all of our compiler's structs. Not sure this is feasible but I'd like to just push in that direction if possible.

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:41):

fitzgen created PR review comment:

Nice.

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:41):

fitzgen created PR review comment:

/// component model provides is that the `u32` is not forgeable by guests and

Wasmtime GitHub notifications bot (Jul 21 2023 at 17:41):

fitzgen created PR review comment:

The parenthetical could be clarified a bit:

///   doesn't go further and test for equality in the guest itself (for example
///   two different heap allocations of `Box<u32>` can be equal in normal Rust if they contain
///   the same value, but will never be considered equal when compared as `Val::Resource`s).

Wasmtime GitHub notifications bot (Jul 21 2023 at 18:49):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2023 at 18:49):

alexcrichton created PR review comment:

This branch is hit at runtime so it can't be unreachable!, but I'll update the comment to clarify that.

Wasmtime GitHub notifications bot (Jul 21 2023 at 18:50):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2023 at 18:50):

alexcrichton created PR review comment:

Oops heh yes I mean the exact opposite of what I say

Wasmtime GitHub notifications bot (Jul 21 2023 at 19:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2023 at 19:01):

alexcrichton created PR review comment:

Correct! Added a test as well :+1:

Wasmtime GitHub notifications bot (Jul 21 2023 at 19:01):

alexcrichton edited PR review comment.

Wasmtime GitHub notifications bot (Jul 21 2023 at 19:24):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2023 at 19:24):

alexcrichton created PR review comment:

It's a bit subtle and worth a comment that's not currently present, but this is doing 2 things where first it's determining if a trampoline is required and if so creating a CompileKey for it. Next it's detecting if that key was already compiled for some previous trampoline, and if not it adds it.

In that sense this is the "root" where resource_drop_wasm_to_native_trampoline is first set to Some, so it can't piggy-back on anything else.

That being said coupled with your below concern I'll see if I can clean this up a bit.

Wasmtime GitHub notifications bot (Jul 21 2023 at 19:48):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 21 2023 at 19:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2023 at 19:48):

alexcrichton created PR review comment:

True! There's a newtype for core wasm libcalls but I was a bit hesitant to duplicate all the macro machinery for that as well since the libcalls already duplicate other macro machinery.

Wasmtime GitHub notifications bot (Jul 21 2023 at 19:54):

alexcrichton has enabled auto merge for PR #6691.

Wasmtime GitHub notifications bot (Jul 21 2023 at 20:27):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 21 2023 at 20:28):

alexcrichton has enabled auto merge for PR #6691.

Wasmtime GitHub notifications bot (Jul 21 2023 at 20:58):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 21 2023 at 20:58):

alexcrichton has enabled auto merge for PR #6691.

Wasmtime GitHub notifications bot (Jul 21 2023 at 21:59):

alexcrichton updated PR #6691.

Wasmtime GitHub notifications bot (Jul 21 2023 at 21:59):

alexcrichton has enabled auto merge for PR #6691.

Wasmtime GitHub notifications bot (Jul 22 2023 at 00:06):

alexcrichton merged PR #6691.

Last updated: Nov 22 2024 at 16:03 UTC