jit: stop truncating lowering at abort_permanent, attach the missing catch edge, and let residual calls re-enter the JIT - #860
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (16)
WalkthroughThe PR filters nursery objects from moving major-GC marking, adds JIT inline hazard handling, restructures exception-edge and abort emission, routes residual calls through JIT-aware evaluation, adjusts write-barrier state, and adds or updates benchmark regression oracles. ChangesMajor marking worklist
JIT control-flow and residual dispatch
Benchmark and regression oracles
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant FBWWalker
participant InlineResolver
participant Codewriter
participant ResidualCall
FBWWalker->>InlineResolver: attempt callee inline
InlineResolver->>FBWWalker: decline hazardous or unsupported callee
FBWWalker->>Codewriter: emit abort or split exception edge
Codewriter->>ResidualCall: route non-inlined execution
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 9cc4940). 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: 744331d899
ℹ️ 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".
| /// away once every `gc_ptr_offsets` target is a real GC allocation. | ||
| fn grey_child(&mut self, addr: usize) { | ||
| if self.is_managed_heap_object(addr) { | ||
| if self.is_managed_heap_object(addr) && self.may_enter_marking_worklist(addr) { |
There was a problem hiding this comment.
When marking finishes through the standalone gc_step() between minor collections, a black old object may have been updated to point to a young container that contains a still-white old object. rescan_remembered_black_and_drain requeues the old parent, but this new condition skips the young container entirely, so its old child remains white and is swept while still reachable, leaving a dangling pointer in the live nursery object. Upstream's nursery-worklist exclusion relies on major progress being driven after a minor; preserve that scheduling here or synchronously trace nursery objects without retaining their addresses on the worklist.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
744331d to
1eefdf3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eefdf308a
ℹ️ 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".
| static FBW_HAZARDOUS_INLINE_DENY: std::cell::RefCell<std::collections::HashSet<usize>> = | ||
| std::cell::RefCell::new(std::collections::HashSet::new()); |
There was a problem hiding this comment.
When a hazardous callee is encountered, this records the upstream JC_DONT_TRACE_HERE decision in a parallel per-thread HashSet rather than on the callee's JitCell. Consequently the normal warmstate consumers never see the flag: each worker thread must rediscover the same static hazard and abort once, and the denied callee misses upstream's immediate bound_reached behavior and remains residual until its ordinary counter warms up. Store the decision on the callee JitCell and route both inlining and warmstate behavior through that authoritative field instead.
AGENTS.md reference: AGENTS.md:L126-L132
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 `@pyre/bench/synth/exception_nested_exc_info_restore.py`:
- Line 3: Rewrite the comment near the nested exception handling example to
clearly state that POP_EXCEPT restores the value saved by its matching
PUSH_EXC_INFO, removing the malformed “the prev its matching” wording.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 1417-1442: Replace the thread-local CALLEE_ABORT_PERMANENT_SEEN
cache used by callee_body_has_abort_permanent with a process-global,
lock-guarded map shared across threads, following the existing
call_descr_stub_cache pattern. Preserve the stable CodeObject-pointer key and
memoized abort_permanent scan while ensuring concurrent access is synchronized
and duplicate per-thread scans are eliminated.
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 10143-10149: In the FOR_ITER handling at the catch_for_pc call to
emit_catch_exception_and_split!, set exception_edge_handled = true immediately
after the macro invocation. Match the existing handling in the PopJumpIfFalse
and PopJumpIfTrue arms so the generic per-PC catch check does not reprocess the
already-split exception edge.
🪄 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 Plus
Run ID: 29a3e47c-972b-42c1-97d9-2af39beda939
📒 Files selected for processing (14)
majit/majit-gc/src/collector.rspyre/bench/synth/_pending/exception_nested_exc_info_restore.pypyre/bench/synth/exception_nested_exc_info_restore.pypyre/bench/synth/gc_bug_bridge_flavor_traceback_names.pypyre/bench/synth/loop_callee_shared_mutation.pypyre/bench/synth/sre_wasm_min.pypyre/bench/synth/sre_wasm_min1.pypyre/pyre-interpreter/src/call.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/jit/codewriter.rs
💤 Files with no reviewable changes (1)
- pyre/bench/synth/_pending/exception_nested_exc_info_restore.py
| @@ -0,0 +1,48 @@ | |||
| # Regression oracle: nested try/except where each handler reads | |||
| # `sys.exc_info()`. After an inner handler unwinds, POP_EXCEPT must restore the | |||
| # slot to the prev its matching PUSH_EXC_INFO saved (the outer ValueError), and | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the PUSH_EXC_INFO comment.
Line 3 contains the malformed phrase “the prev its matching,” which makes the documented restoration behavior difficult to understand. Rewrite it to state that POP_EXCEPT restores the value saved by the matching PUSH_EXC_INFO.
🤖 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/bench/synth/exception_nested_exc_info_restore.py` at line 3, Rewrite the
comment near the nested exception handling example to clearly state that
POP_EXCEPT restores the value saved by its matching PUSH_EXC_INFO, removing the
malformed “the prev its matching” wording.
1eefdf3 to
1f79ec9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f79ec980f
ℹ️ 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".
| callable: PyObjectRef, | ||
| args: &[PyObjectRef], | ||
| ) -> PyResult { | ||
| call_user_function_with_eval(frame, callable, args, get_eval_fn()) |
There was a problem hiding this comment.
When a compiled CALL residualizes a user function, this new entry bypasses the increment_call_depth() guard used by call_user_function; consequently eval_with_jit performs its entry stack_check() using the caller's depth. Residual or indirect recursion can therefore run past the configured Python recursion limit, and sys.setrecursionlimit() observes too few active frames. Wrap this call with the same depth guard as the ordinary user-function entry.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pyre/pyre-jit/src/jit/codewriter.rs (1)
10143-10149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing
exception_edge_handled = true;afteremit_catch_exception_and_split!inFOR_ITER— same gap flagged previously.Unlike the
PopJumpIfFalse/PopJumpIfTruesites, this call does not setexception_edge_handled = true;afterward. It stays safe only because the two unconditionalmergeblockcalls immediately below populatecurrent_block'sexitsbefore the generic per-PC catch check (if !exception_edge_handled && let Some(catch_label) = catch_for_pc[py_pc] {...}) runs, soblock_already_closedevaluatestrueand the redundant emission is skipped. That safety net is incidental, not explicit — a future change to the exhaustion-branch ordering could silently reintroduce a duplicate/garbled catch-exception emission on an already-split block. This exact concern was raised in an earlier review pass on this same call site and reportedly addressed, but the code as provided still lacks the fix.🐛 Proposed fix for consistency with sibling call sites
if let Some(catch_label) = catch_for_pc[py_pc] { emit_catch_exception_and_split!( catch_label, py_pc, [next_value.clone()] ); + exception_edge_handled = true; }🤖 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/src/jit/codewriter.rs` around lines 10143 - 10149, Set exception_edge_handled = true immediately after emit_catch_exception_and_split! in the FOR_ITER catch-label branch, matching the PopJumpIfFalse and PopJumpIfTrue call sites. Keep the existing mergeblock handling unchanged.pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs (1)
1417-1443: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTLS cache for a thread-independent, static property still lacks the required justification.
CALLEE_ABORT_PERMANENT_SEEN's own comment states the cached value is "a static property of the callee's assembled body" — not thread-specific, not disposable. Contrast withFBW_HAZARDOUS_INLINE_DENYinfbw_state.rs, which citeswarmstate.py:331and explains why walk state must be per-thread. This same gap was flagged previously; a shared, lock-guarded map (mirroringcodewriter.rs'scall_descr_stub_cache: Mutex<HashMap<...>>) would avoid duplicate per-thread scans and satisfy the guideline.♻️ Proposed fix: share the memo across threads
-thread_local! { - static CALLEE_ABORT_PERMANENT_SEEN: std::cell::RefCell<std::collections::BTreeMap<usize, bool>> = - const { std::cell::RefCell::new(std::collections::BTreeMap::new()) }; -} +static CALLEE_ABORT_PERMANENT_SEEN: std::sync::Mutex<std::collections::BTreeMap<usize, bool>> = + std::sync::Mutex::new(std::collections::BTreeMap::new()); fn callee_body_has_abort_permanent(w_code: *const (), body: &SubJitCodeBody) -> bool { let key = w_code as usize; - if let Some(hit) = CALLEE_ABORT_PERMANENT_SEEN.with(|m| m.borrow().get(&key).copied()) { + if let Some(hit) = CALLEE_ABORT_PERMANENT_SEEN.lock().unwrap().get(&key).copied() { return hit; } let hit = crate::jitcode_runtime::decoded_ops(body.code).any(|op| op.opname == "abort_permanent"); - CALLEE_ABORT_PERMANENT_SEEN.with(|m| { - m.borrow_mut().insert(key, hit); - }); + CALLEE_ABORT_PERMANENT_SEEN.lock().unwrap().insert(key, hit); hit }Based on learnings, the
**/*.rsguideline: "Do not use TLS for process-global, interpreter-owned, identity-sensitive, semantic, registry, cache, or GC-relevant runtime state... Use TLS only when upstream state is genuinely thread-specific or is a disposable temporary cache; all other TLS uses require an upstream citation and written justification."🤖 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/inline_call.rs` around lines 1417 - 1443, Replace the thread-local CALLEE_ABORT_PERMANENT_SEEN cache with a process-shared, lock-guarded map keyed by the stable CodeObject pointer, mirroring the existing call_descr_stub_cache pattern. Update callee_body_has_abort_permanent to read and populate this shared cache while preserving its current memoization 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 `@majit/majit-gc/src/collector.rs`:
- Around line 2005-2009: Establish the promotion boundary before normal
moving-major root seeding: in the relevant collector entry flow, run a minor
collection for moving-major cycles initiated through gc_step(), while preserving
the explicit non-moving-major exception. At majit/majit-gc/src/collector.rs
lines 2005-2009, update the lifecycle before the gcref gates; at lines
2529-2534, retain the nursery exclusion only under that established invariant.
Preserve strict line-by-line structural parity with the RPython/PyPy
implementation.
---
Duplicate comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 1417-1443: Replace the thread-local CALLEE_ABORT_PERMANENT_SEEN
cache with a process-shared, lock-guarded map keyed by the stable CodeObject
pointer, mirroring the existing call_descr_stub_cache pattern. Update
callee_body_has_abort_permanent to read and populate this shared cache while
preserving its current memoization behavior.
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 10143-10149: Set exception_edge_handled = true immediately after
emit_catch_exception_and_split! in the FOR_ITER catch-label branch, matching the
PopJumpIfFalse and PopJumpIfTrue call sites. Keep the existing mergeblock
handling unchanged.
🪄 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 Plus
Run ID: b817e16f-65a2-429e-9d83-794c472cec2b
📒 Files selected for processing (15)
majit/majit-gc/src/collector.rspyre/bench/synth/_pending/exception_nested_exc_info_restore.pypyre/bench/synth/exception_nested_exc_info_restore.pypyre/bench/synth/gc_bug_bridge_flavor_traceback_names.pypyre/bench/synth/loop_callee_shared_mutation.pypyre/bench/synth/sre_wasm_min.pypyre/bench/synth/sre_wasm_min1.pypyre/pyre-interpreter/src/call.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rs
💤 Files with no reviewable changes (1)
- pyre/bench/synth/_pending/exception_nested_exc_info_restore.py
…ltiframe seed preconditions `try_walker_inline_resolved_user_call`'s multiframe seed block answered "this callee cannot be seeded" with `Err(DispatchError::callee_inline_unsupported)`. `trace.rs` maps that variant to `TraceAction::Abort`, so the caller's whole loop trace was discarded rather than the one inline. The arm does not call `fbw_decline`, so the identical static precondition failed again on every retrace and the loop never settled. The strict-seed path at the same five checks already declines with `break 'seed`. Return `Ok(None)` for the `try_multiframe` path at `raw.is_null()`, `ncells != 0`, `ensure_jitcode_index` None, unresolved `portal_red_regs_at`, and null `snapshot_sym`. `Ok(None)` is this function's existing did-not-inline answer; every caller follows it to the ordinary residual call. All five precede the first recorded op (`GETFIELD_GC_R`), so nothing is left half-recorded. Upstream has no cannot-inline-this-callee abort: `pyjitpl.py` `do_residual_or_indirect_call` residualizes a callee it cannot follow, the recursion-budget path calls `dont_trace_here` and still falls through to `do_residual_call`, and `rlib/jit.py`'s ABORT_* set is TOO_LONG / BRIDGE / BAD_LOOP / ESCAPE / FORCE_QUASIIMMUT / SEGMENTED_TRACE. `bench/synth/_pending/wasm_parse_template_int_ref` compiles 4 loops where it compiled 0. The POP_JUMP_IF_NONE scan in the same block keeps its abort: residualizing it clears 98 aborts in `_pending/gc_bug_bridge_flavor_traceback_names` and 5 in `_pending/exception_nested_exc_info_restore`, but the newly compiled loops emit traceback tuples missing their outermost frame. Recorded at the site. The try-block caller-frame decline further down has the same wrong `Err` and needs a hoist above the seed block to fix; recorded as a FIXME. check.py: dynasm 326/326, cranelift 326/326, wasm 323/323. Assisted-by: Claude
…allsite
Inlining a callee whose assembled body carries an `abort_permanent` marker
walks the sub-walk into it. That surfaces as `TraceAction::AbortPermanent`,
which stamps `DONT_TRACE_HERE` on the CALLER loop's green key, so one unported
opcode anywhere in a callee permanently un-JITs every loop that calls it.
Scan the callee body in `try_walker_inline_user_call` once the body is
resolved, and return `Ok(None)` on a hit, which routes the call through the
ordinary residual path. Memoized per code object: the answer is a static
property of the assembled body, while the callsite is reached on every retrace
of every caller.
`loop_inlines_abort_permanent_callee` (`trace.rs`) already screens this before
the walk, but only for callees it resolves statically out of globals and frame
slots; a bound method, a container element or a call result reaches the
callsite unscreened. There the callee is concrete.
The screen is whole-body, matching `codewriter/policy.py:48-84`
`look_inside_graph`, whose own comment at :78-79 states the consequence of a
negative answer: "the call will be turned into a residual call".
bench/synth/list_length_hint_validate loops_compiled 0 -> 2, aborted 19 -> 8
bench/synth/slots_class_var_conflict loops_compiled 0 -> 2
Both matched `PYRE_NO_JIT=1` output before and after.
check.py: dynasm 326/326, cranelift 326/326, wasm 323/323.
Assisted-by: Claude
… aborting `decline_inline_caller_frame_for_catch_marker` was consulted only from inside the seed block of `try_walker_inline_resolved_user_call`, after `GETFIELD_GC_R` / `emit_new_pyframe_inline_with_params` had already recorded IR and a concrete `FrameBox` had been stamped. Its `Err` there became `LoopBearingCalleeInlineUnsupported`, which `trace.rs` maps to `TraceAction::Abort` — discarding the caller's whole loop trace. Add `inline_caller_frame_declines_for_catch_marker`, which replicates the two caller-frame lookup preambles (nested: framestack top `w_code` → `ensure_jitcode_index` → `pyjitcode_for_jitcode_index`; top-level: `fbw_mode.snapshot_sym`) and runs only the catch-marker predicate, which is a pure function of the caller jitcode and the CALL pc. Call it above the seed block under `try_multiframe` and return `Ok(None)`, so the call falls through to the ordinary residual call. The original in-seed arm stays `Err` for the strict-seed path, which reaches it with IR already recorded. `Unavailable` keeps going through `compute_inline_caller_frame`: it depends on live register banks and its fallback is the single-frame collapse, not a decline. `list_length_hint_validate` reports `LoopBearingCalleeInlineUnsupported` 10 → 0 with no `[lb-site]` output remaining; `loops_compiled` and `loops_aborted` there are unchanged, since 714d109320's callee `abort_permanent` screen already declines the same calls earlier. check.py: dynasm 326/326, cranelift 326/326, wasm 323/323. Assisted-by: Claude
…of retrying it
`fbw_abort_nested_unjournaled_residual`'s hazard arm declines when a frame on
the inline framestack is loop-bearing or self-recursive. That is a static
property of the callee's `CodeObject`, so the next trace attempt rebuilt the
same framestack and reached the same abort: `PYRE_FBW_DEBUG_ABORT=1` on
`foriter_exempt_nested_foriter` printed five byte-identical records
(`start_pc=51 Err=LoopBearingCalleeInlineUnsupported { pc: 240 }`), which is
`MAX_TRACE_ABORT_COUNT` (`majit/majit-metainterp/src/warmstate.rs:294`,
consumed at `:745`), so the enclosing green key was marked `DONT_TRACE_HERE`
for a decline belonging to the callee.
`fbw_inline_callee_hazardous` now returns the offending frame's `w_code`
instead of a bool. The hazard arm records it in `FBW_HAZARDOUS_INLINE_DENY`
and `try_walker_inline_resolved_user_call` declines that callee to `Ok(None)`,
so the call residualizes and the enclosing loop compiles. `w_code` and
`callee_code_key` are the same value (`inline_call.rs:1955` pushes
`w_code: callee_code_key`). The `DeferredCall` arm's existing
`FBW_FORITER_DEFERRED_DENY` is the same shape; upstream's is
`disable_noninlinable_function` (`warmstate.py:331`), applied to the callee
`find_biggest_function` (`pyjitpl.py:3538`) names while the enclosing loop is
left to retrace (`pyjitpl.py:2818-2828`). Like the upstream flag the set has
no removal path.
`bridge_rec_root_selfrec` is exempt at the callsite: it carries its own
`SELFREC_CA_FOLD_ACTIVE` exemption from the hazard arm (`inline_call.rs:2696`).
loops_compiled / loops_aborted, dynasm:
foriter_exempt_nested_foriter 1/5 -> 2/1
foriter_exempt_shared_generator 1/5 -> 2/1
inline_subwalk_user_iterator 2/5 -> 3/1
selfrec_tail_exception_unwind 2/2 -> 3/1
bridge_recursion_overflow 1 bridge -> 2 bridges, 1/1
ca_bridge_multiframe_resume_double_call, exception_escape_caller_frame_tb_node,
wasm_ca_trampoline_decline unchanged
All seven produce byte-identical output under JIT and PYRE_NO_JIT=1.
check.py: dynasm 326/326, cranelift 326/326, wasm 323/323.
Assisted-by: Claude
…stead of aborting"
This reverts commit 14765c632c.
The hoist made `_pending/gc_bug_bridge_flavor_traceback_names` diverge from the
interpreter on dynasm, whose header records that dynasm and the no-JIT run are
both clean and pins the expected output. Disabling only the hoisted condition
and rebuilding isolates it:
hoist off: A [('T', 'a_bridge_two_classes', 'mid_two', 'leaf_two'),
('V', 'a_bridge_two_classes', 'mid_two', 'leaf_two')]
loops_compiled=4 loops_aborted=98 (LoopBearing 97)
hoist on: A [('T', 'a_bridge_two_classes', 'mid_two', 'leaf_two'),
('T', 'mid_two', 'leaf_two'),
('V', 'a_bridge_two_classes', 'mid_two', 'leaf_two'),
('V', 'mid_two', 'leaf_two')]
loops_compiled=4 loops_aborted=2 (LoopBearing 1)
The extra tuples lack the outermost frame, so the loops the hoist newly compiles
build a traceback without a `PyTraceback` node for the frame that catches. That
is the same signature already recorded at the `POP_JUMP_IF_NONE` seed
precondition in `try_walker_inline_resolved_user_call`, which keeps its `Err`
for this reason; the abort was masking the missing node rather than preventing
it. `check.py` did not catch this: `Path(SYNTHETIC_BENCH_DIR).glob(pattern)`
(`check.py:1483`) does not descend into `_pending/`.
The hoist is otherwise sound and worth relanding once the node is recorded.
Assisted-by: Claude
…urn abort
`ExcEdgeCrossFrameReturnUnsupported` is raised when an exception-guard bridge
was routed to the exc edge but the handler returns out of the frame, which
`bridge_subwalk.rs:211-227` decides with `find_catch_for_exc_resume` and
`exc_handler_rejoins_loop` over `(jitcode_code, position)` alone. The same
guard therefore reaches it on every retrace: `PYRE_FBW_DEBUG_ABORT=1` prints 47
records of `start_pc=14 Err=ExcEdgeCrossFrameReturnUnsupported { pc: 202 }` in
`type_name_surrogate_reject` and 57 of `start_pc=34 ... { pc: 392 }` in
`inline_subwalk_mutating_residual`, with no other record between them. Each
retry re-walks the whole body and concretely executes its residual calls before
failing again, which is the cost `trace.rs`'s `AbortPermanent` bridge-decline
already documents for the same premise: a bridge entry is keyed on the guard
descr, which the location's `DONT_TRACE_HERE` cell never gates.
The error cannot take the `AbortPermanent` mapping — it is raised before any
recording precisely so the guard resumes via the blackhole, which is the
correct caught-exception + callee-return handling, not a location to retire.
Record only the bridge-guard decline (`fbw_bridge_decline` →
`take_fbw_bridge_declined` → `record_declined_bridge_guard`), leaving the plain
`Abort` and the blackhole resume unchanged.
ExcEdgeCrossFrameReturnUnsupported records, dynasm:
type_name_surrogate_reject 47 -> 1
inline_subwalk_mutating_residual 57 -> 1
inline_subwalk_property_mutates 57 -> 1
list_length_hint_validate 2 -> 2
All four produce byte-identical output under JIT and PYRE_NO_JIT=1.
check.py: dynasm 328/328, cranelift 328/328, wasm 325/325.
Assisted-by: Claude
`check.py --synthetic-only --synthetic-pattern '_pending/*.py'` passes these on
dynasm, cranelift and wasm, so they now run in the default gate:
exception_nested_exc_info_restore JIT answered 3320000 (trait) / 3360000
(FBW walker); now 360000, matching the
interpreter and CPython
gc_bug_bridge_flavor_traceback_names cranelift aborted 8/10 to 9/10 runs with
`GC BUG: invalid type_id=` out of
`incremental_mark_step`; now 20/20 clean
with the output its header pins
loop_callee_shared_mutation JIT printed 2*N + 3; now 2*N
sre_wasm_min, sre_wasm_min1 pass on all three backends
Their headers described the failures as current, so each is restated as a
regression oracle keeping what the defect was.
The remaining four stay in `_pending/`, none of them for JIT reasons:
`dict_set_clear_in_eq_ops` and `dict_set_clear_in_eq_restart` record a
deliberate CPython/PyPy divergence, which check.py reports as BASEFAIL by
construction; `wasm_parse_template_int_ref` imports `re._parser`, absent before
CPython 3.11, and the local baseline is 3.9.6; `cranelift_finalizer_ordering_crash`
exceeds the 20s per-benchmark timeout.
check.py: dynasm 333/333, cranelift 333/333, wasm 330/330.
Assisted-by: Claude
`emit_abort_permanent!` appended a returnblock link, which set the block's `exits`. The `block_closed_by_terminator` gate in the lowering loop then skipped op dispatch for every later PC, so nothing behind the opcode was lowered: `merge_entry_by_green` lost every loop header that followed, and `compile_and_run_once` refused through `pjc.n(target_pc).is_none()` without recording an abort. One `class` statement (`LoadBuildClass`), `del` (`DeleteName` / `DeleteGlobal`), or annotated assignment (`SetupAnnotations`) in a module prologue therefore left every loop in that module permanently un-JITtable and silent — `loops_compiled=0 loops_aborted=0`, only a repeated `[jit][bound-reached]`. Emit the marker without closing the block. Each arm already models its stack effect (`push_fresh_ref` / `pop_and_decr_depth`), so the fall-through FrameState is well typed and a real terminator closes the block; the runtime bails to the interpreter at the marker and never reaches the continuation. The returnblock link stays for the one case with no successor to close the block (abort at the last instruction), where `flatten.py:107-109` would read an exit-less block's FrameState tuple as return arguments. `pyopcode.py:865-870 LOAD_BUILD_CLASS` and `:777-778 LOAD_LOCALS` are plain value pushes, and `flowcontext.py` has no notion of blacklisting the graph an untraceable operation appears in. Ladder (CPU time, interleaved, min of 3, dynasm), `for _ in range(40000): for i in range(64): <shape>`: shape JIT before / after speedup before -> after t = i + 1 0.09s / 0.10s 9.22x -> 8.37x t = [i] 0.21s / 0.22s 4.67x -> 4.30x F() 2.59s / 1.85s 0.91x -> 1.16x F(i) with __init__ 8.44s / 4.29s 0.55x -> 1.12x check.py: dynasm 333/333, cranelift 333/333, wasm 330/330. Assisted-by: Claude
… stack model The previous commit stopped closing the block at every `emit_abort_permanent!` site. Two of the 23 do not fall through: the `Call` arm with `nargs > 14` `continue`s past its `push_and_bump!`, and the `LoadFastCheck` unbound arm switches into a dedicated dead-end block whose fall-through PC the bound arm has already merged. Leaving those open walked the next PC with an incomplete stack model, and cost `load_fast_check.py` its loop (loops_compiled 1 -> 0, loops_aborted 0 -> 5). Add an explicit `emit_abort_permanent!(pc, closes_block)` form and use it at those two sites; the auto-close for an abort at the last instruction moves into the same parameter. 319-benchmark synth census (dynasm, both binaries built with the same `--no-default-features --features dynasm`): loops_compiled 1814 -> 1823, bridges_compiled +0, loops_aborted +1, guard_failures +4, no returncode change. Five benchmarks change: list_inplace_mul_parity, module_body_truncated_jitcode_replay and module_getattr_surrogate_cls newly compile a loop (the last also drops 5 aborts to 0), exception_reraise_tb_depth_hot 1 -> 6 loops, gc_deque_backing_list 4 -> 5. Assisted-by: Claude
…lock POP_JUMP_IF_FALSE / POP_JUMP_IF_TRUE record the `bool` graph op and then close the block with its two Bool exits in the same dispatch arm. The generic per-PC catch emission runs after the arm and skips a block whose exits are already set, so a `bool` covered by a `try` range got no `catch_exception/L`: a raising `__bool__` deopted into a frame with no catch and exited via ExitFrameWithExceptionDescr instead of reaching the in-frame handler. Cut the block at the can-raise op inside the arm — the `flowcontext.py:130-156 guessexception` shape — and wire the branch into the successor. The cut is factored out of the FOR_ITER site into `emit_catch_exception_and_split!`, which additionally threads the branch input through the link (`unsimplify.py:59-76 split_block`) so `regalloc.py:26-77 make_dependencies`, whose liveness is per block from `inputargs`, sees it live in the successor. Assisted-by: Claude
`seed_major_root` and `grey_child` pushed any managed address, nursery included. The marking worklist outlives the mutator resuming, so a nursery entry is popped after the next `reset_nursery` has recycled those bytes and `object_total_size` reads a garbage `type_id` out of the recycled header — `GC BUG: invalid type_id=... site=object_total_size`. `incminimark.py:2739-2753 _collect_obj` appends to `objects_to_trace` only `if not self.is_in_nursery(obj)`, and `visit` (:2797-2799) asserts `not self.is_in_nursery(obj)` on each popped entry. Gate both push sites on the same condition and mirror the `visit` assertion in `mark_object`. The non-moving oldgen major keeps its nursery marking: it leaves the nursery bytes in place by contract and `note_nonmoving_nursery_mark` clears the marks as its last step. Assisted-by: Claude
`bh_call_fn_impl` wrapped its callee in `force_plain_eval()` and called `call_user_function_plain`, so every Python frame under a residual CALL — the whole nested subtree, not just the callee — ran on `eval_frame_plain`. A hot inner loop therefore stopped compiling for as long as its caller's loop was compiled. `blackhole.py:1225 bhimpl_residual_call_r_i` is `cpu.bh_call_i(func, ...)`: it invokes the translated function, and a callee whose graph reaches a `jit_merge_point` enters the JIT. Residual means opaque to the trace, not "JIT off for the extent of the call"; `bhimpl_recursive_call_*` (`blackhole.py:1095-1132`) is the statically-known-portal form, not the only route back to the portal. Re-entrant tracing stays blocked on the green key by `driver.is_tracing()` (`warmstate.py:473-477` JC_TRACING). Add `call_user_function_residual`, which keeps `get_eval_fn()`, and use it here. The seven other `force_plain_eval` sites are unchanged. Nested-loop ladder (dynasm, CPU time, min of 3, 6000 outer x N inner): inner N=50/200/800 goes 0.25s/0.76s/2.89s -> 0.08s/0.08/0.09s, i.e. from tracking the interpreter (0.29s/0.97s/3.64s) to flat. Corpus census: loops_compiled 1823 -> 1870, bridges 373 -> 404. Assisted-by: Claude
…Object A parity review read the code-address key as a truncated green key that could suppress inlining at unrelated callsites. The flag this mirrors is consumed by `can_inline_callable` (`warmstate.py:669-677`), whose only caller `_opimpl_recursive_call` (`pyjitpl.py:1376-1382`) passes the CALLEE's green args; a callee reached through a CALL is entered at its own entry, so `next_instr` is constant and `pycode` carries the decision. The deny is recorded and queried at exactly that CALL-boundary inline, never a mid-body resume, so the key is complete. Also note the half of `JC_DONT_TRACE_HERE` that is not ported: `warmstate.py:485-495` treats it as "trace from here as soon as possible" and reaches `bound_reached` immediately for a denied cell that never had a procedure token. Comments only. Assisted-by: Claude
…entry `bh_call_fn_impl`'s user-function arm calls `call_user_function_residual` (4cd51e8), so the callee can enter the JIT. A nested compiled or blackhole execution then writes the same two cells the helper publishes to: the thread-local `BH_LAST_EXC_VALUE` and the backend `_store_exception` pair. Upstream cannot alias them — the raise lives in `metainterp.last_exc_value`, a field of the MetaInterp instance that owns the call, and `llmodel.py:194 _store_exception` is read back by the same `bh_call_*` that armed it. Take both cells out of their slots for the span of the call and put them back, so the helper publishes only its own outcome. A value taken out is pinned on the shadow stack for the span: the cell is the only root it has (`walk_bh_last_exc_value` / `walk_jit_exc_value`). The other seven residual sites keep `force_plain_eval` and cannot nest. `jit_exc_value_peek_backend` becomes `pub(crate)` for the non-destructive read. Measured on `lib-python/3/test/test_strftime.py` with `time.time()` pinned through the environment, swept over 180 consecutive `now` values: 7 fail without this change and 0 with it, the same 7 that separate 4cd51e8 from its parent. The failures were an `IndexError` escaping `re/_parser.py:243`'s own `except IndexError`, and one `compile.py:458 assert i == len(inputargs)`. dynasm 337/337, cranelift 337/337, wasm 334/334, CPython suite gate 39/39. The nested-loop ladder 4cd51e8 measured is unchanged (6000 outer x 50/200/800 inner: 0.06s each, interpreted 0.09/0.21/0.69s). Assisted-by: Claude
…ly on `FBW_HAZARDOUS_INLINE_DENY` and `CALLEE_ABORT_PERMANENT_SEEN` are permanent maps keyed by a raw `PyCode` address with no removal path. That is sound only because `w_code_new` (`pycode.rs`) allocates every `PyCode` with `Box::into_raw` and nothing frees it, so the address is unique for the process and never moves — upstream instead keys on a `JitCell` that holds its greens and is pruned by `should_remove_jitcell` (`warmstate.py:212`). `eval.rs`'s `PyCode` GC registration already names the change that would end the immortality (switching `w_code_new` to `try_gc_alloc_stable`); cite it from both memos, and name what breaks there — a reclaimed address returning another code object's answer, which for `CALLEE_ABORT_PERMANENT_SEEN` is a stale `false` admitting an inline whose body does carry `abort_permanent`. Comments only. Assisted-by: Claude
`pycode.py:159-165 frame_stores_global` stamps `w_globals` permanently. pyre code objects are Box-immortal (`w_code_new` uses `Box::into_raw`), so the collector never traces into them; the slot was forwarded only where `walk_raw_code_roots` ran on a `frame.pycode` or `func.code` that the root walk already reached. A stamped code object off the frame chain and absent from every walked frame slot kept a pre-move nursery address once its globals dict was promoted, and the next call through that code object forwarded the stale pointer. Register every stamped code object in `W_GLOBALS_STAMPED_CODES` and forward its `w_globals` slot from the PyFrame root area, next to the existing `_mapdict_caches` `w_method` walk. `lib-python/3/test/test_tokenize.py` aborted in `copy_nursery_object` with `invalid type_id` 3/3 before, and completes 3/3 after with the two failures the `PYRE_NO_JIT=1` run also reports. Assisted-by: Claude
1f79ec9 to
9cc4940
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/9cc49409c3afcbf518e69e24136e8c25d6894d1f/pyre-interpreter/src/pycode.rs#L1966-L1967
When a code object first stamps w_globals on a short-lived worker thread, this TLS registry is destroyed when that thread unregisters, even though the Box-immortal PyCode can remain callable from another thread. Because w_code_frame_stores_global only registers during the initial null-to-value transition, a later thread does not restore the entry; after a moving collection, code.w_globals can therefore retain the old nursery address and subsequent LOAD_GLOBAL accesses a dangling pointer. Store this GC-root registry on the shared interpreter/process owner rather than the stamping thread.
AGENTS.md reference: AGENTS.md:L148-L155
ℹ️ 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".
1 participant
Twelve commits, continuing the call-abort epic. The last five are this cycle's
findings; each was reduced to a reproducer and grounded in the RPython/PyPy
source before the fix.
abort_permanenttruncated the whole code objectemit_abort_permanent!appended a returnblock link, which madeexitsnon-empty; the dead-code gate in the lowering loop then skipped op dispatch for
every later PC, so
merge_entry_by_greenlost all following loop headers andcompile_and_run_oncereturnedNone— silently (loops_compiled=0 loops_aborted=0). Any code object containing aclassstatement(
LOAD_BUILD_CLASS), adel, an annotation, or ayieldtherefore nevercompiled anything after that opcode.
abort_permanentterminates the run, not the graph. The block is now closedonly at the two arms that skip their own stack model (
Callwith nargs > 14 andthe
LoadFastCheckunbound arm), via an explicitcloses_blockmacro form.Corpus:
loops_compiled1814 → 1823, three benchmarks newly compile.A branch arm closed its can-raise block before the catch edge was attached
POP_JUMP_IF_FALSE/TRUErecord the can-raiseboolop and then close theblock with its two Bool exits in the same dispatch arm. The generic per-PC
catch emission runs after the arm and skips a block whose exits are already set,
so a
boolcovered by atryrange got nocatch_exception/L: a raising__bool__deopted into a frame with no catch and exited viaExitFrameWithExceptionDescrinstead of reaching the in-frame handler.flowcontext.py:130-156 guessexceptioncloses the recording block at thecan-raise op and resumes normal flow in a fresh
EggBlock;guessboolthen setsthe exitswitch on that second block. The cut is now performed inside the arm and
factored out of the FOR_ITER site into
emit_catch_exception_and_split!, whichadditionally threads the branch input through the link (
unsimplify.py:59-76 split_block) soregalloc.rs make_dependencies— per-block liveness frominputargs— sees it live in the successor. The FOR_ITER site had that gap too.Nursery objects on the major marking worklist
seed_major_rootandgrey_childpushed any managed address, nursery included.The worklist outlives the mutator resuming, so a nursery entry is popped after
the next
reset_nurseryrecycled those bytes andobject_total_sizereads agarbage
type_id.incminimark.py:2739-2753 _collect_objappends onlyif not self.is_in_nursery(obj), andvisit(:2797-2799) asserts the same. Both pushsites are now gated on that condition and the assertion is mirrored in
mark_object. The non-moving oldgen major keeps its nursery marking — it leavesthose bytes in place by contract.
Measured:
synth/inline_subwalk_mutating_residualon cranelift went 14/20 crashesto 0/20.
A residual call forced plain eval for its whole subtree
bh_call_fn_implwrapped its callee inforce_plain_eval(), a thread-local thatpins every Python frame under the residual CALL to
eval_frame_plain. A hotinner loop therefore stopped compiling for as long as its caller's loop was
compiled.
blackhole.py:1225 bhimpl_residual_call_r_iiscpu.bh_call_i(func, ...): itinvokes the translated function, and a callee whose graph reaches a
jit_merge_pointenters the JIT. Residual means opaque to the trace, not "JIToff for the extent of the call";
bhimpl_recursive_call_*(
blackhole.py:1095-1132) is the statically-known-portal form, not the onlyroute back. Re-entrant tracing stays blocked on the green key by
driver.is_tracing()(warmstate.py:473-477JC_TRACING).Nested-loop ladder (dynasm, CPU time, min of 3, 6000 outer x N inner):
PYRE_NO_JIT=1Before, the compiled case tracked the interpreter linearly; after, it is flat.
Corpus census:
loops_compiled1823 → 1870, bridges 373 → 404.Gate
python3 ./pyre/check.py— dynasm 333/333, cranelift 333/333, wasm 330/330.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
try/except+sys.exc_info()and regex workloads.