Component host functions#597
Conversation
2b9d837 to
47c3512
Compare
|
I am taking a look at this, it might take a while, since this is a considerable chunk of code to go through. |
Not really feedback, but a thought: wasmex uses the wit file when defining a component, instead of having a DSL for defining signatures in Elixir. Documented here. I think it's worth considering, but haven't given it much thought recently though; maybe there's a valid reason not to go with that approach. Feel free to push back! |
saulecabrera
left a comment
There was a problem hiding this comment.
In general the approach looks reasonable, however, I am concerned about the maintainability and performance aspects of the type system: we are in a sense duplicating some of the type checks that normally should be handed to Wasmtime.
To say: I am not fully convinced that the UX gains justify the complexity and the potential performance hit.
I think that a reasonable alternative to explore is a variant of Option A, as described in #433, which is somewhat similar to what wasmtime-py does (IIUC) lazy value coercion checks rather than eager type checks. Trade-offs that I can think of: (i) errors will surface at function call time rather than instantiation time (ii) we lose the homogeneous nature of the Component/Core APIs.
My assumption is that it shouldn't be too hard to try out the Option A variant on top of the changes present here, IIUC, it will be mostly a matter of removing the type check that is happening at instantiation time. Having some performance numbers will probably help us finalize the decision.
I looked a bit at Wasmex's approach, and my understanding is that their API is philosophically different to ours: they do not try to stay as close to Wasmtime's API, which is why I think the approach of using a WIT file suits their needs best. Personally I was not able to convince myself that we could adopt a similar approach.
| let result_value = if let Ok(result_array) = RArray::to_ary(proc_result) { | ||
| if result_array.len() != 1 { | ||
| return Err(wasmtime::Error::msg(format!( | ||
| "expected 1 result, got {}", | ||
| result_array.len() | ||
| ))); | ||
| } | ||
| unsafe { result_array.as_slice()[0] } | ||
| } else { | ||
| proc_result | ||
| }; | ||
|
|
There was a problem hiding this comment.
Can you add a bit more context on why this approach is needed/beneficial? What if the return type is a single list/tuple itself? e.g.,
import get-numbers: func() -> list<s32>;I think that the natural implementation from Ruby would be something like:
linker.root do |root|
root.func_new("get-numbers", [], [t.list(t.s32)]) do
[1, 2, 3]
end
endBut if I am understanding correctly, this snippet will flow though the 1 arm and error out?
With this approach if we actually want to return the array, we would need to double wrap:
linker.root do |root|
root.func_new("get-numbers", [], [t.list(t.s32)]) do
[[1, 2, 3]]
end
endRelated: I think we are missing more test cases in which the components return container types e.g., list, tuple.
There was a problem hiding this comment.
Good point. I hve now made changes to type the return values, so that it is possible to distinguish between an array and multiple return values.
| /// Stores type information for a registered host function | ||
| #[derive(Clone, Debug)] | ||
| struct FuncTypeInfo { | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
I think we can get rid of this?
| } | ||
|
|
||
| /// Validate that host functions defined via func_new match the component's expected import signatures | ||
| fn validate_host_function_signatures( |
There was a problem hiding this comment.
We have multiple helper functions defined inside this function, could we make a different module with all this logic instead? It might make it easier to reason about.
| root.func_new("make-point", [t.s32, t.s32], [point_type]) do |x, y| | ||
| {"x" => x, "y" => y} | ||
| end |
There was a problem hiding this comment.
In validate_host_function_signatures we are structurally validating that records match i.e., names and types must be identical and in the same order; we should probably add a test to ensure that such branch is covered?
| _ => { | ||
| // Other types (Module, CoreFunc, Type, etc.) are not validated here | ||
| } |
There was a problem hiding this comment.
Meaning that this validation is handed over to wasmtime?
4b6d60b to
2bf92f3
Compare
2bf92f3 to
70ed74e
Compare
|
I have now removed the instantiation-time type validations. I also added a "type wrapper" class for return values of the host functions as described in Option A of the issue, which allows wasmtime to perform type checks. With this approach, the host functions can perform type validations and raise an exception if they wish. |
There was a problem hiding this comment.
Can we add some tests covering round trips for the following: s8, u8, s16, u16, s64, u64, f32, f64, char, variant, enum and flags? I think they are not covered in the tests.
| let(:t) { Type } | ||
|
|
||
| context "simple host functions" do | ||
| it "defines a function with primitives" do |
There was a problem hiding this comment.
With the new implementation (validation at call time, rather than at definition time), we would need to call the function in order to validate the right behavior, right?
There was a problem hiding this comment.
True. I think this is already covered by the integration tests, so I will reduce the number of non-integration tests to the bare minimum (testing that providing a block works as expected and that providing no block fails)
| results[0] = converted; | ||
| Ok(()) | ||
| } | ||
| n => { |
There was a problem hiding this comment.
Should we have a test for this branch? I think we currently only cover <tuple<t>>, which still counts as a single value?
There was a problem hiding this comment.
Good point. Looking into this I realized that this branch might be dead code, it doesn't seem like it is possible to specify multiple return values for functions in WIT.
From here:
To express a function that returns multiple values, you can use any compound type (such as tuples or records).
So maybe we should just raise an error if a wasm component returns multiple values and have it be unsupported for now.
There was a problem hiding this comment.
Maybe I missed it, but I do not see tests for arity mismatch? If so, could we add some of those as well?
There was a problem hiding this comment.
Do you mean that wasmtime-rb should raise an error if the arity of the block does not match? One thing that could complicate this is if the block has splats or keyword arguments, would we disallow such blocks entirely and enforce positional block arguments only?
| + Sync | ||
| + 'static { | ||
| move |mut store_context: wasmtime::StoreContextMut<'_, StoreData>, | ||
| _func: wasmtime::component::types::ComponentFunc, |
There was a problem hiding this comment.
Given that all the validation is happening at this point, I am wondering what do we gain with the t.wrap approach. Couldn't we drop it entirely and rely on the type information present in wasmtime::component::types::ComponentFunc for validation when calling wrapped_to_component_val?
There was a problem hiding this comment.
Yes, I think you are right. Removing the t.wrap approach even means we can rely on the existing component_val_to_rb instead of the new wrapped_to_component_val.
The one caveat is that component_val_to_rb needs a StoreContextValue which is currently unused, and this is not available in the component function closure. I changed it into acceping an Option instead, so that it is possible to supply None. I assume that the StoreContextValue is intended for use in the future? Does it make sense to switch to an Option?
d4da436 to
64e411e
Compare
Fixes #433
This PR introduces host functions for components, with strict type validation such that if the function signatures do not match between what the component expects and the host function, an error would be raised. Let me know what you think about the direction!