Refactor internals of table initialization and management #11416
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 refactors the evaluation of constant expressions during instantiation for example to use `Val` instead of `ValRaw`. Previously the usage of `ValRaw` meant that wasm was disallowed from performing a GC during const evaluation, but currently constant expressions can indeed perform a GC. The goal of this commit is to lift this limitation. This is expected to be a minor slowdown for modules that hit this path, but most modules shouldn't hit this in a hot loop since LLVM doesn't generate modules that use this branch of const eval. The usage of `Val` brings a number of benefits and refactorings associated with it: * Const-evaluation is generally safer than before since everything is higher-level. * GC types in const-eval were almost already using `Val` meaning that there's actually fewer conversions now. * Instantiation code was refactored to use `wasmtime::*`-API types instead of low-level VM types. This deduplicates a good deal and lifts complexity out of the raw VM bits. Another issue that this commit fixes is to change how table initialization is modeled internally in `vm::Instance::table_init_segment`. Previously this was done by removing the tables from an instance to get a split borrow into the store and the table. This is not valid though because if, during initialization, a GC is performed then the table is not present to find roots through the table. This function is refactored to scope borrows to within a loop instead of over a loop via various refactorings and such and usage of higher level APIs. This is again, like above, expected to pessimize performance but this is also not known to be a hot path for modules at this time.
This commit is a refactoring of how tables work within Wasmtime to avoid
funneling table elements through a `TableElement` enum internally.
Instead methods are "exploded" to `grow_{gc_ref,func,cont}` which means,
for example, funcrefs don't need a GcStore. The main motivation for this
change was to avoid the idiom where `TableElement` represents a cloned,
but unrooted, GC reference.
Prior to this commit there were a number of subtle bugs in the table
code for Wasmtime where write barriers were forgotten on `table.init`,
`table.set` (via the embedder API), and `table.grow`. While `table.fill`
correctly handled the GC references it was awkward to get everything
else working consistently so I opted to remove `TableElement` entirely
to make it more clear that `&VMGcRef` is ubiquitously used meaning that
the write barriers, for example, are the same as other parts of the
Wasmtime codebase.
This has a few extra tests for "make sure this doesn't leak" to ensure
that GC works correctly with new barriers in place.
github-actions
bot
added
wasmtime:api
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
Looks like instantiation is indeed regressing a bit, so I'll dig in. 
|
fitzgen
reviewed
Aug 12, 2025
Comment on lines
+296
to
+300
| // FIXME: `grow_gc_ref` shouldn't require the whole store | ||
| // and should require `AutoAssertNoGc`. For now though we | ||
| // know that table growth doesn't trigger GC so it should be | ||
| // ok to create a copy of the GC reference even though it's | ||
| // not tracked anywhere. |
There was a problem hiding this comment.
Why don't we make grow_gc_ref take an AutoAssertNoGc?
There was a problem hiding this comment.
That can't work right now as growth requires the whole dyn VMStore to perform a fiber suspension but AutoAssertNoGc only requires the StoreOpaque. Soon though...
| Val::AnyRef(None) | ||
| } | ||
|
|
||
| pub(crate) const fn null_top(top: WasmHeapTopType) -> Val { |
There was a problem hiding this comment.
Aside: we should probably add a wasmtime::HeapTopType because the environ version is so useful all over the place and it is really nice to exhaustive match on top types. Then we could expose this helper to embedders too, which seems like it would be useful.
copying a message I sent Alex in private chat here for posterity:
|
|
@fitzgen ok with the most recent commits I've clawed back all of the performance and then some, so should be good to go. This makes me want to generate a Cranelift-compiled function for wasm instance initialization really. |
bongjunj
pushed a commit
to prosyslab/wasmtime
that referenced
this pull request
Oct 20, 2025
…liance#11416) * Refactor const-eval to use `Val`, not `ValRaw` This commit refactors the evaluation of constant expressions during instantiation for example to use `Val` instead of `ValRaw`. Previously the usage of `ValRaw` meant that wasm was disallowed from performing a GC during const evaluation, but currently constant expressions can indeed perform a GC. The goal of this commit is to lift this limitation. This is expected to be a minor slowdown for modules that hit this path, but most modules shouldn't hit this in a hot loop since LLVM doesn't generate modules that use this branch of const eval. The usage of `Val` brings a number of benefits and refactorings associated with it: * Const-evaluation is generally safer than before since everything is higher-level. * GC types in const-eval were almost already using `Val` meaning that there's actually fewer conversions now. * Instantiation code was refactored to use `wasmtime::*`-API types instead of low-level VM types. This deduplicates a good deal and lifts complexity out of the raw VM bits. Another issue that this commit fixes is to change how table initialization is modeled internally in `vm::Instance::table_init_segment`. Previously this was done by removing the tables from an instance to get a split borrow into the store and the table. This is not valid though because if, during initialization, a GC is performed then the table is not present to find roots through the table. This function is refactored to scope borrows to within a loop instead of over a loop via various refactorings and such and usage of higher level APIs. This is again, like above, expected to pessimize performance but this is also not known to be a hot path for modules at this time. * Remove the `TableElement` type This commit is a refactoring of how tables work within Wasmtime to avoid funneling table elements through a `TableElement` enum internally. Instead methods are "exploded" to `grow_{gc_ref,func,cont}` which means, for example, funcrefs don't need a GcStore. The main motivation for this change was to avoid the idiom where `TableElement` represents a cloned, but unrooted, GC reference. Prior to this commit there were a number of subtle bugs in the table code for Wasmtime where write barriers were forgotten on `table.init`, `table.set` (via the embedder API), and `table.grow`. While `table.fill` correctly handled the GC references it was awkward to get everything else working consistently so I opted to remove `TableElement` entirely to make it more clear that `&VMGcRef` is ubiquitously used meaning that the write barriers, for example, are the same as other parts of the Wasmtime codebase. This has a few extra tests for "make sure this doesn't leak" to ensure that GC works correctly with new barriers in place. * Fix some lints and warnings * Fix wmemcheck build * Review comments * Optimize const eval and global initialization * Fix compile * Fix lints
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 is a pair of commit inspired by semi-related work in Wasmtime to make progress on #11262. I got stuck in one particular area and as I stared more at the code I realized there were deeper issues that needed fixing before I start tackling #11262. Namely this PR fixes bugs such as:
table.init, meaning that overwritten values were leaked.table.setvia the embedder API, meaning overwritten values were leaked.table.growwas not properly dropped via the embedder API, meaning that whatever reference that was passed in was leaked.table.initbecause an instances tables were temporarily removed to help work within the borrow checker.All of the above have now been addressed with a set of new tests that assert no leaks and which trigger GC-during-const-eval. More details can be found in the commit messages but my hope with this refactoring is to close out this class of issues or make it more obvious when they show up in the future. Some algorithms were pessimized here by having to repeatedly re-access things from the store, but it's expected that these are not performance hot-spots and feature-complete-ness and correctness is more paramount.