wasmtime / PR #13466 cranelift: avoid per-variable value ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #13466 cranelift: avoid per-variable value ...

Wasmtime GitHub notifications bot (May 23 2026 at 17:57):

vouillon opened PR #13466 from vouillon:gc-fix to bytecodealliance:main:

Follow-up to #13449 addressing review feedback from this comment:

SecondaryMap<Variable, EntitySet<Value>> is dense — O(max value id) bits per variable — and wasteful for the sparse sets that show up in practice.

The bookkeeping should drop to zero when no caller has flagged any variable as needing stack maps.

Instead of tracking every SSA value ever bound to a variable and propagating the needs-stack-map bit at finalize time, this PR records values eagerly at the two binding sites — SSABuilder::def_var and SSABuilder::find_var (the latter when SSA construction synthesizes a block parameter). A new helper record_stack_map_binding(var, val) checks stack_map_vars and inserts on a hit; the per-variable set, values_for_var, and the finalize loop all go away.

stack_map_vars and stack_map_values move from FunctionBuilderContext into SSABuilder so the binding sites can reach them. The public FunctionBuilder API is unchanged.

Result:

Sparse memory: only the single EntitySet<Value> of values that need stack maps is ever allocated — no per-variable structure.

Free when unused: with no tracked variables, stack_map_vars.contains(var) short-circuits inside record_stack_map_binding and nothing is touched.

Wasmtime GitHub notifications bot (May 23 2026 at 17:57):

vouillon requested wasmtime-compiler-reviewers for a review on PR #13466.

Wasmtime GitHub notifications bot (May 23 2026 at 17:57):

vouillon requested cfallin for a review on PR #13466.

Wasmtime GitHub notifications bot (May 23 2026 at 18:46):

vouillon updated PR #13466.

Wasmtime GitHub notifications bot (May 23 2026 at 20:40):

vouillon updated PR #13466.

Wasmtime GitHub notifications bot (May 23 2026 at 23:01):

github-actions[bot] added the label cranelift on PR #13466.

Wasmtime GitHub notifications bot (May 26 2026 at 22:58):

:memo: cfallin submitted PR review:

Thanks -- general shape looks OK, but the API behavior does change per below, so we'll need to be careful.

Wasmtime GitHub notifications bot (May 26 2026 at 22:58):

:speech_balloon: cfallin created PR review comment:

This implies an ordering constraint in the API that wasn't there before, right? In particular: one needs to declare_var_needs_stack_map before one does any def_var. That wasn't the case before -- all defs were found during finalize.

I think that's fine, but we should (i) add a note to the public API docs, and (ii) audit our use in Wasmtime to make sure this is truly the case.

Wasmtime GitHub notifications bot (May 27 2026 at 09:49):

vouillon updated PR #13466.

Wasmtime GitHub notifications bot (May 27 2026 at 09:51):

:memo: vouillon submitted PR review.

Wasmtime GitHub notifications bot (May 27 2026 at 09:51):

:speech_balloon: vouillon created PR review comment:

Indeed, this implies an ordering constraint in the API.

I considered keeping track of overridden values, using a set of pairs (variable, value), but the code was awkward. Also, it is not possible to skip maintaining any additional info when stack maps are not requested by the user of Cranelift without imposing an ordering contraint.

I have added a comment, and an assertion which gets triggered if declare_var_needs_stack_map is called after def_var.

Function declare_var_needs_stack_map is used in the following files:

crates/cranelift/src/translate/func_translator.rs

when declaring parameters (declare_wasm_parameters);

when declaring locals ('declare_locals');

cranelift/fuzzgen/src/function_generator.rs
in build_variable_pool;

cranelift/frontend/src/frontend/safepoints.rs
in various tests.

I have checked that all calls to declare_var_needs_stack_map are right after the call to declare_var that defines the variable. I fixed the now incorrect order in cranelift/fuzzgen/src/function_generator.rs.

Wasmtime GitHub notifications bot (May 27 2026 at 17:06):

:memo: cfallin submitted PR review:

Thanks -- one more change on your latest commit and I think this is good to go.

Wasmtime GitHub notifications bot (May 27 2026 at 17:06):

:speech_balloon: cfallin created PR review comment:

Let's make this a debug_assert since it's doing a potentially expensive linear search.

Wasmtime GitHub notifications bot (May 27 2026 at 17:21):

:memo: vouillon submitted PR review.

Wasmtime GitHub notifications bot (May 27 2026 at 17:21):

:speech_balloon: vouillon created PR review comment:

In the intended call pattern, the variable has no recorded definitions yet, so the scan is O(1). It's only linear in the misuse case, which panics anyway. But I can switch to debug_assert! if that feels safer to you.

Wasmtime GitHub notifications bot (May 27 2026 at 18:53):

:memo: cfallin submitted PR review.

Wasmtime GitHub notifications bot (May 27 2026 at 18:53):

:speech_balloon: cfallin created PR review comment:

All the same, we try to keep checks like these debug_assert. Please make the change, thanks.

Wasmtime GitHub notifications bot (May 27 2026 at 19:11):

vouillon updated PR #13466.

Wasmtime GitHub notifications bot (May 27 2026 at 19:56):

:memo: cfallin submitted PR review.

Wasmtime GitHub notifications bot (May 27 2026 at 19:56):

:speech_balloon: cfallin created PR review comment:

We shouldn't remove the "panics" comment here: it still panics if it panics in debug-builds.

Wasmtime GitHub notifications bot (May 27 2026 at 20:04):

vouillon updated PR #13466.

Wasmtime GitHub notifications bot (May 27 2026 at 22:18):

:thumbs_up: cfallin submitted PR review.

Wasmtime GitHub notifications bot (May 27 2026 at 22:18):

cfallin added PR #13466 cranelift: avoid per-variable value sets when tracking stack-map bindings to the merge queue.

Wasmtime GitHub notifications bot (May 27 2026 at 22:43):

:check: cfallin merged PR #13466.

Wasmtime GitHub notifications bot (May 27 2026 at 22:43):

cfallin removed PR #13466 cranelift: avoid per-variable value sets when tracking stack-map bindings from the merge queue.

Last updated: Jun 01 2026 at 09:49 UTC