perf(interp): hoist loop-invariant container state in JIT loops#154
Conversation
An entryLoop trace plan now hoists one loop-invariant container: hoistable picks the most-accessed ref local whose recorded ARRAY_GET/ARRAY_SET steps agree on one primitive typed-array itab, provided no block writes the local and the plan is call-free. The backend prologue derives the heap cell, tag and itab guard, and slice header once per native entry, so matching accesses keep only the bounds check and element op, and a hoisted primitive ARRAY_SET stops being a state barrier. A prologue guard exit resumes at the loop header, whose dispatch slot holds the native stub; the loop wrapper now runs the shadowed threaded handler once for any fallback that resumes at the header, so such exits cannot livelock. Sieve's loop callables shrink ~40% (vm_jit_entry_bytes_total 179,992 -> 107,168 and 209,804 -> 124,644); wall time is unchanged within noise on M4 Pro because the removed loads sat off the out-of-order critical path. The remaining gap is native entry/exit churn (loop-kind branch legs are never folded) and the per-iteration operand flush, tracked as follow-ups. Closes #153.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughLoop plans detect invariant array containers. ARM64 loop entries hoist guarded slice headers for array accesses, while interpreter fallback prevents header-resume livelocks. Tests and documentation cover eligibility, execution parity, guards, errors, and benchmark results. ChangesLoop-invariant container hoisting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LoopPlan
participant arm64Lowerer
participant NativeLoop
participant Interpreter.loop
participant ThreadedHandler
LoopPlan->>arm64Lowerer: provide hoist descriptor
arm64Lowerer->>NativeLoop: guard container and cache slice header
NativeLoop->>NativeLoop: reuse cached header for array accesses
NativeLoop->>Interpreter.loop: deopt at loop header
Interpreter.loop->>ThreadedHandler: advance on same-header resume
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
==========================================
+ Coverage 29.57% 29.63% +0.06%
==========================================
Files 86 86
Lines 60187 60292 +105
==========================================
+ Hits 17801 17870 +69
- Misses 41119 41146 +27
- Partials 1267 1276 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/jit-internals.md`:
- Line 346: Update the trace-loop hoisting description around “every matching
access” to explicitly state that hoisted I64 loads retain their value/boxability
side exit, while terminal primitive ARRAY_SET operations continue through the
existing path. Keep the documentation terse, factual, and consistent with the
surrounding formatting.
In `@interp/jit_arm64.go`:
- Line 4465: Update the LDR emission in the surrounding hoisted-local handling
to avoid silently truncating (f.base+h.local)*8 when the layout exceeds the
encodable immediate range. Prefer the existing register-offset addressing path
for oversized offsets, or explicitly reject layouts beyond 4,095 slots while
preserving the current immediate path for valid offsets.
In `@interp/jit_plan.go`:
- Around line 405-447: Filter hits before selecting best in the hoist-ranking
logic so only itabs supported by the ARM64 prologue and non-terminal-store
candidates can win the single hoist slot. Update the ARRAY_GET/ARRAY_SET hit
collection or selection around best and bestHits, preserving valid
primitive-array candidates when hotter unsupported ref-array or terminal-store
candidates exist. Add a regression test covering mixed ref/primitive candidates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f044da9-5d3f-45a7-9904-17e0d525289c
📒 Files selected for processing (9)
AGENTS.mddocs/benchmarks.mddocs/jit-internals.mdinterp/interp.gointerp/jit.gointerp/jit_arm64.gointerp/jit_arm64_test.gointerp/jit_plan.gointerp/jit_plan_test.go
Summary
Implements #153: an
entryLooptrace plan now hoists one loop-invariant container's derivation out of the loop body into a per-entry prologue.interp/jit_plan.go):hoistablepicks the most-accessed ref local whose recordedARRAY_GET/ARRAY_SETsteps agree on one primitive typed-array itab, provided no block writes that local and the plan contains noCALL/RETURN_CALL(aBLwould clobber the hoisted registers). Container provenance comes from a conservative per-block marker stack; anything unknown declines.interp/jit_arm64.go,arm64Lowerer.hoist): once per native entry — load the container local, tag + itab guard against an empty-snapshot exit resuming at the header, loaddataPtr/leninto registers that stay live for the whole body. Safe with no pinning because a call-free trace loop callable has only forward control flow.ARRAY_GETkeeps only its bounds exit,guardIndex, and the element load (shared tail; only the(dataPtr, n)source branches). A hoisted primitiveARRAY_SETkeeps bounds + store and stops being a state barrier (noflush(flushSnapshot)+ local-cache clear). Non-matching containers, ref arrays, globals/upvals, and terminal stores use the existing paths byte-for-byte.interp/interp.go): a loop fallback exit that resumes at the header itself made no progress, and the header dispatch slot holds the native stub — redispatching would livelock. The loop wrapper now runs the shadowed threaded handler (i.exits[root]) once. The hoist prologue's shape guard is the first exit that can resume at the header; covered by a regression test that alternates the container between an array and null across entries.Measured effect (honest)
vm_jit_entry_bytes_total179,992 → 107,168 (inner) and 209,804 → 124,644 (scan).Sieve(256)wall time is unchanged within noise (interleaved A/B on M4 Pro: ~1–2%). The removed per-access loads sat off the out-of-order critical path, so the issue's projected 2.5–3 µs did not materialize.TypedArraySum(256)andAllocationGraph(128)unchanged (interleaved A/B).Test plan
go test -race ./...(root andbenchmarks/modules)TestARM64_HoistedContainerLoop: native execution with onlyloop-exitexits, prologue shape deopt with exact refcounts, mid-loop bounds deopt with identical error,LOCAL_SET-in-loop rejection, mixed hoisted + slow-path containers — all against a threaded oracle comparing results andrc[1:]TestHoistableplan-level unit testsmake fuzz,make lintmake coverage-check— fails from the pre-existing goenv go1.25/go1.26 toolchain split, unrelated to this diffCloses #153.
Summary by CodeRabbit
Performance
Bug Fixes
Tests
Documentation