add component-model-async/fused.wast test #10106

dicej · 2025-01-24T21:09:56Z

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

This test exercises fused adapter generation for various flavors of intercomponent async->async, async->sync, and sync->async calls.

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

This is another piece of bytecodealliance#9582 which I'm splitting out to make review easier. This test exercises fused adapter generation for various flavors of intercomponent async->async, async->sync, and sync->async calls. The remaining changes fill in some TODOs to make the test pass. Signed-off-by: Joel Dice <[email protected]>

alexcrichton

I think this is as far as I'm gonna get today. I haven't gotten to most of trampoline.rs yet which I realize is most of the guts of this PR, but I'll do that on Monday

crates/environ/src/fact/signature.rs

crates/wasmtime/src/runtime/store.rs

crates/wasmtime/src/runtime/vm.rs

crates/cranelift/src/compiler/component.rs

crates/environ/src/fact/trampoline.rs

Signed-off-by: Joel Dice <[email protected]>

alexcrichton

Ok I'm trying to decipher trampoline.rs but if it's ok with you I'd prefer to ask for more documentation first before diving further into these methods. Currently there's very little documentation on how things are set up and understanding enough to be able to review this PR I think would require cross-referencing both all the details in the spec along with Wasmtime internal implementation details. A lot of this is also pretty specific to Wasmtime itself in the sense that it's all internal adapter details and we're implementing halves of the spec in some places.

Would you be ok writing up some docs for this? Ideally I'd find it most helpful to have a high-level description of how the adapters piece together and what the expected flow between them is. My hope is that such documentation would also be pretty valuable for future readers to understand this as well.

crates/environ/src/fact/trampoline.rs

alexcrichton · 2025-01-27T20:24:36Z

crates/environ/src/fact/trampoline.rs

+        self.set_flag(adapter.lower.flags, FLAG_MAY_LEAVE, false);
+        self.translate_results(adapter, &param_locals, &param_locals);
+        self.set_flag(adapter.lower.flags, FLAG_MAY_LEAVE, true);


I'm a bit perplexed by this and I've been trying to think about this but to no avail.

High-level request: mind writing up some longer-form documentation in this module or on a function here about adapters in the async world? For example I see the use of globals here and it makes me wonder:

Why are globals needed here? For example what function will pick up these globals later?

What prevents these globals from being set again before the "other side" picks up the results?

I suspect there's answers to these and it's where I think some docs could help. I realize as well that I didn't do a great job of documenting most of this file the first time I wrote it, so I'm also sort of using this opportunity to request some docs while it's still fresh in your head as well.

At a lower-level: why is param_locals passed twice here? It feels surprising and I'm not sure the code below was meant to handle the same values in/out, but I suspect you've been testing with this as well. Mind leaving some comments as to why it's ok to do that here? I'm lacking the high-level picture of what these adapters are doing so that might also help piece together why it's ok to pass the two sides in here too.

(also if you're up for it adding docs to preexisting methods like translate_results would be most welcome, but I understand if you'd prefer to not or defer that to later)

Yeah, it's confusing and needs docs; I'll add them.

Why are globals needed here? For example what function will pick up these globals later?

Quoting the comment I just pushed:

// Like the async->async case above, for the sync->async case we // also need `async-start` and `async-return` helper functions to // allow the callee to asynchronously "pull" the parameters and // "push" the results when it is ready. // // However, since the caller is using the synchronous ABI, the // parameters may have been passed via the stack rather than linear // memory. In that case, we use global variables to store them such // that they can be retrieved by the `async-start` function. // Similarly, the `async-return` function may write its result to // global variables from which the adapter function can read and // return them via the stack to the caller.

What prevents these globals from being set again before the "other side" picks up the results?

~~Since these globals are only used for sync-lowered imports, and the caller instance can only call one of those at a time, we know nobody will touch them.~~ EDIT: this isn't necessarily true for the async-without-callback ABI; see my comment below.

At a lower-level: why is param_locals passed twice here?

Hmm, I think you found a bug (and a hole it the test coverage). I think my intention was that translate_results would use the last element of its param_locals parameter as the destination pointer in the case where the result does not fit on the stack. But that doesn't make sense because it's a pointer to the callee's memory -- it only makes sense as a source pointer. I think I was just confused, and the code is wrong.

I added tests to tests/all/component_model/import.rs to cover the various combinations of passing parameters and returning results via the stack and the heap, but am now realizing I'm missing component composition tests for those scenarios.

Also now realizing that testing async-lifted exports that return more than MAX_FLAT_RESULTS is not sufficient. Since task.return accepts up to MAX_FLAT_PARAMS, I need to exceed that also.

I'll add those tests and fix any issues I find.

What prevents these globals from being set again before the "other side" picks up the results?

Since these globals are only used for sync-lowered imports, and the caller instance can only call one of those at a time, we know nobody will touch them.

Actually, I'm going to take this back. For sync-lowered imports called from an async-without-callback-lifted export, we could have more than one running concurrently, in which case we'd have a problem. The good news is that we've decided not to include the async-without-callback ABI in WASI 0.3.0, so I don't need to solve that right away, but we do need to make sure we reject such exports until we've solved it.

This shouldn't be a problem for sync-lowered imports called from sync-lifted exports or async-with-callback-lifted exports since neither can be reentered until the import returns.

FYI, I'm currently tackling the tedious but inevitable task of rebasing my async branch onto this one. Tons of conflicts, as expected. Once I've finished that, I'll make sure all the existing tests pass, then add new tests and docs per the above discussion, and finally copy any relevant changes/fixes back into this PR.

crates/environ/src/fact/trampoline.rs

This temporarily switches to my fork of `wasm-tools` until bytecodealliance/wasm-tools#1989 is merged. Signed-off-by: Joel Dice <[email protected]>

Also, switch to upstream `wasm-tools` main branch. Signed-off-by: Joel Dice <[email protected]>

Signed-off-by: Joel Dice <[email protected]>

alexcrichton

I'm sorry I know I sound like a broken record but I'm personally still very lost trying to understand this. On one hand one way I can fix this is to go study the component-model specification, try to piece together what an expected implementation would be, and then try to connect those dots back to this implementation. On the other hand though I also think it'd be valuable to have enough local documentation to not require that because although I can do that it would also be required for any future readers as well.

If you feel like I'm requesting too much documentation though or there's something that is well outside the purview of this module's documentation and it's more basic fundamental understanding please let me know though. I don't think we should just mirror the entire specification in comments into this repository, but at the same time I still think there are critical implementation pieces lacking comments such as the protocol between the async-start/return adapters as well as the host-provided async-enter/exit functions.

alexcrichton · 2025-01-29T18:44:02Z

crates/environ/src/component.rs

+            task_return(vmctx: vmctx, ty: u32, storage: ptr_u8, storage_len: size) -> bool;
+            #[cfg(feature = "component-model-async")]
+            async_enter(vmctx: vmctx, start: ptr_u8, return_: ptr_u8, caller_instance: u32, task_return_type: u32, params: u32, results: u32) -> bool;
+            #[cfg(feature = "component-model-async")]


No need to do this here but for the future might be good to add this attribute to the future/stream/error transfer down below too

alexcrichton · 2025-01-29T18:45:42Z

crates/environ/src/fact/signature.rs

@@ -26,24 +34,50 @@ impl ComponentTypesBuilder {
        let ty = &self[options.ty];
        let ptr_ty = options.options.ptr();

+        if let (Context::Lower, true) = (&context, options.options.async_) {


Mind leaving a small comment here explaining a bit about what's going on?

alexcrichton · 2025-01-29T18:46:04Z

crates/environ/src/fact/signature.rs

                vec![ptr_ty]
            }
        };

+        if options.options.async_ {


Mind also leaving a comment here too?

alexcrichton · 2025-01-29T18:47:04Z

crates/environ/src/fact/signature.rs

+        let mut results_indirect = false;
+        let results = match self.flatten_types(
+            &options.options,
+            // Async functions return results by calling `task.return`, which


Is this a copy/paste typo with task.return when this function is about async start?

alexcrichton · 2025-01-29T18:53:11Z

crates/environ/src/fact/trampoline.rs

+                    lower_sig
+                        .params
+                        .iter()
+                        .take(if lower_sig.results_indirect {
+                            lower_sig.params.len() - 1
+                        } else {
+                            lower_sig.params.len()
+                        })


Could this iterator be a method on lower_sig itself? That'd help move out some of this complexity and makes it a bit easier to see that the if here is unrelated to async/sync and it's just about signatures.

alexcrichton · 2025-01-29T19:07:38Z

crates/environ/src/fact/trampoline.rs

+        self.instruction(LocalGet(0));
+        self.instruction(LocalGet(1));


Mind leaving comments as to what these two locals are?

alexcrichton · 2025-01-29T19:09:38Z

crates/environ/src/fact/trampoline.rs

+        self.instruction(I32Const(param_count));
+        self.instruction(I32Const(1)); // leave room for the guest context result


Mind leaving a comment as to what these are doing? For example why nparams is being passed and expanding a bit on what it means to leave room for the guest context?

alexcrichton · 2025-01-29T19:12:58Z

crates/environ/src/fact/trampoline.rs

+        if let Some(globals) = result_globals {
+            for global in globals {
+                self.global_set(*global);
+            }
+        }


I fear I'm personally still pretty lost here. In a sync->async call what I was expecting is that the results would be stored in globals and async-start would read from those globals when called by somone (the host? unsure). The results would then be stored-to in async-return. Here I was expecting something to block (as the caller is sync) until the nested async call was finished and then the globals would be read.

That doesn't appear to be what's happening though? I commented above that's it's not clear what async-start/async-return are doing but the setting of globals here rather than reading is what really threw me astray... Mind leaving more comments as to what the host intrinsics are doing and the various protocol that this is adhering to?

alexcrichton · 2025-01-29T19:18:05Z

crates/environ/src/fact/trampoline.rs

+        self.finish()
+    }
+
+    fn compile_async_start_adapter(


I'm personally pretty nervous about the use of globals here because it seems like it'll be easy to get wires crossed by accident and overwrite globals or use uninitialized globals. To help alleviate this concern though, what do you think about some debug-only code?

In wasmtime-cranelift we perform some extra debug checks in cfg!(debug_assertions) in the generated code itself. I'm wondering if we could do something similar here where in debug_assertions mode there's an "in_use" global which is asserted to be zero when we store to it and then it's asserted to be 1 when we read from it. (and stores switch from 0->1 where reads switch from 1->0). That'd help make me more confident that although I don't fully understand everything here we have at least a layer of defense against bugs in debug mode for testing/fuzzing.

alexcrichton · 2025-01-29T19:21:00Z

crates/environ/src/fact/trampoline.rs

+            sig.params
+                .iter()
+                .enumerate()
+                .map(|(i, ty)| (i as u32, *ty))
+                .collect::<Vec<_>>()


Ok I'll admit I'm still very lost as to what's going on here. My current understanding of async-start is that an async task calls it to get the parameters of the original async call. Here it looks like the adapter is performing the "translate from one component to another call", but I'm pretty confused why this signature's locals would ever be used. Why would a parameter be passed in if you're trying to learn the original parameters of the initial caller?

The globals bits make sense to me where the initial caller put some stuff in globals and that's read here, but I'm not sure how this works without the globals?

dicej · 2025-01-29T19:28:44Z

To be clear: I absolutely agree that more docs and comments are needed -- I've only added a bit of that so far but plan to add more. Feel free to ignore this PR until that's done.

dicej requested a review from alexcrichton January 24, 2025 21:09

dicej requested a review from a team as a code owner January 24, 2025 21:09

dicej force-pushed the async-fused-test branch 2 times, most recently from ecd9754 to 7e41099 Compare January 24, 2025 21:33

dicej force-pushed the async-fused-test branch from 7e41099 to 098bd1c Compare January 24, 2025 21:40

alexcrichton reviewed Jan 24, 2025

View reviewed changes

github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jan 24, 2025

address review feedback

b172b1e

Signed-off-by: Joel Dice <[email protected]>

alexcrichton reviewed Jan 27, 2025

View reviewed changes

update task.return to match spec

d98b85c

This temporarily switches to my fork of `wasm-tools` until bytecodealliance/wasm-tools#1989 is merged. Signed-off-by: Joel Dice <[email protected]>

dicej requested a review from a team as a code owner January 28, 2025 18:51

dicej requested review from abrown and removed request for a team January 28, 2025 18:51

refactor component-model-async cfg attrs per review feedback

5f70e56

Also, switch to upstream `wasm-tools` main branch. Signed-off-by: Joel Dice <[email protected]>

dicej mentioned this pull request Jan 28, 2025

Support Abi::Array when compiling calls to async intrinsics #10143

Open

dicej added 3 commits January 28, 2025 15:53

add Compiler::new constructor

840af68

Signed-off-by: Joel Dice <[email protected]>

add comments covering various lower/lift combos in FACT generator

8045e34

Signed-off-by: Joel Dice <[email protected]>

add Compiler::global_set helper function

4dced39

Signed-off-by: Joel Dice <[email protected]>

alexcrichton reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add component-model-async/fused.wast test #10106

add component-model-async/fused.wast test #10106

dicej commented Jan 24, 2025

alexcrichton left a comment

alexcrichton left a comment

alexcrichton Jan 27, 2025

dicej Jan 29, 2025 •

edited

Loading

dicej Jan 29, 2025 •

edited

Loading

dicej Jan 29, 2025 •

edited

Loading

alexcrichton left a comment

alexcrichton Jan 29, 2025

alexcrichton Jan 29, 2025

alexcrichton Jan 29, 2025

alexcrichton Jan 29, 2025

alexcrichton Jan 29, 2025

alexcrichton Jan 29, 2025

alexcrichton Jan 29, 2025

alexcrichton Jan 29, 2025

alexcrichton Jan 29, 2025

alexcrichton Jan 29, 2025

dicej commented Jan 29, 2025

		self.instruction(LocalGet(0));
		self.instruction(LocalGet(1));

		self.instruction(I32Const(param_count));
		self.instruction(I32Const(1)); // leave room for the guest context result

add component-model-async/fused.wast test #10106

Are you sure you want to change the base?

add component-model-async/fused.wast test #10106

Conversation

dicej commented Jan 24, 2025

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dicej Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

dicej Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

dicej Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

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

dicej commented Jan 29, 2025

dicej Jan 29, 2025 •

edited

Loading

dicej Jan 29, 2025 •

edited

Loading

dicej Jan 29, 2025 •

edited

Loading