dicej edited PR #5934 from wit-templates-minimal
to main
:
templates, allowing WIT files to define interfaces which contain a single wildcard function, which worlds may import or export.
I've taken a "minimalist" approach to this, such that the host binding generator just skips wildcards entirely, leaving it to the host to use dynamic APIs to reflect on the component and do the necessary func_wrap (for imports) and typed_func (for exports) calls directly.
This adds two new functions to the public API:
Component::names
: provides an iterator over the names of the functions imported by the specified instance.ExportInstance::funcs
: provides an iterator over the names of the functions exported by this instance.In both cases, I'm open to alternative API designs for getting that information.
Note that I've added a temporary dependency override to Cargo.toml, pointing to our fork of wasm-tools, which includes the necessary wit-parser changes. We're still iterating on that and will PR those changes separately. We also have a fork of wit-bindgen with a new "wildcards" test to verify everything works end-to-end: https://github.com/bytecodealliance/wit-bindgen/compare/main...dicej:wit-templates. I'll PR that last once everything else is stable.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
dicej edited PR #5934 from wit-templates-minimal
to main
:
Per WebAssembly/component-model#172, this implements "part 1" of WIT templates, allowing WIT files to define interfaces which contain a single wildcard function, which worlds may import or export.
I've taken a "minimalist" approach to this, such that the host binding generator just skips wildcards entirely, leaving it to the host to use dynamic APIs to reflect on the component and do the necessary func_wrap (for imports) and typed_func (for exports) calls directly.
This adds two new functions to the public API:
Component::names
: provides an iterator over the names of the functions imported by the specified instance.ExportInstance::funcs
: provides an iterator over the names of the functions exported by this instance.In both cases, I'm open to alternative API designs for getting that information.
Note that I've added a temporary dependency override to Cargo.toml, pointing to our fork of wasm-tools, which includes the necessary wit-parser changes. We're still iterating on that and will PR those changes separately. We also have a fork of wit-bindgen with a new "wildcards" test to verify everything works end-to-end: https://github.com/bytecodealliance/wit-bindgen/compare/main...dicej:wit-templates. I'll PR that last once everything else is stable.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
dicej updated PR #5934 from wit-templates-minimal
to main
.
alexcrichton submitted PR review.
dicej updated PR #5934 from wit-templates-minimal
to main
.
dicej updated PR #5934 from wit-templates-minimal
to main
.
dicej updated PR #5934 from wit-templates-minimal
to main
.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Mind adding a note somewhere in/around this function to flesh this out in the future? Ideally there'd be a whole type-reflection-style API for components rather than just this specific accessor (which is pretty specialized to just star-imports), but it's not required for this PR naturally
alexcrichton created PR review comment:
If you'd like, the
crates/component-macro/tests/codegen
folder has a bunch of*.wit
files where all of them get bindings generated in a few different flavors, includingasync
, which is useful for "I just want to make sure the generated code compiles" and you could add some stuff there too.
alexcrichton created PR review comment:
Do y'all have a concrete use case for wildcard exports right now? I ask because if possible I'd like to be conservative and not have them supported. If you need them though, that's fine of course!
The other reason I bring this up is that so far we've avoided baking in hashmap-based string lookups in the component model in Wasmtime and in various support systems. That could sort of be fixed here maybe with something like more generics or similar but it may also be inevitable to some degree. Ideally though the
HashMap
here could be avoided along with the string lookups at runtime for invoking methods.
alexcrichton created PR review comment:
While this isn't part of Wasmtime itself I think that the representation here might be better to avoid using
*
as the function's name and having it as a portion of thefunctions
map. I'm worried about generators all having to haveif
clauses on the name being a bit brittle and easy to forget.What about something like
wildcard: Option<Function>
inside ofstruct Interface
? (which you may have very well already considered and rejected!)
alexcrichton created PR review comment:
I would recommend adding some codegen tests for wildcards with "interesting" types as I think that a wildcard function which takes a
string
parameter might break this since it'll be printed as&str
which I think will complain about not having the right lifetime. (this is the motivation for storing aFunc
rather than aTypedFunc
in these structures)
alexcrichton created PR review comment:
Do you think it's worth trying to ferry along the
name: &str
here as well?
dicej submitted PR review.
dicej created PR review comment:
We don't have an immediate need for them yet, no. I implemented them mainly because they were in scope for https://github.com/WebAssembly/component-model/issues/172. I also just wanted some experience implementing them to surface any issues that might require design work.
Note that the
__wildcard_matches
variable is private, so we're not committing to usingHashMap
forever. If we want to get rid of it now, though, we could instead haveinstantiate{_pre}
take aVec<&str>
containing the names of each export the host wants access to and return aVec<WildcardMatch>
. Would you prefer that? I'm open to other ideas.I'd rather not postpone implementing wildcard exports altogether, though. We've got momentum now, so I'd rather just get it done than come back to it later.
dicej submitted PR review.
dicej created PR review comment:
Absolutely agreed. @fibonacci1729 and I were already planning to do that, so we'll make sure it happens.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Ok keeping the implementation makes sense. I think it's fine to keep this in the back of our heads though and possibly defer to later. The issue isn't so much with the
HashMap
itself but moreso that "some string-related lookup is happening" during instantiation and execution. That's not to say that _zero_ happen today when using Wasmtime though since you're already looking up exports via string lookups, so while it would be nice to avoid I think keeping the simpler interface you've got going here is the way to start.We can always revisit this later if necessary.
dicej submitted PR review.
dicej created PR review comment:
Sounds good. I'll leave it as is for now, but one other thought while we're on the topic: we could change the
HashMap
to aVec<WildcardMatch>
, add afn name() -> &str
accessor toWildcardMatch
, and changefn get_wildcard_match(&self, name: &str) -> Option<&WildcardMatch>
toget_wildcard_matches(&self) -> &[Wildcard]
. Then there's no string lookup anywhere, but if the embedder _wants_ to look them up by name, they can always collect the&[Wildcard]
into aHashMap
themselves.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I think that your original idea is probably the way to go in the long run where the embedder presents a list of "I expect these things" and then later you get a list in the same order. Otherwise if simply presented a list of what a component instance has an embedder would have to do a bunch of string comparisons to figure out which one is the one they want.
In the interim I think it's reasonable to preserve the functionality of "invoke this specific export" in some sort of mostly-ergonomic fashion, aka keeping the hash map for that.
dicej submitted PR review.
dicej created PR review comment:
I'm just thinking my original idea only moves the string lookup elsewhere -- the code that translates a
&[&str]
to aVec<WildcardMatch>
would itself need to do a lookup for each name in order to populate the result.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Oh so string lookups at
WildcardMatch
time I think is fine, it's just that after you have anInstancePre<T>
you should theoretically absolutely minimize string lookups since that's the thing which is repeatedly instantiated and is perf-sensitive.So yeah what you proposed I think would work, adding
fn name() -> &str
and then returning the same-ordered list. To reiterate though it's not necessarily critical to fix this now as I'm sure there's other more pressing things to take care of.
dicej updated PR #5934 from wit-templates-minimal
to main
.
alexcrichton submitted PR review.
dicej updated PR #5934 from wit-templates-minimal
to main
.
dicej updated PR #5934 from wit-templates-minimal
to main
.
dicej updated PR #5934 from wit-templates-minimal
to main
.
dicej updated PR #5934 from wit-templates-minimal
to main
.
dicej updated PR #5934 from wit-templates-minimal
to main
.
dicej updated PR #5934 from wit-templates-minimal
to main
.
Last updated: Dec 23 2024 at 12:05 UTC