wasmtime / PR #5934 implement "part 1" of WIT Templates · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #5934 implement "part 1" of WIT Templates

Wasmtime GitHub notifications bot (Mar 05 2023 at 22:33):

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.

Wasmtime GitHub notifications bot (Mar 05 2023 at 22:34):

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.

Wasmtime GitHub notifications bot (Mar 05 2023 at 22:34):

dicej updated PR #5934 from wit-templates-minimal to main.

Wasmtime GitHub notifications bot (Mar 06 2023 at 16:38):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 01:43):

dicej updated PR #5934 from wit-templates-minimal to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 01:49):

dicej updated PR #5934 from wit-templates-minimal to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 01:55):

dicej updated PR #5934 from wit-templates-minimal to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:10):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:10):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:10):

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

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:10):

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, including async, which is useful for "I just want to make sure the generated code compiles" and you could add some stuff there too.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:10):

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.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:10):

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 the functions map. I'm worried about generators all having to have if clauses on the name being a bit brittle and easy to forget.

What about something like wildcard: Option<Function> inside of struct Interface? (which you may have very well already considered and rejected!)

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:10):

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 a Func rather than a TypedFunc in these structures)

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:10):

alexcrichton created PR review comment:

Do you think it's worth trying to ferry along the name: &str here as well?

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:29):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:29):

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 using HashMap forever. If we want to get rid of it now, though, we could instead have instantiate{_pre} take a Vec<&str> containing the names of each export the host wants access to and return a Vec<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.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:31):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:31):

dicej created PR review comment:

Absolutely agreed. @fibonacci1729 and I were already planning to do that, so we'll make sure it happens.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:35):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:35):

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.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:53):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:53):

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 a Vec<WildcardMatch>, add a fn name() -> &str accessor to WildcardMatch, and change fn get_wildcard_match(&self, name: &str) -> Option<&WildcardMatch> to get_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 a HashMap themselves.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:01):

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.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:15):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:15):

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 a Vec<WildcardMatch> would itself need to do a lookup for each name in order to populate the result.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:25):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:25):

alexcrichton created PR review comment:

Oh so string lookups at WildcardMatch time I think is fine, it's just that after you have an InstancePre<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.

Wasmtime GitHub notifications bot (Mar 07 2023 at 23:52):

dicej updated PR #5934 from wit-templates-minimal to main.

Wasmtime GitHub notifications bot (Mar 08 2023 at 14:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 20 2023 at 15:49):

dicej updated PR #5934 from wit-templates-minimal to main.

Wasmtime GitHub notifications bot (Mar 20 2023 at 16:12):

dicej updated PR #5934 from wit-templates-minimal to main.

Wasmtime GitHub notifications bot (Mar 20 2023 at 16:25):

dicej updated PR #5934 from wit-templates-minimal to main.

Wasmtime GitHub notifications bot (Mar 22 2023 at 21:44):

dicej updated PR #5934 from wit-templates-minimal to main.

Wasmtime GitHub notifications bot (Mar 22 2023 at 22:00):

dicej updated PR #5934 from wit-templates-minimal to main.

Wasmtime GitHub notifications bot (Mar 22 2023 at 22:02):

dicej updated PR #5934 from wit-templates-minimal to main.

Last updated: Nov 22 2024 at 17:03 UTC