-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
c-api/component-model: proof of concept #7801
Conversation
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
#[repr(C)] | ||
#[derive(Clone)] | ||
pub struct wasmtime_component_val_variant_t { | ||
pub index: u32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Rust API uses the string name as the interface to which discriminant is chosen, personally I think it's more ergonomic in the C-API to use the index, and that's the underlying value stored in wasmtime::component::Val
anyways.
#[repr(C)] | ||
#[derive(Clone)] | ||
pub struct wasmtime_component_val_enum_t { | ||
pub discriminant: u32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Rust API uses the string name as the interface to which discriminant is chosen, personally I think it's more ergonomic in the C-API to use the index, and that's the underlying value stored in wasmtime::component::Val
anyways.
Flags(wasmtime_component_val_flags_t), | ||
} | ||
|
||
impl wasmtime_component_val_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the to/from conversion from wasmtime_component_val_t <-> component::Val are both inefficient and awkward. I would like to either expose new_unchecked
methods for individual Val
subtypes (list, record, etc). These would not do "typechecking", as we're going to have to already do that as we convert. Alternatively, we could upgrade wasmtime_component_val_t
into the wasmtime
crate as component::RawVal
or something and then the conversions could be internal to the crate.
#[test] | ||
fn bit_fiddling() { | ||
let mut flags: wasmtime_component_val_flags_t = Vec::new().into(); | ||
wasmtime_component_val_flags_set(&mut flags, 1, true); | ||
assert!(wasmtime_component_val_flags_test(&flags, 1)); | ||
assert!(!wasmtime_component_val_flags_test(&flags, 0)); | ||
wasmtime_component_val_flags_set(&mut flags, 260, true); | ||
assert!(wasmtime_component_val_flags_test(&flags, 260)); | ||
assert!(!wasmtime_component_val_flags_test(&flags, 261)); | ||
assert!(!wasmtime_component_val_flags_test(&flags, 259)); | ||
assert!(wasmtime_component_val_flags_test(&flags, 1)); | ||
assert!(!wasmtime_component_val_flags_test(&flags, 0)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you mentioned tests lacking for the C-API @alexcrichton can you elaborate what is the preferred way of adding better testing? I see maybe 3 options outside of the "examples" that we have today.
- Add in crate unit tests like here, these would be written in Rust, but using the C-API directly, so would feel like a very C style of Rust.
- Add in tests to the "integration" style of tests in the
tests
top level director for the C-API, but written in Rust using this crate directly. There is probably a variant of this where we use bindgen to regenerate Rust bindings from the headers to also test the headers are typed correctly. - Add new "integration" style of tests written in C (or C++) similar to option 2, but just using the binding headers instead of rust directly.
@@ -722,7 +722,8 @@ impl Type { | |||
} | |||
} | |||
|
|||
fn desc(&self) -> &'static str { | |||
/// Return a string description of this type | |||
pub fn desc(&self) -> &'static str { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be a format
trait implementation?
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:c-api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Thanks for starting on this! I like the idea of the approach here to start with C/C++ APIs and kinda go from there. I want to point out some points though and provide some thoughts on what you've already implemented:
Drawing from the parallels of the C API for wasm there's sort of two modes for values - one where a value is tagged with its type and one where a value is "unchecked" and you assert it's the right type. That might be good to follow with components as well where what you've sketched out so far is the untagged/unchecked version so far (I know there's a tag for a kind but it's more of a discriminant than "this is the type of the value"). That'd roughly correspond to |
Thanks, yes I need to dig into how they work today a bit before going forward.
Agreed, I've not spent time on this yet, but was planning on exposing this in a similar manner to
Could you clarify this? I don't quite follow, there seems to only be the ability to call the dynamic function variants ( If you're talking about having |
Oh you are correct, no The component model is trickier in this regard where it's not as obvious how a list of records with variants of strings should be laid out in memory (compared to a |
Maybe, I mean there would have to be a codegen portion to think about here too. I'm not really sure how passing values back would work in this model. Reguardless, I think that can come in a v2 and I'm going to focus on just supporting the |
Makes sense yeah. In that sense I'd recommend ensuring that type information can be tagged on values at the same time, and that'll help a lot with resources and resolve the string-vs-index problem as well (aka always use an index and then the index is relative to what's in the type info) |
Curiosity: why doesn't C and Golang use a static lib built from the Rust crate? |
It does (from the C API crate) - however all these languages don't have the same ABI, so C is common set of ABI all these languages speak |
Zulip link
This is a proof of concept for the component model exposed in the C API. I mostly took the approach of "how would I want this to look in the C API" and let the rust implementation fall out from that. I think there are certainly improvements that could be made to the implementation that I will point out as review comments.
This is a draft to foster discussion, but I don't expect actually merge this.