perf: optimize primitive array mutation loops#147
Conversation
|
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 adds anchor-specific backedge tracing, queued JIT compilation, continuable primitive typed-array writes with ARM64 lowering, expanded runtime tests, and refreshed JIT, opcode, and benchmark documentation. ChangesBackedge tracing and primitive array-set JIT support
Documentation and benchmark refresh
Estimated code review effort: 4 (Complex) | ~75 minutes 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 #147 +/- ##
==========================================
+ Coverage 29.36% 29.54% +0.17%
==========================================
Files 86 86
Lines 59969 60158 +189
==========================================
+ Hits 17612 17775 +163
- Misses 41109 41116 +7
- Partials 1248 1267 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
interp/jit.go (1)
231-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve caller-before-callee ordering for the new helpers.
interp/jit.go#L231-L239: moverequestedPlansbelow(*compiler).Compile.interp/trace.go#L353-L385: movecontinuableArraySetaboveprimitiveArray, or inline the helper.As per coding guidelines, Go declarations should follow the repository's declaration-order rule: callers before callees.
🤖 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.go` around lines 231 - 239, Reorder the new helper declarations to follow caller-before-callee ordering: in interp/jit.go lines 231-239, move requestedPlans below (*compiler).Compile; in interp/trace.go lines 353-385, move continuableArraySet above primitiveArray or inline it. No behavioral changes are needed.Source: Coding guidelines
interp/jit_arm64.go (1)
586-593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated terminal-detection logic between dispatcher and
arraySet.
terminal := op.terminal || ctx.count() > 0 && ctx.values[len(ctx.values)-1].kind == types.KindRefre-derives exactly the conditionarraySetitself computes internally (kind == types.KindRef || op.terminal) to decide whether to calll.exit. Keeping this logic in two places risks drift if one side changes independently (e.g., ifarraySetgains another terminal condition in future).Consider having
arraySetreport whether it treated the op as terminal (e.g., via its second bool-ish behavior or a small out-param) instead of recomputing the predicate insteps.🤖 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 586 - 593, Remove the duplicated terminal predicate in the dispatcher around arraySet and have arraySet return or otherwise report whether the operation was treated as terminal. Update the caller to use that result for its terminal return path, ensuring terminal detection remains defined solely by arraySet.interp/jit_arm64_test.go (1)
120-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the three new
TestARM64_Backedge*functions under one top-level test witht.Runsubtests.
TestARM64_BackedgeCompilesModuleLoop,TestARM64_BackedgeWarmsEagerLoop, andTestARM64_BackedgeKeepsSampleThresholdare three separate top-level tests covering closely related backedge-observation scenarios. As per coding guidelines, "Use one top-level test per public symbol (Test<Func>orTest<Type>_<Method>), put sub-cases undert.Run."The
TestCompiler_Compiletest in this same file already follows this pattern well; consider folding these three into a singleTestARM64_Backedgewitht.Run("compiles module loop", ...),t.Run("warms eager loop", ...),t.Run("keeps sample threshold", ...).🤖 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_test.go` around lines 120 - 205, Consolidate TestARM64_BackedgeCompilesModuleLoop, TestARM64_BackedgeWarmsEagerLoop, and TestARM64_BackedgeKeepsSampleThreshold into one top-level TestARM64_Backedge function. Move each scenario into a descriptive t.Run subtest, preserving its existing setup, assertions, and arm64 skip behavior.Source: Coding guidelines
🤖 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/architecture.md`:
- Line 176: Update the spill-frame documentation sentence near the “noSpill”
behavior to state that spilling is disabled specifically for plans containing
ARRAY_SET or STRUCT_SET operations, rather than broadly for all heap mutation.
Preserve the existing explanation about control-flow requirements and avoid
implying that other heap operations are rejected.
In `@docs/benchmarks.md`:
- Around line 35-51: Update the benchmark command block so the initial
benchmarks command runs from the benchmarks directory without changing the
caller’s working directory, using a subshell or restoring the repository root
before the ./interp and ./types commands. Keep all existing command arguments
and benchmark selections unchanged.
In `@interp/jit_arm64.go`:
- Around line 3196-3208: Remove the redundant arm64.MOV(n, addr) emission in the
non-reference, non-terminal branch of the ARRAY_SET logic, while preserving
rcAddr, rc, and guardRCTo setup and leaving n available as the later scratch
destination.
---
Nitpick comments:
In `@interp/jit_arm64_test.go`:
- Around line 120-205: Consolidate TestARM64_BackedgeCompilesModuleLoop,
TestARM64_BackedgeWarmsEagerLoop, and TestARM64_BackedgeKeepsSampleThreshold
into one top-level TestARM64_Backedge function. Move each scenario into a
descriptive t.Run subtest, preserving its existing setup, assertions, and arm64
skip behavior.
In `@interp/jit_arm64.go`:
- Around line 586-593: Remove the duplicated terminal predicate in the
dispatcher around arraySet and have arraySet return or otherwise report whether
the operation was treated as terminal. Update the caller to use that result for
its terminal return path, ensuring terminal detection remains defined solely by
arraySet.
In `@interp/jit.go`:
- Around line 231-239: Reorder the new helper declarations to follow
caller-before-callee ordering: in interp/jit.go lines 231-239, move
requestedPlans below (*compiler).Compile; in interp/trace.go lines 353-385, move
continuableArraySet above primitiveArray or inline it. No behavioral changes are
needed.
🪄 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: 699477c1-5c28-4634-bd83-d595868a9311
📒 Files selected for processing (19)
README.mdbenchmarks/README.mdbenchmarks/control_test.godocs/architecture.mddocs/benchmarks.mddocs/jit-internals.mddocs/profile.mdinternal/cmd/geninterp/generate.gointernal/cmd/geninterp/lower.gointerp/interp.gointerp/jit.gointerp/jit_arm64.gointerp/jit_arm64_test.gointerp/jit_plan.gointerp/jit_plan_test.gointerp/jit_stub.gointerp/threaded.gointerp/trace.gointerp/trace_test.go
| - Guard failure materializes VM state and resumes threaded dispatch. | ||
| - ARM64 label branches are range-checked and relaxed only to replacements that are already in range; an unreachable target falls back to threaded execution. | ||
| - Spill frames use a stable base register, and every internal call resume point must restore the active spill-frame depth. Loop traces and terminal mutation traces disable spilling when control flow cannot preserve that contract. | ||
| - Spill frames use a stable base register, and every internal call resume point must restore the active spill-frame depth. Loop traces and plans containing heap mutation disable spilling when control flow cannot preserve that contract. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the exact no-spill condition.
Line 176 says plans containing heap mutation disable spilling, but noSpill specifically rejects plans containing ARRAY_SET or STRUCT_SET; other unsupported heap operations remain threaded. Narrow this wording to the implemented condition.
🤖 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 `@docs/architecture.md` at line 176, Update the spill-frame documentation
sentence near the “noSpill” behavior to state that spilling is disabled
specifically for plans containing ARRAY_SET or STRUCT_SET operations, rather
than broadly for all heap mutation. Preserve the existing explanation about
control-flow requirements and avoid implying that other heap operations are
rejected.
| ```bash | ||
| # Full canonical package and VM-kernel suite | ||
| make benchmark-core | ||
|
|
||
| # Pull-request-sized benchmark suite | ||
| make benchmark-pr | ||
|
|
||
| # Cross-runtime comparison, matching the table below | ||
| # Full external comparison used by the complete matrix | ||
| cd benchmarks | ||
| go test -tags=compare -run='^$' -bench='.' -benchmem -benchtime=300ms -count=3 ./... | ||
| ``` | ||
|
|
||
| The cross-runtime command runs each benchmark three times. The tables below report the median `ns/op`, `B/op`, and `allocs/op` for every workload/runtime combination. | ||
| # Public interpreter and pool API costs | ||
| go test -run='^$' \ | ||
| -bench='^(BenchmarkNew|BenchmarkInterpreter_(Reset|Push|Pop|PopBoxed|Peek|Alloc|Retain|Release)|BenchmarkPool_(Get|Put))$' \ | ||
| -benchmem -benchtime=300ms -count=3 ./interp | ||
|
|
||
| The comparison is informational rather than a strict end-to-end latency ranking. minivm reports execution-only `Interpreter.Run` time and excludes result extraction and reset, while other runtimes may include host-call result materialization and conversion. This boundary difference is most significant for short workloads, so small ratios should not be treated as precise runtime speedups. | ||
| # Exact threaded execution samples | ||
| go test -run='^$' -bench='^BenchmarkInterpreter_Run/.*/Threaded$' \ | ||
| -benchmem -benchtime=300ms -count=3 ./interp | ||
|
|
||
| ## Summary | ||
| # Reference traversal | ||
| go test -run='^$' -bench='^Benchmark(Array|Struct|TypedMap|Map)_Refs$' \ | ||
| -benchmem -benchtime=300ms -count=3 ./types |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep reproduction commands in a consistent working directory.
The block changes into benchmarks at Line 37, then runs the ./interp and ./types commands at Lines 41-51. When copied as one shell block, those paths are resolved under benchmarks and fail. Use a subshell for the benchmarks command or return to the repository root before the interp and types commands.
🤖 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 `@docs/benchmarks.md` around lines 35 - 51, Update the benchmark command block
so the initial benchmarks command runs from the benchmarks directory without
changing the caller’s working directory, using a subshell or restoring the
repository root before the ./interp and ./types commands. Keep all existing
command arguments and benchmark selections unchanged.
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 (1)
interp/trace.go (1)
345-404: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
STRUCT_SETneeds to isolate*HostObjectbefore the store runs.cloneMutationhandles typed arrays,*types.Array, and*types.Struct, but not*HostObject. Because the heap clone is shallow andSTRUCT_SETstill reachesSetField/SetRawon host objects, speculative capture can mutate the same receiver as the live interpreter. Either deep-clone that case or abort capture here.🤖 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/trace.go` around lines 345 - 404, Update cloneMutation to handle *HostObject before speculative STRUCT_SET storage can proceed: either create an independent deep clone that preserves host-object behavior, or return/abort capture for that target when safe cloning is unavailable. Ensure HostObject mutations cannot reach the live interpreter through shared shallow state, while preserving existing typed-array, array, and struct handling.
🧹 Nitpick comments (1)
interp/trace_test.go (1)
76-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM! Once the
*HostObjectgap incloneMutation(seeinterp/trace.go) is fixed, consider extending this table with a*HostObject-backedSTRUCT_SETcase — it would have caught that isolation gap directly.🤖 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/trace_test.go` around lines 76 - 130, Extend the “isolates captured mutations” table in the trace tests with a *HostObject-backed STRUCT_SET case, using the existing cloneMutation behavior in interp/trace.go and verifying the original host object remains unchanged after capture. Follow the existing struct test setup and read callback pattern.
🤖 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/trace.go`:
- Around line 345-404: Update cloneMutation to handle *HostObject before
speculative STRUCT_SET storage can proceed: either create an independent deep
clone that preserves host-object behavior, or return/abort capture for that
target when safe cloning is unavailable. Ensure HostObject mutations cannot
reach the live interpreter through shared shallow state, while preserving
existing typed-array, array, and struct handling.
---
Nitpick comments:
In `@interp/trace_test.go`:
- Around line 76-130: Extend the “isolates captured mutations” table in the
trace tests with a *HostObject-backed STRUCT_SET case, using the existing
cloneMutation behavior in interp/trace.go and verifying the original host object
remains unchanged after capture. Follow the existing struct test setup and read
callback pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b87a72f8-058f-43a5-a094-db8b3c0a4a30
📒 Files selected for processing (13)
docs/instruction-set.mddocs/jit-internals.mddocs/testing.mdinternal/cmd/geninterp/generate.gointernal/cmd/geninterp/lower.gointerp/interp.gointerp/jit.gointerp/jit_arm64.gointerp/jit_arm64_test.gointerp/jit_plan_test.gointerp/threaded.gointerp/trace.gointerp/trace_test.go
💤 Files with no reviewable changes (2)
- internal/cmd/geninterp/generate.go
- interp/jit_plan_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cmd/geninterp/lower.go
- docs/jit-internals.md
- interp/jit_arm64.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
interp/interp_test.go (1)
7004-7016: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
b.ResetTimer()withb.Loop().
testing.B.Loop()already resets the timer on its first call, so the explicitb.ResetTimer()on line 7009 has no effect. Harmless but unnecessary.🧹 Optional cleanup
ctx := context.Background() b.ReportAllocs() - b.ResetTimer() for b.Loop() {🤖 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/interp_test.go` around lines 7004 - 7016, Remove the redundant b.ResetTimer() call from BenchmarkInterpreter_PreHotBackedge, leaving b.ReportAllocs() and the b.Loop() benchmark flow unchanged.
🤖 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.
Nitpick comments:
In `@interp/interp_test.go`:
- Around line 7004-7016: Remove the redundant b.ResetTimer() call from
BenchmarkInterpreter_PreHotBackedge, leaving b.ReportAllocs() and the b.Loop()
benchmark flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a006e8a-53e9-4031-84e2-88ea5d013473
📒 Files selected for processing (11)
README.mddocs/benchmarks.mddocs/jit-internals.mddocs/profile.mdinterp/cache.gointerp/cache_test.gointerp/interp.gointerp/interp_test.gointerp/jit_arm64_test.gointerp/trace.gointerp/trace_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- README.md
- docs/profile.md
- interp/trace_test.go
- interp/trace.go
- interp/jit_arm64_test.go
- docs/jit-internals.md
- docs/benchmarks.md
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
interp/jit_arm64_test.go (2)
165-177: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAssert observable JIT behavior instead of cache internals.
i.triedandi.exitscouple this test to private state. Use profiler/native execution metrics to verify warmup and installation while retaining the result assertions.As per coding guidelines, tests must assert behavior rather than private implementation shape.
🤖 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_test.go` around lines 165 - 177, Update the JIT test loop around i.Run and PopBoxed to stop asserting i.tried and i.exits; retain the returned value assertions, and use the available profiler/native execution metrics to verify the expected warmup attempts and installed-JIT behavior for each test case.Source: Coding guidelines
443-450: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winProve that
ARRAY_SETexecutes natively.These cases also pass if the compiled entry side-exits at
ARRAY_SETand threaded dispatch performs the write. Assert a profiler signal that execution continues natively past the store, with no terminalARRAY_SETfallback.As per coding guidelines, tests must assert behavior rather than private implementation shape.
Also applies to: 500-508, 528-537
🤖 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_test.go` around lines 443 - 450, Strengthen the ARRAY_SET test cases around compiler.Compile and i.Run to verify execution continues natively past the store, rather than allowing threaded dispatch to perform it after a side exit. Use the existing profiler or execution signal to assert native continuation and confirm no terminal ARRAY_SET fallback occurs, while retaining the current result assertions; apply the same validation to the additional cases at 500-508 and 528-537.Source: Coding guidelines
🤖 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/cache_test.go`:
- Around line 32-33: Update both cache cleanup sites in the affected subtests to
handle the error returned by cache.Close instead of deferring it directly. Use a
deferred closure that checks or reports the Close error, preserving cleanup
behavior and satisfying errcheck.
In `@interp/jit_arm64.go`:
- Around line 3725-3730: Move the sliceHeaderTo method below the sliceHeader
wrapper in the arm64Lowerer declarations, preserving its implementation
unchanged and ensuring the caller appears before the callee.
In `@interp/trace.go`:
- Around line 146-158: Move the anchor bounds validation for a.addr and a.ip to
the start of this flow, before the nil-tree branch invokes r.tree(a) or
tree.attempts is checked and incremented. Return CaptureOutcomeRejected with
CaptureReasonInvalidAnchor immediately for invalid anchors, while preserving the
existing attempt-limit handling for valid anchors.
---
Outside diff comments:
In `@interp/jit_arm64_test.go`:
- Around line 165-177: Update the JIT test loop around i.Run and PopBoxed to
stop asserting i.tried and i.exits; retain the returned value assertions, and
use the available profiler/native execution metrics to verify the expected
warmup attempts and installed-JIT behavior for each test case.
- Around line 443-450: Strengthen the ARRAY_SET test cases around
compiler.Compile and i.Run to verify execution continues natively past the
store, rather than allowing threaded dispatch to perform it after a side exit.
Use the existing profiler or execution signal to assert native continuation and
confirm no terminal ARRAY_SET fallback occurs, while retaining the current
result assertions; apply the same validation to the additional cases at 500-508
and 528-537.
🪄 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: 007fadca-60db-4a3c-849f-f798bca7cbd3
📒 Files selected for processing (13)
README.mddocs/benchmarks.mddocs/jit-internals.mdinterp/cache.gointerp/cache_test.gointerp/interp.gointerp/interp_test.gointerp/jit.gointerp/jit_arm64.gointerp/jit_arm64_test.gointerp/pool_test.gointerp/trace.gointerp/trace_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- interp/jit.go
- README.md
- interp/cache.go
- interp/trace_test.go
- docs/benchmarks.md
- interp/interp.go
| cache := NewCache(program.New(nil)) | ||
| defer cache.Close() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unchecked cache.Close() error (errcheck).
Both new subtests use defer cache.Close() without checking the returned error, flagged by golangci-lint.
🧹 Proposed fix
- cache := NewCache(program.New(nil))
- defer cache.Close()
+ cache := NewCache(program.New(nil))
+ defer func() { require.NoError(t, cache.Close()) }()Apply the same change at both flagged sites (lines 33 and 77).
Also applies to: 76-77
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 33-33: Error return value of cache.Close is not checked
(errcheck)
🤖 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/cache_test.go` around lines 32 - 33, Update both cache cleanup sites
in the affected subtests to handle the error returned by cache.Close instead of
deferring it directly. Use a deferred closure that checks or reports the Close
error, preserving cleanup behavior and satisfying errcheck.
Source: Linters/SAST tools
| func (arm64Lowerer) sliceHeaderTo(ctx *lowering, data, ptr, n asm.VReg, base int16) { | ||
| ctx.assembler.Emit( | ||
| arm64.LDR(ptr, data, base+sliceData), | ||
| arm64.LDR(n, data, base+sliceLen), | ||
| ) | ||
| return ptr, n | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Place sliceHeaderTo after sliceHeader.
The new helper precedes its caller, reversing the required declaration order. Move it below the sliceHeader wrapper.
As per coding guidelines, Go declarations should follow the repository's declaration-order rule: callers before callees.
🤖 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 3725 - 3730, Move the sliceHeaderTo method
below the sliceHeader wrapper in the arm64Lowerer declarations, preserving its
implementation unchanged and ensuring the caller appears before the callee.
Source: Coding guidelines
| if tree == nil { | ||
| tree = r.tree(a) | ||
| } | ||
| if tree.attempts >= attemptLimit { | ||
| r.mu.Unlock() | ||
| return captureResult{outcome: prof.CaptureOutcomeRejected, reason: prof.CaptureReasonAttemptLimit}, nil | ||
| return captureResult{outcome: prof.CaptureOutcomeRejected, reason: prof.CaptureReasonAttemptLimit} | ||
| } | ||
| tree.attempts++ | ||
| r.mu.Unlock() | ||
|
|
||
| if a.addr < 0 || a.addr >= len(i.instrs) || a.ip < 0 || a.ip >= len(i.instrs[a.addr]) { | ||
| return captureResult{outcome: prof.CaptureOutcomeRejected, reason: prof.CaptureReasonInvalidAnchor}, nil | ||
| return captureResult{outcome: prof.CaptureOutcomeRejected, reason: prof.CaptureReasonInvalidAnchor} | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first, then inspect the relevant section.
ast-grep outline interp/trace.go --view expanded || true
echo '---'
wc -l interp/trace.go
echo '---'
sed -n '120,180p' interp/trace.goRepository: siyul-park/minivm
Length of output: 4249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the tree constructor and capture callers to see whether invalid anchors are reachable.
sed -n '660,690p' interp/trace.go
echo '---'
rg -n "capture\\(" interp -g '*.go'Repository: siyul-park/minivm
Length of output: 2411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how InvalidAnchor and AttemptLimit are exercised in tests.
rg -n "CaptureReasonInvalidAnchor|CaptureReasonAttemptLimit|attemptLimit|anchor\\{" interp/trace_test.go interp/*.goRepository: siyul-park/minivm
Length of output: 11652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' interp/trace_test.goRepository: siyul-park/minivm
Length of output: 8336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '260,420p' interp/trace_test.goRepository: siyul-park/minivm
Length of output: 4061
Validate anchor bounds before creating the tree Move the a.addr/a.ip bounds check before r.tree(a) and tree.attempts++ so invalid anchors don’t create persistent tree entries or burn attempts before being rejected.
🤖 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/trace.go` around lines 146 - 158, Move the anchor bounds validation
for a.addr and a.ip to the start of this flow, before the nil-tree branch
invokes r.tree(a) or tree.attempts is checked and incremented. Return
CaptureOutcomeRejected with CaptureReasonInvalidAnchor immediately for invalid
anchors, while preserving the existing attempt-limit handling for valid anchors.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
interp/jit_arm64.go (1)
3129-3263: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winRedundant
MOV(n, addr)still present — previously flagged and marked addressed.This exact sequence (
rcBase = scratch; l.rcBaseTo(ctx, rcBase); ctx.assembler.Emit(arm64.MOV(n, addr)); rcAddr = addr; rc = n; l.guardRCTo(...)) was flagged in an earlier review round as a dead instruction —guardRCToimmediately overwritesrc(aliased ton) with the loaded refcount, so the precedingMOVvalue is never consumed. That earlier comment was marked "Addressed", but the same redundantMOVis back in the continuable primitive-store path at Line 3197. This still adds one extra instruction to the hotARRAY_SETmutation-loop path this PR is trying to optimize.🐛 Proposed fix
rcBase = scratch l.rcBaseTo(ctx, rcBase) - ctx.assembler.Emit(arm64.MOV(n, addr)) rcAddr = addr rc = n l.guardRCTo(ctx, rc, rcAddr, rcBase, valueFail)#!/bin/bash # Re-confirm guardRCTo overwrites rc unconditionally (making the MOV dead). rg -n -A20 'func .*guardRCTo\(' interp/jit_arm64.go🤖 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 3129 - 3263, Remove the redundant arm64.MOV emitted in the continuable branch of the array-store refcount setup. In the sequence assigning rcBase, rcAddr, and rc before guardRCTo, pass the existing registers directly to guardRCTo without copying addr into n; preserve the surrounding refcount guard and store behavior.
🤖 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.
Duplicate comments:
In `@interp/jit_arm64.go`:
- Around line 3129-3263: Remove the redundant arm64.MOV emitted in the
continuable branch of the array-store refcount setup. In the sequence assigning
rcBase, rcAddr, and rc before guardRCTo, pass the existing registers directly to
guardRCTo without copying addr into n; preserve the surrounding refcount guard
and store behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a824db8f-98a3-4d58-8f20-ce91258e3c81
📒 Files selected for processing (6)
docs/benchmarks.mddocs/jit-internals.mdinterp/interp.gointerp/interp_test.gointerp/jit_arm64.gointerp/trace.go
🚧 Files skipped from review as they are similar to previous changes (4)
- interp/interp_test.go
- docs/benchmarks.md
- docs/jit-internals.md
- interp/trace.go
Summary
ARRAY_SEToperations inside ARM64 native traces while preserving safe side exitsReview follow-up
Repeated code-quality reviews tightened the implementation beyond the initial performance fix:
BRhandler and rethread once only after periodic sampling marks them hotTracer.exitnamingmutationTarget,primitiveArray,branchIndex,isLeaf, and duplicatezeroValuedocs/coding-patterns.mdLocal added-executable-line coverage now covers every new shared-cache branch; the remaining uncovered ARM64 paths are primarily register-exhaustion and compile-rejection defenses.
Performance
Apple M4 Pro,
darwin/arm64, Go 1.26.2:Default and eager JIT remain about 3x faster than threaded execution for this workload. Repeated JIT compilation allocations remain reduced from roughly 407 allocs/op to 2 allocs/op.
Verification
make checkgo test -race ./interp -count=1cd benchmarks && go test ./... -count=1 -timeout=120sgo test -run='^$' -bench='^BenchmarkInterpreter_ColdBackedge$' -benchmem -benchtime=300ms -count=3— median 2.446 us, 0 allocs/opgo test -run='^$' -bench='^BenchmarkControl_Sieve' -benchmem -benchtime=300ms -count=3-benchtime=300ms -count=3Summary by CodeRabbit
Performance
Documentation
Tests