majit: the cross-loop close outcomes, and a segmented bridge that compiles - #1040
Merged
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe metainterpreter now supports segmented bridge traces in addition to segmented loops. It tracks cross-loop-cut keys, classifies bridge outcomes, rejects invalid retries, compiles bridge finishes, and records the corresponding abort reasons. ChangesSegmented trace compilation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MetaInterp
participant JitDriver
participant TracingSession
MetaInterp->>JitDriver: dispatch segmented loop or bridge
JitDriver->>TracingSession: compile loop or exception finish
TracingSession-->>JitDriver: return token and abort reason
JitDriver->>MetaInterp: clear tracing state and record abort
Possibly related PRs
Suggested reviewers: 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 3f4cf5d). Files in the reviewed diff1. 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: 914b343f53
ℹ️ 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
+5815
to
+5816
| if let Some(cut_key) = cut_inner_green_key { | ||
| self.cut_compiled_keys.insert(cut_key); |
There was a problem hiding this comment.
With this insert happening before the compile is validated and sent to the backend, any later Cancelled/Aborted return in compile_loop_body leaves the key in cut_compiled_keys even though no cut-owned CompiledEntry was installed. If that inner loop subsequently compiles normally from its own header, nothing clears this mark, so future interpreter-origin closes hit is_cross_loop_cut_key(target_key) and decline instead of entering the normal preamble. Move the marker into the successful install path (or clear it on every non-compiled/replacement path).
AGENTS.md reference: AGENTS.md:L109-L118
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-metainterp/src/jitdriver.rs`:
- Around line 2950-2963: Add diagnostic counter bumps to the new
BridgeCompileResult::Declined arm. Record separate counters for
attempted-and-rejected closes versus unattempted gate declines, using the
existing diag-counter mechanism and distinct IDs consistent with the sibling
Declined arms, while preserving the existing note_cross_loop_close_declined
behavior.
- Around line 3616-3633: In the exception branch of the active-session finish
flow, update the operand type passed to compile_finish_from_active_session from
Type::Int to Type::Ref, preserving the existing exception_box and
exit_with_exception handling.
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 5809-5817: The cross-loop-cut compile failure path leaves
cut_inner_green_key in cut_compiled_keys, causing later non-cut bridge
compilation to be skipped. Update the failure cleanup around
cross_loop_cut_label_jump_null_guard_slot to remove the corresponding key from
cut_compiled_keys, matching the existing stale-mark cleanup behavior, while
preserving the mark for successful loop compilation.
🪄 Autofix
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 Plus
Run ID: 5b3fdea0-0d88-4782-bf54-7bab1936d2c1
📒 Files selected for processing (4)
majit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/lib.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rs
Comment on lines
+2950
to
+2963
| crate::pyjitpl::BridgeCompileResult::Declined => { | ||
| // pyjitpl.py:3009-3010: `compile_trace` returned without raising, so the | ||
| // trace is NOT given up — tracing continues. Latch the decline so the walk | ||
| // does not re-run the optimizer over the same key, and re-enter the walk at | ||
| // the merge point's own pc (pyjitpl.py:1577 `self.pc = saved_pc`). | ||
| // | ||
| // Only latch what an attempt actually rejected. A close the gate | ||
| // above never evaluated cost no optimizer pass, which is the whole | ||
| // reason the latch exists, and the gate's own conditions can be | ||
| // false now and true at the next visit of this header. | ||
| if let Some(ctx) = self.meta.trace_ctx() { | ||
| if attempted { | ||
| ctx.note_cross_loop_close_declined(target_key); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a diag counter to the new Declined path.
The two sibling Declined arms report to the diag counters: line 3159 bumps 50 (bridge_declined_close) and line 3186 bumps 51 (bridge_no_targets_close). This new arm bumps nothing, so a decline here leaves no counter trace.
The PR objectives state that the new bridge path was not exercised by the verification suites and that abrt_segmented remained zero. A counter on this arm gives that verification a signal to read.
Consider separate counter ids for the two decline causes this arm now covers: an attempted-and-rejected close, and an unattempted gate.
♻️ Proposed counter bump
crate::pyjitpl::BridgeCompileResult::Declined => {
+ // Mirror the sibling decline arms' diag
+ // reporting so this path is observable.
+ crate::mc_diag_bump(if attempted { 50 } else { 51 });
// pyjitpl.py:3009-3010: `compile_trace` returned without raising, so the🤖 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 `@majit/majit-metainterp/src/jitdriver.rs` around lines 2950 - 2963, Add
diagnostic counter bumps to the new BridgeCompileResult::Declined arm. Record
separate counters for attempted-and-rejected closes versus unattempted gate
declines, using the existing diag-counter mechanism and distinct IDs consistent
with the sibling Declined arms, while preserving the existing
note_cross_loop_close_declined behavior.
Comment on lines
+3616
to
+3633
| Some(exception_box) => { | ||
| // Read before the compile drains the tracer: the | ||
| // bridge's own key is what its abort hook reports. | ||
| let green_key = self.meta.trace_ctx().map(|ctx| ctx.green_key); | ||
| let result = self.meta.compile_finish_from_active_session( | ||
| &[exception_box], | ||
| vec![majit_ir::Type::Int], | ||
| /* exit_with_exception */ true, | ||
| ); | ||
| // On the giveup path `abort_trace_live` already | ||
| // staged this key; on the success path it was | ||
| // cleared again. Stage it either way so the hook | ||
| // below does not report key 0. | ||
| self.meta.pending_abort_green_key = green_key; | ||
| match result { | ||
| Ok(()) => crate::counters::ABORT_SEGMENTED_TRACE, | ||
| Err(stb) => stb.reason, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Resolve compile_finish_from_active_session and check the exception operand type contract.
set -uo pipefail
echo "=== locate the helper definition ==="
fd -e rs --exec ast-grep outline {} --match compile_finish_from_active_session --view expanded \; 2>/dev/null | head -60
echo
echo "=== helper body (structural) ==="
ast-grep run --lang rust \
--pattern $'fn compile_finish_from_active_session($$$) {
$$$
}' . 2>/dev/null | head -160
echo
echo "=== every call site with its declared finish_arg_types ==="
rg -nP -C 6 '\bcompile_finish_from_active_session\s*\(' --type=rust .
echo
echo "=== how finish_arg_types / exit_with_exception select the FINISH descr and slot kind ==="
rg -nP -C 5 '\b(finish_arg_types|exit_with_exception|exit_frame_with_exception_descr)\w*' --type=rust . | head -120
echo
echo "=== does the helper drain the trace session / profiler on both edges? ==="
rg -nP -C 4 '\b(take_trace_meta|abort_trace_live|leave_profiler_tracing|clear_trace_session|finish_trace_live)\s*\(' --type=rust . | head -140Repository: youknowone/pyre
Length of output: 34307
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== pyjitpl helper definition ==="
sed -n '14455,14675p' majit/majit-metainterp/src/pyjitpl.rs
echo
echo "=== ActiveTraceSession methods / finish_arg_types use ==="
rg -n -C 8 '\bfinish_arg_types\b|\bcompile_trace\b|\bActiveTraceSession\b|\bexit_frame_with_exception_descr\b|\bdone_with_this_frame_descr\b|\bleave_profiler_tracing\b|\bswap_profiler\b|\brecover_from_profiler\b' majit/majit-metainterp majit/majit-trace majit/majit --type=rust | head -300
echo
echo "=== jitdriver affected block ==="
sed -n '3520,3665p' majit/majit-metainterp/src/jitdriver.rsRepository: youknowone/pyre
Length of output: 49158
Use Type::Ref for the exception FINISH operand.
The bridge exception path passes an exception GcRef but labels the FINISH argument as Type::Int. Exception-carrying FINISH paths use Type::Ref, including compile_exit_frame_with_exception, the dispatcher TraceAction::Finish, and this segmented-loop test case. Use vec![majit_ir::Type::Ref] here so the exit descriptor makes the exception value a GC ref.
🤖 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 `@majit/majit-metainterp/src/jitdriver.rs` around lines 3616 - 3633, In the
exception branch of the active-session finish flow, update the operand type
passed to compile_finish_from_active_session from Type::Int to Type::Ref,
preserving the existing exception_box and exit_with_exception handling.
Comment on lines
+5809
to
+5817
| // compile.py:269-270 `jitcell_token = cross_loop.jitcell_token`: mark | ||
| // the key this loop is about to be stored under as cut-owned, before | ||
| // the ctx that carries `cut_inner_green_key` is drained. Recorded on | ||
| // the way in rather than on success so a retried compile at the same | ||
| // key cannot leave the mark behind; `forget_loop_side_tables` retires | ||
| // it with the loop. | ||
| if let Some(cut_key) = cut_inner_green_key { | ||
| self.cut_compiled_keys.insert(cut_key); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find consumers of is_cross_loop_cut_key to assess the impact of a stale/false-positive mark.
rg -n -B3 -A8 'is_cross_loop_cut_key' --type=rustRepository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the reviewed function and related symbols, then inspect the relevant implementation details.
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('pyjitpl.rs'):
if p.is_file():
text = p.read_text(errors='replace')
if 'fn compile_loop_body' in text or 'cut_compiled_keys' in text or 'is_cross_loop_cut_key' in text:
print(f'--- {p}')
for i, line in enumerate(text.splitlines(), 1):
if any(s in line for s in ['compile_loop_body', 'cut_compiled_keys', 'is_cross_loop_cut_key', 'cut_inner_green_key', 'compiled_loops']):
start=max(1,i-8); end=min(len(text.splitlines()), i+12)
lines=text.splitlines()
print(f'\nLines {start}-{end}')
for n in range(start,end+1):
print(f"{n:5}: {lines[n-1]}")
print()
PYRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect `is_cross_loop_cut_key` definitions/usages and successful cross-loop-cut publishing paths without reading the whole Rust file.
rg -n -B5 -A15 'fn is_cross_loop_cut_key|is_cross_loop_cut_key\(' `pwd`
python3 - <<'PY'
from pathlib import Path
p=Path('majit/majit-metainterp/src/pyjitpl.rs')
text=p.read_text()
for needle in ['fn finish_and_compile', 'cut_compiled_keys.remove', 'is_cross_loop_cut_key(', 'compiled_loops.insert', 'forget_loop_side_tables']:
print(f'\n--- occurrences: {needle}')
for i,line in enumerate(text.splitlines(),1):
if needle in line:
print(f'{i}: {line}')
PYRepository: youknowone/pyre
Length of output: 6711
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the compile success paths and the failed cross-loop-cut helper around lines with fresh insertion.
sed -n '6730,6805p' majit/majit-metainterp/src/pyjitpl.rs
sed -n '8885,8945p' majit/majit-metainterp/src/pyjitpl.rs
sed -n '9130,9165p' majit/majit-metainterp/src/pyjitpl.rs
rg -n -B5 -A20 'fn cross_loop_cut_label_jump_null_guard_slot|fn is_cross_loop_cut_key|compile_trace_entry_data' majit/majit-metainterp/src/jitdriver.rs majit/majit-metainterp/src/pyjitpl.rsRepository: youknowone/pyre
Length of output: 21057
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the stale cut-key false-positive path from cross-loop-cut failure through bridge compilation.
rg -n -B40 -A60 'cross_loop_cut_label_jump_null_guard_slot\(&optimized_ops\)|None if self\.meta\.is_cross_loop_cut_key\(target_key\)' majit/majit-metainterp/src/jitdriver.rs
rg -n -B5 -A25 'Abort.*cross-loop|compile outcome|CompileOutcome::Aborted|classify_compile_outcome' majit/majit-metainterp/src/jitdriver.rs majit/majit-metainterp/src/pyjitpl.rs | head -n 220Repository: youknowone/pyre
Length of output: 29456
Clear the stale cross-loop-cut mark when the compile fails.
A failed cross-loop-cut attempt still inserts cut_inner_green_key into cut_compiled_keys, and that key is never removed. Later unrelated interp-origin bridge attempts can read the stale mark via is_cross_loop_cut_key and decline compilation instead of compiling the non-cut loop. Apply the same stale-mark fix that handles the failed cross_loop_cut_label_jump_null_guard_slot path.
🤖 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 `@majit/majit-metainterp/src/pyjitpl.rs` around lines 5809 - 5817, The
cross-loop-cut compile failure path leaves cut_inner_green_key in
cut_compiled_keys, causing later non-cut bridge compilation to be skipped.
Update the failure cleanup around cross_loop_cut_label_jump_null_guard_slot to
remove the corresponding key from cut_compiled_keys, matching the existing
stale-mark cleanup behavior, while preserving the mark for successful loop
compilation.
The block above the cross-loop-cut arm still said "The JUMP-into-ptoken half of :3001-3007 is not implemented here", which 944024e made false: the `already_compiled_here` arm directly below it now publishes the token key and returns `CloseLoop` for the driver to run `close_bridge` or `compile_trace_from_interp`. State what the arm does, and record that the key derived here is the one the interpreter enters by — the property 25ca549 established and the reason the jump is sound, where the previous `green_key_from_code_ptr(green_key_raw.0, pc)` key allowed a jump into a loop nothing enters (measured as a logo miscompile, 992635 against 996310, and a SIGSEGV). Mark the cel `nested_list_loop_varying_trip_count` deopt numbers cited against this lever as taken before that key unification, so they are re-measured rather than inherited. Keep the note that a declined attempt still does not reach upstream's `current_merge_points` scan. Assisted-by: Claude
The check reached at the merge point tests `is_loop_trace`, so a guard-origin bridge is not segmented and runs on to the ordinary over-limit abort. The note above it claimed this was the pre-change behaviour; the driver-level fallback it replaced had no loop-vs-bridge test and set `TraceAction::SegmentedLoop` for bridges too, compiling them through `compile_simple_loop`. Name the unported arm (pyjitpl.py:1665-1668 `compile_trace(metainterp, resumekey, [exception_box])` with the `target_token is not token` give-up) and the FINISH descriptor slot the port needs. Assisted-by: Claude
from the interp origin The `compile_trace(live_arg_boxes, ptoken)` close (pyjitpl.py:3001-3007) collapsed its result to a bool, so `RetraceNeeded` and `Failed` both took the decline tail: a retrace kept tracing instead of reaching `compile_retrace` in the same call, and a give-up walked on with a possibly drained tracing context. Match on all four `BridgeCompileResult` variants, mirroring the ordinary close handler below — `RetraceNeeded` falls through to the merge-point path, `Failed` runs `abort_trace` and returns. Extract `close_bridge`'s `CompileOutcome` -> `BridgeCompileResult` mapping into `classify_compile_outcome` and route the interp-origin `compile_trace_from_interp` result through it too; both origins reach it via `compile_trace_inner`, which is where `retrace_after_bridge` is armed. Record the keys `compile_loop` stores under `cut_inner_green_key` in `cut_compiled_keys` (from `cel`'s a7cc560) and decline the interp-origin close when the target is one of them: a cut's `target_tokens[0]` is the cut prefix, not a preamble, and an entry bridge carries no runtime values, so `jump_to_preamble` (unroll.py:238-242) lands there with the specialized label's invariants unproven. The guard origin keeps its resume storage and is not gated; gating it as well makes `pi/pi.jinseo` under `MAJIT_THRESHOLD=50` diverge from its un-compiled run at output byte 865. Stage the segmented loop's green key in `pending_abort_green_key` so the `aborted_tracing` hook reports it instead of 0 (pyjitpl.py:2760). aheui `jitstats.py check`: 6 fixtures + 62 corpus + 62 stress, 0 failed. `cargo test -p majit-metainterp`: 1436 passed. Assisted-by: Claude
…it abort `_create_segmented_trace_and_blackhole` branches two ways at pyjitpl.py:1639. Only the loop arm was ported; the merge-point check tested `is_loop_trace` and let everything else fall through, so a bridge ran on to the ordinary over-limit abort. 0042e88 recorded that as blocked on a FINISH descriptor slot pyre's IR does not have. It has one: `Op::setdescr`, `Trace::record_op_with_descr` behind `recorder.finish`, and `exit_frame_with_exception_descr_ref` on the staticdata. Move the loop-vs-bridge test from the merge-point check into `create_segmented_trace`, where upstream keeps it, and return `TraceAction::SegmentedBridge` for the else-arm. Its driver arm calls `compile_finish_from_active_session([exception_box], Int, exception)`, which for a bridge origin is pyjitpl.py:1666 `compile_trace(metainterp, resumekey, [exception_box])`: it records the FINISH under `exit_frame_with_exception_descr_ref` and returns `Err` where upstream's `target_token is not token` reaches `compile.giveup()` (compile.py:27, ABORT_BRIDGE). The bridge arm therefore leaves the FINISH to that call; the loop arm still records it, still without a descr. The flag this arm answers to was already being armed with nothing to act on it: `prepare_trace_segmenting` (pyjitpl.py:2849-2857) sets FORCE_BRIDGE_SEGMENTING on the source loop token and `start_retrace_from_guard` reads it back into `force_finish_trace` (compile.py:725-731). `compile_simple_loop` plus `attach_procedure_to_interp` move to `compile_segmented_loop` so the two arms sit inside one match, sharing the single-pass handoff and the blackhole tail; the abort reason that tail reports is now the arm's, ABORT_BRIDGE for a bridge that gave up. `pyre/check.py`: dynasm 377/377, cranelift 377/377, wasm 373/373, no counter moved. aheui `jitstats.py check`: 6 fixtures + 62 corpus + 62 stress, 0 failed. Neither suite reaches the new arm (`abrt_segmented` is 0 throughout), so it is measured as not disturbing what compiles today, not as exercised. Assisted-by: Claude
…pted The JUMP-into-ptoken block (pyjitpl.py:3001-3007) starts its result at `Declined` and reaches the decline tail whether or not the gate above it ran, so `note_cross_loop_close_declined` also latched keys no `compile_trace` was ever attempted for. Both gate conditions are transient — `partial_trace` clears with the retrace it belongs to, and a key acquires compiled targets when its loop compiles — and upstream re-tests them at every visit of the header, keeping no record of the last one. The latch exists so a later crossing does not re-run the optimizer over a growing trace-so-far for a decline that will repeat; a close the gate never evaluated ran no optimizer pass. Latch only an attempt that ran and returned without compiling. Assisted-by: Claude
The cut-decline gate's comment already stated that widening the decline to the guard origin makes aheui's pi/pi.jinseo diverge from byte 866. Add what the divergence consists of: the resumed walk closes a second time one aheui instruction later, so the bridge carries that instruction's pop and jumps into a four-input label instead of the three-input one the declined close targeted. Declining and then giving the trace up outright reproduces the un-compiled output. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f4cf5d32f
ℹ️ 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".
| // below does not report key 0. | ||
| self.meta.pending_abort_green_key = green_key; | ||
| match result { | ||
| Ok(()) => crate::counters::ABORT_SEGMENTED_TRACE, |
There was a problem hiding this comment.
When this segmented bridge compile is cancelled (for example because the optimizer returns no final bridge or the source loop is gone), compile_finish_from_active_session currently treats CompileOutcome::Cancelled as Ok(()) (pyjitpl.rs:14644-14651). That makes this new arm report ABORT_SEGMENTED_TRACE and clear the trace as if compile_trace(...) returned the exception FINISH token, but upstream's pyjitpl.py:1666-1668 runs compile.giveup() whenever the returned target token is not that token, so cancelled segmented bridge compiles should surface ABORT_BRIDGE instead of being counted as successful segmentation.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
youknowone
added a commit
that referenced
this pull request
Aug 6, 2026
The two sibling cross-loop close sites bump `bridge_declined_close` (50) when an attempt is declined; the JUMP block's own Declined arm bumped nothing. Bump it on the same `attempted` condition the decline latch uses, so the tally counts closes an optimizer pass rejected rather than headers the gate skipped. Reported by CodeRabbit on #1040. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 7, 2026
The two sibling cross-loop close sites bump `bridge_declined_close` (50) when an attempt is declined; the JUMP block's own Declined arm bumped nothing. Bump it on the same `attempted` condition the decline latch uses, so the tally counts closes an optimizer pass rejected rather than headers the gate skipped. Reported by CodeRabbit on #1040. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 7, 2026
…itCodeBuilder follow-ups (#1084) * _pickle: reuse the unpickler stacks and cache the pickler write callable `Unpickler.load` allocated a fresh stack and metastack list on every call; both are now owned by the constructor, initialized lazily for the `__new__`-only path, and cleared in place instead. `Pickler` resolves `file.write` once in the constructor and stores it in a rooted `w_write` field; `Framer::flush` calls it directly rather than re-resolving the method on every flush. The constructor's error order is unchanged: the `write` check still precedes the protocol-5 `buffer_callback` check. Assisted-by: Claude * jit: trace the force token as a GC slot `virtualref.py:19` declares `virtual_token` and `rvirtualizable.py:29` declares `vable_token` as `llmemory.GCREF`, and `jitframe.py:59-61` makes JITFRAME a `GcStruct` allocated by `lltype.malloc` at `:50`. cranelift allocates its JITFRAME from the nursery accordingly, but both token slots and the FORCE_TOKEN result were excluded from GC tracing on the written premise that a JITFRAME address is `libc::calloc`'d and therefore outside the GC heap — which holds only for the dynasm backend. Register `virtual_token` in the vref `gc_ptr_offsets`, visit `vable_token` in `pyframe_object_custom_trace`, and stop excluding the FORCE_TOKEN result from the cranelift relocatable ref-root slots. `TOKEN_TRACING_RESCALL` was the `u64::MAX` sentinel, which is not a legal value for a traced slot; it becomes the address of a registered GC leaf, matching `virtualizable.py:326-330` where the sentinel is the prebuilt `_dummy` object. Host-side active-token stores now take the same write barrier a compiled SETFIELD_GC store would. Assisted-by: Claude * majit: report the JUMP block's declined close to the diag census The two sibling cross-loop close sites bump `bridge_declined_close` (50) when an attempt is declined; the JUMP block's own Declined arm bumped nothing. Bump it on the same `attempted` condition the decline latch uses, so the tally counts closes an optimizer pass rejected rather than headers the gate skipped. Reported by CodeRabbit on #1040. Assisted-by: Claude * majit: drop two dead allocation sources in JitCodeBuilder `add_struct_field_descr` deep-copied the whole parent `BhSizeSpec` (one owned `String` per field) on every field-descr mint, although `patch_field_descr_parents` — called unconditionally from `try_finish` after the decline early-return — replaces that snapshot with the final merged spec, and `struct_size_specs` entries are only inserted or merged, never removed. Carry the scalar fields and leave `all_fielddescrs` empty; `type_id` is the only part the patch pass reads. `register_struct_layout` rebuilt `field_specs_from_layout` on each of its ~211 calls for ~5 distinct layouts. When the cached spec already lists an offset for every incoming field the merge pushes nothing and the following re-sort/re-index are no-ops, and the branch never writes `size`, `is_gc_managed` or `headerless`, so return before building the discarded vector. Measured on aheui's `mainloop` fixed per-process init (never-tracing, 200 iterations, min of 5 interleaved rounds): 243.7 -> 214.6 us/call. Assisted-by: Claude * jit: mint the tracing sentinel on the runtime GC condition, not cfg(test) `allocate_tracing_rescall_dummy` guarded its unmanaged fallback with `#[cfg(test)]`, which is set only while majit-metainterp compiles its own test harness. Built as an ordinary dependency the arm disappears, so `pyre-jit-trace`'s `may_force_vable_escape_surfaces_typed_abort` and `may_force_with_active_vable_executes_and_clears_token` — which drive the token protocol without a collector — reached the `assert_ne!` and panicked. Branch on the unset type id itself: the leaf type is registered by the same setup that installs a collector, so an unset id means there is no managed heap to mint the object in, and the host address stays outside it where `is_managed_heap_object` rejects it before any tracing path reads its header. `alloc_virtual_ref` spells the same window the same way. The ordering requirement moves to `set_tracing_rescall_dummy_gc_type_id`, which asserts the sentinel has not already been minted. Assisted-by: Claude * _pickle: record the measured write-resolution and constructor-order divergences `interp_pickle.py` resolves `file.write` at `:555-560` only to validate and re-resolves it per write in `_Framer.file_write` (`:353`), and checks `buffer_callback` before the file in `descr__new__` (`:1822`). Measured on 3.14.5 neither holds: rebinding `file.write` after construction is not observed by a later `dump()` — `pickle.py:465` captures the callable the same way — and a call carrying both constructor faults reports the `write` TypeError. pyre matches the measurements; note them at both sites. Assisted-by: Claude * jit: re-mint the tracing sentinel when the GC is rebuilt `set_tracing_rescall_dummy_gc_type_id` is called from `build_gc` (`pyre-jit/src/eval.rs:1338,3639`), which `reset_gc_fresh_for_test` runs again per GC-stress worker. The sentinel was a `OnceLock`, so a second heap kept the address minted in the first — `is_managed_heap_object` no longer recognises it once the heap it belongs to has been replaced, putting the traced `virtual_token` / `vable_token` slots back on an address the collector does not own. The assertion added with the previous commit turned that into a panic on the second registration instead. Hold the address in an `AtomicUsize` the setter clears, so the next request mints in the heap that is now current, and publish it with a compare-exchange so racing minters agree on one address. The GC-stress harness produces this ordering the moment one of its programs reaches a traced residual call; none does today, so the new test mints the sentinel between two resets directly. It panics at the old assertion without this change. Reported by the Codex review bot on #1084. Assisted-by: Claude * _pickle: allocate the unpickler stack and metastack per load again `interp_pickle.py:2042-2043` installs a fresh `stack` and `metastack` on every `load`, and `_pickle.c` reuses one `Pdata` for the unpickler's whole lifetime with MARK tracked by `num_marks`/`fence` indices inside it (`Pdata_New`, `_pickle.c:454`). Reuse is coherent only inside the second structure; layered onto the metastack-of-lists port it produced a shape neither has, and `mark()` rebinding `w_stack` to a fresh list meant the constructor's list was dropped by any load that reached STOP with an open MARK. It is observable. Capturing the live stack during one `load` through `gc.get_referents` and appending through it during a later one changes that load's result: load3: (<function poison>, 'z') reused stacks load3: (1, 'z') 3.14.5, pypy3 3.11.15, and this commit Reported by the Codex review bot on #1084. Assisted-by: Claude * majit: reformat the tracing sentinel publish match rustfmt collapses the `compare_exchange` call in `token_tracing_rescall` onto one line. No behaviour change. Assisted-by: Claude * majit: measure the optimizer and backend times passed to log_compile `WarmState::log_compile` takes `opt_time` and `compile_time` and forwards them to the jitlog, which prints them as the `Optimization time:` / `Compilation time:` lines of the `MAJIT_STATS` summary (`majit-trace/src/logger.rs:194-207`, summed over every compile). Both production call sites passed `Duration::ZERO`, so both lines read `0.0ms` for every program. `compile_loop_body` now times the optimize block — the primary `optimize_trace_with_constants_and_inputs_vable_out`, the without-unroll retry taken on `InvalidLoop`, and loop vectorization — and separately the `self.backend.compile_loop` call already wrapped by `profiler.enter_backend()`. `compile_retrace` gets the same treatment for its own optimizer and backend calls. `jitprof.rs` privately imported `std::time::Instant`, or its `wasm_clock::Instant` shim on wasm32 where `Instant::now()` panics; both become `pub use` so `pyjitpl.rs` reaches the platform-agnostic type instead of naming `std::time::Instant` directly. Measured on aheui: `standard/loop` (85 recorded ops) reports 3.1ms / 1.3ms, `logo` (33177) reports 149.0ms / 25.3ms. Assisted-by: Claude * majit: log the three loop compiles that reached no jitlog `self.stats.loops_compiled += 1` appears at five places in pyjitpl.rs, but only `compile_loop_body` (:6808) and `compile_retrace` (:7978) called `warm_state.log_compile`. `finish_and_compile`, `compile_simple_loop` and `compile_entry_bridge` compiled a loop and told the jitlog nothing. `log_compile` is the sole source of the `=== JIT Statistics ===` block that `MAJIT_STATS=1` prints (`majit-trace/src/logger.rs:194-207`), so a run whose loop arrived through one of those three paths reported `Traces compiled: 0`, `Total ops recorded: 0` and both times `0.0ms` while the run had in fact compiled a loop. Each of the three now times its own optimizer invocation and its own `backend.compile_loop` call and passes the counts and durations, matching the two working sites. On aheui's logo at `MAJIT_TRACE_LIMIT=30000` — a limit low enough that the whole-program trace aborts and the loop arrives through one of these paths — the block goes from `0 / 0 / 0.0ms / 0.0ms` to `Traces compiled: 1`, 24004 recorded ops, 7065 after optimization, 42.3ms and 9.7ms. At the default limit the same program is unchanged at 1 / 33177 / 20564, so no compile is now counted twice. Assisted-by: Claude
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.
Follow-ups to the two review channels on #1005 — the six inline comments and the
CI parity review, which was scoped to that PR's four files and carried a
section-1 item.
What changed
Every cross-loop close outcome is classified (
b8726a5). Thecompile_trace(live_arg_boxes, ptoken)close (pyjitpl.py:3001-3007) collapsedits result to a bool, so
RetraceNeededandFailedboth took the declinetail: a retrace kept tracing instead of reaching
compile_retracein the samecall, and a give-up walked on with a possibly drained tracing context. All four
BridgeCompileResultvariants are now handled, mirroring the ordinary closehandler below.
close_bridge'sCompileOutcomemapping is extracted asclassify_compile_outcomeand the interp-origincompile_trace_from_interpresult routes through it too — both reach it via
compile_trace_inner, which iswhere
retrace_after_bridgeis armed.A cut target is declined from the interp origin (
b8726a5, cherry-pickedfrom
cel's a7cc560). A cross-loop CUT'starget_tokens[0]is the cutprefix, not a preamble, and an entry bridge carries no runtime values, so
jump_to_preamble(unroll.py:238-242) lands there with the specialized label'sinvariants unproven. Scoped to the interp origin: the guard origin keeps its
resume storage, and gating it too makes
pi/pi.jinseounderMAJIT_THRESHOLD=50diverge from its un-compiled run at output byte 865 — samelength, same exit code, different digits.
A segmented bridge now compiles instead of aborting (
f9b00dc) — the CIreview's section-1 item.
_create_segmented_trace_and_blackholebranches twoways at pyjitpl.py:1639 and only the loop arm was ported. The else-arm's driver
step is
compile_finish_from_active_session([exception_box], Int, exception),which for a bridge origin is pyjitpl.py:1666
compile_trace(metainterp, resumekey, [exception_box]), giveup included.The flag it answers to was already being armed with nothing to act on it:
prepare_trace_segmentingsetsFORCE_BRIDGE_SEGMENTINGon the source looptoken and
start_retrace_from_guardreads it back intoforce_finish_trace—and the bridge then ran on to the ordinary over-limit abort, the
retrace-forever outcome the flag is set to prevent.
The decline latch fires only on a real attempt (
914b343) — the CI review'ssection-2 item.
note_cross_loop_close_declinedwas also latching keys whoseclose the gate never evaluated. Both gate conditions are transient and upstream
re-tests them at every visit.
Plus two comment corrections (
653ad38,0042e88) where #1005 left claims theJUMP arm had invalidated.
Note on 0042e88's message
That commit records the segmented-bridge port as blocked on "the FINISH
descriptor slot pyre's IR does not have". That was wrong —
Op::setdescr,Trace::record_op_with_descrbehindrecorder.finish, andexit_frame_with_exception_descr_refon the staticdata all exist.f9b00dcsupersedes it and says so. The loop arm's FINISH still carries no descr, which
is now stated as the remaining gap rather than as a blocker.
Verification
pyre/check.pyjitstats.py checkcargo test -p majit-metainterpcargo check --workspace --all-targets/cargo fmt --checkNo baseline was re-recorded; no counter moved.
What the gates do not show: neither suite reaches the new bridge arm —
abrt_segmentedreads 0 throughout. It is measured as not disturbing whatcompiles today, not as exercised.
Open, not fixed
Declining the cut close makes aheui compute the wrong digits, so the JUMP is
currently masking a latent miscompile in the fallback path rather than
creating one. Separate issue.
🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Reliability