Exception-path JIT: lower DELETE_FAST/DELETE_DEREF + exit-frame raise FINISH#562
Exception-path JIT: lower DELETE_FAST/DELETE_DEREF + exit-frame raise FINISH#562youknowone wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 48 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)
✨ 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 |
![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: 5aaf76f9a3
ℹ️ 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".
| // disposition `Raise` so the `Terminate` consumer builds a | ||
| // `TraceAction::Finish { exit_with_exception: true }`. | ||
| FBW_FINISH_PAYLOAD.with(|c| c.set(Some((exc, Type::Ref)))); | ||
| fbw_finish_raise_set(exc_concrete); |
There was a problem hiding this comment.
When the exception's concrete shadow is unavailable (for example an unseeded ref register on a bridge/resume path, where read_ref_reg_concrete returns ConcreteValue::Null), this now records Some(FinishConcrete::Raise(Null)). The no-replay epilogue only checks that the concrete stash is Some, so it can keep this value and the later eval.rs/call_jit.rs consumers hit unreachable!("FinishConcrete::Raise must hold a concrete Ref") instead of deopting or replaying. Keep the symbolic exception FINISH payload separate from the concrete no-replay result, or only stash a concrete raise when it is a non-null Ref.
Useful? React with 👍 / 👎.
| if matches!( | ||
| crate::jitcode_dispatch::fbw_finish_concrete_peek(), | ||
| Some(crate::jitcode_dispatch::FinishConcrete::Raise(_)) | ||
| ) { |
There was a problem hiding this comment.
When an uncaught raise follows an unjournaled effect, run_perfn_walk clears FBW_FINISH_CONCRETE before this Terminate mapping but leaves FBW_FINISH_PAYLOAD intact for replay. Because this new block uses the concrete no-replay stash as the only Raise discriminator, that case falls through to the normal-return payload handling below and compiles FINISH(exc, done_with_this_frame_descr_ref) instead of exit_frame_with_exception_descr_ref, making the replayed raising trace return the exception object. The raise/return disposition needs to be stored independently of the concrete no-replay value.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 303864c). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
DELETE_FAST was lumped with the unsupported delete opcodes and emitted `abort_permanent`, so any loop or bridge reaching it fell back to the interpreter. Because the implicit `del e` at the end of every `except E as e:` handler is a DELETE_FAST, exception-bearing hot loops and their guard-failure bridges could not compile. Lower DELETE_FAST traceably by composing existing graph ops: read the local, `load_fast_check` to raise UnboundLocalError when the slot is unbound (pyopcode.py:998), then clear the slot to NULL via `setarrayitem_vable_r` (as LOAD_FAST_AND_CLEAR does) and mirror the clear in the shadow state. No new residual or HLOp; the traced lowering stays byte-identical to the interpreter. DELETE_DEREF / DELETE_GLOBAL / DELETE_NAME / SETUP_ANNOTATIONS still abort. check.py dynasm 187/187, cranelift 187/187; pyre-jit lib 306/0. swap_except_return_resume 2.79s->0.06s, finally_bare_raise 0.86s->0.05s, exception_oserror_fields 1.12s->0.51s. Assisted-by: Claude
DELETE_DEREF still emitted `abort_permanent`, so a hot loop reaching it fell back to the interpreter. This blocks `except E as e:` handlers whose exception variable is captured by a nested function: the capture makes `e` a cell, so the implicit handler cleanup emits DELETE_DEREF (not DELETE_FAST). Lower it traceably by composing existing graph ops, mirroring the LoadDeref and StoreDeref arms: `load_deref_value` dereferences the cell and raises the unbound error when the contents are empty (pyopcode.py:597), then `store_deref_value(cell, NULL)` clears the cell contents while the slot keeps holding the same cell. No new residual; byte-identical to the interpreter. DELETE_GLOBAL / DELETE_NAME / SETUP_ANNOTATIONS still abort. Add exception_as_cell_cleanup guard bench (no bench previously exercised DELETE_DEREF). check.py dynasm 187/187, cranelift 187/187; pyre-jit lib 306/0. Assisted-by: Claude
A top-level frame that exits by raising (walk()'s top-level SubRaise arm
with no local handler) recorded FINISH(exc, exit_frame_with_exception_
descr_ref) inline and set a Raise disposition, but full_body_walk_trace's
Terminate handler only consulted the normal-return payload channel
(fbw_finish_payload) — which was empty — so it aborted with "Terminate
without finish payload", discarding a valid exit-with-exception trace.
The fbw_finish_raise disposition setter was write-only: no consumer read
it, and every TraceAction::Finish hardcoded exit_with_exception: false.
Result: exception-exit bridges never compiled (bridges_compiled=0) and
every raise deopted to the interpreter.
Defer the exit-frame FINISH recording to finish_and_compile, symmetric
with the normal-return path's fbw_terminate_with_finish stash: walk()'s
top-level SubRaise arm stashes the exception OpRef as the finish payload
under a Raise disposition instead of recording the FINISH itself. Both
Terminate handlers (full_body_walk_trace and drive_outer_continuation_
and_map) now detect the Raise disposition and build TraceAction::Finish
{ exit_with_exception: true }, which finish_and_compile records with
exit_frame_with_exception_descr_ref (pyjitpl.py:3238-3245).
exceptions.py / exception_args_virtual: bridges_compiled 0->1,
loops_aborted 43->5, guard_failures 7758->200; output byte-identical to
the interpreter. Updated four dispatch unit tests to the deferred
contract (walk() stashes the payload + Raise disposition; the FINISH is
recorded downstream). check.py dynasm 188/188 + cranelift 188/188,
pyre-jit-trace lib 263/0.
Assisted-by: Claude
A `Copy` handler landing reached by an explicit raise seeded its top-of-stack exception with a `last_exc_value` read. A residual call between the raise landing and that read clears the metainterp exception transient (`execute_varargs` clears before a call that may itself raise), so the read found no value and the walk aborted with LastExcValueWithoutActiveException, leaving the loop interpreter-only. Reload the propagated exception from the frame's durable top stack slot via `getarrayitem_vable_r` for those landings — catch dispatch has already stored it there. Reraise-entered landings keep the `last_exc_value` read, which still carries the live payload supplied by the exception edge. The per-thread current-exception slot is not used as the source: at this landing it can still name the older exception whose handler raised this one, which would mislink `__context__`. Also apply the `__context__` write to the concrete exception built in `try_walker_trace_raise_builtin`, mirroring the recorded SETFIELD, since the full-body walk is also the authoritative execution of the tracing iteration. exception_bare_reraise_restore and exception_context_chain_inhandler now compile (loops_compiled 0 -> 1) with output byte-identical to the interpreter oracle. check.py dynasm + cranelift 188/188 each; pyre-jit 306, pyre-jit-trace 263, majit-metainterp 1396 lib tests pass. Assisted-by: Claude
The top-level raise arm marked its terminal disposition by stashing the exception in FBW_FINISH_CONCRETE, and the two Terminate consumers read that stash back as their only raise-vs-return discriminator. The two cells have different lifetimes: run_perfn_walk clears the concrete no-replay stash when the shortcut is declined, while the symbolic payload stays live for the compile. A declined walk therefore lost the raise disposition and built FINISH(exc, done_with_this_frame_descr_ref), so the trace returned the exception object instead of raising it. Carry the disposition in FBW_FINISH_PAYLOAD as (OpRef, Type, FinishDisposition), so every payload writer names its disposition and the two cannot drift. A Raise payload without a value becomes unrepresentable, retiring the Terminate::RaiseNoFinishPayload and P2Framestack::OuterRaiseNoFinishPayload census keys. fbw_finish_raise_set now stashes only a non-null Ref. An absent concrete shadow recorded Raise(Null), which the no-replay epilogue passed on to consumers that require a raisable object. Without a concrete value the portal degrades to the ContinueRunningNormally replay while the trace still compiles the exit-frame FINISH. The guard moves into the setter, so the two callers that hand-rolled it call it bare. In drive_outer_continuation_and_map, hoist the root green-key retarget that both return arms performed to the top of the Terminate arm; the raise arm returned Finish without it, leaving the outer frame's exit-with-exception loop attached under the bridge's trace-start key. Assisted-by: Claude
|
Rebased onto Both findings traced to one root: the disposition was conflated with the no-replay value
The fix went further than "a separate disposition channel"The first attempt added a sibling So the disposition is carried in the payload: Auditing that fix surfaced a third defect in the same commit: in
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ddf972953
ℹ️ 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".
| let result_value = emit_graph_op_with_result( | ||
| &mut graph, | ||
| ¤t_block.block(), | ||
| "store_deref_value", | ||
| vec![cell_value.into(), super::flow::Constant::none().into()], |
There was a problem hiding this comment.
When DELETE_DEREF is inside a try and the cell is empty, the preceding load_deref_value residual raises the NameError, but this added store_deref_value residual is also lowered as a Plain/can-raise residual. The flattening path anchors a block's catch_exception to the last can-raise residual, so the handler is placed after this store call instead of next to the load_deref_value guard; an unbound-cell del x in an enclosing except NameError path will therefore escape instead of being caught. Split before the store or make the store helper cannot-raise so the catch stays attached to the unbound check.
Useful? React with 👍 / 👎.
1 participant
Removes three JIT trace aborts on the exception hot path, each of which forced
the enclosing loop/bridge back to the interpreter.
1.
DELETE_FAST/DELETE_DEREFno longerabort_permanentEvery
except E as e:handler ends with an implicitdel e, compiled toDELETE_FAST(orDELETE_DEREFwhen a nested function closes overe, turningthe local into a cell). The codewriter previously emitted
abort_permanentforboth opcodes, so any loop or bridge containing an exception handler fell out of
the JIT entirely.
Both are now lowered by composing existing graph ops — no new residual:
DELETE_FAST—load_fast_check(raise-if-unbound) + aConstRef(0)slot clear (reusing
LoadFastAndClear) +clear_local_value.DELETE_DEREF—load_deref_value(raise-if-unbound) +store_deref_value(cell, NULL)(clears the cell contents, keeps the cell inthe slot) +
store_local_value. The live interpreter impl isdelete_deref.Both are byte-identical to the interpreter path by construction. Added a guard
bench
exception_as_cell_cleanup.pyfor the cell (DELETE_DEREF) case.2. Top-level raise builds the exit-frame FINISH
A top-level frame that exits by raising (no local handler) recorded its FINISH
inline with
exit_frame_with_exception_descr, and set aRaisedisposition —but that disposition flag was write-only.
trace.rs's twoTerminatehandlersonly read the normal-return payload channel, so they discarded the trace with
"Terminate without finish payload" and every raising exit deopted (bridges
compiled = 0).
The
TraceAction::Finish { exit_with_exception }field andfinish_and_compile's exit-frame-with-exception branch already existed but wereunfed. The fix defers the FINISH to
finish_and_compile(symmetric with thenormal-return path):
walk()stashes the exception inFBW_FINISH_PAYLOADplusthe
Raisedisposition, and bothTerminatehandlers detectRaiseand buildTraceAction::Finish { exit_with_exception: true }. Four dispatch unit testsupdated to the deferred contract.
Results
exceptions.py/exception_args_virtual: bridges 0 → 1, aborts 43 → 5,guard failures 7758 → 200; output byte-identical.
swap_except_return_resume2.79 → 0.06s,finally_bare_raise0.86 → 0.05s,
exception_oserror_fields1.12 → 0.51s,residual_raise_except_resume→ 0.06s.check.pydynasm + cranelift 188/188 each;majit-metainterplib 263/0.🤖 Generated with Claude Code