Stream: git-wasmtime

Topic: wasmtime / PR #11416 Refactor internals of table initiali...

Wasmtime GitHub notifications bot (Aug 12 2025 at 03:14):

alexcrichton requested fitzgen for a review on PR #11416.

Wasmtime GitHub notifications bot (Aug 12 2025 at 03:14):

alexcrichton opened PR #11416 from alexcrichton:refactor-tables to bytecodealliance:main:

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:

• There were no write barriers on table.init, meaning that overwritten values were leaked.
• There were no write barriers on table.set via the embedder API, meaning overwritten values were leaked.
• The GC referenced passed to table.grow was not properly dropped via the embedder API, meaning that whatever reference that was passed in was leaked.
• Const-expression evaluation disallowed GC, but due to allocation of GC values it's possible to trigger a GC
• When const-expression evaluation triggers a GC it would be problematic during table.init because 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.

Wasmtime GitHub notifications bot (Aug 12 2025 at 03:14):

alexcrichton requested wasmtime-core-reviewers for a review on PR #11416.

Wasmtime GitHub notifications bot (Aug 12 2025 at 03:26):

alexcrichton updated PR #11416.

Wasmtime GitHub notifications bot (Aug 12 2025 at 03:29):

alexcrichton updated PR #11416.

Wasmtime GitHub notifications bot (Aug 12 2025 at 04:49):

github-actions[bot] commented on PR #11416:

Subscribe to Label Action

cc @fitzgen

<details>
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:

• fitzgen: wasmtime:ref-types

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Aug 12 2025 at 16:21):

alexcrichton commented on PR #11416:

Looks like instantiation is indeed regressing a bit, so I'll dig in.

<img width="628" height="514" alt="Screenshot 2025-08-12 at 11 21 37 AM" src="https://github.com/user-attachments/assets/120a0743-4143-4ca9-96f8-ae3b2fe4c23e" />

Wasmtime GitHub notifications bot (Aug 12 2025 at 16:54):

fitzgen submitted PR review:

LGTM but withholding approval until we figure out the perf regression, since I expect that I will probably need to give this another once-over after resolving that

Wasmtime GitHub notifications bot (Aug 12 2025 at 16:54):

fitzgen created PR review comment:

Why don't we make grow_gc_ref take an AutoAssertNoGc?

Wasmtime GitHub notifications bot (Aug 12 2025 at 16:54):

fitzgen created PR review comment:

:tada:

Wasmtime GitHub notifications bot (Aug 12 2025 at 16:54):

fitzgen created PR review comment:

:eyes:

Wasmtime GitHub notifications bot (Aug 12 2025 at 16:54):

fitzgen created PR review 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.

Wasmtime GitHub notifications bot (Aug 12 2025 at 16:54):

fitzgen created PR review comment:

Here an elsewhere: we have already been using the convention foo for the generic version and _foo for the opaque version, not foo_ for the opaque version. Should be consistent and have underscores at the front.

Wasmtime GitHub notifications bot (Aug 12 2025 at 16:55):

fitzgen commented on PR #11416:

Looks like instantiation is indeed regressing a bit, so I'll dig in.

copying a message I sent Alex in private chat here for posterity:

another idea: basically nothing uses non-constant const-expressions. we could make ConstExpr be something like

rust enum ConstExpr { Const(Val), Expr(Vec<ConstOp>), }

Wasmtime GitHub notifications bot (Aug 12 2025 at 17:57):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 12 2025 at 17:57):

alexcrichton created PR review comment:

Heh yeah I've done that historically as well (_foo) but Joel pointed out recently that if we stop using the function for whatever reason it won't get lints about dead code so I started switching to foo_ instead. I'll keep it consistent for now internally in Wasmtime.

Wasmtime GitHub notifications bot (Aug 12 2025 at 17:57):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 12 2025 at 17:57):

alexcrichton created PR review 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...

Wasmtime GitHub notifications bot (Aug 12 2025 at 18:06):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 12 2025 at 18:06):

fitzgen created PR review comment:

Ah yeah good point we should fix everything at once in a follow up.

Wasmtime GitHub notifications bot (Aug 12 2025 at 20:34):

alexcrichton updated PR #11416.

Wasmtime GitHub notifications bot (Aug 12 2025 at 20:37):

alexcrichton updated PR #11416.

Wasmtime GitHub notifications bot (Aug 12 2025 at 20:37):

alexcrichton commented on PR #11416:

@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.

Wasmtime GitHub notifications bot (Aug 12 2025 at 21:13):

fitzgen submitted PR review:

Looks great!

Wasmtime GitHub notifications bot (Aug 12 2025 at 22:19):

alexcrichton updated PR #11416.

Wasmtime GitHub notifications bot (Aug 12 2025 at 22:19):

alexcrichton has enabled auto merge for PR #11416.

Wasmtime GitHub notifications bot (Aug 12 2025 at 22:52):

alexcrichton merged PR #11416.

Last updated: Dec 06 2025 at 06:05 UTC