add component-model-async/lift.wast test #10083

dicej · 2025-01-22T22:42:54Z

This is another piece of #9582 which I'm splitting out to make review easier.

This test includes two components: one which exports a function using the async-with-callback ABI, and another which uses the async-without-callback ABI. It doesn't actually instantiate or run either component yet.

The rest of the changes fill in some TODOs to make the test pass.

crates/wasmtime/src/runtime/vm/component/states.rs

github-actions · 2025-01-23T01:05:03Z

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "fuzzing", "wasmtime:api", "wasmtime:config"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

github-actions · 2025-01-23T02:06:59Z

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

If you added a new Config method, you wrote extensive documentation for
it.

Our documentation should be of the following form:

Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

# Example

Optional example here.

If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in wasmtime_fuzzing::Config (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See our docs on fuzzing for more details.
If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

alexcrichton

I'm definitely pretty far out of my comfort zone reviewing this in the sense that there's enough pieces that I don't see how they're connected just in this PR that I'm not confident I have the ability to review without going over to the spec. At a high level the test added here seems like it probably doesn't require the lift/lower impls, but then again I suspect that removing those impls would remove all the meat of the PR. Would it be possible to add some tests exercising the impls added here in this PR?

crates/wast/Cargo.toml

alexcrichton · 2025-01-23T23:11:41Z

crates/wasmtime/src/runtime/component/mod.rs

@@ -672,3 +672,158 @@ pub mod bindgen_examples;
 #[cfg(not(any(docsrs, test, doctest)))]
 #[doc(hidden)]
 pub mod bindgen_examples {}
+
+#[cfg(not(feature = "component-model-async"))]
+pub(crate) mod concurrent {


To confirm, this'll eventually be a concurrent.rs in a future PR? So just a placeholder for now?

concurrent.rs is already in main and was added in #10044 (and will be expanded in subsequent PRs). This is the stub module to be used when the component-model-async feature is disabled, so it will only go away when that feature is removed.

Alternatively, I could remove this stub module and make concurrent.rs the only implementation, but that would mean adding more #[cfg(...)] annotations elsewhere, so it's kind of a tradeoff. Happy to do whichever you think is best, though.

Ah I see, sorry I missed the not(...)!

I'll take a closer look in a future PR to see how it fits together 👍

crates/wasmtime/src/runtime/component/values.rs

crates/wasmtime/src/runtime/component/concurrent/futures_and_streams.rs

alexcrichton · 2025-01-23T23:42:58Z

crates/wasmtime/src/runtime/component/concurrent/futures_and_streams.rs


 /// Represents the readable end of a Component Model `future`.
 pub struct FutureReader<T> {
+    rep: u32,


Mind adding some comments as to what this rep is? My naive understanding of component model async is probably showing here but I'm familiar with "rep" of a resource where it's user-supplied but I didn't think that there was anything user-supplied here but rather this is an index to something (but I'm not sure)

I might have the terminology mixed up myself, but I've been using "rep" to mean "the instance-agnostic index for the waitable" which used to look up the state of the stream/future/task in a table (which will be added in a later PR), akin to the ResourceTable index for resources. When lifting and lowering we convert between that index and a per-instance local index visible to the guest.

If there's a more conventional way to refer to these "global" and "local" indexes, I'm happy to adopt it.

How about something like rep: WaitableIndex? Basically using a type here to indicate more clearly what it's pointing to. My history with resources makes me think that "rep" comes from guests themselves and needs to be preserved and handed back to the guest but I don't think that's what's going on here, so this isn't so much a "representation" internally but moreso an index somewhere else, which is where I think a custom newtype might help to distinguish

crates/wasmtime/src/runtime/component/concurrent/futures_and_streams.rs

alexcrichton · 2025-01-23T23:51:47Z

crates/wasmtime/src/runtime/vm/component.rs

+/// Represents the state associated with an error context
+#[derive(Debug, PartialEq, Eq, PartialOrd)]
+pub struct ErrorContextState {
+    /// Debug message associated with the error context
+    pub(crate) debug_msg: String,
+}


This might be one part about error-context I don't understand b/c I think I'm missing where this is used...

I don't think it is being used yet; again, I may have copied over more than necessary from the Big PR.

crates/wasmtime/src/runtime/vm/component/states.rs

dicej · 2025-01-24T15:55:05Z

@alexcrichton I've "rebooted" this PR from scratch, this time only pulling in the minimum code needed for the lift.wast test. It's much smaller now.

As a consequence, most of your review comments no longer apply, but I'll address them in future PRs. Sorry for the churn; I'll try to keep future PRs more focused.

alexcrichton

Looks good to me, thanks!

This is another piece of bytecodealliance#9582 which I'm splitting out to make review easier. This test includes two components: one which exports a function using the async-with-callback ABI, and another which uses the async-without-callback ABI. It doesn't actually instantiate or run either component yet. The rest of the changes fill in some TODOs to make the test pass. Signed-off-by: Joel Dice <[email protected]>

dicej requested a review from alexcrichton January 22, 2025 22:42

dicej requested review from a team as code owners January 22, 2025 22:42

dicej commented Jan 22, 2025

View reviewed changes

crates/wasmtime/src/runtime/vm/component/states.rs Outdated Show resolved Hide resolved

dicej force-pushed the async-lift-test branch from f15cf87 to 0b43dd5 Compare January 22, 2025 23:23

github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime labels Jan 23, 2025

alexcrichton reviewed Jan 23, 2025

View reviewed changes

dicej force-pushed the async-lift-test branch from 0b43dd5 to f6fc8d0 Compare January 24, 2025 15:50

dicej requested a review from alexcrichton January 24, 2025 15:59

alexcrichton approved these changes Jan 24, 2025

View reviewed changes

alexcrichton added this pull request to the merge queue Jan 24, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 24, 2025

dicej force-pushed the async-lift-test branch from f6fc8d0 to 0eae83f Compare January 24, 2025 16:44

dicej enabled auto-merge January 24, 2025 16:44

dicej force-pushed the async-lift-test branch from 0eae83f to 167e590 Compare January 24, 2025 16:45

dicej added this pull request to the merge queue Jan 24, 2025

Merged via the queue into bytecodealliance:main with commit 3ba13d1 Jan 24, 2025
39 checks passed

dicej deleted the async-lift-test branch January 24, 2025 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add component-model-async/lift.wast test #10083

add component-model-async/lift.wast test #10083

dicej commented Jan 22, 2025

github-actions bot commented Jan 23, 2025

github-actions bot commented Jan 23, 2025

alexcrichton left a comment

alexcrichton Jan 23, 2025

dicej Jan 24, 2025

alexcrichton Jan 24, 2025

alexcrichton Jan 23, 2025

dicej Jan 24, 2025

alexcrichton Jan 24, 2025

alexcrichton Jan 23, 2025

dicej Jan 24, 2025

dicej commented Jan 24, 2025

alexcrichton left a comment

add component-model-async/lift.wast test #10083

add component-model-async/lift.wast test #10083

Conversation

dicej commented Jan 22, 2025

github-actions bot commented Jan 23, 2025

Subscribe to Label Action

github-actions bot commented Jan 23, 2025

Label Messager: wasmtime:config

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

dicej commented Jan 24, 2025

alexcrichton left a comment

Choose a reason for hiding this comment