Stream: git-wasmtime

Topic: wasmtime / PR #9812 Expose component model in C api

Wasmtime GitHub notifications bot (Dec 13 2024 at 11:17):

lanfeust69 opened PR #9812 from klave-network:component_model_c_api to bytecodealliance:main:

The goal is to be able to host wasm components in a wasmtime runtime embedded through the C api.

This was discussed in issue #8036.

Thanks to @rockwotj for the basis of the initial commit, in #7801.

I didn't see tests specific to the c-api, currently only tested through the example (and in our internal use !).

I'm not sure it the sizeable component.rs file should be split...

Wasmtime GitHub notifications bot (Dec 13 2024 at 11:17):

lanfeust69 requested fitzgen for a review on PR #9812.

Wasmtime GitHub notifications bot (Dec 13 2024 at 11:17):

lanfeust69 requested wasmtime-core-reviewers for a review on PR #9812.

Wasmtime GitHub notifications bot (Dec 13 2024 at 12:56):

rockwotj commented on PR #9812:

Nice work! BTW one of the more complex things I had punted on in my POC was handling resources. From my conversations with @alexcrichton those significantly changed the Rust API when adding them, and might do the same in the C API (although maybe this has changed in the time since I did the POC?). I notice there is no resource support in this PR - it's probably worth a plan for that. At least that was the sentiment prior.

Wasmtime GitHub notifications bot (Dec 13 2024 at 13:46):

fitzgen requested alexcrichton for a review on PR #9812.

Wasmtime GitHub notifications bot (Dec 13 2024 at 16:24):

lanfeust69 commented on PR #9812:

Nice work! BTW one of the more complex things I had punted on in my POC was handling resources. From my conversations with @alexcrichton those significantly changed the Rust API when adding them, and might do the same in the C API (although maybe this has changed in the time since I did the POC?). I notice there is no resource support in this PR - it's probably worth a plan for that. At least that was the sentiment prior.

Indeed, having started from your change, and not needing resources on my side, I decided not to support that for now, but I should have made that clear.

I'm currently looking at the CI failures :

• I've been "fooled" by clang-format, because there is no .clang-format file in the repo, so my default one would be picked up, with a significantly different formatting, and I assumed there was no strict formatting rules for C files.
• Not sure if a function only used with some features should be marked #[allow(dead_code)] (which may miss the time it is no longer used) or #[cfg(any(feature = "feature1", feature = "feature2"))] (which gets unwieldy, and may prevent discoverability if useful elsewhere)
• CI (reasonably) wants to run the C example for component, but the workaround in component/main.rs is not available, something else is needed.

Wasmtime GitHub notifications bot (Dec 13 2024 at 16:51):

lanfeust69 updated PR #9812.

Wasmtime GitHub notifications bot (Dec 13 2024 at 16:55):

lanfeust69 updated PR #9812.

Wasmtime GitHub notifications bot (Dec 13 2024 at 17:03):

dicej commented on PR #9812:

Regarding resources: we'll definitely need to support them eventually, given that they're already used heavily by WASIp2 and beyond; plus they're just really useful in general. This PR doesn't need to address that, but we should start thinking about what a follow-up PR might look like.

A minimal implementation would include:

• A way to add new host-implemented resource types to a LinkerInstance, equivalent to https://docs.rs/wasmtime/latest/wasmtime/component/struct.LinkerInstance.html#method.resource
• A C equivalent to (https://docs.rs/wasmtime/latest/wasmtime/component/struct.ResourceTable.html) for allocating host resource reps (as uint32_t) and pairing them with resource instances (as void*, which may be cast to the expected type; maybe paired with a type ID for runtime sanity checking)

Otherwise, I think it's just a matter of passing the uint32_t reps (representing either host-implemented or guest-implemented resource instances) back and forth between the host and guest, letting Wasmtime handle the details of lifting and lowering. @alexcrichton does that sound plausible to you?

Wasmtime GitHub notifications bot (Dec 13 2024 at 17:14):

lanfeust69 updated PR #9812.

Wasmtime GitHub notifications bot (Dec 13 2024 at 17:21):

lanfeust69 updated PR #9812.

Wasmtime GitHub notifications bot (Dec 13 2024 at 17:54):

fitzgen commented on PR #9812:

Not sure if a function only used with some features should be marked #[allow(dead_code)] (which may miss the time it is no longer used) or #[cfg(any(feature = "feature1", feature = "feature2"))] (which gets unwieldy, and may prevent discoverability if useful elsewhere)

We generally prefer the cfg-based approach, even though it can get unweildy, because it provides better compile times and code sizes. Discoverability is not an issue because we build docs for docs.rs with feature(doc_auto_cfg) which automatically adds "this method depends on blah feature" to all cfged functions.

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:10):

