-
Notifications
You must be signed in to change notification settings - Fork 21
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
Initial support for components #388
Conversation
@jbourassa Awesome! 🎉 Maybe that'll come later, but I think just 2-3 simple examples in Ruby code on how to use it (that would explain what the interface looks like) might help to provide some feedback. I've read the yard docs, so I think I get the main points, but an example would still be helpful. Thoughts
|
Fair point. There are tests and a benchmark that show the gist of it, but they're not doing much. I'll add a better examples once I knock out some of the meatier features.
Enums are more like C enums, example from the WIT explainer:
Data wouldn't be a good fit here, but it'd work perfectly for records. The gotcha is that we need some kind of WIT-based bindgen to generate & attach the data class to a specific Wasm type. I haven't started looking into bindgen yet, but I suspect it's quite a bit of work. |
Thanks @jbourassa! We have a great use-case for this coming up within a few months (I hope), so eager to try this out in a little while. |
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.
On a first pass this all looks very reasonable to me. I left a couple of thoughts/questions, I wouldn't consider any of them as blocking, if you're good with it we can consider landing this change as-is and continue iterating in-tree.
Once it's in main, it'll be released in the next version. I'm not sure it's good enough to be released: missing features, potentially unstable API, etc. The main con of iterating here is the review burden: we'll end up with one very large PR. I see a couple options:
I think option 2 (experimental mention in the doc) along with some more polish on this PR is reasonable, but I'm okay with other options too. |
Right -- I think that if we're not comfortable with the state of this PR for it to be available by default, instead of having to mark the entirety of the components API as experimental I'm leaning toward option 3 (this should be trivial in Rust, but I'm unsure about Ruby). The reason that I'm leaning toward 3 is that once we feel the implementation is good enough, it'll be easier to mark it as such, rather than having to audit and update the entire components API surface. |
84a0fd2
to
391d2ad
Compare
Dug into it today and unfortunately it's not trivial, at least if we want to do it well. Some of the problems we have to solve:
All in all, it doesn't seem worth it, no?
How does a feature flag avoid the need to audit the API? The way I see it, removing the flag is akin to dropping the doc mention or merging the feature branch in main. None of the options guarantees the APIs is good, and none prevent thorough PR reviews. I think I can get to a place where the API is good enough soon, once the remaining types and host-defined functions are implemented. I'm sure there will still be breaking changes, but nothing too difficult to handle. |
Assuming I understood point 2 correctly, I'd imagine that in order to move us out of "work-in-progress" state we'd need to un-mark all the methods in the component API documentation as unstable; with a flag on the other hand it's mostly a matter of enabling it by default? This is mostly to avoid toil on our end. Edit (clicked too soon the comment button)
I assumed that this was going to be easier, at least from Rust's perspective, but I do agree that working on all the steps that you've outlined is probably not worth it and probably the easiest is option 2. |
Oh, I see. I was thinking a single mention on the top level I guess another pro of that approach is that it's easier for people to provide feedback. |
391d2ad
to
18126d4
Compare
The component has identity function for all Wasm types, but could be extended for various testing needs.
18126d4
to
5f4577c
Compare
Initial stab at component support. I'm sharing this work for early feedback and in case it's useful; it's not quite ready (edit: we'll iterate in main). Any feedback is welcomed!
Disclaimer: I am learning about the component model, there are most likely still gaps in my understanding.
Current state
What it supports:
TODOs:
Design choices
The bindings wrap
wasmtime::component::Val
out of necessity & simplicityComponent model types are mapped to Ruby types as much as possible, taking into account what
wasmtime::component::Val
allowsrecord
:Hash
withString
keys¹option<T>
: eithernil
orT
tuple<T, U>
:Array
([T, U]
)result<T, E>
:Edit: given how result can be nested, raising an exception is not a good fit.T
or to aWasmtime::Component::ResultError
exception wrappingE
.Updated proposal:
Wasmtime::Component::Result
mimicking a RustResult
(to be implemented)flags
:Array<String>
¹enum
:String
¹ (to be implemented)variant
:Wasmtime::Component::Variant
wrapping the kind (String
¹) and the optional value (T
ornil
) (to be implemented)"3"
does not get coerced to a component modelu32
,1
does not get coerced to a component modelbool
, etc.¹: The
Val
union often has ownedString
s, converting those (e.g. intoSymbol
s) is an additional performance hit. If we had WIT-based codegen, we could pre-build Ruby objects for types once in the linker. At runtime, any components from that linker could re-use these objects. Examples of such objects: RubySymbol
s for componentenum
s andvariant
s,Data
class for componentrecord
s.Performance
We want the Ruby bindings to be very fast. This version leaves some performance on the table due to the usage of
wasmtime::component::Val
:Val
, then copied again into a Ruby string. This could be avoided (requiring changes to Wasmtime's API IIUC).Val
uses owned-String
s forenums
,variant
,flags
. Ditto, maybe avoided with a different API (but likely not easy).Yet, the current perf might be acceptable for an initial release. Benchmarking a no-op function that round trips a point record (
{ "x" => u32, "y" => u32 }
), on my M2 MacBook Air:For comparison, core Wasm:
On those examples, the overhead for core Wasm is ~200ns vs 900ns for a component call that round-trips a 2-fields record.