Add Send/Sync bounds to ComponentType
#11160
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit is extracted from from review of bytecodealliance#11123 and bytecodealliance#11127. While not literally present in those PRs it's my own personal conclusion that it's best to just go ahead and add these bounds at the "base" of the component trait hierarchy. The current implementation in bytecodealliance#11123 adds bounds in many locations and this would remove the need to add bounds everywhere and instead have everything inherited through the `Lift` and `Lower` traits. This raises the question of: why? The main conclusion that I've reached leading to this change is that Wasmtime currently will store `R`, a return value, on the stack during the lowering process back into linear memory. This might involve allocation, however, meaning that wasm can be invoked and a context switch could happen. For Wasmtime's `unsafe impl` of `Send` and `Sync` on fibers to be sound it requires that this stack-local variable is also `Send` and `Sync` as it's an entirely user-provided type. Thus I've concluded that for results it's always required for these to be both `Send` and `Sync` (or at the very least, `Send`). Given that I've gone ahead and updated to require both `Send` and `Sync` for both params and results. This is not expected to actually have any impact in practice since all primitives are already `Send`/`Sync` (minus `Rc` impls all removed here) and all `bindgen!`-generated types are compositions of `Send`/`Sync` primitives meaning that they're also `Send` and `Sync`.
|
cc @dicej |
alexcrichton
added a commit
to dicej/wasmtime
that referenced
this pull request
Jun 30, 2025
github-actions
bot
added
the
wasmtime:api
pchickey
approved these changes
Jun 30, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 3, 2025
* [DO NOT MERGE] update `component-model-async` plumbing
This pulls in the latest Component Model async ABI code from the
`wasip3-prototyping` repo, including various API refactors and spec updates.
This includes all the changes to the `wasmtime` crate from `wasip3-prototyping`
_except_ that the `concurrent` submodule and child submodules contain only
non-functional stubs. For that reason, and the fact that
e.g. `Func::call_async` is now implemented in terms of `Func::call_concurrent`,
most of the component model tests are failing. This commit is not meant to be
merged as-is; a follow-up commit (to be PR'd separately) will contain the real
`concurrent` implementation, at which point the tests will pass again. I'm
splitting these into separate PRs to make review easier.
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
* Undo wit-bindgen changes
No longer necessary after other refactors
* Move back to crates.io-based wit-bindgen
* Undo upgrade of http-body-util
(deferred for future PR)
* Add back in arbitrary use of async
Looks like it may have been lost by accident
* Make imports more conventional for Wasmtime
* Some minor changes
* Privatize a component field
* Cut down a bit on #[cfg]
* Undo a no-longer-necessary `pub`
* add doc comments for `{Future,Stream,ErrorContext}Any`
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
* rename `concurrent` stub module to `concurrent_disabled`
...and avoid panicking in the stubs.
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
* fix test regression
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
* revert `call_async` and `post_return_impl` changes
These will need to wait until the `component-model-async` feature is fully
implemented.
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
* remove unused struct
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
* add `Options::callback` field
This isn't used yet, but will be used when the real `component-model-async`
implementation is merged.
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
* Remove no-longer-needed feature
* Trim reexports from Wasmtime
Some of these are no longer needed or can be avoided with small changes.
Some deps are likely needed in the next commit but they'll be best added
there.
* Update test expectations
* More trimming of Cargo.toml
* Defer `*Buffer` traits to next PR
Not needed for this PR I believe.
* Use conventional Wasmtime imports + remove dummy_waker
* Reduce duplication in `*_disabled`
* Remove some unncessary bounds
* Remove some `for<'a>` bounds where unnecessary
* Remove another bound
* Defer more functions to the next PR
`drop_fibers` is different in the next PR, so defer it to then.
* Remove some reexports no longer necessary
Bindings generation changed awhile back so these aren't needed, defer
the implementations to the next PR.
* Remove unnecessary drop
This was already moved to `run_manual_drop_routines`
* Defer a `pub(crate)` to a future PR
* Expand comments in traphandlers
* Defer some types to the next PR
* Update linker documentation
* Add `Send`/`Sync` bounds to `ComponentType`
This commit is extracted from from review of #11123 and #11127. While
not literally present in those PRs it's my own personal conclusion that
it's best to just go ahead and add these bounds at the "base" of the
component trait hierarchy. The current implementation in #11123 adds
bounds in many locations and this would remove the need to add bounds
everywhere and instead have everything inherited through the `Lift` and
`Lower` traits.
This raises the question of: why? The main conclusion that I've reached
leading to this change is that Wasmtime currently will store `R`, a
return value, on the stack during the lowering process back into linear
memory. This might involve allocation, however, meaning that wasm can be
invoked and a context switch could happen. For Wasmtime's `unsafe impl`
of `Send` and `Sync` on fibers to be sound it requires that this
stack-local variable is also `Send` and `Sync` as it's an entirely
user-provided type. Thus I've concluded that for results it's always
required for these to be both `Send` and `Sync` (or at the very least,
`Send`).
Given that I've gone ahead and updated to require both `Send` and `Sync`
for both params and results. This is not expected to actually have any
impact in practice since all primitives are already `Send`/`Sync` (minus
`Rc` impls all removed here) and all `bindgen!`-generated types are
compositions of `Send`/`Sync` primitives meaning that they're also
`Send` and `Sync`.
* Remove some now-unnecessary bounds
* Fix build after #11160
* Remove some now-unnecessary duplicate bounds
* Uncomment test that now works
* Undo accidental doc wrap
* Clarify comment on `Value` types
Don't leave `TODO` in public-facing documentation ideally
* Actually resolve the conflict (forgot to commit)
* Avoid returning boxed futures in APIs
* Defer making constructors more public to a future PR
* Make a method name more conventional
* Refactor `linear_lift_into_from_memory`
* Drop the `max_count` parameter in favor of slicing the `WasmList`
itself. Avoids situations such as what happens if `max_count` is
larger than the length of the list.
* Don't have the default implementation collect to a vector and then
push all that onto a different vector. Instead push each item
individually through `extend`.
* De-indent a block of code added
* Remove unsafety from `prepare_call`
Mostly move the parameters themselves to the closure to avoid raw
pointers/drop/etc.
This will have the consequence of in the future `call_async` is going to
now require `Params: 'static` but that seems more-or-less inevitable at
this point.
* Go back to returning box, alas.
* Apply same treatment to lift function
Make it a closure and reduce some levels of indirection of the various
functions in play.
* Refactor `lower_params` to require less context.
Relax the bounds on the closure specified since it's immediately called
and then additionally take out parameters/captures that the closure can
carry itself.
* Don't pass extraneous `Instance` parameter
This can now be inferred from `Func`.
* Clean up some SAFETY comments
* Generalize the signature of `lift_results`
* Move `lift_results` function to `Func`
Also rename the lift/lower helpers to `with_{lift,lower}_context`
* Remove parameter from `with_lift_context`
Like `with_lower_context` this is fine to capture in the closure passed
in.
* Simplify the dynamic lifting logic
Don't call `with_lift_context` in two locations, only call it once with
a dynamic parameter.
* Refactor away the `Func::lift_results_sync` helper
* Use `with_lift_context` in `call_raw`
* Simplify a call to `Func::call_unchecked_raw`
* Ungate `with_lift_context` to fix non-cm-async build
* Fix compile (bad cherry-pick conflict resolution)
* Use `with_lower_context` in `call_raw`
Trying to unify the async/concurrent paths as much as possible.
* Move params out of `call_raw`
Let closures capture the params, no need to thread it through as an
unnecessary argument.
* Clean up unsafety in `Func::call_raw`
* Accurately mark `call_raw` itself as `unsafe`, then document why
callers should be safe.
* Don't have one large `unsafe` block in `call_raw`, instead split it up
with separate safety comments.
* Move a one-off type definition closer to its use
* Avoid intermediate allocations in dynamic calls
* Simplify a future-return site
* Deduplicate checking parameter count
* Simplify a future invocation with `?`
* Simplify a variable declaration
* Simplify some function signatures
* Remove outdated safety comment
* Refactor to not require `Params: 'static` on `call_async`
* Remove no-longer-necessary SAFETY comment
* Fix typos
* Add a fast-path with no `Box` for sync host functions
Speeds up host calls by ~20% and puts them back on parity with the
beforehand numbers Wasmtime has.
* Synchronize signatures of async/concurrent dynamic calls
Use slices for both instead of vecs for one and slices for the other.
Required some slight rejiggering. Apparently one can solve a closure
problem with another closure, then one surely has no more closure
problems.
* Fix non-cm-async build
---------
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Co-authored-by: Alex Crichton <alex@alexcrichton.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit is extracted from from review of #11123 and #11127. While not literally present in those PRs it's my own personal conclusion that it's best to just go ahead and add these bounds at the "base" of the component trait hierarchy. The current implementation in #11123 adds bounds in many locations and this would remove the need to add bounds everywhere and instead have everything inherited through the
LiftandLowertraits.This raises the question of: why? The main conclusion that I've reached leading to this change is that Wasmtime currently will store
R, a return value, on the stack during the lowering process back into linear memory. This might involve allocation, however, meaning that wasm can be invoked and a context switch could happen. For Wasmtime'sunsafe implofSendandSyncon fibers to be sound it requires that this stack-local variable is alsoSendandSyncas it's an entirely user-provided type. Thus I've concluded that for results it's always required for these to be bothSendandSync(or at the very least,Send).Given that I've gone ahead and updated to require both
SendandSyncfor both params and results. This is not expected to actually have any impact in practice since all primitives are alreadySend/Sync(minusRcimpls all removed here) and allbindgen!-generated types are compositions ofSend/Syncprimitives meaning that they're alsoSendandSync.