-
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
Replace Instance#invoke
with Func#call
#399
Conversation
let mut results = vec![wasmtime::component::Val::Bool(false); results_ty.len()]; | ||
let params = convert_params(store, &func.params(store.context()?), args)?; | ||
let params = convert_params( | ||
&store_context_value, |
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.
I noticed Caller
doesn't exist for Wasm components, so I think this can be a plain &Store
instead of a &StoreContextValue
. But I'll get to host calls first before making the change, in case I'm missing something.
Edit: even if Caller
itself no longer exists, the host calls get passed a StoreContextMut
, so the "Store or Caller" abstraction is still necessary.
ad4aba7
to
074fb4d
Compare
Following the rust crate API, `Instance#invoke` is removed and replaced with 2 calls: `Instance#get_func` and `Func#call`. Other notable changes: - Wasm component types <-> Ruby conversion is now documented under `Func` - `Instance#get_func` supports nested exports by accepting an array of strings. ```ruby instance.get_func("foo") # looks up the `foo` export instance.get_func(["bar", "baz"]) # looks up the `bar` export, then `baz` ``` This API can be extended later to accept an instance of `ExportIndex` to avoid the cost of comparing exports by name each time, like the Rust API. For example: ```ruby index = component.get_export("foo") # or get_export(["bar", "baz"]) # => #<Wasmtime::Component::ComponentExportIndex ...> instance.get_func(index) ```
074fb4d
to
834e57a
Compare
/// Ruby +Array+. | ||
/// tuple:: | ||
/// Ruby +Array+ of the same size of tuple. Example: +tuple<T, U>+ would be converted to +[T, U]+. | ||
/// record:: |
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.
If I'm not wrong there was a valid reason not to use symbols, right? In case I'm remember this correctly, would it make sense to explicitly document it?
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 main reason is that creating a non-interned symbol is more work: you first need a RString
and then intern it. Now I didn't benchmark (taking a note to do it).
Ideally we'd create symbols once and cache them for a record type, but I don't see how it can be done. We don't have anything that's "stable" to attach the symbols cache to. wasmtime::component::types::Record
is specific to a given component.
The way to fix that, I think, would be to codegen for a WIT file, and convert to/from those codegen'd types.
Now that's long version of the explaination. The short version might be "for performance (see issue # for details)".
Thinking about it some more, I don't feel like string keys are a wart to the API. The JSON gem returns string keys by default, so does the postgre gem, etc. For anything that's looks dynamic, strings "feel" normal to me. Whether component model records are dynamic depends on how you look at it; a case could be made either way.
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.
I created #400 to track this. I'll go ahead and merge as is so that we get this API in before the next release, given Wasmtime 27 was just released.
Following the rust crate API,
Instance#invoke
is removed and replaced with 2 calls:Instance#get_func
andFunc#call
.Other notable changes:
Func
. I ran into Yard limitations when trying to style certain things, but I think it renders well enough:Instance#get_func
supports nested exports by accepting an array of strings.This API can be extended later to accept an instance of
ExportIndex
to avoid the cost of comparing exports by name each time, like the Rust API. For example: