jit: close gh#152 deep-kept operand-stack resume gap (RPython register/liveness channel)#436
Merged
Conversation
WalkthroughThis PR changes success-path teardown in JIT trace merge points to avoid abort-accounting double counts, adds a ChangesTrace success-path teardown
type(x) init skip shortcut
Deep-kept operand-stack slot recovery
Null-MRO subtype fallback
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JitDriver
participant TraceState
participant PendingAbort
JitDriver->>TraceState: abort_trace_live(false)
JitDriver->>PendingAbort: clear_pending_abort()
JitDriver->>JitDriver: clear_trace_session()
JitDriver->>JitDriver: mark compile_trace_success
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit f9845b7). 1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87599d818d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // NOT reached on success (the loop/bridge counter was already | ||
| // bumped at backend-compile time). Call the live-teardown half | ||
| // only, not the accounting half. | ||
| self.meta.abort_trace_live(false); |
There was a problem hiding this comment.
When this success path tears down a live trace, abort_trace_live still stages pending_abort_green_key/pending_abort_permanent for a subsequent aborted_tracing call. No aborted_tracing follows on successful compile, and the later re-observe path calls abort_trace_live with no live ctx, so the staged key can survive until an unrelated path that reports an abort after consuming its own ctx; with abort hooks/diagnostics enabled this can fire on_trace_abort for the previously successful greenkey. The bridge finish helper explicitly clears these fields after success for the same reason, so this success teardown should do that as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 152de8f2fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+10215
to
+10218
| if box_op != OpRef::NONE | ||
| && !opref_is_null_const_ptr(box_op) | ||
| && box_op.ty() == Some(majit_ir::Type::Ref) | ||
| { |
There was a problem hiding this comment.
When a FOR_ITER body keeps an unboxed int temporary below an in-body short-circuit/conditional branch, this Ref-only recovery leaves that operand-stack slot as a NULL hole in the virtualizable snapshot; the removed hazard gate previously forced these call+branch loop bodies to abort before consuming the iterator, but now the trace can compile and later resume with the missing slot. Please either conservatively decline when the guard-PC slot resolves to a non-Ref OpRef, or synthesize the boxed NEW_W_INT value before allowing the snapshot.
Useful? React with 👍 / 👎.
Four trace-close success edges in JitDriver called abort_trace(false), which runs both the live history teardown and aborted_tracing accounting. On a successful compile the accounting must not fire: raise_if_successful() raises ContinueRunningNormally (pyjitpl.py:3095-3123), bypassing the `except SwitchToBlackhole` handler that reaches aborted_tracing, and the loop/bridge counter was already bumped at backend-compile time. Calling the accounting half here bumped stats.loops_aborted on success and double-counted the re-observe edge. Change the four success edges (CompileTrace arm, the bridge Compiled close, the CloseLoopWithArgs bridge Compiled close, and the take_compile_trace_success re-observe) to abort_trace_live(false) so only the live teardown runs. loops_aborted now reads 0 on nbody/fannkuch/ spectral_norm/nested_loop; loops_compiled, bridges_compiled, and guard_failures are unchanged. Assisted-by: Claude
…cuit_mutate The loop appends three times per iteration (g, h, and one conditional g/h), so len(log) for f(40000) is 120000, not the 80000 the "2 mutations per iteration" comment implied. State the 3x invariant. Assisted-by: Claude
…lete FOR_ITER hazard gate
Restore deep kept operand-stack slots at a branch-guard resume from the
guard-PC register file instead of the pop-cleared vable positional shadow,
porting get_list_of_active_boxes (rpython/jit/metainterp/pyjitpl.py:177-234),
which captures resume boxes from registers_r[index] via the per-PC -live- set.
In walker_capture_snapshot_for_last_guard_impl (jitcode_dispatch.rs), a kept
operand slot the walk mirror (ctx.vstack_boxes) leaves NULL across a not-taken
branch merge is recovered from ctx.registers_r[color], where color is the
pcdep_color_slots[guard_py_pc] inverse (new state::semantic_slot_color_for_ref_slot).
The recovery is capture-only (transient snapshot overlay, never the live shadow)
and fills only Type::Ref slots; an unboxed-int kept temp is left a hole.
Delete the foriter_deep_kept_call_hazard gate (codewriter.rs) that withheld the
FOR_ITER per-iteration getarrayitem_vable_r reload for loop bodies with a CALL
across an in-body conditional branch. The reload is now unconditional for
portals, matching w_iterator = self.peekvalue() (pypy/interpreter/pyopcode.py:1303).
kept_stack_deep_var_{condexpr,nested_call,shortcircuit} and the shortcircuit
mutate canary run resident without the deep-guard SIGSEGV; check.py 155/155 on
dynasm and cranelift.
Assisted-by: Claude
Reformat the `iter_value` binding after the `foriter_deep_kept_call_hazard` gate removal collapsed the condition to a single `if is_portal`. No logic change; `cargo fmt --check` clean. Assisted-by: Claude
The four successful-compile teardown edges (CompileTrace, both bridge-Compiled paths, and the re-observed-success path) call abort_trace_live(false) for live cleanup but fire no aborted_tracing, so the pending_abort_green_key/permanent that abort_trace_live stages was never consumed. A later unrelated abort would then take() that stale key and fire on_trace_abort for the earlier successfully-compiled greenkey. Add MetaInterp::clear_pending_abort() and call it on each success edge, matching the existing bridge-FINISH success path (pyjitpl.rs) which already cleared these fields inline for the same reason. No aborted_tracing follows on success (raise_if_successful raises ContinueRunningNormally, pyjitpl.py:3095-3123). check.py 155/155 dynasm and cranelift; aborted_tracing tests pass. Assisted-by: Claude
…allback type.__call__ suppresses __init__ when self is the `type` builtin with no keyword arguments and exactly one positional argument (type(x) returns the class of x from __new__). Add `type_call_type_x_shortcut` and gate the three plain-instantiation __init__ dispatch sites in call.rs on it (typeobject.py:735-736). w_type_issubtype falls back to a find_best_base base-chain walk when mro_w is null instead of returning false, matching _issubtype_slow_and_wrong for a partially initialised type (typeobject.py:1640-1655, find_best_base 1335-1354; the "incomplete type" raise is omitted since the fn returns bool). Assisted-by: Claude
…ult OFF) Port the i-bank half of get_list_of_active_boxes (pyjitpl.py:206-210, add_box_to_storage(registers_i[index])) to the deep-kept operand-stack recovery in walker_capture_snapshot_for_last_guard_impl. A bank-0 (Int) stack pcdep entry names the Int-bank color owning the slot; registers_i[color] holds the raw int, boxed into a W_IntObject via wrapint so the uniformly Ref-typed vable array carries a Ref. Generalize semantic_slot_color_for_ref_slot into a bank-generic semantic_slot_color_for_slot core; add semantic_slot_color_for_int_slot (bank=0) alongside the ref (bank=1) delegate. Gated default-OFF: flag-off is byte-identical to leaving the int a hole (resume re-materializes it from its defining IR). On the current frontend the operand stack is uniformly Ref-banked so no bank-0 stack entry exists and the block fires nowhere; the real int-hole (a Ref-bank color whose OpRef is Int-typed) cannot be boxed at capture time — wrapint emits into a settled trace and trips store_final_boxes_in_guard (resume.py:397) — so it must be synthesized at operand-stack push time, a frontend change tracked separately. This is the RPython-parity read channel for when bank-0 stack entries exist. Assisted-by: Claude
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch.rs`:
- Around line 10242-10243: Replace the new HashSet side table in the stack
synchronization logic with a small Vec-based structure that matches the existing
stack/list shape. In the code around stack_sync and covered in
jitcode_dispatch.rs, keep the bounded operand-stack index tracking in a simple
linear collection and update the membership checks accordingly instead of using
a Rust-native set. Ensure the rest of the ported path continues to follow the
original RPython/PyPy data-structure style.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81626490-3d6d-4f20-afc5-f638d55a0840
📒 Files selected for processing (8)
majit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/pyjitpl.rspyre/bench/synth/kept_stack_deep_var_shortcircuit_mutate.pypyre/pyre-interpreter/src/call.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-object/src/typeobject.rs
Comment on lines
+10242
to
+10243
| let mut covered: std::collections::HashSet<usize> = | ||
| stack_sync.iter().map(|&(idx, _)| idx).collect(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Avoid introducing a HashSet side table here.
covered only tracks bounded operand-stack vable indices; a small Vec keeps this closer to the existing stack/list shape and avoids adding a Rust-native container in the ported path.
As per coding guidelines, **/*.rs: “When porting RPython/PyPy code, do not casually introduce Rust-native containers like HashMap, HashSet, or BTreeMap; match the original RPython data structure shape instead.” <coding_guidelines>
Proposed refactor
- let mut covered: std::collections::HashSet<usize> =
- stack_sync.iter().map(|&(idx, _)| idx).collect();
+ let mut covered: Vec<usize> = stack_sync.iter().map(|&(idx, _)| idx).collect();
let mut augmented = stack_sync;
for s in 0..depth {
let vidx = nvs + nlocals + s;
if covered.contains(&vidx) {
continue;
@@
augmented.push((vidx, box_op));
- covered.insert(vidx);
+ covered.push(vidx);
@@
let boxed = crate::state::wrapint(ctx.trace_ctx, raw);
augmented.push((vidx, boxed));
- covered.insert(vidx);
+ covered.push(vidx);📝 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
| let mut covered: std::collections::HashSet<usize> = | |
| stack_sync.iter().map(|&(idx, _)| idx).collect(); | |
| let mut covered: Vec<usize> = stack_sync.iter().map(|&(idx, _)| idx).collect(); | |
| let mut augmented = stack_sync; | |
| for s in 0..depth { | |
| let vidx = nvs + nlocals + s; | |
| if covered.contains(&vidx) { | |
| continue; | |
| } |
🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch.rs` around lines 10242 - 10243,
Replace the new HashSet side table in the stack synchronization logic with a
small Vec-based structure that matches the existing stack/list shape. In the
code around stack_sync and covered in jitcode_dispatch.rs, keep the bounded
operand-stack index tracking in a simple linear collection and update the
membership checks accordingly instead of using a Rust-native set. Ensure the
rest of the ported path continues to follow the original RPython/PyPy
data-structure style.
Source: Coding guidelines
youknowone
added a commit
that referenced
this pull request
Jul 9, 2026
…r/liveness channel) (#436) * majit: route successful-compile trace teardown to abort_trace_live Four trace-close success edges in JitDriver called abort_trace(false), which runs both the live history teardown and aborted_tracing accounting. On a successful compile the accounting must not fire: raise_if_successful() raises ContinueRunningNormally (pyjitpl.py:3095-3123), bypassing the `except SwitchToBlackhole` handler that reaches aborted_tracing, and the loop/bridge counter was already bumped at backend-compile time. Calling the accounting half here bumped stats.loops_aborted on success and double-counted the re-observe edge. Change the four success edges (CompileTrace arm, the bridge Compiled close, the CloseLoopWithArgs bridge Compiled close, and the take_compile_trace_success re-observe) to abort_trace_live(false) so only the live teardown runs. loops_aborted now reads 0 on nbody/fannkuch/ spectral_norm/nested_loop; loops_compiled, bridges_compiled, and guard_failures are unchanged. Assisted-by: Claude * synth: correct mutation-count comment in kept_stack_deep_var_shortcircuit_mutate The loop appends three times per iteration (g, h, and one conditional g/h), so len(log) for f(40000) is 120000, not the 80000 the "2 mutations per iteration" comment implied. State the 3x invariant. Assisted-by: Claude * jit: deep-kept operand-stack resume via register/liveness channel; delete FOR_ITER hazard gate Restore deep kept operand-stack slots at a branch-guard resume from the guard-PC register file instead of the pop-cleared vable positional shadow, porting get_list_of_active_boxes (rpython/jit/metainterp/pyjitpl.py:177-234), which captures resume boxes from registers_r[index] via the per-PC -live- set. In walker_capture_snapshot_for_last_guard_impl (jitcode_dispatch.rs), a kept operand slot the walk mirror (ctx.vstack_boxes) leaves NULL across a not-taken branch merge is recovered from ctx.registers_r[color], where color is the pcdep_color_slots[guard_py_pc] inverse (new state::semantic_slot_color_for_ref_slot). The recovery is capture-only (transient snapshot overlay, never the live shadow) and fills only Type::Ref slots; an unboxed-int kept temp is left a hole. Delete the foriter_deep_kept_call_hazard gate (codewriter.rs) that withheld the FOR_ITER per-iteration getarrayitem_vable_r reload for loop bodies with a CALL across an in-body conditional branch. The reload is now unconditional for portals, matching w_iterator = self.peekvalue() (pypy/interpreter/pyopcode.py:1303). kept_stack_deep_var_{condexpr,nested_call,shortcircuit} and the shortcircuit mutate canary run resident without the deep-guard SIGSEGV; check.py 155/155 on dynasm and cranelift. Assisted-by: Claude * jit: rustfmt the FOR_ITER iterator-reload block Reformat the `iter_value` binding after the `foriter_deep_kept_call_hazard` gate removal collapsed the condition to a single `if is_portal`. No logic change; `cargo fmt --check` clean. Assisted-by: Claude * majit: clear pending_abort payload on successful trace teardown The four successful-compile teardown edges (CompileTrace, both bridge-Compiled paths, and the re-observed-success path) call abort_trace_live(false) for live cleanup but fire no aborted_tracing, so the pending_abort_green_key/permanent that abort_trace_live stages was never consumed. A later unrelated abort would then take() that stale key and fire on_trace_abort for the earlier successfully-compiled greenkey. Add MetaInterp::clear_pending_abort() and call it on each success edge, matching the existing bridge-FINISH success path (pyjitpl.rs) which already cleared these fields inline for the same reason. No aborted_tracing follows on success (raise_if_successful raises ContinueRunningNormally, pyjitpl.py:3095-3123). check.py 155/155 dynasm and cranelift; aborted_tracing tests pass. Assisted-by: Claude * interp: type(x) one-arg __init__ skip + null-mro subtype base-chain fallback type.__call__ suppresses __init__ when self is the `type` builtin with no keyword arguments and exactly one positional argument (type(x) returns the class of x from __new__). Add `type_call_type_x_shortcut` and gate the three plain-instantiation __init__ dispatch sites in call.rs on it (typeobject.py:735-736). w_type_issubtype falls back to a find_best_base base-chain walk when mro_w is null instead of returning false, matching _issubtype_slow_and_wrong for a partially initialised type (typeobject.py:1640-1655, find_best_base 1335-1354; the "incomplete type" raise is omitted since the fn returns bool). Assisted-by: Claude * jit: deep-kept Int-bank recovery channel (PYRE_FBW_DEEPKEPT_INT, default OFF) Port the i-bank half of get_list_of_active_boxes (pyjitpl.py:206-210, add_box_to_storage(registers_i[index])) to the deep-kept operand-stack recovery in walker_capture_snapshot_for_last_guard_impl. A bank-0 (Int) stack pcdep entry names the Int-bank color owning the slot; registers_i[color] holds the raw int, boxed into a W_IntObject via wrapint so the uniformly Ref-typed vable array carries a Ref. Generalize semantic_slot_color_for_ref_slot into a bank-generic semantic_slot_color_for_slot core; add semantic_slot_color_for_int_slot (bank=0) alongside the ref (bank=1) delegate. Gated default-OFF: flag-off is byte-identical to leaving the int a hole (resume re-materializes it from its defining IR). On the current frontend the operand stack is uniformly Ref-banked so no bank-0 stack entry exists and the block fires nowhere; the real int-hole (a Ref-bank color whose OpRef is Int-typed) cannot be boxed at capture time — wrapint emits into a settled trace and trips store_final_boxes_in_guard (resume.py:397) — so it must be synthesized at operand-stack push time, a frontend change tracked separately. This is the RPython-parity read channel for when bank-0 stack entries exist. Assisted-by: Claude
This was referenced Jul 9, 2026
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.
Closes the deep-kept operand-stack resume gap in the FOR_ITER residency path
(gh#152 layer-2 census: the
kept_stack_deep_var_*trio +short_circuit_side_effects).What
Three commits, all gh#152:
majit: route successful-compile trace teardown to abort_trace_live— fix theloops_abortedsuccess-teardown double-count.abort_trace(false)was overloaded asgeneric trace-session teardown and bumped
stats.loops_abortedeven on a successfulcompile (a NEW-DEVIATION vs RPython's
aborted_tracing, which is reached only on theSwitchToBlackholeunwind). The four success edges now route toabort_trace_live(false)(live teardown, no accounting);
loops_abortedreads 0 on all loop-nest benches withloops_compiled/bridges_compiled/guard_failuresunchanged.synth: correct mutation-count comment— thekept_stack_deep_var_shortcircuit_mutatecanary appends 3× per iteration (g, h, conditional g/h); comment corrected to match.
jit: deep-kept operand-stack resume via register/liveness channel; delete FOR_ITER hazard gate— the core fix. A depth>1 kept operand stack could not be reconstructed ata compiled interior branch guard, causing an
ResidualCallArgUnboundabort (walk declined,permanent interpreter fallback) or a SIGSEGV when the reload was forced on.
Root cause (RPython parity): pyre restored the deep operand stack through the vable
positional channel (a shadow that
pop_valueNULL-clears), whereas RPython'sget_list_of_active_boxes(rpython/jit/metainterp/pyjitpl.py:177-234) captures guardresume boxes from the register file
registers_r[index]via the per-PC-live-set.The fix:
foriter_deep_kept_call_hazardgate (a FOR_ITER inlining: walker coverage gap (iteration_protocol abort) + liveness gap (polymorphic deopt crash) #342 NEW-DEVIATION with no RPythonbacking) that withheld the FOR_ITER per-iteration
getarrayitem_vable_rreload for loopbodies with a CALL across an in-body conditional branch. The reload is now unconditional
for portals, matching
w_iterator = self.peekvalue()(pypy/interpreter/pyopcode.py:1303).walker_capture_snapshot_for_last_guard_impl, a slot the walk mirror leaves NULL across anot-taken branch merge is filled from
ctx.registers_r[color], wherecoloris thepcdep_color_slots[guard_py_pc]inverse (newstate::semantic_slot_color_for_ref_slot).Capture-only (transient snapshot overlay, never the live shadow). Fills
Type::Refslots;an unboxed-int kept temp stays a hole (follow-up: int-bank
NEW_W_INTcapture).Arm-independence is correct by construction: the recovery is keyed to the guard's resume PC
liveness, matching RPython's per-PC
-live-set.Results
The census 4 benches, previously 1.10×–1.21× slower than the interpreter (pure residency
loss), now all beat it after the fix:
The deep_var trio's shared
ResidualCallArgUnboundabort is gone; thebuilt_as_portal=falsestructural-decline log still fires on the short-circuit pair but is now benign (the trait-tracer
fallback is a net win). That structural decline is a separate architectural item (pyre's
one-artifact-per-
CodeObjectmodel vs RPython's independent per-greenkey portal/callee tokens),not a residency defect on any current bench.
Verification
pyre/check.py155/155 on dynasm and cranelift (flagless, no env gates).kept_stack_deep_var_{condexpr,nested_call,shortcircuit}+ theshortcircuit_mutatecanarybit-exact (
len(log)=120000, no double-delivery / dropped iteration).cargo test -p pyre-jit-trace297/0,-p pyre-jit260/0.nbody/fannkuch/spectral_norm/nested_loop) unchangedexcept
loops_aborted→ 0.Issue body refreshed at the corresponding commit.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
type(x)behavior so it more closely matches Python/PyPy expectations.Bug Fixes