Repository navigation
jit: fix two exception-path compile defects (last-exc lookahead, except-tuple pin) - #706
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes 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 Run ID: 📒 Files selected for processing (4)
WalkthroughAdds a benchmark for tuple and single-class exception clauses, refines JIT exception-match tuple guarding, and replaces linear exception-state scanning with label-aware control-flow traversal. ChangesException matching
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 a53cb16). 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: 5a3739195c
ℹ️ 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".
| if !started.contains(&target) { | ||
| started.push(target); | ||
| pending.push(target); | ||
| } | ||
| pc = op.next_pc; | ||
| true |
There was a problem hiding this comment.
When the folded CHECK_EXC_MATCH is followed by goto_if_not, this now queues the jump target and returns true if either successor reads last_exc_value. That over-preserves the active exception on traces that take the other branch; for example, a matching except TypeError: whose sibling except ValueError as e: is the jump target will keep ctx.last_exc_value even though the interpreter-side residual call would have cleared it on the taken path, so a residual-free handler can hit the later catch_exception/L with a stale active exception and abort as CatchExceptionWithActiveException. The lookahead needs to use the concrete branch outcome already available to the walker rather than OR-ing both successors.
AGENTS.md reference: AGENTS.md:L16-L18
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs (1)
2219-2230: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not treat
is_class_knownas an identity guard.When tuple pinning declines, this condition skips
GuardValuewhenever the heapcache knows onlymatch_op’s physical class. Rebinding a handler global from one exception class to another can preserve that physical class, so the precomputed match result may be reused without detecting the identity change. Only tuple-element pinning should suppress the container identity guard; otherwise emitGuardValuefor every non-constant fallback target.Proposed fix
if !match_op.is_constant() && !walker_guard_exc_match_tuple_items(ctx, op_pc, match_op, match_type)? - && !ctx.trace_ctx.heap_cache().is_class_known(match_op) {🤖 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/specialize.rs` around lines 2219 - 2230, Update the guard condition around match_op so is_class_known no longer suppresses GuardValue. After match_op.is_constant() and walker_guard_exc_match_tuple_items(...) both decline, always record the identity guard, capture the snapshot, and replace the boxed value for every non-constant fallback target.
🤖 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/mod.rs`:
- Around line 2068-2119: The decode_op_at failure path in
reads_last_exc_before_next_catch currently treats undecodable successors as
safe; change the None branch to return true conservatively, while preserving the
existing traversal behavior for successfully decoded operations.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Around line 2060-2103: Update walker_guard_exc_match_tuple_items so the
SPECIALISED_TUPLE_OO branch validates that match_type’s w_class is the canonical
specialized tuple class before calling walker_guard_exc_match_tuple_class or
reading inline values. Reuse the existing canonical-w_class validation from the
array-backed tuple branch and return false on mismatch, preserving the identity
fallback for unsupported tuple subclasses.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Around line 2219-2230: Update the guard condition around match_op so
is_class_known no longer suppresses GuardValue. After match_op.is_constant() and
walker_guard_exc_match_tuple_items(...) both decline, always record the identity
guard, capture the snapshot, and replace the boxed value for every non-constant
fallback target.
🪄 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: 3a9880df-9c3f-4963-a488-3fec1855dd25
📒 Files selected for processing (3)
pyre/bench/synth/except_tuple_clause_hot.pypyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs
`reads_last_exc_before_next_catch` advanced by `op.next_pc` only. The jitcode is one flat byte stream whose blocks are stitched by `goto/L` labels, so that scan leaves the current block at its terminator and answers from whatever block the assembler placed next. A `residual_call` inside an except-handler hit this: its fall-through ends in a `goto/L` back to the block holding the handler's `last_exc_value/>r`, but the scan read past the goto into an unrelated block, found that block's `catch_exception/L`, and reported no pending read. The caller then cleared `last_exc_value`, and the real successor aborted the walk with `LastExcValueWithoutActiveException` on every attempt, so the frame never compiled. Follow the labels: `goto/L` continues at its target, `goto_if_not/iL` queues its taken arm and continues on the fall-through, and `catch_exception/L`, `raise`, `reraise` and the `*_return` family end a path. Targets are visited once, bounding the walk across back edges. An `except E as e:` handler that raises, in a `while` frame, now compiles instead of aborting: 3.3x at N=80k, 8.8x at 200k, 18.0x at 400k against the same binary with the JIT off. Assisted-by: Claude
The `CHECK_EXC_MATCH` fold pinned its match target with a `GuardValue` against the object it saw while tracing. `except (A, B):` lowers to `BUILD_TUPLE`, which allocates a fresh tuple on every visit, so that guard compares against an address the clause can no longer produce. The handler then side-exited once per visit. On a loop raising every iteration the trace collected a guard failure per visit and a bridge per few hundred, and running the clause compiled came out slower than interpreting it. The container is not what the match reads. `check_exc_match_against` walks the elements, and the elements are what a rebound handler global would change; the tuple wrapping them is rebuilt either way. Guard those instead: `GuardClass` on the layout, then a `GuardValue` per element against the class seen while tracing. `w_tuple_new` gives two object elements a `W_SpecialisedTupleObject_oo` with inline immutable `value0` / `value1`, so those reads lower to `GetfieldGcPureR` and a tuple built from constants leaves nothing behind; other arities read through `wrappeditems` with an `arraylen` guard. A match target that is neither layout keeps the identity pin, which holds for a target that is loaded rather than built. `except_tuple_clause_hot`, 60000 iterations, best of 3: dynasm 0.627s -> 0.067s guard failures 29901 -> 2, bridges 149 -> 0 cranelift 0.484s -> 0.072s pypy 0.059s Across 79 exception-shape programs: 46 improved, 33 unchanged, none regressed. Assisted-by: Claude
reads_last_exc_before_next_catch followed only goto/L and goto_if_not/iL; the other 23 label-bearing ops fell through to the physically next op, the successors of switch/id were never considered, and reraise/ was classified as ending a path without reading the exception although opimpl_reraise re-raises last_exc_value. Derive the label operand offset from the key's operand signature so every conditional branch queues its taken arm, answer true for reraise/, and answer true whenever the successor set is not decodable. Over-reporting a read leaves a recorded exception standing; under-reporting drops one a later read still needs. Assisted-by: Claude
walker_guard_exc_match_tuple_items emitted the layout GuardClass and the array-length GuardValue before reading each element, so the element read's bail-out returned false with the match target's class already pinned. The caller gates its own identity pin on is_class_known, so that arm dropped every value guard from the fold. Collect the concrete elements first and bail before recording anything. Assisted-by: Claude
goto_if_exception_mismatch/iL was treated as an ordinary conditional branch, but opimpl_goto_if_exception_mismatch reads last_exc_value and asserts it before testing the class, exactly like opimpl_last_exc_value. The scan could therefore report no later read and let the exception be cleared before it. decode_op_at also returns None for an opcode byte outside the instruction table and for a payload the stream is too short to hold, not only past the end of the code, so answering false there under-approximated the reads the rest of the scan is careful to over-approximate. Assisted-by: Claude
walker_guard_exc_match_tuple_items documents that a tuple subclass whose w_class diverges returns false so the caller pins the object identity, but only the array-backed branch enforced it; the specialised arity-2 branch read value0/value1 through whatever class the target carried. Hoist the check ahead of the layout split so it covers both. Assisted-by: Claude
capture_snapshot_for_last_guard_op_multi_frame_with_vable_vref builds a recorder::Snapshot without the extra_virtual_roots field added to the struct in the same commit that introduced it, so the crate does not compile. Pass an empty vector, as the sibling multi-frame path does. Assisted-by: Claude
|
Went through every review finding against the source and, where a shape was named, against a running reproducer. Dispositions below. Fixed
Not reproducible — kept as isOver-preserving on the untaken sibling branch (Codex inline, P2). The concern is that OR-ing both successors of a conditional keeps The suggested fix — use the concrete branch outcome instead of OR-ing — is not available at the decision point: the clear/preserve choice is made at the residual call, before the branch condition (frequently derived from that call's own result) has been evaluated. A design that defers the clear to the next The preservation is also narrower than it may read: it is gated on Deferred, with the blocker named
Also in this branch
Verification after these changescheck.py 245/245 dynasm + 245/245 cranelift; pyre-jit-trace 321, pyre-jit 241, majit-metainterp 1401 / 1399; 688 differential checks over 348 generated exception shapes on both backends, 0 wrong; the two iteration-drop / crash reproducers that reverted the gate removal still match the interpreter. — commented by Claude |
Two defects made ordinary
try/exceptshapes fall off the compiled path, plus two follow-up fixes from review.1. The last-exc lookahead was control-flow-blind
reads_last_exc_before_next_catchdecides whether a residual call may clear the walker's recorded exception. It advanced byop.next_pconly, but jitcode is one flat byte stream whose blocks are stitched bygoto/L— the physically next op is frequently not the successor.Instrumented on a
raise/except E as e:/raise eframe: the scan from the residual call at jitcode pc=2040 ran pastpc=2077 goto/L → 1017into the unrelated block at 2080, found that block'scatch_exception/L, and answered "no read". The exception was cleared; the real successor1017 → 1020islast_exc_value/>r, so every trace died withLastExcValueWithoutActiveException.permanent=false, so the frame retraced and re-aborted for the whole run —loops_compiled=0.Following labels: 3.3x @80k, 8.8x @200k, 18.0x @400k.
2.
except (A, B):pinned a rebuilt tuple by identityThe clause builds its match target with
BUILD_TUPLEon every visit, so the tuple is a fresh object each time.try_walker_fold_check_exc_matchpinned it withGuardValue(match_op, const_ref(<the traced tuple's address>))— unsatisfiable. Guard failures came in at roughly N/2 (the nursery bump sometimes hands back the same address, which is what made it look intermittent).Discriminator: hoisting
EXC = (TypeError, ValueError)out of the loop took guard failures 49900 → 1;except (TypeError,)with a single element was equally slow, so it was never about tuple length or multi-class matching.Fix pins the elements instead. Two object elements land in
W_SpecialisedTupleObject_oo, whose inlinevalue0/value1are immutable →GetfieldGcPureR→ folded away entirely; other arities go throughwrappeditemsbehind anarraylenguard. Non-tuple targets keep the identity pin, which does hold for a loaded target.dynasm 0.627 → 0.067s (9.4x), cranelift 0.484 → 0.072s, guard failures 29901 → 2, bridges 149 → 0 (pypy 0.059s). 46/79 corpus programs improved, none regressed. The inline tuple-except form appears at 510 sites across 200 files of the bundled
lib-python.Bench added:
pyre/bench/synth/except_tuple_clause_hot.py.3–4. Review follow-ups
goto/Landgoto_if_not/iL; the other 23 label-bearing ops fell through to the physically next op,switch/idsuccessors were never considered, andreraise/was classified as not reading the exception althoughopimpl_reraisere-raiseslast_exc_value. The label offset is now derived from the key's operand signature, and an un-decodable successor set answers "read" — over-reporting leaves a recorded exception standing, under-reporting drops one a later read needs.walker_guard_exc_match_tuple_itemsemitted the layoutGuardClassbefore reading each element, so an element bail-out returned false with the class already pinned; the caller gates its own identity pin onis_class_known, which dropped every value guard from the fold. Elements are now read before anything is recorded.Not included, deliberately
Removing the
for_iter_frame_has_raising_named_handlergate was tried on top of these fixes. It measured 2.5–4.9x faster with zero aborts and passed 1088 differential checks on both backends — and it is still wrong. The gate blocks every abort, not only the one fixed here, and any abort in a FOR_ITER frame drops the in-flight item. A handler body that mutates a container hitsInplaceContainerMutationUnsupportedand the loop returns 3999 items instead of 4000 on both backends; a second shape aborts the process. The differential corpus missed it because no generated handler body mutated a container. The gate stays.Verification
check.py 245/245 dynasm + 245/245 cranelift; pyre-jit-trace 321, pyre-jit 241, majit-metainterp 1401 (dynasm) / 1399 (cranelift); 688 differential checks across 348 generated exception shapes on both backends, 0 wrong.
Known, pre-existing, out of scope
A JIT-compiled same-frame raise/except never attaches
__traceback__, soe.__traceback__readsNoneinside the handler once the loop compiles (4000 → 201 on both backends;e.argsandtype(e)are unaffected). Confirmed pre-existing by rebuilding withorigin/main'sjitcode_dispatch/mod.rs— same result. Filed as follow-up work, untouched here.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Performance
Benchmarks