Stream: git-wasmtime

Topic: wasmtime / PR #2999 Implement `AsContextMut` for `call_as...

Wasmtime GitHub notifications bot (Jun 18 2021 at 16:32):

alexcrichton opened PR #2999 from impl-future to main:

This commit updates the Func::call_async and TypedFunc::call_async
functions to return futures that implement the AsContextMut trait.
Originally brought up as part of #2986 it was pointed out that once you
create a future from an async function in wasmtime you lose access to
the StoreContetMut that was passed in. While by design this is
somewhat unfortunate if you want to otherwise get access to it while the
future is not actually running.

This unfortunately requires changing the function signatures to no
longer use async. While not necessarily the end of the world it does
mean that the documentation might be a bit less readable. The larger
impact, however, is that the implementation of the future needs to be
significantly different. The input store is now threaded through
by-value which requires a few more generics in a few more places which
will end up monomorphizing more code to the user as well.

Overall this wasn't too too bad. The call_async function bottom out in
only a single future (one previous .await) where code is running on a
fiber. That code was already using a manually defined and implemented
future, so it wasn't moving heaven-and-earth to get that working.

The significantly more difficult part, which this PR does not implement,
was to change Instance::new. For now the new_async and
instantiate_async methods all continue to be async functions which
return a bland future that does not give access to the underlying
StoreContetMut closed over in the future. The reason for this is that
the implementation for those functions was significantly more
complicated and would require a great deal of unsafe due to
self-borrows of Pin types (hidden for us today through the usage of
async in Rust).

Overall I don't think this PR is in a great state, but I wanted to put
this out there. I'm not super happy about the inconsistency of some
futures implementing AsContextMut and others not implementing it. I
don't think there's a feasible route to implementing AsContextMut from
the futures returned by instantiation, due to how complicated the
internal logic is for instantiation. It's certainly possible that we
could implement AsContextMut and have manual trait impls, but I'm
not certain that the tradeoff is worth it.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 21 2021 at 19:04):

alexcrichton updated PR #2999 from impl-future to main.

Wasmtime GitHub notifications bot (Oct 04 2022 at 04:45):

ArtBlnd submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 04:45):

ArtBlnd created PR review comment:

I know this is old PR but just want to ask that did you used UnsafeCell + ptr because of self-reference?

Wasmtime GitHub notifications bot (Oct 04 2022 at 15:41):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 15:41):

alexcrichton created PR review comment:

I believe this was done for interior mutability and general "I don't know what I'm doing so please compiler stop trying to optimize things under my feet", it's probably not the best abstraction here.

Wasmtime GitHub notifications bot (Oct 04 2022 at 15:41):

alexcrichton closed without merge PR #2999.

Last updated: Nov 22 2024 at 16:03 UTC