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

Change all WASI impls to being concrete, not blanket #8609

Merged
merged 1 commit into from
May 14, 2024

Conversation

alexcrichton
Copy link
Member

This commit builds on the support from #8448 to remove all blanket impls from the WASI crates and instead replace them with concrete impls. This is slightly functionally different from before where impls are now on trait objects meaning dynamic dispatch is involved where previously dynamic dispatch was used. That being said the perf hit here is expected to be negligible-to-nonexistent since the implementations are large enough that the dynamic dispatch won't be the hot path.

The motivations for this commit are:

  • Removes the need for an odd skip_mut_forwarding_impls option - but this'll be left for a bit in case others need it.
  • Improves incremental compile time of these crates where the crates themselves now contain all object code for all of WASI instead of forcing the final consume to codegen everything (although there's still a significant amount monomorphized).
  • Improves future compatibility with refactorings of bindgen-generated-traits and such because blanket impls are pretty hard to work around where concrete impls are easier to reason about (and document).

This commit builds on the support from bytecodealliance#8448 to remove all blanket impls
from the WASI crates and instead replace them with concrete impls. This
is slightly functionally different from before where impls are now on
trait objects meaning dynamic dispatch is involved where previously
dynamic dispatch was used. That being said the perf hit here is expected
to be negligible-to-nonexistent since the implementations are large
enough that the dynamic dispatch won't be the hot path.

The motivations for this commit are:

* Removes the need for an odd `skip_mut_forwarding_impls` option - but
  this'll be left for a bit in case others need it.
* Improves incremental compile time of these crates where the crates
  themselves now contain all object code for all of WASI instead of
  forcing the final consume to codegen everything (although there's
  still a significant amount monomorphized).
* Improves future compatibility with refactorings of
  bindgen-generated-traits and such because blanket impls are pretty
  hard to work around where concrete impls are easier to reason about
  (and document).
@alexcrichton alexcrichton requested a review from a team as a code owner May 13, 2024 20:07
@alexcrichton alexcrichton requested review from elliottt and removed request for a team May 13, 2024 20:07
@github-actions github-actions bot added the wasi Issues pertaining to WASI label May 13, 2024
Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

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

Love it!

@alexcrichton alexcrichton added this pull request to the merge queue May 14, 2024
Merged via the queue into bytecodealliance:main with commit a11837b May 14, 2024
22 checks passed
@alexcrichton alexcrichton deleted the refactor-wasi branch May 14, 2024 18:09
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jun 10, 2024
This commit is a partial revert of bytecodealliance#8609 to return `wasmtime-wasi` and
`wasmtime-wasi-http` back to using blanket impls. The main change from
before is to change the blanket impls to be in terms of a local newtype
wrapper to avoid trait coherence issues. This is done because otherwise
using the traits before required `&mut dyn WasiView` to exist but
sometimes only a `Foo<'a>` is held which is not easy to get a `&mut dyn
...` view of. By changing to a blanket impl in terms of a newtype
wrapper, `WasiImpl`, it's possible to call `bindgen!`-generated
`add_to_linker_get_host` functions with a return value of
`WasiImpl<Foo<'a>>` which enables hooking into all the generated
bindings.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jun 10, 2024
…#8766)

This commit is a partial revert of bytecodealliance#8609 to return `wasmtime-wasi` and
`wasmtime-wasi-http` back to using blanket impls. The main change from
before is to change the blanket impls to be in terms of a local newtype
wrapper to avoid trait coherence issues. This is done because otherwise
using the traits before required `&mut dyn WasiView` to exist but
sometimes only a `Foo<'a>` is held which is not easy to get a `&mut dyn
...` view of. By changing to a blanket impl in terms of a newtype
wrapper, `WasiImpl`, it's possible to call `bindgen!`-generated
`add_to_linker_get_host` functions with a return value of
`WasiImpl<Foo<'a>>` which enables hooking into all the generated
bindings.
github-merge-queue bot pushed a commit that referenced this pull request Jun 10, 2024
This commit is a partial revert of #8609 to return `wasmtime-wasi` and
`wasmtime-wasi-http` back to using blanket impls. The main change from
before is to change the blanket impls to be in terms of a local newtype
wrapper to avoid trait coherence issues. This is done because otherwise
using the traits before required `&mut dyn WasiView` to exist but
sometimes only a `Foo<'a>` is held which is not easy to get a `&mut dyn
...` view of. By changing to a blanket impl in terms of a newtype
wrapper, `WasiImpl`, it's possible to call `bindgen!`-generated
`add_to_linker_get_host` functions with a return value of
`WasiImpl<Foo<'a>>` which enables hooking into all the generated
bindings.
alexcrichton added a commit that referenced this pull request Jun 10, 2024
* Change WASI trait impls back to being blanket impls (#8766)

This commit is a partial revert of #8609 to return `wasmtime-wasi` and
`wasmtime-wasi-http` back to using blanket impls. The main change from
before is to change the blanket impls to be in terms of a local newtype
wrapper to avoid trait coherence issues. This is done because otherwise
using the traits before required `&mut dyn WasiView` to exist but
sometimes only a `Foo<'a>` is held which is not easy to get a `&mut dyn
...` view of. By changing to a blanket impl in terms of a newtype
wrapper, `WasiImpl`, it's possible to call `bindgen!`-generated
`add_to_linker_get_host` functions with a return value of
`WasiImpl<Foo<'a>>` which enables hooking into all the generated
bindings.

* Update release notes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants