Skip to content

Start moving away from using Stored<T> for wasmtime exports #10870

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

Closed
wants to merge 6 commits into from

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented May 30, 2025

This PR is a series of commits which is intended to lead up to the conclusion of removing usage of Stored<T> from wasmtime exports. This is intended to be an internal refactoring of the implementation guts of Wasmtime that doesn't have an API-level impact on users of Wasmtime. The motivations for this change are:

  • Fix issues such as Memory "leak" when running a GC #10869 and Memory "leak" when passing funcrefs to the host #10868. Effectively it's always been a cost that converting from an internal representation to the external representation pushes onto some internal vectors. Making this cost go away will fix the above issues.
  • Enable using public exports in the internal implementation of wasmtime. Historically the internals of Wasmtime are implemented very differently than what the users of Wasmtime get to use. That creates this weird dichotomy where externally everyone gets to learn one API while internally it's completely different. Internally Wasmtime is already a bit of a mish mash of some bits using raw pointers and others using indices. The goal of this refactoring is to enable pushing more towards the indices style of internal implementation which is safer, easier to reason about, and matches the public API.

While the first bullet here is pretty concrete the second is pretty nebulous. The original motivation was myself working in the wasip3-prototyping branch and brainstorming ways to reduce the amount of unsafety. That led me to wanting to stop using *mut ComponentInstance for example and instead use wasmtime::component::Instance but it was pretty awkward to do so. While none of that has to do with the core wasm bits being changed here I wanted to game out what such changes might look like. Additionally I also wanted to ensure that the chasm between the style of implementation of core wasm and components does not diverge further than it already has.

This PR is a draft PR since it's pretty large and I'll be wanting to split it up over time. I'm going to peel off commits from this PR into separate PRs and land those separately, but I wanted to have this open as a sort of "north star" in case folks are interested to see where this is going.

Sequence-wise this is organized as going through each of the exported items from core wasm modules to remove their usage of Stored<T> one at a time. This will eventually culminate in removing Stored<T> for wasmtime::Instance which will fully migrate core wasm bits away from Stored<T>. Status-wise here's the progress

  • wasmtime::Memory
  • wasmtime::Table
  • wasmtime::Tag
  • wasmtime::Global
  • wasmtime::Func
  • wasmtime::Instance
  • wasmtime::component::Instance
  • wasmtime::component::Func

@alexcrichton alexcrichton requested a review from a team as a code owner May 30, 2025 15:37
@alexcrichton alexcrichton requested review from fitzgen and removed request for a team May 30, 2025 15:37
@alexcrichton alexcrichton marked this pull request as draft May 30, 2025 15:37
@alexcrichton alexcrichton changed the title Mroe indices Start moving away from using Stored<T> for wasmtime exports May 30, 2025
@alexcrichton
Copy link
Member Author

Sorry I hit submit too soon and I've now actually filled out the description/title for anyone interested.

@alexcrichton
Copy link
Member Author

I'll also clarify, I plan on using this to push lots of commit to, so if anyone would prefer please feel free to unsubscribe from this PR as it may be a bit noisy.

@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. labels May 31, 2025
@alexcrichton alexcrichton force-pushed the mroe-indices branch 2 times, most recently from f2b9e3b to 137ccde Compare June 2, 2025 23:56
This commit is similar to a prior commit modifying memories which
refactors the internals of `wasmtime::Table` to not longer use a
`Stored` index. Instead a `StoreInstanceId` and a `DefinedTableIndex` is
used instead. This enables construction of external-facing types to be
trivial and zero-cost.

This additionally notably fixes an issue where triggering a GC in a
store would unconditionally create a `Table`, pushing onto an internal
vector in the store. This then would never be deallocated because the
internal table lived as long as the `Store`. In effect this meant that
triggering a GC would end up leaking memory by pushing more items onto
internal `Store` table. This is fixed through this commit because
creating a `wasmtime::Table` is now "free" and no longer requires any
allocations.
These aren't actually read from compiled wasm code, only from the host.
Everything can work with `&StoreOpaque` now!
This is now a relic of the past now that conversion from the internal
to external representation of Wasmtime items is free. This is
effectively dead code that is no longer needed.
Powered by all previous commits this is a near-trivial change where an
`Instance` is now more-or-less "just" an `InstanceId`.
@alexcrichton
Copy link
Member Author

Final bits here split into #10908 and #10909, so closing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant