-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
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
Stored<T>
for wasmtime exports
Sorry I hit submit too soon and I've now actually filled out the description/title for anyone interested. |
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. |
This was referenced May 30, 2025
5ca7806
to
9633047
Compare
9633047
to
3d1e17c
Compare
3d1e17c
to
c959a4f
Compare
This was referenced Jun 2, 2025
c959a4f
to
ba55fc4
Compare
This was referenced Jun 2, 2025
f2b9e3b
to
137ccde
Compare
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`.
137ccde
to
d241f5b
Compare
This was referenced Jun 3, 2025
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.
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 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: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 usewasmtime::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 removingStored<T>
forwasmtime::Instance
which will fully migrate core wasm bits away fromStored<T>
. Status-wise here's the progresswasmtime::Memory
wasmtime::Table
wasmtime::Tag
wasmtime::Global
wasmtime::Func
wasmtime::Instance
wasmtime::component::Instance
wasmtime::component::Func