support dynamic function calls in component model #4442
Conversation
|
@alexcrichton @jameysharp This addresses #4310. I plan to push one more commit to add "sad path" tests, but this should be complete otherwise. Please pay particular attention to the |
github-actions
bot
added
the
wasmtime:api
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasmtime:api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
I could use some advice about how to address the CI issue. I've added a new Is there a better way to reuse that code, or else some way to bootstrap the dependency and make |
|
I didn't know this, but there's a comment in |
Oh good call. I did know that, but forgot about it. Will try that now. |
There was a problem hiding this comment.
I'll leave it to @alexcrichton to decide whether to approve this, but I have a few small observations scattered through.
Overall I'm happy with this PR. I'm disappointed at how much crates/wasmtime/src/component/values.rs duplicates code that's in crates/wasmtime/src/component/func/typed.rs and elsewhere, but I don't see a solid alternative right now, so let's run with it I guess.
@alexcrichton, I think you can focus your review on the changes in crates/wasmtime/src/component/func.rs. I feel pretty confident approving the rest, but that file has the one new unsafe block and generally the trickiest interactions with other parts of wasmtime.
| @@ -1095,7 +1093,7 @@ impl WasmStr { | |||
| self._to_str(store.into().0) | |||
| } | |||
|
|
|||
| fn _to_str<'a>(&self, store: &'a StoreOpaque) -> Result<Cow<'a, str>> { | |||
| pub(crate) fn _to_str<'a>(&self, store: &'a StoreOpaque) -> Result<Cow<'a, str>> { | |||
There was a problem hiding this comment.
I don't love making a function named _to_str visible outside the current module. But I see all you have at the call site is a &StoreOpaque so you can't call to_str instead. Unless @alexcrichton has a better idea I guess this is fine.
There was a problem hiding this comment.
Yeah, that felt awkward to me, too. Perhaps we could rename it to something like to_str_with_store_opaque? I haven't yet wrapped my head around all the different flavors of store, so perhaps there's a better option.
| ), | ||
| )?; | ||
| } | ||
| (Val::Flags { count, value }, Type::Flags(_)) => { |
There was a problem hiding this comment.
If you got count from Type::Flags here instead of from Val::Flags, I don't see that you'd need to store a count in Val::Flags at all. It looks like everywhere that you need the count, you have the type available.
I guess you did it this way to force the host application to specify how many flags they expect to be passing in as parameters, so you can verify in typecheck that their expectation was correct—and so you can inform the host application about how many flags they received.
I go back and forth on whether that kind of checking is worth doing in this sort of dynamic call situation. @alexcrichton, what's your take?
There was a problem hiding this comment.
Thanks for this! Personally I think that this is a really tricky problem and I don't think that there's any good answer right now to how to implement this. On one hand I think that this PR is probably sufficient from and implementation and functionality point of view, but on the other hand I don't think that this is suitable for any performance-conscious scenario. I don't know if this is the best choice of tradeoffs at this time because to me at least we're generally trying to get to a semi-final state for the component model where the known deficiencies are accounted for.
I'm personally wary of throwing in the towel on performance so soon here but I also don't have any alternatives. Even if we go with a strategy like the one you've implemented here there's a lot of open questions to me which I don't think have a clear answer:
-
One thing is how types are represented. I would naively expect to be able to, given a
Val, ask for its type along the lines of thewasmtime::Val::tymethod. Given this current implementation I don't think that this is possible. I think, though, it might be possible to add this in where a "type index" of some sort stood next to theRecord/Variant/Flagstypes. -
On the topic of type information one thing that we'll eventually want to add to the component model implementation is the ability to introspect a component for the types of its imports and exports. For example currently Wasmtime has
wasmtime::Module::importsandwasmtime::Module::exportsbut the equivalent for the component model isn't implemented yet. I believe an implementation of this will require something along the lines of theenum Typeyou've started here but I don't think we want an "ownedType" as you've sketched out which recursively owns all of its contents. Instead I think we'll want a design similar to theComponentTypesBuilderinterface right now where there's a "bag of everything" and types all index into that. I don't think that you need to implementComponent::{imports,exports}in this PR here, but at the same time I'd prefer to avoid having another internal representation of types different from what we already have. Adding this type information may be inevitable for this API because I do think that records/variants at least should be created with their type information up-front (rather than "if you have the right number of fields it all works out") -
One thing I've worried about historically is the size/alignment calculation. Right now in the typed API it's generally all statically done at compile time but with this dynamic API I'm a little worried that with a recursively-defined size calculation it runs the risk of being
$O(n^2)$ performance if the size is requested at many layers of the type tree. It might be better to start storing canonical size/align information for aggregates in theComponentTypesstructure rather than calculating it on the fly here.
That's at least an initial dump of thoughts I have on the internals here. I wish I had a good answer for this like this ahead of time. Otherwise though while I don't think we need to get this PR to a perfect spot before landing I do think we should get it into a state where the major pieces of functionality are there. For me those are:
- There shouldn't be a duplicate internal representation of type information (currently
enum Typeis crate-local) - Personally I think that
Val::type(maybe_some_arg) -> Somethingshould exist to get the type from a standaloneVal
Those two points may have a significant degree of fallout depending on the implementation direction.
On the topic of code duplication, to some degree that's just life in the component model. I've written the same size/alignment calculation in maybe 4 crates now at this point. It's slightly different each time and I've found it really hard to share code. In general I don't think my development strategy here has been really all that tenable or scalable historically and I'd ideally like to change it.
Where possible I think thinks should be shared. For example reusing Lift::load here and Lower::store I think is a great idea. If possible it might be good to try and reuse ComponentType::typecheck here as well. Otherwise though if something is difficult to share I don't think it's worth bending over backwards to have precisely one implementation of something. To that end that's also why we want a strong fuzzing implementation here eventually.
Specifically for Func::call though I would ideally like to share more of that with TypedFunc::call. There's a fair amount of nontrivial logic, especially around handling post_return, may_enter, etc. If you would be ok with it I think it would be good to try to make a generic helper so that call_unchecked_raw is only called once or something roughly along those lines. Again though I wouldn't bend over backwards too much for this, I just think it's good to try to investigate and see how it works out.
Otherwise as a random final note I think that you'll want to move post_return from TypedFunc to Func. Additionally I think you'll want to assert specific errors on function calls since I think all the .is_err() assertions are passing due to "needs a call to post_return" rather than type-checking errors that should otherwise be happening.
|
Thanks @alexcrichton and @jameysharp for your thoughtful and detailed feedback. Alex: I agree with what you said about representing types and wanting to be able to get a type from a |
|
Hm actually I do think you'll want to pass around I think this will want to be an argument because if an end user creates a |
|
@alexcrichton Would you please point me to the code where you do that trick of pulling the ComponentTypes out of the store temporarily? Also, here's a quick sketch of what I'm thinking. Please let me know what you think. pub enum RecordIndex {
Unit,
Record(TypeRecordIndex),
Tuple(TypeTupleIndex)
}
pub enum VariantIndex {
Variant(TypeVariantIndex),
Enum(TypeEnumIndex),
Union(TypeUnionIndex),
Option(TypeInterfaceIndex),
Expected(TypeExpectedIndex)
}
pub enum Val {
Bool(bool),
S8(i8),
U8(u8),
S16(i16),
U16(u16),
S32(i32),
U32(u32),
S64(i64),
U64(u64),
Float32(u32),
Float64(u64),
Char(char),
String(Rc<str>),
List {
element_type_index: TypeInterfaceIndex,
values: Rc<[Val]>
},
Record {
type_index: RecordIndex,
fields: Rc<[Val]>
},
Variant {
type_index: VariantIndex,
discriminant: u32,
value: Rc<Val>,
},
Flags {
type_index: TypeFlagsIndex,
count: u32,
value: Rc<[u32]>,
},
}
impl Val {
pub fn ty() -> InterfaceType {
...
}
...
} |
|
Regarding the code sketch above: we'd need to add methods to |
|
That seems like a good start to me. I'd probably go ahead and use Otherwise though the type information will be a bit tricky since I would prefer to not simply reexport everything within |
|
@alexcrichton Can you elaborate on what you think the parallel type hierarchy in the Here's my latest sketch: pub struct TypeIndex(TypeInterfaceIndex);
pub struct RecordTypeIndex(TypeRecordIndex);
pub struct TupleTypeIndex(TypeTupleIndex);
pub struct VariantTypeIndex(TypeVariantIndex);
pub struct EnumTypeIndex(TypeEnumIndex);
...
pub enum AnyRecordTypeIndex {
Unit,
Record(RecordTypeIndex),
Tuple(TupleTypeIndex)
}
pub enum AnyVariantTypeIndex {
Variant(VariantTypeIndex),
Enum(EnumTypeIndex),
Union(UnionTypeIndex),
Option(TypeIndex),
Expected(ExpectedTypeIndex)
}
pub enum Type {
Bool,
S8,
U8,
S16,
U16,
S32,
U32,
S64,
U64,
Float32,
Float64,
Char,
String,
List(TypeIndex),
Record(AnyRecordTypeIndex),
Variant(AnyVariantTypeIndex),
Flags(FlagTypeIndex),
}
impl Type {
fn from(ty: &InterfaceType) -> Self {
...
}
}
pub struct Field<'a> {
name: &str,
ty: Type,
}
pub struct Types(ComponentTypes);
impl Types {
pub fn record_name(&self, index: RecordTypeIndex) -> &str {
&self.0[index.0].name
}
pub fn record_fields(&self, index: RecordTypeIndex) -> impl Iterator<Field> {
&self.0[index.0].fields.iter().map(|field| Field { name: &field.name, ty: Type::from(&field.ty) })
}
...
}
pub enum Val {
Bool(bool),
S8(i8),
U8(u8),
S16(i16),
U16(u16),
S32(i32),
U32(u32),
S64(i64),
U64(u64),
Float32(u32),
Float64(u64),
Char(char),
String(Box<str>),
List {
element_type: Type,
values: Box<[Val]>
},
Record {
ty: RecordType,
fields: Box<[Val]>
},
Variant {
ty: VariantType,
discriminant: u32,
value: Box<Val>,
},
Flags {
ty: FlagsType,
count: u32,
value: Box<[u32]>,
},
}
impl Val {
pub fn ty(types: &Types) -> Type {
...
}
...
}The idea is to wrap the relevant |
|
That looks along the lines of what I was thinking, but to be clear I haven't fully thought this through. I've got what I think should be the constraints in my head but said constraints don't always translate well to code... In any case what you've described captures all the main constraints I was thinking of. The actual enum Type {
// ...
Record(RecordType),
// ...
}
struct RecordType {
index: TypeRecordIndex,
types: Arc<ComponentTypes>,
}That's a whole lot of enum Type<'a> {
// ...
Record(RecordType<'a>),
// ...
}
struct RecordType<'a> {
index: TypeRecordIndex,
types: &'a ComponentTypes,
}but then getting that from a enum Val {
// ...
Record(Record),
// ...
}
struct Record {
// ...
ty: TypeRecordIndex,
instance: Instance, // just an index
}or something like that maybe? I may be too naive also trying to prematurely optimize this. It might make the most sense to go with |
|
I've tried a few variations. Here's the latest: pub struct Types<'a>(&'a ComponentTypes);
pub struct Case<'a> {
name: &'a str,
ty: Type,
}
#[derive(Copy, Clone)]
pub struct VariantIndex(TypeVariantIndex);
impl VariantIndex {
pub fn name(self, types: Types) -> &str {
&types.0[self.0].name
}
pub fn cases(self, types: Types) -> impl ExactSizeIterator<Case> {
types.0[self.0].cases.iter().map(|case| Case {
name: &case.name,
ty: Type::from(&case.ty),
})
}
}
pub enum Type {
// ...
Variant(VariantIndex),
// ...
}
impl Type {
fn from(ty: &InterfaceType) -> Self {
match ty {
// ...
InterfaceType::Variant(index) => Type::Variant(VariantIndex(*index)),
// ...
}
}
}
pub enum Val {
// ...
Variant(Variant),
// ...
}
impl Val {
pub fn ty(&self) -> Type {
match self {
// ...
Self::Variant { ty, .. } => Type::Variant(ty),
// ...
}
}
// ...
}
pub struct Variant {
ty: VariantIndex,
discriminant: u32,
value: Box<Val>,
}
impl Variant {
pub fn try_new(
ty: VariantIndex,
types: Types,
discriminant: u32,
value: Box<Val>,
) -> Result<Self> {
// ... (here we would verify that `discriminant` and `value` match `ty` and throw an error otherwise)
Ok(Self {
ty,
discriminant,
value,
})
}
}
impl Func {
pub fn types(&self, store: impl AsContext<'a>) -> Types<'a> {
Types(&store.as_context()[self.0].types)
}
pub fn get_param_type(&self, store: impl AsContext, index: u32) -> Type {
let data = &store.as_context()[self.0];
Type::from(data.types[data.ty].params[index])
}
pub fn get_result_type(&self, store: impl AsContext) -> Type {
let data = &store.as_context()[self.0];
Type::from(data.types[data.ty].result)
}
// ...
}
#[test]
fn foo() -> Result<()> {
let engine = super::engine();
let mut store = Store::new(&engine, ());
let component = Component::new(
&engine,
make_echo_component_with_params(
r#"(variant (case "A" u32) (case "B" s32))"#,
&[super::Type::I32, super::Type::I32],
),
)?;
let instance = Linker::new(&engine).instantiate(&mut store, &component)?;
let func = instance.get_func(&mut store, "echo").unwrap();
let ty = func.get_param_type(&store, 0);
if let Type::Variant(index) = ty {
let input = [Val::Variant(Variant::try_new(ty, func.types(&store))?)];
let output = func.call(&mut store, &input)?;
assert_eq!(input[0], output)
} else {
unreachable!()
}
Ok(())
}The ergonomics aren't outstanding, but the efficiency is pretty good. My rationale for still exposing indexes is that Wasm itself uses indexes everywhere, so the paradigm will at least be familiar to users. Happy to just use an |
|
@alexcrichton Any thoughts on what I posted above? I want to make sure we agree on the design of Meanwhile, I'll start working on #4307 and come back to this once we have consensus. |
|
Apologies for the delay, busy day yesterday and this morning! That looks good to me though. Some things may be tweaked over time but I think that's good to land. |
|
I started implementing this according to what I proposed above, but now I'm having second thoughts. At first, I thought the natural place to do type checking was when creating the impl Variant {
pub fn try_new(
ty: VariantIndex, // just a wrapper around a TypeVariantIndex
types: Types, // just a wrapper around a &ComponentTypes
discriminant: u32,
value: Box<Val>,
) -> Result<Self> {
// ... (here we would verify that `discriminant` and `value` match `ty` and throw an error otherwise)
Ok(Self {
ty,
discriminant,
value,
})
}
}However, that has a big hole: an embedding programmer could accidentally create a Also, my plan was to provide accessor methods for e.g. getting the cases for a So now I think we're better off with @alexcrichton's suggestion of either embedding an Another crazy idea would be to borrow a trick from GhostCell and tag One final thought: @alexcrichton how strongly do you feel that one should be able to get a |
|
Hm yes that is indeed a problem! FWIW this is solved at the store level with the Otherwise though storing I dunno how to feel about getting a Personally I'm trying to stay analgous to the core wasm API, but I realize how it's a lot easier there than it is here. I don't really know the best answer to any of these questions. Our main use case for this is fuzzing at this point where getting a |
This addresses bytecodealliance#4310, introducing a new `component::values::Val` type for representing component values dynamically, as well as `component::types::Type` for representing the corresponding interface types. It also adds a `call` method to `component::func::Func`, which takes a slice of `Val`s as parameters and returns a `Result<Val>` representing the result. Note that I've moved `post_return` and `call_raw` from `TypedFunc` to `Func` since there was nothing specific to `TypedFunc` about them, and I wanted to reuse them. The code in both is unchanged beyond the trivial tweaks to make them fit in their new home. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
dicej
force-pushed
the
component-dynamic-type
branch
from
0012715
to
68f7786
Compare
|
I've overhauled the code according to the above feedback, and I decided to squash and rebase since so much was changing anyway. Please let me know what you think. Highlights to note:
|
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
This also removes the redundant `store` parameter from `Type::load`. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
| self.desc(), | ||
| value.ty().desc() | ||
| )) | ||
| } |
There was a problem hiding this comment.
This doesn't necessarily have to be handled here, but I think this typecheck will eventually want to be more recursive for better error mesages. Right now I think you could get expected record, got record which doesn't describe which record fields mismatch or similar.
There was a problem hiding this comment.
Any e.g. record field mismatch would have been caught already in Type::new_record before we got this far. The only time you'd see expected record, got record would be if you used Vals from one component in another, or if your component had two identical records (i.e. same field names and types) and you mixed up which one you created the Val for. Either way, more recursion wouldn't necessarily help. This is somewhat analogous to when rustc reports a type mismatch between foo::Bar and foo::Bar because you're using two different versions of the foo crate. It's even worse here since types don't really have names in the component model.
But yes, I agree we should give a friendlier diagnostic here, e.g. hinting based on whether Handle::index or Handle::types are different, or both.
There was a problem hiding this comment.
I've improved the error message so you'll never get expected record, got record. I attempted to go much further than that and recursively compare the types, but reusing typed::typecheck_tuple etc. turned out to be difficult in a dynamic context, and I didn't want to duplicate them either.
Also, I'm having trouble finding guidance in https://github.com/WebAssembly/component-model about when interface types are to be considered equal and when not, so I'm not sure what the correct logic is anyway. Are types considered equal if they are structurally identical?
There was a problem hiding this comment.
I don't think we'll get a ton of guidance from the component model itself, but for now from Wasmtime's perspective given how trampolines and lifting/lowering/etc are all implemented we require exact structural equality for the embedder API. I think the intention is that subtyping would eventually be used but we don't have a great means of implementing that right now.
This fixes a few issues: - Bad offset calculation when lowering - Missing variant padding - Style issues regarding `types::Handle` - Missed opportunities to reuse `Lift` and `Lower` impls It also adds forwarding `Lift` impls for `Box<[T]>`, `Vec<T>`, etc. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
|
Thanks so much for the thorough review, @alexcrichton. I've addressed some of the issues you raised (and marked the corresponding conversations "resolved") and will look at the others tomorrow. |
Per review feedback, I've moved `Type::new_record` to `Record::new_val` and added a `Type::unwrap_record` method; likewise for the other kinds of types. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
These types should compare as equal across component boundaries as long as their type parameters are equal. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
There was a problem hiding this comment.
This is looking great, thanks again for all this! I think with some minor fixes here and there this is basically good to go.
Personally I feel that Type::nested should still get removed. I think that the usage in tests can be .unwrap_$thing().field_accessor() perhaps?
We now distinguish between more failure cases to provide an informative error message. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
- Remove `WasmStr::to_str_from_memory` and `WasmList::get_from_memory` - add `try_new` methods to various `values` types - avoid using `ExactSizeIterator::len` where we can't trust it - fix over-constrained bounds on forwarded `ComponentType` impls Signed-off-by: Joel Dice <joel.dice@fermyon.com>
|
@alexcrichton I believe I've addressed everything you mentioned. I'm still unsure about how |
| self.desc(), | ||
| value.ty().desc() | ||
| )) | ||
| } |
There was a problem hiding this comment.
I don't think we'll get a ton of guidance from the component model itself, but for now from Wasmtime's perspective given how trampolines and lifting/lowering/etc are all implemented we require exact structural equality for the embedder API. I think the intention is that subtyping would eventually be used but we don't have a great means of implementing that right now.
|
|
||
| impl<T: PartialEq> PartialEq for Handle<T> { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.index == other.index && Arc::ptr_eq(&self.types, &other.types) |
There was a problem hiding this comment.
I think that the tuple/option/expected bits are probably not going to be necessary in the long-term if the original issue here is fixed. I think this is fine to leave as a FIXME with a follow-up issue for now though.
Otherwise though the current logic in tuple/option/expected I would expect to be baked in here or otherwise into some trait impl on T perhaps. I don't think we'd want to duplicate things and have some using PartialEq and some using manual impls.
- Move functions from `types` to `values` module so we can make certain struct fields private - Rename `try_new` to just `new` Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Instead, I've added a FIXME comment and will open an issue to do recursive structural equality testing. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
|
@alexcrichton I've addressed your latest feedback and opened #4522. |
|
Thanks again for this @dicej! |
This commit builds on bytecodealliance/wasm-tools#690 to add support to testing of the component model to execute functions when running `*.wast` files. This support is all built on bytecodealliance#4442 as functions are invoked through a "dynamic" API. Right now the testing and integration is fairly crude but I'm hoping that we can try to improve it over time as necessary. For now this should provide a hopefully more convenient syntax for unit tests and the like.
This commit builds on bytecodealliance/wasm-tools#690 to add support to testing of the component model to execute functions when running `*.wast` files. This support is all built on bytecodealliance#4442 as functions are invoked through a "dynamic" API. Right now the testing and integration is fairly crude but I'm hoping that we can try to improve it over time as necessary. For now this should provide a hopefully more convenient syntax for unit tests and the like.
This commit builds on bytecodealliance/wasm-tools#690 to add support to testing of the component model to execute functions when running `*.wast` files. This support is all built on bytecodealliance#4442 as functions are invoked through a "dynamic" API. Right now the testing and integration is fairly crude but I'm hoping that we can try to improve it over time as necessary. For now this should provide a hopefully more convenient syntax for unit tests and the like.
This commit builds on bytecodealliance/wasm-tools#690 to add support to testing of the component model to execute functions when running `*.wast` files. This support is all built on #4442 as functions are invoked through a "dynamic" API. Right now the testing and integration is fairly crude but I'm hoping that we can try to improve it over time as necessary. For now this should provide a hopefully more convenient syntax for unit tests and the like.
This addresses #4310, introducing a new
component::values::Valtype forrepresenting component values dynamically. It also adds a
callmethod tocomponent::func::Func, which takes a slice ofVals as parameters and returnsa
Result<Val>representing the result.As an implementation detail, I've also added a
component::values::Typetype,which is an owned, despecialized version of
wasmtime_environ::component::InterfaceType. That serves two purposes:InterfaceTypes.Finally, I happened to notice that the
ComponentType::SIZE32calculation inexpand_record_for_component_typeneeded a correction, so I did that.Signed-off-by: Joel Dice joel.dice@fermyon.com