Repository navigation
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: map range iterators in typed stack storage
This is a focused, well-executed change. It replaces the per-range heap iterator allocation (NewMapIter) with a single caller-owned, entry-block stack slot that is re-zeroed on each range entry and initialized in place by the new MapIterInit. I reviewed correctness, memory safety, GC-root behavior, performance, and documentation across ssa/datastruct.go, ssa/memory.go, the GC-root machinery, and runtime/internal/runtime/{z_map,map}.go.
No correctness, memory-safety, or security defects found. Specific points that hold up well:
- Slot lifetime/reuse is correct. The alloca is hoisted to the entry block (one slot per
Rangeinstruction, no per-iteration stack growth), while the zeroing memset is emitted at the range-entry site, so the slot is cleared on every re-entry. The SSA test asserts the right slot/init counts (sum:1, nested:2, repeated:1). - Re-zeroing is load-bearing, not just defensive.
mapiterinitearly-returns for nil/empty maps leavingit.h/it.key/it.elemuntouched, so pre-zeroing is what keepsMapIterNextfrom reading stale pointers across reuse — exactly whatTestMapRangeRepeatedInitializationprotects. - GC roots are preserved on explicit-root (wasm) targets.
Alloc(..., false)registers the iterator's pointer fields viaregisterStackRoots, keeping the map and its (even pointer-free) overflow buckets live across GC during iteration. - Test coverage is targeted at the genuinely tricky cases this change introduces (reuse across nil/empty maps, nested iterators, and GC interaction / grow), rather than snapshotting incidental IR.
The two inline comments are minor documentation clarifications around implicit couplings; neither requires a logic change.
Minor, non-blocking follow-ups (no inline anchor):
ssa/datastruct.go:1034— the slot type is derived asElem(lastParamType(..., MapIterInit)), i.e.MapIterInit's last parameter stripped of its pointer. This is a deliberate coupling to the runtime signature, but it is non-obvious at the call site; a one-line note that this resolves to*llgoMapIterwould save a cross-reference.- On explicit-root (wasm) targets the iterator slot is zeroed twice (once at the range-entry site, once in the entry-block
initializeStackRoots). This is correctness-neutral and bounded (perRange, not per iteration), but may be worth a follow-up to elide the redundant entry-block zeroing when the object's ownAllocalready emits a dominating zeroinit.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
String equality benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Map range initializes a zeroed, caller-owned iterator in a fixed entry-block stack slot, registering its pointer fields on explicit-root targets.
The immutable deferred-capture case in #3033 is already allocation-free after #3021; this fixes the remaining iterator allocation.
Validation: six-target IR checks; Go and native LLGo (-O0 and -O3/full LTO, three repetitions); actual J32/J64 (-O2) and W32 (-O0) execution; SSA/escape tests and complete Testgo/Testrt/Testmeta compiler fixtures. Regressions cover repeated initialization, nested iteration, clear/grow and GC, including pointer-free overflow buckets.
J32/J64's full test/go package at -O0 hits Node's unicode.init local-count limit, also reproduced on unchanged main; those execution checks use -O2.
Fixes #3033.