alexcrichton submitted PR review:

Thanks so much for your work here on this! I've left a few comments below, but I want to also gauge a bit on how much time/effort you have for this. This is a pretty big PR and there's quite a lot to bikeshed here, so I'd personally prefer to take this one-piece-at-a-time and perhaps break this PR up into a few smaller PRs. Would you have time for that? For example I'd like to land the "compile a component" first, then perhaps "define a function in a linker and instantiate a component", then "load a function from a component", then finally "call the component function". I realize that it wouldn't actually be useful until the end, but I think it'd help to talk about a piece-at-a-time since this is a pretty major feature being added.

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:10):

alexcrichton created PR review comment:

Could this be moved to conifg.h?

(and also probably gated by WASMTIME_FEATURE_COMPONENT_MODEL

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:10):

alexcrichton created PR review comment:

Ideally the component-model feature wouldn't necessarily imply cranelift, so would it be ok to gate some functions (e.g. component compilation) behind both features?

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:10):

alexcrichton created PR review comment:

One thing I might recommend if you're up for it is to organize component model support the same way as the core wasm support. For example in the wasmtime crate there's wasmtime::Foo for core wasm things and wasmtime::component::Foo for component-model things, basically everything component-related is in its own namespace.

For the C API we've got wasmtime/foo.h for a specific core-wasm thing and wasmtime.h for "everything core wasm". For components what do you think about wasmtime/component/foo.h for a specific item (e.g. this part here would be wasmtime/component/component.h like wasmtime/module.h) and then there'd be wasmtime/component.h which would include all the component headers.

While this header isn't too too big just yet I could imagine it becoming much larger over time as it supports more features of the component model.

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:10):

alexcrichton created PR review comment:

Wasmtime's API doesn't quite reflect this yet but we're moving in a direction where flags is just a 64-bit integer, so I think it's reasonable to go ahead and define this on the C side as a small struct with a uint64_t member

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:10):

alexcrichton created PR review comment:

In addition to this #ifdef guard I think this whole file will want to be gated on WASMTIME_FEATURE_COMPONENT_MODEL since it'll otherwise not be available when that feature is disabled.

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:10):

alexcrichton created PR review comment:

One thing I might recommend doing here is to match the get_export style from the Rust API where lookup-by-name isn't what's done in the component model exactly but rather it's "lookup within this optional instance this name" which return "an exported thing" and then the "exported thing" can be downcast into a function. That way this can handle nested exports in instances and such.

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:10):

alexcrichton created PR review comment:

I'm still only reviewing the header file so far, but I'm a bit confused by this in the sense that a linker at least in Rust doesn't have a "build" operation, it can just always be used to instantiate after an item is added to the linker. Given that I'm not sure what this is doing internally and what the expected contract would be with respect to when callers need to use this in relation to instantiate below?

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:10):

alexcrichton created PR review comment:

Is there a missing wasmtime_component_instance_delete?

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:10):

alexcrichton created PR review comment:

Is there a missing wasmtime_component_func_delete?

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:10):

alexcrichton created PR review comment:

Could this be removed in favor of a #include <wasmtime/store.h>?

Wasmtime GitHub notifications bot (Dec 13 2024 at 20:57):

lanfeust69 commented on PR #9812:

@alexcrichton : Thanks for a very quick first look !

I personally prefer sizeable PR broken down through meaningful commits (because of the logical coupling, an oversight in an earlier PR may only be really visible when used later on), but unfortunately GitHub (as well as GitLab) make the review process much more PR/MR oriented than commit-oriented, which kind of defeats this approach. I'll try to see if I can find a split that makes sense to me (without looking closely, I have a feeling that some of the steps you suggest might be too small, at least compared to others).

