Expose component model in C api #9812

lanfeust69 · 2024-12-13T11:17:42Z

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...

Step 1 : * Component/Linker * Value * Calls into the component

rockwotj · 2024-12-13T12:56:02Z

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.

lanfeust69 · 2024-12-13T16:24:39Z

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.

dicej · 2024-12-13T17:02:58Z

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?

fitzgen · 2024-12-13T17:54:57Z

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.

alexcrichton

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.

alexcrichton · 2024-12-13T19:42:54Z

crates/c-api/Cargo.toml

@@ -49,6 +49,7 @@ logging = ['dep:env_logger']
 disable-logging = ["log/max_level_off", "tracing/max_level_off"]
 coredump = ["wasmtime/coredump"]
 addr2line = ["wasmtime/addr2line"]
+component-model = ["wasmtime/component-model", "cranelift"]


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?

alexcrichton · 2024-12-13T19:43:41Z

crates/c-api/include/wasmtime/component.h

+/**
+ * \brief Whether or not to enable support for the component model in
+ * Wasmtime.
+ *
+ * For more information see the Rust documentation at
+ * https://docs.wasmtime.dev/api/wasmtime/struct.Config.html#method.wasm_component_model
+ */
+WASMTIME_CONFIG_PROP(void, component_model, bool)


Could this be moved to conifg.h?

(and also probably gated by WASMTIME_FEATURE_COMPONENT_MODEL

alexcrichton · 2024-12-13T19:44:11Z

crates/c-api/include/wasmtime/component.h

+#ifndef WASMTIME_COMPONENT_H
+#define WASMTIME_COMPONENT_H


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.

alexcrichton · 2024-12-13T19:47:13Z

crates/c-api/include/wasmtime/component.h

+/// A variable sized bitset.
+WASMTIME_COMPONENT_DECLARE_VEC(val_flags, uint32_t);
+WASMTIME_COMPONENT_DECLARE_VEC_NEW(val_flags, uint32_t);


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

alexcrichton · 2024-12-13T19:54:36Z

crates/c-api/include/wasmtime/component.h

+/// Representation of a component in the component model.
+typedef struct wasmtime_component_t wasmtime_component_t;
+
+/**
+ * \brief Compiles a WebAssembly binary into a #wasmtime_component_t
+ *
+ * This function will compile a WebAssembly binary into an owned
+ #wasmtime_component_t.
+ *
+ * It requires a component binary, such as what is produced by Rust `cargo
+ component` tooling.
+ *
+ * This function does not take ownership of any of its arguments, but the
+ * returned error and component are owned by the caller.
+
+ * \param engine the #wasm_engine_t that will create the component
+ * \param buf the address of the buffer containing the WebAssembly binary
+ * \param len the length of the buffer containing the WebAssembly binary
+ * \param component_out on success, contains the address of the created
+ *        component
+ *
+ * \return NULL on success, else a #wasmtime_error_t describing the error
+ */
+wasmtime_error_t *
+wasmtime_component_from_binary(const wasm_engine_t *engine, const uint8_t *buf,
+                               size_t len,
+                               wasmtime_component_t **component_out);
+
+/**
+ * \brief Deletes a #wasmtime_component_t created by
+ * #wasmtime_component_from_binary
+ *
+ * \param component the component to delete
+ */
+void wasmtime_component_delete(wasmtime_component_t *component);


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.

Makes sense, I'll look at this.

alexcrichton · 2024-12-13T19:55:08Z

crates/c-api/include/wasmtime/component.h

+// declaration from store.h
+typedef struct wasmtime_context wasmtime_context_t;


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

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 ?

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.

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

alexcrichton · 2024-12-13T19:57:09Z

crates/c-api/include/wasmtime/component.h

+/**
+ * \brief Builds the linker, providing the host functions defined by calls to
+ * #wasmtime_component_linker_define_func
+ *
+ * \param linker the #wasmtime_component_linker_t to build
+ *
+ * \return wasmtime_error_t* On success `NULL` is returned, otherwise an error
+ *         is returned which describes why the build failed.
+ */
+wasmtime_error_t *
+wasmtime_component_linker_build(wasmtime_component_linker_t *linker);


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?

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.

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)

alexcrichton · 2024-12-13T19:58:17Z

crates/c-api/include/wasmtime/component.h

+void wasmtime_component_linker_delete(wasmtime_component_linker_t *linker);
+
+/// Representation of a component instance
+typedef struct wasmtime_component_instance_t wasmtime_component_instance_t;


Is there a missing wasmtime_component_instance_delete?

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.

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

alexcrichton · 2024-12-13T19:58:34Z

crates/c-api/include/wasmtime/component.h

+    wasmtime_component_instance_t **instance_out);
+
+/// Representation of an exported function in Wasmtime component model.
+typedef struct wasmtime_component_func_t wasmtime_component_func_t;


Is there a missing wasmtime_component_func_delete?

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.

alexcrichton · 2024-12-13T19:59:48Z

crates/c-api/include/wasmtime/component.h

+bool wasmtime_component_instance_get_func(
+    const wasmtime_component_instance_t *instance, wasmtime_context_t *context,
+    const char *name, size_t name_len, wasmtime_component_func_t **item_out);


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.

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.

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.

lanfeust69 · 2024-12-13T20:57:00Z

@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 😉.

lanfeust69 · 2024-12-13T21:00:15Z

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.

alexcrichton · 2024-12-17T18:54:13Z

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.

rockwotj and others added 3 commits December 10, 2024 14:16

Expose component-model to c-api

66c1d4e

Step 1 : * Component/Linker * Value * Calls into the component

Allow definition of host callbacks in c-api component-model

e1d1880

Add documentation and example

9dbce5c

lanfeust69 requested a review from a team as a code owner December 13, 2024 11:17

lanfeust69 requested review from fitzgen and removed request for a team December 13, 2024 11:17

github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. labels Dec 13, 2024

fitzgen requested review from alexcrichton and removed request for fitzgen December 13, 2024 13:46

lanfeust69 force-pushed the component_model_c_api branch from d6c996f to 25a626c Compare December 13, 2024 16:55

lanfeust69 force-pushed the component_model_c_api branch from 25a626c to a9c5dbb Compare December 13, 2024 17:14

Fix CI

f14d457

lanfeust69 force-pushed the component_model_c_api branch from a9c5dbb to f14d457 Compare December 13, 2024 17:21

alexcrichton reviewed Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose component model in C api #9812

Expose component model in C api #9812

lanfeust69 commented Dec 13, 2024

rockwotj commented Dec 13, 2024

lanfeust69 commented Dec 13, 2024

dicej commented Dec 13, 2024

fitzgen commented Dec 13, 2024

alexcrichton left a comment

alexcrichton Dec 13, 2024

alexcrichton Dec 13, 2024

alexcrichton Dec 13, 2024

alexcrichton Dec 13, 2024

alexcrichton Dec 13, 2024

lanfeust69 Dec 14, 2024

alexcrichton Dec 13, 2024

lanfeust69 Dec 14, 2024

alexcrichton Dec 17, 2024

rockwotj Dec 17, 2024

alexcrichton Dec 13, 2024

lanfeust69 Dec 14, 2024

alexcrichton Dec 17, 2024

alexcrichton Dec 13, 2024

lanfeust69 Dec 14, 2024

alexcrichton Dec 17, 2024

alexcrichton Dec 13, 2024

lanfeust69 Dec 14, 2024

alexcrichton Dec 13, 2024

lanfeust69 Dec 14, 2024

alexcrichton Dec 17, 2024

lanfeust69 commented Dec 13, 2024

lanfeust69 commented Dec 13, 2024

alexcrichton commented Dec 17, 2024

		// declaration from store.h
		typedef struct wasmtime_context wasmtime_context_t;

		#ifndef WASMTIME_COMPONENT_H
		#define WASMTIME_COMPONENT_H

Expose component model in C api #9812

Are you sure you want to change the base?

Expose component model in C api #9812

Conversation

lanfeust69 commented Dec 13, 2024

rockwotj commented Dec 13, 2024

lanfeust69 commented Dec 13, 2024

dicej commented Dec 13, 2024

fitzgen commented Dec 13, 2024

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanfeust69 commented Dec 13, 2024

lanfeust69 commented Dec 13, 2024

alexcrichton commented Dec 17, 2024