Repository navigation
jit(fbw): exception-object virtualization perf + polymorphic-raise resume fix - #768
Conversation
WalkthroughThe PR expands exception-slot folding and JIT field mapping, updates unbound-local error construction, adds inline empty and float-list construction, specializes bare-class raises, adjusts JIT call handling, and adds traceback behavior benchmarks. ChangesException and JIT execution updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RaiseVarargs
participant dispatch_residual_call_iRd_kind
participant try_walker_trace_raise_bare_class
participant ExecutionContext
RaiseVarargs->>dispatch_residual_call_iRd_kind: residual bare-class raise
dispatch_residual_call_iRd_kind->>try_walker_trace_raise_bare_class: fold canonical builtin exception class
try_walker_trace_raise_bare_class->>ExecutionContext: link active exception as __context__
try_walker_trace_raise_bare_class-->>dispatch_residual_call_iRd_kind: continue at next PC
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 |
![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: 17463a4bb0
ℹ️ 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 float_type_addr = &pyre_object::pyobject::FLOAT_TYPE as *const _ as i64; | ||
| let mut raws = Vec::with_capacity(final_args.len()); | ||
| for (&arg, value) in final_args.iter().zip(values) { | ||
| let raw = walker_unbox_float(ctx, op.pc, arg, float_type_addr)?; |
There was a problem hiding this comment.
When a trace specializes raise ValueError(x) with an exact float, this new Float-strategy path only calls walker_unbox_float, whose guard is on ob_type == FLOAT_TYPE; float subclasses are represented by float_descr_new as a W_FloatObject with the same ob_type but a subclass w_class. A later iteration passing a float subclass will therefore stay on this trace, build an unboxed Float-strategy args_w, and e.args[0] will be reboxed as a plain float instead of preserving the original subclass object. Add an exact w_class guard here or decline the Float path for exception args.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
| let Some(ec) = walker_ensure_execution_context(ctx) else { | ||
| return Ok(None); |
There was a problem hiding this comment.
If walker_ensure_execution_context returns None here (it explicitly can when the walk has no seeded snapshot/frame), this helper falls through to the generic RaiseVarargs residual after it has already recorded the class guard, empty args list, and NewWithVtable exception above. That mixes a partial inline construction with the residual construction/publish path and leaves heap-cache state for objects the fallback did not actually produce; recover the EC before the commit section or abort/rollback instead of returning Ok(None) after emitting ops.
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Around line 4440-4483: Resolve the execution context in
try_walker_trace_raise_bare_class before the commit section records any guard or
construction IR, returning Ok(None) there if walker_ensure_execution_context
fails. Remove the later post-commit EC resolution and reuse the pre-resolved ec
for exception context chaining, ensuring committed traces never decline into the
generic RaiseVarargs residual.
🪄 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: c73bc6aa-4af4-4b01-bf6e-5870b36093e3
📒 Files selected for processing (6)
pyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit/src/jit/codewriter.rs
| // --- commit: pin the class identity, emit the construction + raise --- | ||
| // Guard the class operand so the trace-time kind / vtable stay valid | ||
| // across iterations (`implement_guard_value`). | ||
| if !class_op.is_constant() { | ||
| let expected = ctx.trace_ctx.const_ref(concrete_class as i64); | ||
| ctx.trace_ctx | ||
| .record_guard(OpCode::GuardValue, &[class_op, expected], 0); | ||
| walker_capture_snapshot_for_last_guard(ctx, op.pc)?; | ||
| ctx.trace_ctx | ||
| .heap_cache_mut() | ||
| .replace_box(class_op, expected); | ||
| } | ||
|
|
||
| // Empty `args_w` list (zero-argument construction), stamped with the | ||
| // canonical list class exactly as `w_list_new` does. | ||
| let args_list = crate::helpers::emit_empty_list_inline(ctx.trace_ctx); | ||
| let list_w_class = pyre_object::get_instantiate(&pyre_object::pyobject::LIST_TYPE); | ||
| let list_w_class = ctx.trace_ctx.const_ref(list_w_class as i64); | ||
| let list_w_class_descr = crate::descr::list_w_class_descr(); | ||
| let list_w_class_index = list_w_class_descr.index(); | ||
| ctx.trace_ctx.record_op_with_descr( | ||
| OpCode::SetfieldGc, | ||
| &[args_list, list_w_class], | ||
| list_w_class_descr, | ||
| ); | ||
| ctx.trace_ctx | ||
| .heapcache_setfield_cached(args_list, list_w_class_index, list_w_class); | ||
|
|
||
| let new_op = | ||
| crate::helpers::emit_exception_new_inline(ctx.trace_ctx, kind, class_op, args_list); | ||
| ctx.trace_ctx | ||
| .heap_cache_mut() | ||
| .class_now_known(new_op, exc_type_ptr as usize as i64); | ||
| ctx.trace_ctx | ||
| .set_opref_concrete(new_op, majit_ir::Value::Ref(majit_ir::GcRef(exc as usize))); | ||
|
|
||
| // `__context__` chaining on the still-virtual exception, mirroring the | ||
| // `try_walker_trace_raise_builtin` tail: `active = GETFIELD_GC_R(ec, | ||
| // sys_exc_value)` then `SETFIELD_GC(exc, active, w_context)`. The EC is | ||
| // routed through `walker_ensure_execution_context` so the read shares the | ||
| // one seeded EC OpRef the PUSH_EXC_INFO / POP_EXCEPT lowering consumes. | ||
| let Some(ec) = walker_ensure_execution_context(ctx) else { | ||
| return Ok(None); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Post-commit decline emits orphaned IR and double-raises.
try_walker_trace_raise_bare_class reaches its --- commit --- section (Line 4440) and records the class GuardValue, the inline empty list, emit_exception_new_inline, and the concrete/heap-cache shadows before Line 4481 resolves the EC. When walker_ensure_execution_context returns None (null sym / no frame), the function return Ok(None) — but the caller in residual_call.rs (Lines 2751-2758) treats None as "not folded" and falls through to the generic RaiseVarargs residual. The already-recorded construction IR is left orphaned and a second, redundant exception is constructed/raised by the residual.
try_walker_trace_exception_new guarantees "no further declines" after its commit marker (Line 4022); this path must do the same. Resolve the EC as a pre-commit decline.
🐛 Resolve the EC before committing
+ // Resolve the EC BEFORE committing: `walker_ensure_execution_context`
+ // can decline (null sym / no frame). A decline after emitting
+ // construction IR would leave orphaned IR and fall through to the
+ // generic residual, double-raising. Treat it as a pre-commit decline.
+ let Some(ec) = walker_ensure_execution_context(ctx) else {
+ return Ok(None);
+ };
+
// --- commit: pin the class identity, emit the construction + raise ---
// Guard the class operand so the trace-time kind / vtable stay valid
// across iterations (`implement_guard_value`).
if !class_op.is_constant() {Then drop the later let Some(ec) = walker_ensure_execution_context(ctx) else { return Ok(None); }; block (Lines 4481-4483) and keep using ec.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // --- commit: pin the class identity, emit the construction + raise --- | |
| // Guard the class operand so the trace-time kind / vtable stay valid | |
| // across iterations (`implement_guard_value`). | |
| if !class_op.is_constant() { | |
| let expected = ctx.trace_ctx.const_ref(concrete_class as i64); | |
| ctx.trace_ctx | |
| .record_guard(OpCode::GuardValue, &[class_op, expected], 0); | |
| walker_capture_snapshot_for_last_guard(ctx, op.pc)?; | |
| ctx.trace_ctx | |
| .heap_cache_mut() | |
| .replace_box(class_op, expected); | |
| } | |
| // Empty `args_w` list (zero-argument construction), stamped with the | |
| // canonical list class exactly as `w_list_new` does. | |
| let args_list = crate::helpers::emit_empty_list_inline(ctx.trace_ctx); | |
| let list_w_class = pyre_object::get_instantiate(&pyre_object::pyobject::LIST_TYPE); | |
| let list_w_class = ctx.trace_ctx.const_ref(list_w_class as i64); | |
| let list_w_class_descr = crate::descr::list_w_class_descr(); | |
| let list_w_class_index = list_w_class_descr.index(); | |
| ctx.trace_ctx.record_op_with_descr( | |
| OpCode::SetfieldGc, | |
| &[args_list, list_w_class], | |
| list_w_class_descr, | |
| ); | |
| ctx.trace_ctx | |
| .heapcache_setfield_cached(args_list, list_w_class_index, list_w_class); | |
| let new_op = | |
| crate::helpers::emit_exception_new_inline(ctx.trace_ctx, kind, class_op, args_list); | |
| ctx.trace_ctx | |
| .heap_cache_mut() | |
| .class_now_known(new_op, exc_type_ptr as usize as i64); | |
| ctx.trace_ctx | |
| .set_opref_concrete(new_op, majit_ir::Value::Ref(majit_ir::GcRef(exc as usize))); | |
| // `__context__` chaining on the still-virtual exception, mirroring the | |
| // `try_walker_trace_raise_builtin` tail: `active = GETFIELD_GC_R(ec, | |
| // sys_exc_value)` then `SETFIELD_GC(exc, active, w_context)`. The EC is | |
| // routed through `walker_ensure_execution_context` so the read shares the | |
| // one seeded EC OpRef the PUSH_EXC_INFO / POP_EXCEPT lowering consumes. | |
| let Some(ec) = walker_ensure_execution_context(ctx) else { | |
| return Ok(None); | |
| }; | |
| // Resolve the EC BEFORE committing: `walker_ensure_execution_context` | |
| // can decline (null sym / no frame). A decline after emitting | |
| // construction IR would leave orphaned IR and fall through to the | |
| // generic residual, double-raising. Treat it as a pre-commit decline. | |
| let Some(ec) = walker_ensure_execution_context(ctx) else { | |
| return Ok(None); | |
| }; | |
| // --- commit: pin the class identity, emit the construction + raise --- | |
| // Guard the class operand so the trace-time kind / vtable stay valid | |
| // across iterations (`implement_guard_value`). | |
| if !class_op.is_constant() { | |
| let expected = ctx.trace_ctx.const_ref(concrete_class as i64); | |
| ctx.trace_ctx | |
| .record_guard(OpCode::GuardValue, &[class_op, expected], 0); | |
| walker_capture_snapshot_for_last_guard(ctx, op.pc)?; | |
| ctx.trace_ctx | |
| .heap_cache_mut() | |
| .replace_box(class_op, expected); | |
| } | |
| // Empty `args_w` list (zero-argument construction), stamped with the | |
| // canonical list class exactly as `w_list_new` does. | |
| let args_list = crate::helpers::emit_empty_list_inline(ctx.trace_ctx); | |
| let list_w_class = pyre_object::get_instantiate(&pyre_object::pyobject::LIST_TYPE); | |
| let list_w_class = ctx.trace_ctx.const_ref(list_w_class as i64); | |
| let list_w_class_descr = crate::descr::list_w_class_descr(); | |
| let list_w_class_index = list_w_class_descr.index(); | |
| ctx.trace_ctx.record_op_with_descr( | |
| OpCode::SetfieldGc, | |
| &[args_list, list_w_class], | |
| list_w_class_descr, | |
| ); | |
| ctx.trace_ctx | |
| .heapcache_setfield_cached(args_list, list_w_class_index, list_w_class); | |
| let new_op = | |
| crate::helpers::emit_exception_new_inline(ctx.trace_ctx, kind, class_op, args_list); | |
| ctx.trace_ctx | |
| .heap_cache_mut() | |
| .class_now_known(new_op, exc_type_ptr as usize as i64); | |
| ctx.trace_ctx | |
| .set_opref_concrete(new_op, majit_ir::Value::Ref(majit_ir::GcRef(exc as usize))); | |
| // `__context__` chaining on the still-virtual exception, mirroring the | |
| // `try_walker_trace_raise_builtin` tail: `active = GETFIELD_GC_R(ec, | |
| // sys_exc_value)` then `SETFIELD_GC(exc, active, w_context)`. The EC is | |
| // routed through `walker_ensure_execution_context` so the read shares the | |
| // one seeded EC OpRef the PUSH_EXC_INFO / POP_EXCEPT lowering consumes. |
🤖 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 4440 -
4483, Resolve the execution context in try_walker_trace_raise_bare_class before
the commit section records any guard or construction IR, returning Ok(None)
there if walker_ensure_execution_context fails. Remove the later post-commit EC
resolution and reuse the pre-resolved ec for exception context chaining,
ensuring committed traces never decline into the generic RaiseVarargs residual.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 0f964d7). 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
|
CI triage + review responseCI: all three red jobs accounted for
Review findings — both confirmed and fixed1. Float exception args were not guarded by exact class (Codex P2) — confirmed, and it applied to the Integer arm too.
Discriminator (warm the loop with an exact float/int, then re-enter the same compiled loop with a subclass, and read
Fixed by pinning the exact 2. Decline after emitting bare-raise IR (Codex P2 / CodeRabbit Major) — confirmed. Codex parity review (sections 3 and 4)
Two pre-existing defects found while probing — not part of this PR
Gates after the fixes
— commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/9f5397cb9f201e33b0fd441db488936017dd2fdc/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L1544
When a function has zero positional parameters but is not genuinely zero-argument—for example, def f(*, required): return 1—resolve_inlinable_callee still reports nparams == code.arg_count == 0 because it does not reject keyword-only, *args, or **kwargs signatures. This new admission bypasses fill_user_function_args, so a hot f() can compile to return 1 instead of raising the required-argument TypeError; variadic locals are likewise never populated in the synthesized frame. Restrict this path to flat-call signatures or reproduce the normal argument binder.
AGENTS.md reference: AGENTS.md:L194-L196
ℹ️ 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".
try_walker_trace_exception_new declined the Empty args-list strategy (zero-argument raise, e.g. `raise ValueError()` / `raise StopIteration()`) to the residual constructor call, which allocated the exception on the heap and took two may-force barriers every iteration. Emit the Empty-strategy args_w list inline via emit_empty_list_inline (NewWithVtable + strategy store), mirroring w_list_new_with_strategy(vec![], Empty), so OptVirtualize folds the whole exception when it never escapes the frame. Zero-arg raise/except in a hot loop drops from ~4900 ns/iter to ~1 ns/iter on both backends, matching the Integer/Object strategies. The Float strategy keeps the residual fallback. Assisted-by: Claude
Extend try_walker_trace_exception_new to emit the Float args-list strategy inline (per-element walker_unbox_float into a float_gcarray backing block + emit_typed_list_inline), mirroring the Integer strategy and the BUILD_LIST Float decomposition. `raise ValueError(1.5)` drops from ~4800 ns/iter to ~1 ns/iter on both backends. all_floats requires strict W_FloatObject identity, so a float subclass stays in the Object strategy and keeps its instance identity across the raise — verified against pypy3. Assisted-by: Claude
A bare `raise Type` (class operand, no call parentheses) has no preceding CallFn construct residual, so the exception object was allocated per iteration by the normalize_raise_varargs residual and materialized via a may-force call. Add try_walker_trace_raise_bare_class, dispatched at the RaiseVarargs residual after try_walker_trace_raise_builtin declines. For a canonical builtin exception class with a trivial-args constructor and no explicit from-cause, it synthesizes the zero-argument instance inline (empty args_w list + emit_exception_new_inline) and chains __context__, mirroring the try_walker_trace_raise_builtin tail. The instance is marked FBW-built so the following raise/r records its traceback node through the virtual record_fresh_application_traceback (inline PyTraceback NewWithVtable + SETFIELDs on the exception) rather than record_top_level_application_traceback, whose runtime hook passes the exception to a CallN and forces it to materialize. With the tb node kept virtual, the balanced sys_exc_value save/restore is dead-store-eliminated and a locally-caught bare raise DCEs to nothing. Subclasses and non-trivial-args kinds (OSError / Unicode) and explicit `raise X from Y` decline to the residual. Assisted-by: Claude
Fold reads of eight more W_BaseException attributes into a GETFIELD_GC
slot read via exception_attr_slot_fold:
- __traceback__ on every exception kind
- name on ImportError / ModuleNotFoundError / NameError / AttributeError
- obj on AttributeError
- object / start / end / reason on Unicode{Translate,Decode,Encode}Error
- encoding on Unicode{Decode,Encode}Error
ExceptionAttrSlot gains the eight variants; w_exception_slot_descr maps
each to its build_w_exception_group field index. The fold covers loads
only; stores stay on the residual setattr path (__context__/__cause__/
__traceback__ setters have side effects, the rest are plain writes not
folded here).
Keeps a __traceback__-reading handler on an otherwise-virtualized
exception from escaping: raise ValueError(7) + e.__traceback__ drops
from ~4900ns to ~25ns.
check.py: dynasm 303/303, cranelift 303/303. attr_oracle byte-identical
to pypy3 across all eight attributes.
Assisted-by: Claude
filter_liveness_in_place narrows codewriter -live- records to the Ref colors that map to a Python local/stack slot, dropping operand-stack temporaries. A raise/r <reg> reads its exception operand from such a temporary — the residual normalize_raise_varargs result the tracer stored via make_result_of_lastop — so the filter dropped it from the -live- preceding the raise. On a type-change deopt of a raise <call-result> loop, the guard (GUARD_CLASS on the raised type, or the after-residual GUARD_NO_EXCEPTION / GUARD_NOT_FORCED) resumes at that marker and the blackhole seeds registers_r without the exception register, tripping bhimpl_raise's non-null assert. Re-add the raise op's Ref operand register to the retained set, extending the existing precedent that re-adds LOAD_ATTR's tracer-populated residual operand. Mirrors get_list_of_active_boxes keeping every register read after the marker; closes the deferred num_live-aware capture_resumedata(after_residual_call=True) gap for the raise operand. Fixes a pre-existing crash on polymorphic raise <expr> loops on both backends. check.py: dynasm 303/303, cranelift 303/303. A 17-shape polymorphic-raise corpus is crash-free and byte-identical to pypy3. Assisted-by: Claude
w_exception_get_traceback documented its omission of descr_gettraceback's tb.frame.mark_as_escaped() as "pyre does not have yet", and pointed at a TODO on baseobjspace's __traceback__ getattr arm. Both are stale: PyFrame::mark_as_escaped exists and is already called from ExecutionContext::leave, sys._getframe, the frame f_back getter and the traceback tb_frame getter, and that TODO is gone. State the real reason instead — the flag's only consumer is ExecutionContext::leave, whose frame_vref() force stays a no-op until jit.virtual_ref is ported, and a frame a traceback can reach has already left with got_exception set, so leave marked its f_back either way. Note the same coupling on the __traceback__ arm of exception_attr_slot_fold, which folds the read to a raw slot load in traced code and so has to gain the side effect at the same time. Comment-only. Assisted-by: Claude
The typed args_w folds decided their strategy from list_strategy_for(concrete_args), whose Integer/Float arms are strict on w_class (is_plain_int1 / all_floats reject subclasses), but the emitted guard was walker_unbox_int / walker_unbox_float's GuardClass on the payload ob_type — which an int/float subclass instance shares, retagging only w_class. A later iteration passing a subclass therefore stayed on the trace and was unboxed into the typed storage, so e.args[0] came back a plain int/float. Guard the exact w_class at both emit sites so such an argument side-exits to the residual. A constant operand is the same object every iteration, so its trace-time w_class already holds and it keeps the bare unbox. Discriminator (scratchpad/exc_virt/subclass_args_oracle.py): warm the loop with an exact float/int, then re-enter it with a subclass instance and read type(e.args[0]).__name__. Was float/int, now MF/MI on both backends, matching pypy3 and pyre's own PYRE_NO_JIT run. Also resolve the execution context in try_walker_trace_raise_bare_class before its commit marker. walker_ensure_execution_context declines on a null snapshot sym or a frameless walk; declining there left the class guard, the inline args list and the NewWithVtable already recorded while the caller fell through to the generic RaiseVarargs residual, orphaning those ops and leaving heap-cache shadows for an object the residual never built. It also kept the recovery getfield from landing after a guard that references it. Assisted-by: Claude
The inline-at-residual lever declined every zero-parameter callee before computing strict_inlinable, on the stated grounds that "a residual zero-arg call is cheap and has no positional-arg inline win to recover". Measured, it is not cheap: `def f0(): return 1` called from a while loop runs 569 ns/iter against 1.16 ns for the same call with one parameter, and 0.39 ns on pypy. The fast-path register convention is `registers_r[0..nparams] = positional args`, which a zero-param callee satisfies vacuously, so the strict path serves it like any other straight-line leaf. Move the decline below callee_fast_path_inlinable and keep it for a zero-param callee the strict path cannot serve, so such a body still takes the residual instead of the decline-to-interpretation that follows. while-loop `f0()` goes 569 -> 0.81 ns/iter. Under FOR_ITER the callee must also pass fbw_callee_body_side_effect_free: a zero-param callee returning None or a constant string now inlines there too (0.79 ns), while one returning a constant int or a global still declines on the box_int / LoadGlobal residual in its body. check.py: dynasm 307/307, cranelift 307/307. Assisted-by: Claude
The paragraph explaining why a zero-parameter callee is served by the strict path sat with the register-convention block, thirty lines above the `nparams == 0 && !strict_inlinable` decline it describes. Assisted-by: Claude
`UnboundLocalError` subclasses `NameError` and so inherits its `name` descriptor (`interp_exceptions.py:815`, `:1115`), but the three `w_exc_name` arms — getattr, setattr and the traced attr-slot fold — listed only `ExcKind::NameError`. A Python-level `UnboundLocalError(...)` is built by `exc_name_error_new` and carries `ExcKind::NameError`, so the omission was invisible there; an interpreter-raised one carries `ExcKind::UnboundLocalError` and fell through to generic attribute lookup, where reading `name` raised `AttributeError` and writing it landed in the instance dict instead of the slot. `unbound_local_error_with_name` also stamped the undefined local into that slot. `format_exc_check_arg` fills `name` only when the raised class is exactly `NameError`, so both references report `None` there; drop the stamp and the now-unused parameter. Interpreter-raised `UnboundLocalError`: `e.name` was `AttributeError` and `e.name = v` left `__dict__ == ['name']`; both now match CPython 3.14 and pypy3 (`None` and an empty `__dict__`). Assisted-by: Claude
`bh_compare_fn` answered compare tags 8 / 9 with `std::ptr::eq`, while the
interpreter's `is_op` calls `baseobjspace::is_w`. `W_AbstractIntObject.is_w`
(`intobject.py:44`) and `W_FloatObject.is_w` (`floatobject.py:196`) compare two
plain `int`s / `float`s by value, so only the JIT reported a freshly boxed
equal value as a different object.
def isprobe(x, n):
ok = True
for _ in range(n):
y = x + 0.0
ok = ok and (y is x)
return ok
isprobe(1.5, 30000)
was `False` under the JIT against `True` on pypy3 and under `PYRE_NO_JIT=1`;
the int twin (`y = x + 0`) matched. Both now agree.
`is_w` is infallible, so the residual still never publishes
BH_LAST_EXC_VALUE. It also carries the bool / subclass / complex / tuple /
bytes arms, which the pointer compare answered correctly only by accident.
Assisted-by: Claude
The exception-traceback fixtures all drive their workload with `for i in range(N)`. The two loop forms compile through different paths and have failed independently -- a callee raising into a hot `while` loop lost every traceback node while the `for` twin lost exactly one -- so a `for`-only fixture reports the milder symptom or none at all. Both forms across callee raise, same-frame raise, residual builtin raise and bare re-raise, plus the innermost node's line number. Each shape reports the set of chain depths rather than one sample, so an iteration that disagrees with the others surfaces as a second element. Oracle pypy3 and CPython, both backends. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f964d7776
ℹ️ 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".
| for &arg in final_concrete_args { | ||
| values.push(unsafe { pyre_object::w_float_get_value(arg) }); | ||
| } | ||
| ArgsEmit::Float(values) |
There was a problem hiding this comment.
When a hot path constructs ImportError(1.5), admitting the Float strategy virtualizes it using only kind, w_class, and args_w, but the interpreter's ImportError.__init__ also stores the positional argument in w_import_msg. If the exception is caught and e.msg is read, that residual attribute access materializes the virtual exception with a null message slot and returns None instead of 1.5. Either exclude constructors with additional slot writes from this new Float path or emit those slots as well.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/inline_call.rs`:
- Around line 1790-1800: The zero-parameter guard in the inline-call dispatch
admits self-recursive callees without using the existing assembler recursion
handling. Detect zero-argument self-recursion before the `strict_inlinable`
admission in the surrounding dispatch logic, route it through
`try_walker_call_assembler_self_recursive`, and add a regression case verifying
the generated JIT preserves the interpreter’s recursive execution behavior.
In `@pyre/pyre-object/src/interp_exceptions.rs`:
- Around line 813-821: The folded __traceback__ getter must preserve the
tb.frame escape side effect when read inside an active handler. Update the
traceback access path around exception_attr_slot_fold and its equivalent getter
to mark the traceback frame as escaped and perform the required
virtual-reference/materialization mechanics, matching descr_gettraceback
behavior while retaining the raw slot-load optimization.
🪄 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: ca340121-106c-4154-ac00-17eb922b40cd
📒 Files selected for processing (13)
pyre/bench/synth/exception_traceback_loop_forms.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/error.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-object/src/interp_exceptions.rs
| // A zero-param callee has no positional argument to seed, so the register | ||
| // convention above holds vacuously and the strict path serves it like any | ||
| // other straight-line leaf. The residual it would otherwise fall back to | ||
| // is not cheap: `def f0(): return 1` called from a `while` loop measured | ||
| // 569 ns/iter against 1.16 ns for the same call with one parameter. Only a | ||
| // zero-param callee the strict path cannot serve declines here, so such a | ||
| // body still takes the residual rather than the decline-to-interpretation | ||
| // below. | ||
| if nparams == 0 && !strict_inlinable { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Keep zero-argument self-recursion on the existing assembler path.
Line 1798 admits branch-free recursive callees because strict_inlinable does not reject recursive calls. However, the self-recursion handling below requires !strict_inlinable && nparams >= 1, so a zero-argument function such as def f(): return f() bypasses try_walker_call_assembler_self_recursive and is repeatedly unrolled until the generic recursion cap. Detect self-recursion before admitting this strict path and add a regression case.
As per coding guidelines, the generated JIT must preserve interpreter semantics and structural execution behavior.
🤖 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 1790 -
1800, The zero-parameter guard in the inline-call dispatch admits self-recursive
callees without using the existing assembler recursion handling. Detect
zero-argument self-recursion before the `strict_inlinable` admission in the
surrounding dispatch logic, route it through
`try_walker_call_assembler_self_recursive`, and add a regression case verifying
the generated JIT preserves the interpreter’s recursive execution behavior.
Source: Coding guidelines
| /// `interp_exceptions.py:196-201 descr_gettraceback` parity, minus the | ||
| /// `tb.frame.mark_as_escaped()` side effect. `PyFrame::mark_as_escaped` | ||
| /// exists, but the flag's only consumer is `ExecutionContext::leave`, whose | ||
| /// `frame_vref()` force stays a no-op until `jit.virtual_ref` is ported — | ||
| /// and a frame a traceback can reach has already left, with `got_exception` | ||
| /// set, so `leave` marked its `f_back` either way. The mark belongs with | ||
| /// that force, together with the `__traceback__` arm of | ||
| /// `baseobjspace::exception_attr_slot_fold`, which folds this read to a raw | ||
| /// slot load in traced code. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the traceback-frame escape side effect.
ExecutionContext::leave cannot compensate when __traceback__ is read inside an active handler: while_same_frame does so before the raising frame leaves. A retained e.__traceback__.tb_frame therefore misses the upstream descr_gettraceback escape mark and can lose required JIT frame materialization. Implement the equivalent folded-getter effect (and required vref mechanics) rather than documenting the divergence.
🤖 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-object/src/interp_exceptions.rs` around lines 813 - 821, The folded
__traceback__ getter must preserve the tb.frame escape side effect when read
inside an active handler. Update the traceback access path around
exception_attr_slot_fold and its equivalent getter to mark the traceback frame
as escaped and perform the required virtual-reference/materialization mechanics,
matching descr_gettraceback behavior while retaining the raw slot-load
optimization.
Source: Coding guidelines
Extends the FBW walker's exception virtualization and closes a pre-existing
blackhole resume-liveness crash on polymorphic
raise <expr>loops.Perf: virtualize more exception raises + fold more attributes
raise ValueError()): the args-listListStrategy::Emptybuild is emitted inline (NewWithVtable+SetfieldGc(strategy=Empty)), mirroringw_list_new_with_strategy(vec![], Empty), so the constructor no longer falls to a residual heap alloc.~4900ns → ~1ns.
raise ValueError(1.5)): the Integer arm'smirror via
walker_unbox_float+emit_typed_list_inline(Float). Floatsubclasses stay Object-strategy (identity preserved). ~4750ns → ~1ns.
raise ValueError):try_walker_trace_raise_bare_classsynthesizes the zero-arg instance inline and marks it FBW-built so the raise
records its traceback through the virtual
record_fresh_application_tracebackpath (staying virtual and DCE-able) instead of the escaping runtime hook.
~2644ns → ~1ns.
W_BaseExceptionattributes (
__traceback__,name,obj, and the Unicodeobject/start/end/reason/encoding), load-fold only. Keeps a__traceback__-readinghandler on an otherwise-virtualized exception from escaping.
Correctness: keep the raise operand in the filtered resume liveness
Fixes a pre-existing crash: a polymorphic
raise <expr>loop (where theexception is a per-iteration residual-call result) panics on the type-change
deopt with
bhimpl_raise: excvalue must be non-null.filter_liveness_in_placenarrows the codewriter-live-records to the Refcolors that map to a Python local/stack slot, dropping operand-stack
temporaries. A
raise/r <reg>reads its exception operand from such atemporary — the residual
normalize_raise_varargsresult the tracer stored viamake_result_of_lastop— so the filter dropped it from the-live-precedingthe raise. On the deopt the guard (
GUARD_CLASS, or the after-residualGUARD_NO_EXCEPTION/GUARD_NOT_FORCED) resumed at that marker and theblackhole seeded
registers_rwithout the exception → assert.The fix re-adds the
raiseop's Ref operand register to the retained set,extending the existing precedent that re-adds LOAD_ATTR's tracer-populated
residual operand. It mirrors
get_list_of_active_boxeskeeping every registerread after the marker, closing the deferred
num_live-awarecapture_resumedata(after_residual_call=True)gap for the raise operand.Verification
check.py: dynasm 303/303, cranelift 303/303.raisecorpus (both backends) is crash-free andbyte-identical to pypy3.
🤖 Generated with Claude Code
Summary by CodeRabbit
raise Typepaths to reduce unnecessary residual exception work.