Repository navigation
jit: fold the immutable-type STORE_ATTR/DELETE_ATTR raise to a traced inline exception - #815
Conversation
WalkthroughChangesImmutable attribute raise folding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ResidualCallDispatcher
participant StabilityPredicate
participant AttributeOperation
participant ExceptionBuilder
ResidualCallDispatcher->>StabilityPredicate: Check immutable attribute raise stability
StabilityPredicate-->>ResidualCallDispatcher: Return stable or decline
ResidualCallDispatcher->>AttributeOperation: Execute setattr_str or delattr_str
AttributeOperation-->>ResidualCallDispatcher: Return TypeError
ResidualCallDispatcher->>ExceptionBuilder: Emit TypeError and SubRaise
Possibly related PRs
Suggested reviewers: 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 |
8751e1d to
e964688
Compare
… inline exception synth/type_immutable_reject runs ~18x slower than the explicit-raise equivalent (0.88s vs 0.05s loop time for the same 200k raise/catch iterations; cpython 0.17s, pypy 0.02s). Profiling shows the whole gap is the raise being born inside the opaque store_attr/delete_attr residual: every compiled iteration re-materialises the TypeError, its message string, and its args list through PyError::to_exc_object's GC hooks, and the GuardException result box can never virtualize. The explicit `raise TypeError(...)` shape is already fast because the B3 construct fold exposes the allocation to the optimizer, which removes it when the exception never escapes. Extend that treatment to the one attribute-store raise whose outcome is provably iteration-invariant: `int.x = v` / `del str.x` on a non-heap (immutable) builtin type. A new stability predicate (`type_immutable_attr_raise_is_stable`) accepts only a constant non-heaptype receiver whose metaclass is the canonical `type` with no metaclass descriptor for the name — every dict the raise-path decision consults is frozen, so the raise and its message depend on trace-time constants alone. Under the predicate the walker pins the receiver with GuardValue (when not already constant), runs the authentic setattr_str/delattr_str concretely for the authoritative walk, and emits the B3 inline construction (NewWithVtable + args-list SetfieldGcs, message as a rooted trace constant) in place of the CallMayForceN + GuardNotForced + GuardException triple, then routes the raise through the ordinary SubRaise path with the Err-arm state. DELETE_ATTR gains the PyreHelperKind::DeleteAttr calldescr tag (enum tail, existing discriminants preserved) so the walker can recognise it, mirroring StoreAttr. Co-Authored-By: Claude Fable 5 <[email protected]>
e964688 to
6759e5f
Compare
![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
https://github.com/youknowone/pyre/blob/e96468801883f5c032947c2c7b1bb3cb4d2700a4/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L5523-L5524
When Python retains e.args[0] across loop iterations and compares it with is, the interpreter produces a distinct message object for every immutable-type TypeError because PyError::to_exc_object calls w_str_new on each raise. This fold instead bakes the single trace-time msg into a ConstPtr, so every compiled iteration shares it and changes the result of that identity comparison; emit a fresh message allocation in the trace or decline the fold when it cannot be preserved.
AGENTS.md reference: AGENTS.md:L14-L19
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/6759e5f01ac2930795118d7c15153b0145656500/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L5415
This walker-native fold manually recognizes two residual helpers and reconstructs one selected outcome, rather than fixing source translation so the generated JIT traces the existing setattr_str/delattr_str semantics. That creates a second implementation which must be kept synchronized with exception construction, context chaining, traceback handling, and future attribute semantics; the repository explicitly requires JIT/interpreter mismatches to be fixed as generation defects rather than patched with hand-written deviations. Remove this specialization and make the translation pipeline expose and optimize the interpreter raise path itself.
AGENTS.md reference: AGENTS.md:L14-L20
https://github.com/youknowone/pyre/blob/6759e5f01ac2930795118d7c15153b0145656500/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L5521-L5522
When the caught exceptions or their arguments escape the loop, this ConstPtr makes every compiled TypeError reuse the same message object. The interpreter path calls to_exc_object(), which invokes w_str_new for each exception, so collecting e.args[0] values and comparing adjacent entries with is changes from false without the JIT to true after compilation. A literal in co_consts is not equivalent to this dynamically materialized internal error message; emit its allocation in the trace along with the exception and argument list so the generated JIT preserves interpreter-visible identity.
AGENTS.md reference: AGENTS.md:L16-L18
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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 5458-5511: Reorder the immutable-type attribute raise folding in
the surrounding specialization logic so the concrete setattr_str/delattr_str
execution and all exception, kind, and canonical-type validations complete
before emitting GuardValue or updating heap_cache_mut(). Keep the existing
decline returns unchanged, ensuring any Ok(None) path leaves the trace and heap
cache untouched; only commit the guard and cache replacement after validation
succeeds.
🪄 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: a828238e-cd3c-4ea3-abe1-d7dc92afd7af
📒 Files selected for processing (5)
majit/majit-ir/src/effectinfo.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit/src/jit/flatten.rs
| // --- commit: pin the receiver, run the authentic raise, emit inline --- | ||
| // The stability predicate makes the raise a pure function of `(obj, | ||
| // name)`; `GuardValue` pins the one live input (`name` is a co_names | ||
| // constant). | ||
| if !obj_op.is_constant() { | ||
| let expected = ctx.trace_ctx.const_ref(concrete_obj as i64); | ||
| ctx.trace_ctx | ||
| .record_guard(OpCode::GuardValue, &[obj_op, expected], 0); | ||
| walker_capture_snapshot_for_last_guard(ctx, op.pc)?; | ||
| ctx.trace_ctx.heap_cache_mut().replace_box(obj_op, expected); | ||
| } | ||
|
|
||
| // The authoritative walk's concrete execution — the same call the | ||
| // residual executor would have made, raising before any heap | ||
| // mutation. Plain eval: the predicate excludes every user-code path. | ||
| let result = { | ||
| let _plain_guard = pyre_interpreter::call::force_plain_eval(); | ||
| match concrete_value { | ||
| Some(value) => pyre_interpreter::baseobjspace::setattr_str(concrete_obj, &name, value), | ||
| None => pyre_interpreter::baseobjspace::delattr_str(concrete_obj, &name), | ||
| } | ||
| }; | ||
| let Err(mut err) = result else { | ||
| // Unreachable under the predicate (a non-heaptype dict rejects | ||
| // every mutation). Fail loud rather than falling through: the | ||
| // generic residual would re-run the (somehow) committed effect. | ||
| return Err(DispatchError::UnsupportedOpname { | ||
| pc: op.pc, | ||
| key: "immutable-type attr raise fold: stable raise unexpectedly succeeded", | ||
| }); | ||
| }; | ||
| let exc = err.to_exc_object(); | ||
| let kind = unsafe { | ||
| if !pyre_object::is_exception(exc) { | ||
| return Ok(None); | ||
| } | ||
| pyre_object::interp_exceptions::w_exception_get_kind(exc) | ||
| }; | ||
| // The folded raise is exactly the immutable-type TypeError; any other | ||
| // kind means the runtime path diverged from the predicate's model. | ||
| if kind != pyre_object::interp_exceptions::ExcKind::TypeError { | ||
| return Ok(None); | ||
| } | ||
| let exc_type_ptr = unsafe { | ||
| (*(exc as *const pyre_object::interp_exceptions::W_BaseException)) | ||
| .ob_header | ||
| .ob_type | ||
| }; | ||
| if !std::ptr::eq( | ||
| exc_type_ptr, | ||
| pyre_object::interp_exceptions::exc_kind_to_pytype(kind), | ||
| ) { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard/heap-cache commit happens before the exception is validated — decline paths leave a stray trace mutation.
GuardValue + heap_cache_mut().replace_box(obj_op, ...) are emitted at L5462-5468, but the three checks that can still return Ok(None) (!is_exception(exc) L5492, kind != TypeError L5499, exc_type_ptr mismatch L5506-5510) all run after that commit. Every other fold in this file (try_walker_trace_exception_new's --- commit to the specialization: emit IR (no further declines) --- at L4861, and try_walker_trace_raise_bare_class's --- commit: pin the class identity --- at L5310) does the opposite: it runs the concrete construction and validates kind/type/canonical-class identity before emitting any guard, precisely so a decline leaves the trace untouched. Here, if the (claimed-impossible) divergence ever fires, the walk falls through to the generic residual with a spurious GuardValue/heap-cache entry already baked into the trace.
Move the concrete setattr_str/delattr_str call and all three validation checks above the guard-emission block, matching the sibling ordering.
🐛 Proposed reorder
- // --- commit: pin the receiver, run the authentic raise, emit inline ---
- // The stability predicate makes the raise a pure function of `(obj,
- // name)`; `GuardValue` pins the one live input (`name` is a co_names
- // constant).
- if !obj_op.is_constant() {
- let expected = ctx.trace_ctx.const_ref(concrete_obj as i64);
- ctx.trace_ctx
- .record_guard(OpCode::GuardValue, &[obj_op, expected], 0);
- walker_capture_snapshot_for_last_guard(ctx, op.pc)?;
- ctx.trace_ctx.heap_cache_mut().replace_box(obj_op, expected);
- }
-
- // The authoritative walk's concrete execution — the same call the
+ // The authoritative walk's concrete execution — the same call the
// residual executor would have made, raising before any heap
// mutation. Plain eval: the predicate excludes every user-code path.
let result = {
let _plain_guard = pyre_interpreter::call::force_plain_eval();
match concrete_value {
Some(value) => pyre_interpreter::baseobjspace::setattr_str(concrete_obj, &name, value),
None => pyre_interpreter::baseobjspace::delattr_str(concrete_obj, &name),
}
};
let Err(mut err) = result else {
// Unreachable under the predicate (a non-heaptype dict rejects
// every mutation). Fail loud rather than falling through: the
// generic residual would re-run the (somehow) committed effect.
return Err(DispatchError::UnsupportedOpname {
pc: op.pc,
key: "immutable-type attr raise fold: stable raise unexpectedly succeeded",
});
};
let exc = err.to_exc_object();
let kind = unsafe {
if !pyre_object::is_exception(exc) {
return Ok(None);
}
pyre_object::interp_exceptions::w_exception_get_kind(exc)
};
// The folded raise is exactly the immutable-type TypeError; any other
// kind means the runtime path diverged from the predicate's model.
if kind != pyre_object::interp_exceptions::ExcKind::TypeError {
return Ok(None);
}
let exc_type_ptr = unsafe {
(*(exc as *const pyre_object::interp_exceptions::W_BaseException))
.ob_header
.ob_type
};
if !std::ptr::eq(
exc_type_ptr,
pyre_object::interp_exceptions::exc_kind_to_pytype(kind),
) {
return Ok(None);
}
+
+ // --- commit: pin the receiver, emit inline exception construction ---
+ // Every decline above is now ruled out; only past this point may trace
+ // state (guards, heap-cache) be mutated (mirrors
+ // try_walker_trace_raise_bare_class's commit ordering).
+ if !obj_op.is_constant() {
+ let expected = ctx.trace_ctx.const_ref(concrete_obj as i64);
+ ctx.trace_ctx
+ .record_guard(OpCode::GuardValue, &[obj_op, expected], 0);
+ walker_capture_snapshot_for_last_guard(ctx, op.pc)?;
+ ctx.trace_ctx.heap_cache_mut().replace_box(obj_op, expected);
+ }Based on learnings, broad crate-level lint suppressions in this codebase are intentional and not flagged here; this comment is scoped purely to the ordering defect above.
📝 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 receiver, run the authentic raise, emit inline --- | |
| // The stability predicate makes the raise a pure function of `(obj, | |
| // name)`; `GuardValue` pins the one live input (`name` is a co_names | |
| // constant). | |
| if !obj_op.is_constant() { | |
| let expected = ctx.trace_ctx.const_ref(concrete_obj as i64); | |
| ctx.trace_ctx | |
| .record_guard(OpCode::GuardValue, &[obj_op, expected], 0); | |
| walker_capture_snapshot_for_last_guard(ctx, op.pc)?; | |
| ctx.trace_ctx.heap_cache_mut().replace_box(obj_op, expected); | |
| } | |
| // The authoritative walk's concrete execution — the same call the | |
| // residual executor would have made, raising before any heap | |
| // mutation. Plain eval: the predicate excludes every user-code path. | |
| let result = { | |
| let _plain_guard = pyre_interpreter::call::force_plain_eval(); | |
| match concrete_value { | |
| Some(value) => pyre_interpreter::baseobjspace::setattr_str(concrete_obj, &name, value), | |
| None => pyre_interpreter::baseobjspace::delattr_str(concrete_obj, &name), | |
| } | |
| }; | |
| let Err(mut err) = result else { | |
| // Unreachable under the predicate (a non-heaptype dict rejects | |
| // every mutation). Fail loud rather than falling through: the | |
| // generic residual would re-run the (somehow) committed effect. | |
| return Err(DispatchError::UnsupportedOpname { | |
| pc: op.pc, | |
| key: "immutable-type attr raise fold: stable raise unexpectedly succeeded", | |
| }); | |
| }; | |
| let exc = err.to_exc_object(); | |
| let kind = unsafe { | |
| if !pyre_object::is_exception(exc) { | |
| return Ok(None); | |
| } | |
| pyre_object::interp_exceptions::w_exception_get_kind(exc) | |
| }; | |
| // The folded raise is exactly the immutable-type TypeError; any other | |
| // kind means the runtime path diverged from the predicate's model. | |
| if kind != pyre_object::interp_exceptions::ExcKind::TypeError { | |
| return Ok(None); | |
| } | |
| let exc_type_ptr = unsafe { | |
| (*(exc as *const pyre_object::interp_exceptions::W_BaseException)) | |
| .ob_header | |
| .ob_type | |
| }; | |
| if !std::ptr::eq( | |
| exc_type_ptr, | |
| pyre_object::interp_exceptions::exc_kind_to_pytype(kind), | |
| ) { | |
| return Ok(None); | |
| } | |
| // The authoritative walk's concrete execution — the same call the | |
| // residual executor would have made, raising before any heap | |
| // mutation. Plain eval: the predicate excludes every user-code path. | |
| let result = { | |
| let _plain_guard = pyre_interpreter::call::force_plain_eval(); | |
| match concrete_value { | |
| Some(value) => pyre_interpreter::baseobjspace::setattr_str(concrete_obj, &name, value), | |
| None => pyre_interpreter::baseobjspace::delattr_str(concrete_obj, &name), | |
| } | |
| }; | |
| let Err(mut err) = result else { | |
| // Unreachable under the predicate (a non-heaptype dict rejects | |
| // every mutation). Fail loud rather than falling through: the | |
| // generic residual would re-run the (somehow) committed effect. | |
| return Err(DispatchError::UnsupportedOpname { | |
| pc: op.pc, | |
| key: "immutable-type attr raise fold: stable raise unexpectedly succeeded", | |
| }); | |
| }; | |
| let exc = err.to_exc_object(); | |
| let kind = unsafe { | |
| if !pyre_object::is_exception(exc) { | |
| return Ok(None); | |
| } | |
| pyre_object::interp_exceptions::w_exception_get_kind(exc) | |
| }; | |
| // The folded raise is exactly the immutable-type TypeError; any other | |
| // kind means the runtime path diverged from the predicate's model. | |
| if kind != pyre_object::interp_exceptions::ExcKind::TypeError { | |
| return Ok(None); | |
| } | |
| let exc_type_ptr = unsafe { | |
| (*(exc as *const pyre_object::interp_exceptions::W_BaseException)) | |
| .ob_header | |
| .ob_type | |
| }; | |
| if !std::ptr::eq( | |
| exc_type_ptr, | |
| pyre_object::interp_exceptions::exc_kind_to_pytype(kind), | |
| ) { | |
| return Ok(None); | |
| } | |
| // --- commit: pin the receiver, emit inline exception construction --- | |
| // Every decline above is now ruled out; only past this point may trace | |
| // state (guards, heap-cache) be mutated (mirrors | |
| // try_walker_trace_raise_bare_class's commit ordering). | |
| if !obj_op.is_constant() { | |
| let expected = ctx.trace_ctx.const_ref(concrete_obj as i64); | |
| ctx.trace_ctx | |
| .record_guard(OpCode::GuardValue, &[obj_op, expected], 0); | |
| walker_capture_snapshot_for_last_guard(ctx, op.pc)?; | |
| ctx.trace_ctx.heap_cache_mut().replace_box(obj_op, expected); | |
| } |
🤖 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 5458 -
5511, Reorder the immutable-type attribute raise folding in the surrounding
specialization logic so the concrete setattr_str/delattr_str execution and all
exception, kind, and canonical-type validations complete before emitting
GuardValue or updating heap_cache_mut(). Keep the existing decline returns
unchanged, ensuring any Ok(None) path leaves the trace and heap cache untouched;
only commit the guard and cache replacement after validation succeeds.
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
awesome! thank you so much!
Problem
synth/type_immutable_rejectis ~133x slower than CPython (pyre 0.74s vs cpython 0.17s vs pypy 0.02s). Profiling (sample, 5s window) attributes ~65% of the loop time to thebh_store_attr_fn/bh_delete_attr_fnresiduals, almost all of it insidePyError::to_exc_object: every compiled iteration re-materialises the TypeError instance, its message string, and its args list through the runtime GC hooks (dynasm_gc_owns_object→OldGen::contains, shadow-stack pins, write barriers).The trace records the raising store as
CallMayForceN + GuardNotForced + GuardException, so the exception is born inside the opaque residual and comes out as an unvirtualizable guard-result box — even though everything downstream (inline PyTraceback construction,sys_exc_valuesave/restore, handler matching) is already traced. By contrast, an explicitraise TypeError("...")loop already runs at PyPy-class speed (~0.05s loop time) because the B3 construct fold (try_walker_trace_exception_new) exposes the allocation to the optimizer, which virtualizes it away when the exception never escapes.Change
Extend the B3 treatment to the one attribute-store raise whose outcome is provably iteration-invariant: a STORE_ATTR / DELETE_ATTR on a non-heap (immutable) builtin type.
pyre-interpreter: new stability predicatetype_immutable_attr_raise_is_stable(obj, name, is_delete)— accepts only a non-heaptype type receiver whose metaclass is the canonicaltype(MRO(type, object), both frozen) with no metaclass descriptor forname. Under these conditions every dict thesetattr_str/delattr_strraise-path decision consults is frozen, so the raise and its message are a pure function of trace-time constants.pyre-jit-trace: new walker foldtry_walker_trace_immutable_type_attr_raise— pins the receiver withGuardValue(when not already constant), runs the authenticsetattr_str/delattr_strconcretely for the authoritative walk's exception, and emits the inline construction (emit_object_list_inline+emit_exception_new_inline, message as a rooted trace constant shared across iterations like a co_consts string) in place of the residual + guards, then seeds the residual-executor Err-arm state (last_exc_value,BH_LAST_EXC_VALUE,fbw_built_exc) and routes through the ordinarySubRaisepath.majit-ir/pyre-jit:PyreHelperKind::DeleteAttrcalldescr tag (appended at the enum tail, existing discriminants preserved) + tagging inlower_delete_attr_hlop_to_insn, so the walker can recognise the DELETE_ATTR residual — mirroring the existingStoreAttrtag.Python-visible semantics are unchanged: the same TypeError (same message, fresh instance per iteration, traceback attached) is raised; only how the compiled trace produces it changes. Any shape the predicate cannot prove (heap types, custom metaclasses, metaclass descriptors like
type.__name__) declines to the unchanged residual path.Results (Apple Silicon)
synth/type_immutable_reject(dynasm): 0.74s → 0.11s — now faster than CPython (0.15s); 10x from PyPy (was ~37x). The optimized trace contains noCallMayForceNresiduals; both the store and delete exceptions virtualize. cranelift: 0.16s.check.py --synthetic-only: dynasm 314/314, cranelift 314/314 — ALL PASSED.PYRE_NO_JIT=1interpreter output, 7 scenarios): escaping exception message/args/traceback, delete-variant message,int.__name__ = x(metaclass-descriptor decline), heap-type store success in the same loop shape, custom-metaclass__setattr__override, per-iteration fresh exception identity.Status
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
TypeErroroutcomes are now captured and replayed directly in optimized execution paths.Bug Fixes