perf: jit fusion 8phase#106
Conversation
…ating host calls Decoded byte/uint16 operands (LOCAL_GET/SET/TEE, GLOBAL_GET/SET/TEE, CONST_GET, REF_TEST/CAST, ARRAY_NEW[_DEFAULT], STRUCT_NEW[_DEFAULT], MAP_NEW[_DEFAULT], and the arm64 JIT's global lowering) decode idx as unsigned at compile time, so the idx < 0 half of their bound checks can never trigger; only the upper-bound half is reachable and is kept unchanged. Stack-derived runtime indices (ARRAY_SET/ GET/FILL/COPY/DELETE, STRUCT_GET/SET) are untouched since those genuinely can be negative. Also moves the i.fp == len(i.frames) frame-overflow check in the threaded CALL handler (and removes its duplicate in fuseHostFunction's fused CALL path) so it only gates the *types.Function/*types.Closure arms. Host calls never push a VM frame, so they should never be blocked by frame exhaustion; RETURN_CALL's fused host path already had no such check, this makes CALL consistent with it. Phase 1 of an 8-phase perf overhaul (issue #104); pure dead-code removal + a call-path reorder, no behavior change for function/closure overflow (regression test still asserts ErrFrameOverflow). Added a host-call-succeeds-under-frame- exhaustion case to TestWithFrame alongside the existing regression case. Bench (5-sample, Apple M4 Pro, arm64), flat vs. pre-phase baseline as expected: BenchmarkFib35/minivm_interp-12 ~670-675 ms/op 432 B/op 3 allocs/op BenchmarkFib35/minivm_jit-12 ~46.2-46.5 ms/op 8733 B/op 81 allocs/op BenchmarkJITIssue101/interp-12 ~1740-1775 ns/op BenchmarkJITIssue101/jit-12 ~2842-2869 ns/op BenchmarkJITIssue60/indirect_call_fib_via_local/{interp,jit}-12 ~819-822 us / ~57.0-57.5 us BenchmarkJITIssue60/closure_counter_loop/{interp,jit}-12 ~17.3-17.4 us / ~1.12-1.13 us BenchmarkJITIssue60/typed_array_sum/{interp,jit}-12 ~35.6-36.9 us / ~2.55-2.56 us Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 2 of the minivm performance overhaul (issue #104 + extra findings). - types.Traceable.Refs() -> Refs(dst []Ref) []Ref (append-style); callers in interp (release/trace/mark/sweep) now route through a shared *Interpreter scratch buffer via a new refs() helper instead of allocating a fresh slice per Traceable walk. - Interpreter.Reset caches the ref-constant heap-base count once at construction instead of rescanning constants on every pooled Reset. - wrapFunc (host function marshaling) caches its reflect arg-slot storage and return-value seen map across calls instead of allocating fresh per call; safe because HostFunction closures are never shared across Interpreters and recursive vm.Run from a host function is a documented invariant violation. - MapIterator no longer uses reflect.MapRange per Next(); keys/entries are snapshotted once at construction. Attempted a 5th change (reusing the release() worklist as an *Interpreter field) but reverted it after benchmarking: it regressed BenchmarkJITIssue60/closure_counter_loop/jit by ~25-45% (verified against a live re-run of the prior commit, ruling out thermal noise) for no measurable allocation win, since Go's escape analysis already kept the original local slice cheap for this shallow-worklist workload. unboxI64's release/re-box round-trip (a 6th planned change) was investigated and left alone: the interpreter's free-list already reuses the just-freed slot in the common case, so a bespoke in-place rewrite would add refcount-invariant risk for marginal further gain. Benchmarks (Apple M4 Pro, vs a live re-run of the prior commit to control for session-long thermal drift): - Fib35 interp/jit, JITIssue101, indirect_call_fib_via_local, typed_array_sum: flat - closure_counter_loop interp/jit: same latency, allocs 4->2 per op - types.BenchmarkTypedArray_Refs/child_refs: 7.58ns/1 alloc -> 2.08ns/0 allocs - types.BenchmarkMap_Refs/child_refs: 30.5ns/1 alloc -> 24.9ns/0 allocs - types.BenchmarkStruct_Refs/child_refs: 7.41ns/1 alloc -> 1.23ns/0 allocs go test -race ./... green across all 14 packages. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Task reviewer for the Phase 2 commit found a real GC-correctness
regression in the snapshot-based MapIterator: for a generic *Map, the
construction-time snapshot embeds ref-typed key/value components
without retaining them, so a MAP_DELETE on an entry still queued in
the snapshot (not yet visited) could free it via the map's own
refcount before the iterator reached it - a dangling ref / potential
use-after-free. reflect.MapRange over the live map has no such
hazard: Go guarantees a key deleted mid-range is simply never
produced, matching docs/instruction-set.md's existing MAP_ITER
description ("without snapshotting keys"), which was correct all
along and did not need updating.
Also fixes a Minor finding: refsScratch (added in the Phase 2 commit)
was separated from the rest of the runtime-state field group by a
stray blank line; moved adjacent to rc with no separator, per
docs/coding-patterns.md §2.5.
go test -race ./... green across all 14 packages. types Refs/TypedMap_Get
benchmarks unaffected (0 allocs/op preserved for child_refs cases).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ialize UPVAL scalar retain/release Phase 3 of the interpreter/JIT performance overhaul (issue #104 + extra findings): collapses three near-duplicate host-call arg release loops into Interpreter.releaseArgs, and five duplicated typed-array bounds-check blocks (ARRAY_SET/FILL/DELETE/COPY, arrayGetAt) into Interpreter.checkBounds. fuseHostFunction now proves at compile time whether every host param is a non-ref, non-promotable scalar (I32/I1/I8/F32/F64 via Kind.Repr()) and emits a closure that skips the arg-release scan entirely when so; I64 stays excluded because it can promote to a heap ref, and callHost's runtime- dispatched call site (target unknown until CALL executes) keeps the generic scanning path unconditionally. UPVAL_GET/UPVAL_SET gain the same compile-time scalar specialization as the existing LOCAL_GET/SET/TEE, based on Function.Captures' declared per-slot kind (threadedCompiler.captures, threaded through Compile's 3 call sites). I64 and KindRef captures keep the generic ref-aware path. GLOBAL_GET/SET/TEE intentionally left unchanged: globals have no static per-index type (docs/verification.md, program/verify.go), so there is nothing to prove at compile time, and retainBox/releaseBox's runtime check is already fully inlined at every call site (confirmed via `go build -gcflags=-m`) -- a hand-written runtime dispatch would duplicate that check for no measurable benefit. bench (Apple M4 Pro): Fib35 interp/jit and JITIssue60/101 match baseline; closure_counter_loop improves (72B/4 allocs -> 56B/2 allocs, ~16800ns -> ~15100ns interp) from the UPVAL fast path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 3 task review found the brief's required UPVAL_GET+ref-capture retain test was missing (only UPVAL_SET's ref/promoted-i64 cases were covered). Adds a WithTick(1)+WithHook sample of the capture's rc across every tick, proving retainBox's copy briefly holds a second owner alongside the closure's own capture slot. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…+BR_IF fusion
Phase 4 of the interpreter-threaded fusion plan. Finishes work an earlier
session left uncommitted (fuseLocalConst/fuseLocalLocal flattening) and
implements the new comparison+BR_IF superinstruction category.
Fixed two bugs found in the already-written flattening code:
- fuseLocalLocal (LOCAL_GET a; LOCAL_GET b; binop) was implemented but never
wired into LOCAL_GET's compile function -- dead code, now wired in.
- Every fuseLocalLocal{I32,I64,F32,F64} case advanced i.fr.ip by 3 instead of
5 (the fused window is LOCAL_GET(2)+LOCAL_GET(2)+binop(1)=5 bytes), landing
execution mid-instruction. This crashed optimize.TestOptimizer_Optimize's
O3 subtests as soon as the dead code above was activated; fixed by
correcting all 80 occurrences.
Added the new comparison+BR_IF fusion: I32/I64 EQZ + 10 comparisons, F32/F64
6 comparisons, and I32_CONST;BR_IF, via two shared helpers (peekBrIf,
Interpreter.branchIf) so the branch-taking logic exists once instead of once
per opcode. The 4-instruction LOCAL_GET+CONST+cmp+BR_IF composition falls out
naturally by extending fuseLocal{I32,I64,F32,F64}Const's comparison cases
with the same peek, no bespoke closure needed.
Added tests for all of the above, including two branch-into-fused-window
tests proving the probe-then-restore c.ip discipline holds when a separate
branch lands directly on a byte offset a fusion consumed at runtime.
Bench (Apple M4 Pro, -count 5, vs parent commit fb3888b):
Fib35 interp-only: 659.8ms -> 501.6ms avg (~24% faster, within the
plan's expected 10-25% range)
Fib35 JIT mode: 46.86ms -> 46.29ms avg (~1.2%, parity as expected;
this phase doesn't touch JIT lowering)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 5 of the threaded/fusion/JIT plan. Adds SBFX encoding/builder support and uses it for boxed i64 sign-extension and boxability checks. Reworks ARM64 guard lowering so common failure paths branch to deferred side exits, routes BR_IF divergence through branchOrExit labels, shares guardHeap/guardItab/guardIndex/guardRC, and reuses one lowering scratch slice for pre-op guard snapshots. The marked check now blocks only raw ref markers, allowing learned continuations over materialized ref operands. Tests cover SBFX encoding, learned continuations over live materialized refs, and negative-index ARRAY_GET deopt through the unsigned guard. The rc==1 release side-exit was not forced with a white-box refcount mutation because repo test rules forbid fabricating unreachable state via unexported field writes. Benchmarks (Apple M4 Pro): BenchmarkFib35 minivm_jit 53.5-54.1ms, minivm_interp 541.5-544.7ms; BenchmarkJITIssue101 jit 2.94-2.96us; Issue60 indirect-call jit 72.7-73.5us, closure jit 1.14-1.16us, typed_array_sum jit 2.67-2.69us; Refs/TypedMap_Get remain 0 allocs/op. Verification: go test ./asm/arm64 ./interp; go test -race ./...; benchmarks run from benchmarks/ module for BenchmarkFib35 and BenchmarkJIT; go test -bench 'Refs|TypedMap_Get' -benchmem -run xxx ./types.
Phase 6 of the threaded/fusion/JIT plan. Link now writes all code chunks and patches relocations while the active buffer mapping is open, then seals once. Public Buffer.Write keeps the old sealed-on-return contract. Grown-out old mappings are still sealed before archival, and patches to old mappings still use their own writable/executable round-trip. Benchmarks (Apple M4 Pro): BenchmarkFib35 minivm_jit 53.7-54.0ms, minivm_interp 540.8-558.2ms; BenchmarkJITIssue101 jit 2.94-3.04us; Issue60 indirect-call jit 72.7-72.9us, closure jit steady 1.12-1.14us after first sample, typed_array_sum jit 2.70-2.81us; Refs/TypedMap_Get remain 0 allocs/op. Verification: go test ./asm/... ./interp; go test -race ./...; benchmarks run from benchmarks/ module for BenchmarkFib35 and BenchmarkJIT; go test -bench 'Refs|TypedMap_Get' -benchmem -run xxx ./types.
Phase 7 of the threaded/fusion/JIT plan. The tracer now records one narrow input per relevant step, and branch snapshots carry exit hit counts. ARM64 lowering keeps bytecode constants as known i32/i64 facts, removes divide-by-zero guards for known non-zero divisors, guards observed non-zero variable divisors before specializing the hot path, uses immediate masked shifts for known shift counts, orders pending continuations by hot exit count, and reloads i64 operands through the proper boxed-lane sign extension. Observed variable shift guards were tried and removed: they regressed BenchmarkJITIssue60/closure_counter_loop/jit from the steady 1.12-1.14us range to about 1.57-1.64us, so the final code keeps only code-stream known shift specialization. Tracer local-buffer locking was left unchanged because capture replay already runs outside Tracer.mu and a merge buffer would broaden the pool/tracer protocol. Tests cover constant non-zero i32/i64 divisors and variable divisor deopts for alternate non-zero and zero inputs. docs/jit-internals.md now documents speculative observed numeric facts and hot-exit pending ordering. Benchmarks (Apple M4 Pro): BenchmarkFib35 minivm_jit 53.8-55.2ms, minivm_interp 538.4-543.6ms; BenchmarkJITIssue101 jit 2.966-3.019us; Issue60 indirect-call jit 72.6-73.4us, closure jit 1.156-1.167us, typed_array_sum jit 2.690-2.724us; Refs/TypedMap_Get remain 0 allocs/op. Verification: go test ./interp; go test -race ./...; benchmarks run from benchmarks/ module for BenchmarkFib35 and BenchmarkJIT; go test -bench 'Refs|TypedMap_Get' -benchmem -run xxx ./types.
Phase 8 shape-feedback commit from the threaded/fusion/JIT plan. The tracer now records heap shapes for ref, array, struct, error, and coroutine fast paths: heap interface table plus an exact struct type pointer where relevant. ARM64 lowering uses recorded shapes to collapse ref.get and array.len multi-way itab checks into guarded monomorphic paths, validates array get/set shapes, and guards struct get/set against the recorded struct type before using the fields table. Tests warm typed shapes, then run mismatched array and struct shapes and assert both correct threaded results and recorded side-exit hits. docs/jit-internals.md documents recorded heap shape and exact struct type guards. Benchmarks (Apple M4 Pro): BenchmarkFib35 minivm_jit 53.7-54.3ms, minivm_interp 538.7-551.8ms; BenchmarkJITIssue101 jit 2.922-2.977us; Issue60 indirect-call jit 73.1-74.0us, typed_array_sum jit 2.583-2.655us in full run and 2.612-2.651us in focused recount. Full-run closure jit showed a noisy 1.54-1.56us sample, but focused recount was steady 1.120-1.144us. Verification: go test ./interp; go test -race ./...; benchmarks run from benchmarks/ module for BenchmarkFib35 and BenchmarkJIT; focused closure_counter_loop/jit and typed_array_sum/jit recounts; go test -bench 'Refs|TypedMap_Get' -benchmem -run xxx ./types.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates buffered ChangesRefs API buffer-reuse refactor
Interpreter capture, bounds, and fusion changes
JIT lowering, guard paths, and trace shape recording
ARM64 SBFX instruction encoding
Batched buffer writes and relocation patching
Tooling ignore list update
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Sequence Diagram(s)sequenceDiagram
participant Tracer
participant threadedCompiler
participant ARM64Lowerer
participant JITRuntime
Tracer->>threadedCompiler: Compile(code, locals, captures)
threadedCompiler->>ARM64Lowerer: emit fused branches and guards
ARM64Lowerer->>JITRuntime: branchOrExit / guard failures / side exits
JITRuntime->>Tracer: record shapes, legs, and hits
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Allow boxable i64 ARRAY_GET and STRUCT_GET results to remain raw native values so later trace ops can continue without a terminal threaded resume. Guard nonboxable loaded values before committing the read, preserving interpreter-owned heap-promoted i64 boxing. Verification: - go test ./interp - go test -race ./... - benchmarks: go test -bench 'BenchmarkFib35' -run xxx -count 5 . (benchmarks/) - benchmarks: go test -bench 'BenchmarkJIT' -run xxx -count 5 . (benchmarks/) - types: go test -bench 'Refs|TypedMap_Get' -benchmem -run xxx . Benchmark ranges: - Fib35 minivm_interp: 525.7-532.6 ms/op, 15532 B/op, 25 allocs/op - Fib35 minivm_jit: 52.8-53.5 ms/op, 11792 B/op, 97 allocs/op - JITIssue101/jit: 2.904-2.958 us/op, 0 allocs/op - JITIssue60 indirect_call_fib_via_local/jit: 72.8-74.5 us/op, 119-121 B/op, 0 allocs/op - JITIssue60 closure_counter_loop/jit: 1.119-1.169 us/op, 57 B/op, 2 allocs/op - JITIssue60 typed_array_sum/jit: 2.633-2.658 us/op, 8 B/op, 0 allocs/op - Types Refs/TypedMap_Get: 0 allocs/op across measured cases
8c31fcc to
043d9f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
interp/marshal.go (1)
607-609: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: use
SetZero()to reset each argument.
reflect.Zero(...)allocates a fresh zeroValueper parameter per call;in[idx].SetZero()(Go 1.20+) resets in place without that allocation, which is in the spirit of this PR's scratch-reuse changes.♻️ Proposed tweak
for idx := range in { - in[idx].Set(reflect.Zero(fnType.In(idx))) + in[idx].SetZero() if err := unmarshal.value(params[idx], in[idx]); err != nil {🤖 Prompt for 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. In `@interp/marshal.go` around lines 607 - 609, In the argument-reset loop inside marshal.go, replace the per-parameter zero-value assignment on in[idx] with in-place resetting via SetZero() so the scratch slice reuse avoids extra allocations. Keep the existing unmarshal.value flow in the same loop, and make sure the change is applied in the function that prepares fnType.In(idx) inputs before invoking unmarshal.asm/buffer.go (1)
139-142: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDocument the
batch && currentprecondition. The fast path copies directly intomemand depends onwriteBatch’s unsealed window. A short note here would make the invariant explicit.🤖 Prompt for 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. In `@asm/buffer.go` around lines 139 - 142, Document the fast path in writeBatch by adding a short note near the batch && current branch that makes the unsealed-window precondition explicit. Reference writeBatch and the direct mem copy path so it is clear this optimization only applies when the batch is current and the buffer is still writable.
🤖 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 `@interp/interp.go`:
- Around line 283-289: baseHeap is being computed too broadly in Reset and can
overcount constants that do not actually allocate heap slots. Update the logic
around the i.constants scan so it only increments for constants that really
consume heap, and exclude Ref/Boxed cases that go through BoxRef without
allocating. Keep the fix localized to Reset and use the existing Kind/BoxRef
handling in interp.go to align the count with the initialized heap/rc length.
---
Nitpick comments:
In `@asm/buffer.go`:
- Around line 139-142: Document the fast path in writeBatch by adding a short
note near the batch && current branch that makes the unsealed-window
precondition explicit. Reference writeBatch and the direct mem copy path so it
is clear this optimization only applies when the batch is current and the buffer
is still writable.
In `@interp/marshal.go`:
- Around line 607-609: In the argument-reset loop inside marshal.go, replace the
per-parameter zero-value assignment on in[idx] with in-place resetting via
SetZero() so the scratch slice reuse avoids extra allocations. Keep the existing
unmarshal.value flow in the same loop, and make sure the change is applied in
the function that prepares fnType.In(idx) inputs before invoking unmarshal.
🪄 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: 89cc0cf3-41b3-4805-8494-a9c679462d61
📒 Files selected for processing (33)
.gitignoreasm/arm64/encoder.goasm/arm64/encoder_test.goasm/arm64/instr.goasm/buffer.goasm/link.godocs/architecture.mddocs/jit-internals.mdinterp/coroutine.gointerp/coroutine_test.gointerp/fuse.gointerp/host.gointerp/host_test.gointerp/interp.gointerp/interp_test.gointerp/jit.gointerp/jit_arm64.gointerp/marshal.gointerp/threaded.gointerp/trace.gotypes/array.gotypes/array_test.gotypes/closure.gotypes/closure_test.gotypes/error.gotypes/error_test.gotypes/map.gotypes/map_test.gotypes/string.gotypes/string_test.gotypes/struct.gotypes/struct_test.gotypes/value.go
|
Overall, the direction looks good. In particular, the PR correctly targets real hot paths: removing unreachable guards, fixing the host-call frame check, eliminating Reset/Refs allocations, unifying host argument cleanup, adding compare+BR_IF fusion, and allowing i64 get traces to continue. That said, the PR’s optimization scope is large, and the amount of added code and abstraction complexity is significant. Some commits also appear to introduce possible performance regressions based on the benchmark numbers included in the PR itself. I think the following items should be checked or addressed before merge.
Looking at the benchmark numbers in the commit messages for the same benchmark, BenchmarkFib35/minivm_jit is around 46.2 The JIT allocation numbers also appear to increase from 8733 B/op, 81 allocs/op in the earlier benchmark to 11792 B/op, 97 allocs/op in the final benchmark. Since this is a performance PR, an increase in both runtime and allocations on a hot JIT benchmark needs an explanation before merge. There may be differences in measurement conditions, thermal noise, or benchmark setup, so please include:
The regression appears to start around share ARM64 guards and cold exits, so that commit may be safer as a separate PR unless the cause is identified and justified.
In interp/interp.go, checkBounds(offset, size, length) currently uses this shape: if offset < 0 || offset+size > length { This does not explicitly reject size < 0, and it is also vulnerable to offset + size overflow. For paths like ARRAY_FILL and ARRAY_COPY, where size comes from the stack, a negative size may pass this check if offset+size <= length, potentially leading to a slice panic or incorrect behavior later. Since the bounds logic is now centralized, I suggest using the safer form: if offset < 0 || size < 0 || size > length-offset { It would also be good to add tests for:
The commit perf(interp): flatten local-const/local-local fusions, add comparison+BR_IF fusion adds a very large amount of code across fuseLocalI32Const, fuseLocalI64Const, fuseLocalF32Const, fuseLocalF64Const, and the fuseLocalLocal* family. The optimization is useful, but the implementation repeats the same structure many times:
The commit message mentions fixing “80 occurrences” of the wrong fused window width. That is a strong signal that this manual matrix is easy to get wrong. I recommend simplifying this using one of the following approaches:
The current structure may be fast, but it is likely to be fragile whenever a new opcode or fusion pattern is added.
The JIT lowering code replaces fresh copies with a reused scratch slice: ctx.saved = append(ctx.saved[:0], ctx.values...) This can reduce allocations, but if the returned slice is stored or used later by sideExit, releaseRef, releaseBox, or any deferred side-exit path, a later pre() call may overwrite the same backing array and corrupt the previous snapshot. If sideExit immediately emits code and does not store the slice, this may be safe. But the name pre() makes it look like a stable snapshot, while it is actually scratch storage. Please consider one of the following:
In perf(jit): guard heap reads with recorded shapes, trace shape stores a struct type pointer as uintptr: type shape struct { The code records it using something like uintptr(unsafe.Pointer(s.Typ)), then later uses it as a JIT guard. Storing a Go object pointer as uintptr for a long lifetime is unsafe because the GC cannot see it as a live pointer. This may appear to work today because Go’s GC is non-moving, but it is still not a good invariant to rely on. Safer options:
Unsafe code may be acceptable in this part of the VM, but hiding a long-lived Go pointer from the GC should be avoided.
The ARM64 lowering now has several related guard helpers:
Their responsibilities overlap across tag checking, heap cell loading, itab/type guarding, and side-exit wiring. This means callers must remember the correct combination of helpers for each path, which makes the abstraction less reliable. A simpler design might be a single guardedHeapValue(...) path that validates a heap ref, loads the heap cell, and optionally applies shape/type guards. That would keep the invariant in one place instead of spreading it across call sites.
The direction in perf(asm): batch executable buffer sealing during link is good. However, Link now builds an intermediate chunks := make([][]byte, len(codes)), and writeBatch also allocates bases := make([]unsafe.Pointer, len(codes)). Since this PR is partly about removing allocation hotspots, the intermediate chunks slice is avoidable. writeBatch could accept []*Code directly or use an accessor callback so it can iterate over c.Bytes without building another slice.
The host function wrapper now reuses the reflect argument slice and seen map across calls. This is good for allocations, but the closure is now stateful. If the host function wrapper is guaranteed to be called sequentially within a single interpreter, and recursive VM entry from a host function is forbidden, this is probably okay. But if that invariant is ever violated, this can lead to:
If this is intentionally forbidden by design, please make that constraint explicit in comments and tests. Otherwise, the scratch state should probably move to a call frame or remain per-call.
The trace continuation path now sorts pending continuations by hit count. If the pending list is small, this is probably fine. But sorting a suffix repeatedly inside the lowering loop could add unnecessary compile-time cost. Please include a trace compile-time benchmark or document the bound that makes this safe. If the pending limit is small and intentional, a comment near the sorting logic would help.
The added paragraph in docs/architecture.md is useful, but it packs too many concepts into one long paragraph. It would be much easier to review and maintain if split into sections such as:
The same concern applies to some code comments: optimization rationale and current implementation details should be separated where possible, so future performance regression analysis is easier. Per-commit summary
Conclusion I agree with the optimization direction, but this PR should not be merged yet without clarifying the JIT time/allocation regression and tightening the safety of a few new abstractions. Minimum items I would like to see before merge:
Once these are addressed, the PR’s optimization intent should be much safer to merge. |
…reter races
wrapFunc's doc comment claimed a marshaled *HostFunction is never
cached/shared across Interpreters or goroutines, so it closed over
call-scoped `in []reflect.Value` and `seen map[uintptr]bool` and reused
them across every invocation. That premise is false: a *HostFunction
returned by vm.Marshal(fn) is a types.Value, and program.WithConstants
stores it directly (New's default constant case does
`types.BoxRef(i.keep(v))`, which keeps the Go value itself, not a
copy). Two pooled Interpreters built from the same program therefore
share one *HostFunction, and concurrent calls from separate goroutines
raced on `in[idx].Set(...)` and `clear(seen)`.
Fix moves `in` and `seen` inside the returned closure so each call
gets fresh locals - no locks or sync.Pool needed.
Verification:
- go build ./... && go vet ./... clean.
- Added TestInterpreter_Marshal/marshaled_function_is_shared_and_race-safe_across_interpreters
in interp/interp_test.go: marshals a Go func, builds two programs that
reference the same *HostFunction via WithConstants, runs two
Interpreters concurrently, asserts both results correct.
- Confirmed the race before the fix: `go test -race -run TestInterpreter_Marshal
./interp -count=5` reported a concurrent map write and a concurrent
reflect.Value.SetInt race, and failed assertions (expected 3 got 2,
expected 30 got 20) from cross-contaminated scratch.
- After the fix: `go test -race ./interp` passes (5/5 repeats of the
new subtest, full package race-clean).
- Also corrected a stale host_test.go comment ("wrapFunc caches it
across calls") that asserted the same now-removed caching behavior.
Deviation: task instructions named interp/marshal_test.go for the new
test, but no such file exists and none ever has (checked git log
--all). docs/coding-patterns.md §6.2 states Interpreter methods'
tests belong in interp_test.go even when implemented in a smaller
supporting file - marshal.go only supports Interpreter.Marshal's
public behavior - so the subtest was added to the existing
TestInterpreter_Marshal in interp_test.go instead of a new file.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
- Coverage 52.57% 46.34% -6.24%
==========================================
Files 77 77
Lines 14684 17390 +2706
==========================================
+ Hits 7720 8059 +339
- Misses 6125 8408 +2283
- Partials 839 923 +84 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
interp/threaded.go (1)
5123-5127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick wininterp/threaded.go:5123 — reject negative sizes in
bounds
boundsstill accepts negativesize, soARRAY_COPYcan reachcopy(dst[dstOffset:dstOffset+size], ...)with a negative slice bound and panic at runtime instead of raisingErrIndexOutOfRange.ARRAY_FILLalso treats negative sizes as a no-op. Use an overflow-safe check that rejectssize < 0too.🤖 Prompt for 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. In `@interp/threaded.go` around lines 5123 - 5127, The bounds helper currently only checks offset and overflow past length, so negative size values can slip through and cause runtime slice panics or silent no-ops in callers like ARRAY_COPY and ARRAY_FILL. Update the Interpreter.bounds check to explicitly reject size < 0 in addition to the existing range validation, and make the validation overflow-safe so offset+size cannot wrap. Keep the fix localized to bounds so all callers inherit the corrected ErrIndexOutOfRange behavior.interp/jit_arm64.go (2)
2539-2638: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftConsider a shared helper for the repeated heap-guard/RC boilerplate in
arrayGet/structGet/structSet.All three functions repeat the same
pre()→box→sideExit→guardHeap/guardItab→guardIndex→guardRC/retainBoxsequence with only the element-read/write logic differing. The divergence found betweenarrayGetandstructGet's retain/rc ordering (see the critical comment onstructGet) is a direct consequence of this duplication. Extracting the shared guard/RC sequence into one helper would remove this class of copy-paste drift risk.🤖 Prompt for 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. In `@interp/jit_arm64.go` around lines 2539 - 2638, Extract the shared lowering/GC safety sequence used by arrayGet, structGet, and structSet into a helper so the guard and reference-counting logic stays consistent. Move the repeated pre()/box()/sideExit()/guardHeap()/guardItab()/guardIndex()/guardRC()/retainBox flow behind a common function, and keep only the element load/store specifics in the callers. Make sure the helper preserves the correct ordering for rc updates and box retention so arrayGet and structGet cannot drift apart again.
2712-2775: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMove the ref-field retain after the RC store
structGetretainsresultbefore it decrements/stores the container RC, so a self-referential field can have its increment overwritten by the laterSTRR. This drops the retain and can free the object too early.🤖 Prompt for 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. In `@interp/jit_arm64.go` around lines 2712 - 2775, structGet is retaining ref fields too early, before the container’s reference count is decremented and written back, which can overwrite the increment for self-referential values. Update arm64Lowerer.structGet so the RC decrement/store via guardRC, SUBI, and STRR happens before calling retainBox on the loaded result when out is types.KindRef. Keep the existing guards and result handling intact, but move the ref-retain to after the RC store using the same result and rcBase/addr flow.
🧹 Nitpick comments (1)
asm/buffer_test.go (1)
45-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnchecked
Freeerror (pre-existing pattern, static-analysis flagged).Static analysis flags
defer b.Free()(Line 48) for its unchecked error return. Same pattern already exists elsewhere in the file (e.g. earlier tests), so this is consistent style rather than a new regression — flagging only because the linter surfaced it on this diff.🤖 Prompt for 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. In `@asm/buffer_test.go` around lines 45 - 60, Unchecked error from Buffer.Free in the test cleanup is being ignored. Update the deferred cleanup in the NewBuffer-based test to capture and assert the error returned by b.Free rather than discarding it, using the same test helper style already used around writeBatch and patch. Keep the fix localized to this test so the buffer is still freed while satisfying the linter.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@interp/jit_arm64.go`:
- Around line 2539-2638: Extract the shared lowering/GC safety sequence used by
arrayGet, structGet, and structSet into a helper so the guard and
reference-counting logic stays consistent. Move the repeated
pre()/box()/sideExit()/guardHeap()/guardItab()/guardIndex()/guardRC()/retainBox
flow behind a common function, and keep only the element load/store specifics in
the callers. Make sure the helper preserves the correct ordering for rc updates
and box retention so arrayGet and structGet cannot drift apart again.
- Around line 2712-2775: structGet is retaining ref fields too early, before the
container’s reference count is decremented and written back, which can overwrite
the increment for self-referential values. Update arm64Lowerer.structGet so the
RC decrement/store via guardRC, SUBI, and STRR happens before calling retainBox
on the loaded result when out is types.KindRef. Keep the existing guards and
result handling intact, but move the ref-retain to after the RC store using the
same result and rcBase/addr flow.
In `@interp/threaded.go`:
- Around line 5123-5127: The bounds helper currently only checks offset and
overflow past length, so negative size values can slip through and cause runtime
slice panics or silent no-ops in callers like ARRAY_COPY and ARRAY_FILL. Update
the Interpreter.bounds check to explicitly reject size < 0 in addition to the
existing range validation, and make the validation overflow-safe so offset+size
cannot wrap. Keep the fix localized to bounds so all callers inherit the
corrected ErrIndexOutOfRange behavior.
---
Nitpick comments:
In `@asm/buffer_test.go`:
- Around line 45-60: Unchecked error from Buffer.Free in the test cleanup is
being ignored. Update the deferred cleanup in the NewBuffer-based test to
capture and assert the error returned by b.Free rather than discarding it, using
the same test helper style already used around writeBatch and patch. Keep the
fix localized to this test so the buffer is still freed while satisfying the
linter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4618e8bf-0535-4811-a2ea-5ea6a6a1028f
📒 Files selected for processing (12)
asm/buffer.goasm/buffer_test.goasm/link.godocs/architecture.mddocs/memory-model.mdinterp/fuse.gointerp/interp.gointerp/interp_test.gointerp/jit.gointerp/jit_arm64.gointerp/threaded.gointerp/trace.go
✅ Files skipped from review due to trivial changes (1)
- docs/architecture.md
🚧 Files skipped from review as they are similar to previous changes (4)
- interp/jit.go
- asm/buffer.go
- interp/interp.go
- interp/trace.go
Issue #104, covering guard emission, fusion, and tracer feedback optimization, is mostly unimplemented. Findings from the investigation:
idx < 0checks, misplaced host frame-overflow check, nested dispatch infuseLocalConst, unspecialized upvalue/global access, and triple-duplicated host cleanup all remain.guardIndexhelper exists but has zero callers (dead code); approximately 40 lines of duplicated guard chains exist across 4 array/struct handlers; most guards use inline exits;boxableI64uses an LSL/ASR pattern; thebrIfhot path is the taken branch;markedis overly conservative; i64 GET is trace-terminal.releaseslow-path slice allocation;Refs()per-walk allocation;Resetperforms an O(constants) rescan; JIT constant rematerialization / snapshot copies; W^X per-relocmprotect;wrapFuncreflection allocations.User decision: full scope — all of #104, additional findings, and risky items including i64 continuation and shape feedback, with phase-by-phase benchmark gates.
Benchmark Protocol — Common to All Phases
Run before starting and after completing each phase, and record the numbers in the commit body:
Gate: the target benchmark must improve, or remain tied for pure refactor phases, with no regressions elsewhere. Before committing,
go test -race ./...must be green.Each phase is an independent commit unit. Proceed from lower risk to higher risk.
Phase 1 — Remove Impossible Checks + Relocate CALL Overflow Check — Lowest Risk
interp/threaded.go: removeidx < 0checks for byte/uint16 decoded operands — LOCAL_GET:415, LOCAL_SET:501, LOCAL_TEE:546, GLOBAL_GET:344, GLOBAL_SET:361, GLOBAL_TEE:390, CONST_GET:590, REF_TEST:746, REF_CAST:773, ARRAY_NEW:2693, ARRAY_NEW_DEFAULT:2830, STRUCT_NEW:3408/:3444, MAP_NEW:3543/:3664. Keep upper-bound checks. Runtime indices such as ARRAY_SET/FILL/DELETE/COPY legitimately needidx < 0; keep those.interp/jit_arm64.go:811: remove deadidx < 0check in global lowering.interp/threaded.go:151: movei.fp == len(i.frames)check from before the callee type switch:155into the function/closure arms. Host calls:220do not push a frame.interp/fuse.go:307-309: remove the same check fromfuseHostFunctionCALL. Fused RETURN_CALL:340already does not have it — keep behavior consistent.runTeststable — host call succeeds when frames are exhausted, while function/closure calls still returnErrFrameOverflow.Phase 2 — Remove Core Runtime Allocations — Outside Issue, Largest Allocation Impact
Refs()to append style: change thetypes.Traceableinterface toRefs(dst []Ref) []Ref. Implementations:types/array.go:76,map.go:205,struct.go:122,closure.go:29,coroutine.go:43,host.go:74. Callers, including GC mark/sweep, release walk, and keep, should use a reusable buffer owned by*Interpreter.stack := []int{addr}atinterp.go:1530with a reused*Interpreterfield, resliced with[:0]. Keep release iterative; do not make it recursive.interp.go:643-647with a value computed once, targeting the pool Put → Reset hot path.unboxI64:interp.go:1349-1357— reduce operand-level release/rebox round trips in i64 arithmetic. Preserve net refcount behavior.wrapFunc:marshal.go:589-632— cache thereflect.Newargument slots andseenmap per wrapped closure, then reset them between calls.types/map.go:112-145,298-328— remove per-Nextcost fromreflect.MapRange.Test<Type>_Refstests for the newRefssignature, keeping one test per symbol.Refsbenchmark should reach approximately zero allocs/op, and Fib35 should be tied or better.Phase 3 — Remove Threaded Duplication + Scalar Specialization
threaded.go:4422-4436,fuse.go:321-335, andfuse.go:356-370should become a single*Interpretermethod. If the host signature is known at compile time to contain only non-ref/non-promotable scalar parameters — I32/I1/I8/F32/F64 — emit a scalar-only closure with no scan. If any ref or I64 param exists, keep the generic path.:2969-2999, FILL:3029-3077, DELETE:3270-3312, COPY:3124+, and:4762-4792into a shared method, per §1.4. Keep runtime negative checks.threaded.go:468-482and related code, skipping retain/release for I32/F32/F64.:661-662/:679-682— skip retain/release for I32/F32/F64 based on capture kind. Keep I64 because it can be promoted; keep KindRef behavior.runTestsrows — scalar-only host call, ref-param host call using fused+generic path, scalar upvalue, and promoted-i64 upvalue.Phase 4 — Fusion: Flatten
fuseLocalConst+ Add Branch Fusion — Largest Interpreter ImpactfuseLocalConstinfuse.go:450-532: current behavior is push → advance IP →inner(i), causing a second indirect call. Replace with flat closures per(kind, op)— directly read the local slot, write the result once, and remove the SP round trip and innersp == 0check. Preserve theretainBoxsemantics for the I64 variant at:512.I32_LT/LE/GT/GE/EQ/NE, plus I64/F32/F64 variants, followed by BR_IF into a single closure that compares and branches directly without boxed bool push/pop. Also combine with chore: Configure Renovate #1 for loop-header shapes such asLOCAL_GET; CONST; cmp; BR_IF.LOCAL_GET a; LOCAL_GET b; binopshould directly read both slots.containsYieldguard; abandon fusion if a branch target enters the fused region, which is the key correctness risk for multi-instruction windows; verify that the existing label checks cover 4-wide windows; keep fusion disabled inprecisemode, because the tracer must observe exact boundaries.WithThreshold(-1)must improve — expected 10–25%. JIT must remain tied.Phase 5 — JIT Structural Cleanup: Shared Guards + Cold Exit + Branch Shape
New helpers, following issue naming policy:
guardItab,guardRC, andretainRefare allowed; avoid*OrFailand*Unsigned.guardItab(ctx, itab, want, pre, ip)— fail on mismatch. Keep existingmatchItabfor multi-way cases.guardRC(ctx, addr, pre, ip)— rc check, following the signature conventions of existingguardTag:3308andreleaseRef:3291.retainRef(ctx, addr)— counterpart toreleaseRef.Rewrite
guardIndexas a one-branch unsigned guard at:3094-3114: current code usesCMPI/BGE + CMP/BLT, two branches. Replace withCMP idx, n+BCS fail; negative values become huge values under unsigned comparison and are caught together. Sincelen <= 63-bit, this is safe — document this in a comment. Then replace inline two-branch copies in array/struct handlers, such as:2586-2589, with calls toguardIndex, resolving the current zero-caller dead-code state.Compose guard chains in
arrayGet(:2510),arraySet(:2645),structGet(:2743),structSet(:2836)usingheapValue+guardItab+sliceHeader+guardIndex+guardRC/releaseRef.refGet,arrayLen,errorGet,coroDone, andcoroValuealready follow this style. Remove around 120 lines of duplication.Move inline exits to cold
sideExitforguardI64 :1510,boxableI64 :1528, i32/i64 divide-by-zero:924/:1178,guardTag :3316, call-target:1719, andreleaseRef rc <= 1 :3297. ForreleaseRef, preserve check-first ordering — decrement only after the guard passes.Add SBFX builder in
asm/arm64/instr.go: the encoder already uses SBFM internally inencoder.go:55,297; only builder/plumbing is needed. Replace LSL/ASR pairs inboxableI64 :1522-1526,sign64 :1539,arrayGet i64 :2621, andstructGet i64 :2812with one SBFX instruction.Invert
brIfdirection at:1583-1592: make the recorded direction the physical fall-through path, and make divergence the cold branch, matching the backedge shape at:2162.Reduce
markedat:3409: changekind == KindReftokind == KindRef && raw, meaning function markers only. This expands continuation coverage.Reduce compile-time churn: replace 16 occurrences of
append([]value(nil), ctx.values...)with a reusable lowering scratch; clean up doubleappend(ctx.values[:0], pre...)inguardTag/releaseRef; memoize itab/rcBase/heap-base constants per trace, resolving repeated LDIs such as the 7matchItabloads inarrayLen :2479-2485.TestSBFXinasm/arm64comparing encoded bytes; addt.Runcases to existing JIT tests — unsigned guard deopts on negative index,rc == 1release side-exit, and continuation enabled by relaxedmarked.jit_issue60/101and Fib35-JIT must improve — expected 5–15%. Interpreter must remain tied.Phase 6 — W^X Batching — Warmup Latency
asm/buffer.go:Write :42currently does unseal → copy → seal, causing twomprotectcalls plus icache flush.Linkreloc patching at:87-108currently does unseal/seal per relocation. Batch this so each compilation unit performs one unseal → all writes and relocations → one seal + icache sync. Preserve the Unseal → Append → Seal → Call invariant; missing icache flush on Darwin/ARM64 can cause intermittent SIGILL.grow :129-148, because installed callables may still reference it. Only document this.Phase 7 — Tracer Feedback: Narrow Input + Constant Facts + Exit Ordering
trace.go:34-44: record divisor, shift amount, array/struct index, branch condition, and BR_TABLE raw index. At:345, only the resolved target is currently recorded. Keep step size constrained because it scales per trace ×opLimit=1024.jit.goat:71-77: addknown boolplus i32/i64 fields. Lowerer removal targets: div-zero guards for known non-zero divisors, boxable guards for proven in-range values, and shift masks for known shift amounts. Facts derived from observations/seen values still require guards; only constants originating from the code stream can remove guards without guards. Document this distinction.tree.hits/exitsintrace.go:52-58are already tracked, but pending order uses discovery order with a fixedpendingLimit=256injit.go:26. Prioritize hot exits.Tracer.muper sampled tick inobservewith a per-interpreter local buffer, merging once when the trace completes. This targets pool warmup contention.ErrDivideByZero, and exit-heat ordering. Keep tests inline; do not add helpers, per §6.8.Phase 8 — Risky Items: Shape Feedback + i64 GET Continuation — Two Independent Commits, Individually Revertible
Shape feedback: add
shape struct{ itab, typ uintptr }to steps. Record shapes for REF_GET/ARRAY_LEN/ARRAY_GET/ARRAY_SET/STRUCT_GET/STRUCT_SET/ERROR_GET/ERROR_CODE/CORO_DONE/CORO_VALUE. The lowerer should replace multi-waymatchItabcascades with a singleguardItab, with mismatch going toop.ipside-exit. For structs, guard bothitab == heapStructand exacttypmatch. Ensure the container ref is still unreleased at the side-exit point — guard before release.i64 ARRAY_GET/STRUCT_GET continuation: remove the tracer terminal rule in
trace.go:209-217and the lowerer terminal behavior at:2639, :2830, then push raw KindI64 and continue the trace. Validate three cases:guardItabconfirmation, arithmetic on i64 elements greater than2^47followed by forced deopt, and confirmation that loops over i64 fields extend the trace.go test -race ./...plus deopt tests are hard gates. If they fail, revert only the corresponding commit.Convention Notes — Repo Rules
Interpreter/loweringfields: scratch buffers are runtime-state slots; compiler caches are infrastructure slots, per §2.5.*Interpreter/lowerermethods, per §1.5. Keep single-call-site helpers of 15 lines or less inline, per §1.4.t.Run, per §6.1/§6.3/§6.8.docs/jit-internals.mdin the same commit. If terminality changes, updatedocs/instruction-set.mdin the same commit, per §8.Impact / Risk Ranking
Summary by CodeRabbit
SBFXinstruction support for bitfield-related encoding.RefsAPIs to append into a caller-provided slice.Refsacross traceable/runtime types fromRefs()toRefs(dst []Ref) []Ref.