Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for async/streams/futures to Rust generator #1082

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

dicej
Copy link
Collaborator

@dicej dicej commented Nov 6, 2024

This adds support for generating bindings which use the Async ABI along with the stream, future, and
error-context
types.

By default, normal synchronous bindings are generated, but the user may opt-in to async bindings for all or some of the imported and/or exported functions in the target world and interfaces -- provided the default-enabled async feature is enabled.

In addition, we generate StreamPayload and/or FuturePayload trait implementations for any types appearing as the T in stream<T> or future<T> in the WIT files, respectively. That enables user code to call new_stream or new_future to create streams or futures with those payload types, then write to them, read from them, and/or pass the readable end as a parameter to a component import or return value of a component export.

Note that I've added new core::abi::Instruction enum variants to handle async lifting and lowering, but they're currently tailored to the Rust generator and will probably change somewhat as we add support for other languages.

This does not include any new tests; I'll add those in a follow-up commit.

This is ready for review, but I'll leave it in draft mode until the following are complete:

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for this! I didn't dive too too deep into the implementation guts since I think that'll be best to shake out over time, but I left some comments about higher-level structure and where this touches other parts. Overall looks grat to me 👍

tests/runtime/flavorful/wasm.rs Show resolved Hide resolved
crates/rust/tests/codegen.rs Outdated Show resolved Hide resolved
crates/core/src/types.rs Show resolved Hide resolved
crates/core/Cargo.toml Outdated Show resolved Hide resolved
crates/guest-rust/macro/Cargo.toml Outdated Show resolved Hide resolved
crates/guest-rust/rt/src/lib.rs Outdated Show resolved Hide resolved
crates/guest-rust/rt/src/lib.rs Outdated Show resolved Hide resolved
crates/rust/Cargo.toml Outdated Show resolved Hide resolved
crates/guest-rust/macro/src/lib.rs Show resolved Hide resolved
crates/rust/src/stream_and_future_support.rs Outdated Show resolved Hide resolved
This adds support for generating bindings which use the [Async
ABI](https://github.com/WebAssembly/component-model/blob/main/design/mvp/Async.md)
along with the [`stream`, `future`, and
`error-context`](WebAssembly/component-model#405) types.

By default, normal synchronous bindings are generated, but the user may opt-in
to async bindings for all or some of the imported and/or exported functions in
the target world and interfaces -- provided the default-enabled `async` feature
is enabled.

In addition, we generate `StreamPayload` and/or `FuturePayload` trait
implementations for any types appearing as the `T` in `stream<T>` or `future<T>`
in the WIT files, respectively.  That enables user code to call `new_stream` or
`new_future` to create `stream`s or `future`s with those payload types, then
write to them, read from them, and/or pass the readable end as a parameter to a
component import or return value of a component export.

Note that I've added new `core::abi::Instruction` enum variants to handle async
lifting and lowering, but they're currently tailored to the Rust generator and
will probably change somewhat as we add support for other languages.

This does not include any new tests; I'll add those in a follow-up commit.

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

add `async: true` case to Rust `codegen_tests`

This ensures that all the codegen test WIT files produce compile-able bindings
with `async: true` (i.e. all imports lowered and all exports lifted using the
async ABI).  That revealed some issues involving resource methods and
constructors, as well as missing stub support, which I've resolved.

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

add codegen tests for futures, streams, and error-contexts

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

remove async_support::poll_future

It was both unsafe to use and intended only for testing (and not even good for
that, it turns out).

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

add stream/future read/write cancellation support

Also, fix some issues with stream/future payload lifting/lowering which I
_thought_ I had already tested but actually hadn't.

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

support callback-less (AKA stackful) async lifts

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

revert incorrect test change in flavorful/wasm.rs

I had thoughtlessly removed test code based on a clippy warning, not realizing
it was testing (at compile time) that the generated types implemented `Debug`.

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

test `async: true` option in Rust codegen tests

I had meant to do this originally, but apparently forgot to actually use the
option.

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

add docs for new `debug` and `async` Rust macro options

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

address `cargo check` lifetime warning

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

minimize use of features based on PR feedback

Signed-off-by: Joel Dice <[email protected]>
@dicej
Copy link
Collaborator Author

dicej commented Jan 7, 2025

Thanks for the review, @alexcrichton. I believe I've addressed everything so far with either explanations or code changes.

@cpetig
Copy link

cpetig commented Jan 7, 2025

One thing which keeps me wondering is the asymmetry between argument passing of imported vs exported async functions. Imported functions pass a pointer to an argument buffer in memory, freed by the callee (host) while exported functions receive all arguments in line (on the argument stack).

Historically both directions used a buffer, but I wonder whether there is a good enough reason for the import still passing via a buffer - the host should have enough resources to remember all arguments in case of back-pressure and this could optimize the imported function call for few arguments (or e.g. a string/list) considerably.

@dicej
Copy link
Collaborator Author

dicej commented Jan 7, 2025

One thing which keeps me wondering is the asymmetry between argument passing of imported vs exported async functions.

As you mentioned, I think the main motivation is backpressure.

the host should have enough resources to remember all arguments in case of back-pressure

I don't know if that's necessarily true; we could be dealing with arbitrarily large and arbitrarily nested lists, for example, which the host would need to copy into its own memory, hold there indefinitely, and then copy to the callee once the backpressure clears. Generating fused adapters for that scenario sounds tough, and if we don't generate adapters then we'll need to fall back to the slow dynamic path in the host since we can't use the static lift and lower APIs for component->component calls in general. EDIT: or not; see my next comment.

In any case, https://github.com/WebAssembly/component-model is probably a better place to discuss this kind of thing.

@dicej
Copy link
Collaborator Author

dicej commented Jan 7, 2025

I don't know if that's necessarily true; we could be dealing with arbitrarily large and arbitrarily nested lists, for example, which the host would need to copy into its own memory, hold there indefinitely, and then copy to the callee once the backpressure clears.

Responding to myself: I suppose we wouldn't need to copy everything -- just up to MAX_FLAT_PARAMS -- assuming the caller knows not to free any allocated memory those params point to until it receives a STARTED event for the call.

@cpetig
Copy link

cpetig commented Jan 7, 2025

I don't know if that's necessarily true; we could be dealing with arbitrarily large and arbitrarily nested lists, for example, which the host would need to copy into its own memory, hold there indefinitely, and then copy to the callee once the backpressure clears.

Responding to myself: I suppose we wouldn't need to copy everything -- just up to MAX_FLAT_PARAMS -- assuming the caller knows not to free any allocated memory those params point to until it receives a STARTED event for the call.

But the extra arguments would likely be passed on the linear stack, and thus returning from the function doing the call would destroy this data.

Your answer clearly indicated that this has been discussed in depth before and isn't an oversight. So I will close this thread. Edit: Not necessary on github it seems.

@dicej
Copy link
Collaborator Author

dicej commented Jan 7, 2025

Your answer clearly indicated that this has been discussed in depth before and isn't an oversight. So I will close this thread.

Correct that it's not an oversight, but there may also be room for improvement, so definitely consider opening an issue on the component-model repo if you think there might be more to discuss.

FWIW, when Luke and I last discussed this, the vision was to make deferring tasks based on backpressure as efficient as possible in the host, e.g. O(1), without any allocations or copies at all, simply by (re)connecting tasks together in a linked list configuration. Any overhead we add to that would need to be justified, but it's not out of the question -- especially if it helps us optimize other, more common scenarios.

dicej added 2 commits January 8, 2025 12:24
Per a discussion with Alex, this moves most of the stream and future code into
the `wit-bindgen-rt` crate.  That's where I had intended to put it to begin
with, but ran into orphan rule issues given the trait-based approach I was
using.

The new approach uses dynamic dispatch via a vtable type.  Thus we've traded a
small (theoretical) amount of performance for much better compatibility in cases
of separately-generated bindings (e.g. passing `FutureWriter<T>` between crates
should work fine now), easier debugging, etc.

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

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks great to me!

crates/guest-rust/macro/src/lib.rs Outdated Show resolved Hide resolved
@dicej dicej added this pull request to the merge queue Jan 8, 2025
Merged via the queue into bytecodealliance:main with commit 3f6096b Jan 8, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants