wasmtime / PR #14797 Cranelift: preserve unchanged alias-... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #14797 Cranelift: preserve unchanged alias-...

Wasmtime GitHub notifications bot (Oct 17 2026 at 20:52):

nv3ifu opened PR #14797 from nv3ifu:fix-laststores-perf-14312 to bytecodealliance:main:

Hello, I have attempted to fix #14312.
In the previous implementation, new extents were generated at branch merge points. This meant that even if a specific memory region had not been written to, the read operation following a merge could not reuse previous results, leading to redundant reads.
I have introduced a table to track extents for each memory region and added checks for writes occurring along the various paths leading up to a merge point. For regions that have not been modified—provided they pass the necessary state checks—the original markers are preserved, allowing existing optimization logic to eliminate redundant reads. A test case has also been added.
Testing locally with signals-based traps disabled—using the original .wasm program provided in the issue—execution time decreased by approximately 23%, while compilation time increased by about 0.9%.

original text(Chinese,I am not good at English):
你好，我尝试修复了 #14312。
之前的代码会在分支汇合处生成新的 extent，导致即使某个内存区域没有被写过，汇合后的读取也可能无法复用之前的结果，进而重复读取。
我增加了一张按内存区域保存 extent 的表，并检查汇合前各条路径上的写入。对于没有被写过、且状态检查通过的区域，保留原来的标记，让已有的优化逻辑消除重复读取。同时补充了一个测试。
在 issue 提供的原始 .wasm 程序上，本地测试关闭 signals-based traps 后，执行耗时约减少 23%，编译耗时约增加 0.9%。

Wasmtime GitHub notifications bot (Oct 17 2026 at 20:52):

nv3ifu requested cfallin for a review on PR #14797.

Wasmtime GitHub notifications bot (Oct 17 2026 at 20:52):

nv3ifu requested wasmtime-compiler-reviewers for a review on PR #14797.

Wasmtime GitHub notifications bot (Oct 18 2026 at 01:12):

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

Wasmtime GitHub notifications bot (Oct 19 2026 at 18:47):

cfallin removed the review request for cfallin from PR #14797.

Wasmtime GitHub notifications bot (Oct 19 2026 at 18:47):

cfallin requested fitzgen for a review on PR #14797.

Wasmtime GitHub notifications bot (Oct 19 2026 at 18:47):

cfallin commented on PR #14797:

@fitzgen would you mind reviewing, since you have a more detailed mental model of this code?

Wasmtime GitHub notifications bot (Oct 22 2026 at 14:28):

:memo: fitzgen submitted PR review:

Thanks for the PR @nv3ifu.

Can you run our Sightglass benchmark suite comparing this commit to its head? Something like this:

$ cargo build --release -p wasmtime-bench-api
$ cp target/release/libwasmtime_bench_api.so /tmp/issue-14797.so # or .dylib on macos or .dll on windows
$ git co HEAD^
$ cargo build --release -p wasmtime-bench-api
$ cp target/release/libwasmtime_bench_api.so /tmp/baseline.so
$ cd /path/to/sightglass
$ cargo run -- benchmark -e /tmp/baseline.so -e /tmp/issue-14797.so -- benchmarks/default.suite

I am concerned about the compile time impacts that this PR might have.

Thanks!

Wasmtime GitHub notifications bot (Oct 22 2026 at 18:07):

:memo: fitzgen submitted PR review:

Some feedback on the code itself.

Thanks again!

Wasmtime GitHub notifications bot (Oct 22 2026 at 18:07):

:speech_balloon: fitzgen created PR review comment:

Blocks are generally dense and reachable, so we can avoid the cost of hashing for the outer map here. The inner maps are sparse, so they seem fine to leave as FxHashMaps.
    /// Regions written by each block before optimization, so eliminated stores
    /// remain conservatively accounted for. `None` means a fence can clobber
    /// every region.
    block_clobbers: SecondaryMap<Block, Option<FxHashSet<AliasRegion>>>,

Wasmtime GitHub notifications bot (Oct 22 2026 at 18:07):

:speech_balloon: fitzgen created PR review comment:

Hard-coding None as the region argument to extent_for is done a few times, but requires that the LastStore is also not Unknown, or else this could lead to miscompiles. The existing call sites are sound, but this feels pretty subtle and like something we could easily accidentally mess up in the future.

I think we could avoid this footgun by changing extent_for into something like this:

    fn extent_for(&self, func: &Function, inst: Inst) -> u32 {
        match self.stores.get_last_store(func, inst) {
            // Each of these already names a version of memory all on its own,
            // function wide, and so doesn't need an additional extent token.
            LastStore::NoStore | LastStore::Inst(_) => NULL_EXTENT,

            // This one, on the other hand, only names a version of memory
            // relative to the control-flow join that lost track of it.
            LastStore::Unknown => {
                debug_assert_ne!(self.extent, NULL_EXTENT);
                func.dfg
                    .insts[inst]
                    .alias_region(&func.dfg)
                    .map_or(self.extent, |region| self.region_extents[region])
            }
        }
    }

Wasmtime GitHub notifications bot (Oct 22 2026 at 18:07):

:speech_balloon: fitzgen created PR review comment:

Please factor this out into helper method(s).

Wasmtime GitHub notifications bot (Oct 22 2026 at 18:07):

:speech_balloon: fitzgen created PR review comment:

We could avoid allocating a hash set on every push_scope call to this function, if this instead

took a clobbers: &mut FxHashSet<AliasRegion> parameter

debug_assert!(clobbers.is_empty()) at the start of the function

returned bool describing whether the clobbers is sound or not (i.e. false is semantically equivalent to today's None)

and then we would also add an AliasAnalysis::clobbers_scratch: FxHashSet<AliasRegion> field that we always use to pass to this function, .clear()ing it just before the call.

Wasmtime GitHub notifications bot (Oct 22 2026 at 18:07):

:speech_balloon: fitzgen created PR review comment:

Rather than cloning region_extents twice in push_scope, can we instead:

clone it only once, for putting it into Scope

not have a MemoryState::region_extents field

in the places where we access MemoryState::region_extents today, instead access alias_analysis.scopes.last().unwrap().region_extents

?

This may require making MemoryState have a lifetime parameter and hold a reference to its AliasAnalysis, but I think that should be pretty straightforward. If not, then we can revisit.

Wasmtime GitHub notifications bot (Oct 24 2026 at 11:36):

nv3ifu updated PR #14797.

Wasmtime GitHub notifications bot (Oct 24 2026 at 11:43):

:memo: nv3ifu submitted PR review.

Wasmtime GitHub notifications bot (Oct 24 2026 at 11:43):

:speech_balloon: nv3ifu created PR review comment:

Updated to derive the state and region from the instruction.

Wasmtime GitHub notifications bot (Oct 24 2026 at 11:48):

:memo: nv3ifu submitted PR review.

Wasmtime GitHub notifications bot (Oct 24 2026 at 11:48):

:speech_balloon: nv3ifu created PR review comment:

Indeed, I've changed the outer map to SecondaryMap.

Wasmtime GitHub notifications bot (Oct 24 2026 at 11:50):

:memo: nv3ifu submitted PR review.

Wasmtime GitHub notifications bot (Oct 24 2026 at 11:50):

:speech_balloon: nv3ifu created PR review comment:

Added

Wasmtime GitHub notifications bot (Oct 24 2026 at 12:05):

:memo: nv3ifu submitted PR review.

Wasmtime GitHub notifications bot (Oct 24 2026 at 12:05):

:speech_balloon: nv3ifu created PR review comment:

I moved the lookup function from MemoryState to AliasAnalysis. This lets us remove the region_extents table from MemoryState. Does this approach look reasonable to you?

Wasmtime GitHub notifications bot (Oct 24 2026 at 12:08):

:memo: nv3ifu submitted PR review.

Wasmtime GitHub notifications bot (Oct 24 2026 at 12:08):

:speech_balloon: nv3ifu created PR review comment:

Moved this logic into the compute_region_extents helper method.

Wasmtime GitHub notifications bot (Oct 24 2026 at 12:17):

nv3ifu commented on PR #14797:

Thanks for the review！I have updated the code.
I have run the default Sightglass benchmark suite on the updated version against the PR’s parent commit and attached the reports(hardware:9950x os:windows):
effect-size.txt
summarize.txt

Wasmtime GitHub notifications bot (Oct 26 2026 at 17:57):

:memo: fitzgen submitted PR review:

Thanks!

It looks like this is providing a real speedup on at least one of our benchmarks, but is slowing down compilation of a few benchmarks a little bit. I think that it is worth taking right now, especially given the requested change below, which should slightly help out compilation times. Will merge after that final nitpick is addressed.

If you are interested in investigating ways to speed up alias analysis while preserving its functionality as follow up work after this PR merges, that would be very appreciated.

Thanks again!

Wasmtime GitHub notifications bot (Oct 26 2026 at 17:57):

:speech_balloon: fitzgen created PR review comment:

Let's also reuse pending and visited across each call to clobbers_since, the same way we are doing with clobbers, so that we don't make new allocations for each call.

Sorry, should have caught this in the first review.

Wasmtime GitHub notifications bot (Oct 27 2026 at 18:15):

nv3ifu updated PR #14797.

Wasmtime GitHub notifications bot (Oct 27 2026 at 18:16):

nv3ifu commented on PR #14797:

Thanks a lot for your review! This makes me learn a lot about perf,I'd be happy to look into further alias analysis optimizations after this PR merges.
I have update the code to reuse their allocations.
Compared to the "no reuse" version:
Compilation time decreased by 1.08% for shootout-heapsort and 0.41% for dotnet-raytracer.
Compilation time decreased by about 0.25% on average.
Thanks again.

Wasmtime GitHub notifications bot (Oct 27 2026 at 18:16):

:memo: nv3ifu submitted PR review.

Wasmtime GitHub notifications bot (Oct 27 2026 at 18:16):

:speech_balloon: nv3ifu created PR review comment:

updated code to reuse

Wasmtime GitHub notifications bot (Oct 27 2026 at 18:24):

:thumbs_up: fitzgen submitted PR review:

Thank you!

Wasmtime GitHub notifications bot (Oct 27 2026 at 18:24):

fitzgen has enabled auto merge for PR #14797.

Wasmtime GitHub notifications bot (Oct 27 2026 at 18:26):

fitzgen added PR #14797 Cranelift: preserve unchanged alias-region extents across joins to the merge queue.

Wasmtime GitHub notifications bot (Oct 27 2026 at 18:49):

:check: fitzgen merged PR #14797.

Wasmtime GitHub notifications bot (Oct 27 2026 at 18:49):

fitzgen removed PR #14797 Cranelift: preserve unchanged alias-region extents across joins from the merge queue.

Last updated: Oct 31 2026 at 23:03 UTC