fix(interp): let a committing flush pass through a live ref operand#149
Merged
Conversation
arm64Lowerer.flush refused to commit whenever any live operand was a KindRef. selfCall is the only caller that reaches that loop with live values, so every self-recursive function forwarding a ref argument to itself failed to lower, rejecting the whole compile. observe() never retries a rejected anchor, so the function stayed interpreted for the process lifetime. The hazard is only the raw marker: box() materializes and retains it for an interpreter frame to release, and a commit rebuilds no frame. A non-raw ref already carries the retain taken when it was pushed, so committing transfers that edge to the stack, exactly as the inlined call path does when it stores arguments and drops them from the operand stack. Reject only raw refs. BenchmarkCall_IndirectRecursiveFib/default: 598,302 -> 80,548 ns/op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe ARM64 JIT now rejects raw references during committing flushes. A new test validates self-recursive reference forwarding and native profiler entries, while benchmark and JIT-internals documentation are updated. ChangesARM64 self-call reference forwarding
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/jit_arm64_test.go`:
- Around line 1107-1152: The “compiles natively” subtest manually invokes
capture, compilation, and installation instead of exercising the runtime path.
Remove that manual setup and attach the existing profiler to the repeated-run
test, then assert that vm_jit_native_entries_total is greater than zero after
normal execution through observe/compile gating; avoid assertions about private
trace or module structure.
In `@interp/jit_arm64.go`:
- Around line 3892-3898: Move the raw-reference preflight loop in the lowering
function containing ctx.values before any store emission or localDirty clearing.
Ensure commit with a raw KindRef returns false before mutating IR, stack,
parameters, facts, or labels, while preserving the existing lowering behavior
for supported references.
🪄 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: e0d9f235-fddc-40cf-84a4-86f04b66215c
📒 Files selected for processing (4)
docs/benchmarks.mddocs/jit-internals.mdinterp/jit_arm64.gointerp/jit_arm64_test.go
Comment on lines
+3892
to
3898
| // A non-raw ref carries the retain taken when it was pushed, so committing it | ||
| // transfers that edge to the stack. A raw marker instead retains inside box() | ||
| // for an interpreter frame to release; a commit rebuilds no frame, so nothing | ||
| // would balance it. | ||
| for j, v := range ctx.values { | ||
| if v.kind == types.KindRef && commit { | ||
| if v.kind == types.KindRef && commit && v.raw { | ||
| return false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preflight raw references before mutating lowering state.
Line 3897 can return false after stores were emitted and localDirty flags cleared. Move this check before any flush mutation.
Proposed fix
func (l arm64Lowerer) flush(ctx *lowering, commit bool) bool {
+ if commit {
+ for _, v := range ctx.values {
+ if v.kind == types.KindRef && v.raw {
+ return false
+ }
+ }
+ }
a := ctx.assembler
...
for j, v := range ctx.values {
- if v.kind == types.KindRef && commit && v.raw {
- return false
- }
boxed, ok := l.boxHome(ctx, v)As per coding guidelines, unsupported JIT lowering must return false without mutating IR, stack, parameters, facts, or labels.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // A non-raw ref carries the retain taken when it was pushed, so committing it | |
| // transfers that edge to the stack. A raw marker instead retains inside box() | |
| // for an interpreter frame to release; a commit rebuilds no frame, so nothing | |
| // would balance it. | |
| for j, v := range ctx.values { | |
| if v.kind == types.KindRef && commit { | |
| if v.kind == types.KindRef && commit && v.raw { | |
| return false | |
| func (l arm64Lowerer) flush(ctx *lowering, commit bool) bool { | |
| if commit { | |
| for _, v := range ctx.values { | |
| if v.kind == types.KindRef && v.raw { | |
| return false | |
| } | |
| } | |
| } | |
| // A non-raw ref carries the retain taken when it was pushed, so committing it | |
| // transfers that edge to the stack. A raw marker instead retains inside box() | |
| // for an interpreter frame to release; a commit rebuilds no frame, so nothing | |
| // would balance it. | |
| for j, v := range ctx.values { | |
| boxed, ok := l.boxHome(ctx, v) |
🤖 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 3892 - 3898, Move the raw-reference
preflight loop in the lowering function containing ctx.values before any store
emission or localDirty clearing. Ensure commit with a raw KindRef returns false
before mutating IR, stack, parameters, facts, or labels, while preserving the
existing lowering behavior for supported references.
Source: Coding guidelines
The test used a fixture-builder closure and drove compilation by hand through private state, against docs/coding-patterns.md 6.1, 6.4 and 6.8. Inline the program, drop the grouping subtests, and observe native entry through Interpreter plus the profiler instead. Metrics publish on Close, so close before reading them. The default policy is what emits an entry here: WithThreshold(1) compiles before a representative trace exists and emits nothing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
interp/jit_arm64_test.go (1)
1071-1090: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
t.Cleanupto guaranteei.Close()runs even on assertion failure.
i.Close()is only reached after the loop completes; if anyrequirecall inside the loop (lines 1075-1079) fails, the test aborts viat.FailNow()beforeClose()runs, leaking the compiler's native code buffer/cache for the remainder of the test binary.♻️ Proposed fix
profile := prof.New() i := New(prog, WithProfiler(profile)) + t.Cleanup(func() { _ = i.Close() }) for range 64 { require.NoError(t, i.Run(context.Background())) value, err := i.PopBoxed() require.NoError(t, err) require.Equal(t, types.BoxI32(6765), value) i.Reset() } - require.NoError(t, i.Close())🤖 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 1071 - 1090, Register i.Close with t.Cleanup immediately after creating the interpreter so it runs even when a require assertion aborts the test. Remove the later explicit i.Close call while preserving the existing profiler metrics assertions.
🤖 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/jit_arm64_test.go`:
- Around line 1071-1090: Register i.Close with t.Cleanup immediately after
creating the interpreter so it runs even when a require assertion aborts the
test. Remove the later explicit i.Close call while preserving the existing
profiler metrics assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ba3c22b-7dd4-47d8-be94-0cb2e3e6a178
📒 Files selected for processing (1)
interp/jit_arm64_test.go
4 tasks
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
1 participant
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.
Summary
BenchmarkCall_IndirectRecursiveFib/defaultdrops from 598,302 → 80,548 ns/op (7.4x). It previously ran slower thanthreaded; it now beats it by 6.9x.The cause was not per-call overhead — nothing compiled at all. The profiler recorded exactly one attempt,
outcome=rejected, reason=lowering-rejected, and zero native entries.arm64Lowerer.flushrefused to commit whenever any live operand was aKindRef:selfCallis the only caller that reaches that loop with live values (tailLooppops all args and bails unless the operand stack is empty).indirectRecursiveFibpasses its own callee as a ref argument, so aKindRefwas always live there and the whole compile was rejected.observe()never retries a rejected anchor, so the function stayed interpreted for the process lifetime.recursiveFib's args are i32-only, which is the entire difference between 38 us and 598 us.Why this is refcount-safe
The real hazard is only the raw marker:
box()materializes and retains it because the interpreter frame being rebuilt will release it — and a commit rebuilds no frame, so nothing balances that retain. A non-raw ref already carries the retain taken when it was pushed (localGetretains beforectx.push), andbox()returns it with no extra retain. Committing therefore transfers that single ownership edge to the stack — precisely what the ordinary inlined call path already does when it stores arguments and truncatesctx.valueswithout releasing.So the check is simply over-broad. Rejecting only raw refs is both necessary and sufficient: raw markers are produced only fused with their immediate consumer and cannot survive as an unrelated live operand at a self-call.
Test plan
TestARM64_SelfCallWithRefArg— differential-tests the JIT result against aWithThreshold(-1)threaded reference, and asserts a trace is actually installed (entryFunction,vm_jit_native_entries_total > 0).compiles nativelyfail (compiled.modulenil). The correctness subtest alone passes either way via interpreted fallback, which is why the install assertion carries the regression.go test -race ./...green (both modules);make fuzzclean;gofmt/go vetclean.benchtime=1s -count=5;docs/benchmarks.mdupdated with those medians.Eager
jit(threshold 0) is unchanged at ~575 us — it can compile before representative traces are learned, already documented in the threshold-zero note. Only adaptivedefaultbenefits.Notes
docs/jit-internals.mdgains the committing-flush ownership contract.🤖 Generated with Claude Code
Summary by CodeRabbit