I'll try to answer your code-related comments tomorrow. I'm reasonably confident I'll eventually be able to actually address what should be done, though I won't make any promise on the timeline :wink:.

Wasmtime GitHub notifications bot (Dec 13 2024 at 21:00):

lanfeust69 commented on PR #9812:

Not sure if a function only used with some features should be marked #[allow(dead_code)] (which may miss the time it is no longer used) or #[cfg(any(feature = "feature1", feature = "feature2"))] (which gets unwieldy, and may prevent discoverability if useful elsewhere)

We generally prefer the cfg-based approach, even though it can get unweildy, because it provides better compile times and code sizes. Discoverability is not an issue because we build docs for docs.rs with feature(doc_auto_cfg) which automatically adds "this method depends on blah feature" to all cfged functions.

Well, clippy barked at the allow(dead_code) version, so I ended up using the cfg-based version.

Wasmtime GitHub notifications bot (Dec 14 2024 at 14:52):

lanfeust69 submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2024 at 14:52):

lanfeust69 created PR review comment:

I tend to favor forward declarations when possible, mostly for build time reasons (more to include is slower, and changes in the included header imply a re-build that could be avoided). What are the reasons for preferring a full #include ?

Wasmtime GitHub notifications bot (Dec 14 2024 at 15:12):

lanfeust69 created PR review comment:

This was the way I tried to "hide" the difference between Linker and LinkerInstance, the latter didn't seem useful to expose to the C api. But in order to add host functions to the linker, you need the appropriate LinkerInstance, and that can only be created once (through .root() of .instance(name)). An alternative could be to keep the LinkerInstances around in a map, or to provide a lookup mechanism to Linker.

Wasmtime GitHub notifications bot (Dec 14 2024 at 15:12):

lanfeust69 submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2024 at 15:21):

lanfeust69 submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2024 at 15:21):

lanfeust69 created PR review comment:

Unless I missed something (very possible !) these instances are always created inside a store, belong to it, and will be deleted when the store goes down.

Wasmtime GitHub notifications bot (Dec 14 2024 at 15:29):

lanfeust69 submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2024 at 15:29):

lanfeust69 created PR review comment:

Possibly, I assumed get_func would return some kind of handle to existing data, but even (not so sure of that looking at the code) there is still the C wrapper to delete.

Wasmtime GitHub notifications bot (Dec 14 2024 at 15:30):

lanfeust69 submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2024 at 15:30):

lanfeust69 created PR review comment:

Makes sense, I'll look at this.

Wasmtime GitHub notifications bot (Dec 14 2024 at 15:42):

lanfeust69 submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2024 at 15:42):

lanfeust69 created PR review comment:

Indeed, as this was part of the initial POC I started from, and matched my needs, I didn't look much more at the underlying structures on the Rust side. This may be a bit similar to my attempt to "hide" the LinkerInstances nested structures within the Linker.

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:49):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:49):

alexcrichton created PR review comment:

AFAIK build-time concerns matter a lot for C++ but far less so for C, so I'd prefer to not duplicate definitions because structures like this can change over time.

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:50):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:50):

alexcrichton created PR review comment:

FWIW I think it's fine to change the Rust APIs to better accomodate the C APIs if necessary, but otherwise I'd prefer to keep the two relatively in-sync to make them more managable to maintain, so having a LinkerInstance equivalent in the C API I think would be reasonable (or changing the Rust API to not be based on LinkerInstance)

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:51):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:51):

alexcrichton created PR review comment:

That's true yeah, but as an opaque pointer isn't this allocating something to make it pointer-sized in the implementation?

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:52):

alexcrichton created PR review comment:

Yeah I can see how this works for getting top-level exports, but ideally we'll want to at least set this API design up for extending to the entire component model even if it initially doesn't support it. In that sense I think it'd be good to model the nested instance structure of exports here.

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:53):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:53):

rockwotj created PR review comment:

Plus one - C is super cheap to build and this is a trivial amount of just declarations.

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:54):

alexcrichton commented on PR #9812:

Personally I'd prefer to split this into separate PRs if you're ok with that because there's a fair amount to discuss here and it's easy to lose track of discussions when it's all in one PR. I agree this is an unfortunate artifact of using github, but there's not a whole lot we can do about that other than work around it.

Last updated: Dec 23 2024 at 12:05 UTC