jit: widen the FOR_ITER callee inline; fix a silent dunder-dispatch abort - #919
Conversation
`is_type` compares the physical `ob_type` against `TYPE_TYPE`, which every `W_TypeObject` literal hardcodes, so it answers true for a class built with any metaclass; the metaclass is `w_class`, read through `typedef::type`. Two guards read `is_type` as "the metatype is exactly `type`": - `classmethod_on_type_fast_path` declined names the metatype defines by calling `lookup_in_type` on `&TYPE_TYPE`. That is a `PyType`, not a `W_TypeObject`, so `lookup_where`'s own `is_type` gate answered false and the check returned `None` for every name. Read the metatype off the class, require it to be `type`, and run the name check against it. - `compute_load_method_bound`'s type-receiver arm returned the class as `self` whenever the class's own MRO held a classmethod, whatever produced `attr`. A metatype data descriptor or `__getattribute__` override returns its own value there, and binding the class onto it passes an argument the value never declared. Require the metatype to be `type` before the arm's shape inference. `type_metatype_method_call` covers both shapes: a metaclass `property` and a metaclass `__getattribute__`, each returning a zero-argument lambda shadowed by a same-named classmethod, plus an ordinary class whose classmethod still binds. Both previously raised `TypeError: <lambda>() takes 0 positional arguments but 1 was given`, with and without the JIT. Assisted-by: Claude
`alloc_virtual_ref` fell back to `Box::into_raw` whenever `alloc_oldgen_typed` answered `GcRef(0)`, including after `set_vref_gc_type_id` had run. A host box is invisible to the collector, so its `forced` slot stops tracing the frame the vref exists to keep reachable, and the frame can move or be freed while `ExecutionContext.topframeref` still names the vref. Once the type id is set the allocation now asserts instead. The box stays only for the window before registration, where there is no registered type and no vref has reached the collector yet. Assisted-by: Claude
The multiframe seed block's PopJumpIfNone/PopJumpIfNotNone precondition returned `DispatchError::callee_inline_unsupported`, which trace.rs maps to a plain `TraceAction::Abort` with no decline recorded. The predicate is static and callee-shaped, so every retrace of the enclosing loop hit it again and aborted again; the guard whose bridge the retrace was building never got one. It now returns `Ok(None)` on the try_multiframe path, joining every other precondition in the same block. `while tb is not None:` lowers to exactly this instruction, so a handler calling a traceback-walking helper was the common trigger. Measured on the exception family: loops_aborted 208 -> 75, guard_failures 39330 -> 21448. gc_bug_bridge_flavor_traceback_names alone goes from 97 aborts / 20702 guard failures to 1 / 2218. The comment's stated blocker -- residualized loops printing traceback tuples that lost their outermost frame -- was closed by the bridge handler-entry arms that attach the catching frame's own node. Assisted-by: Claude
`tb.tb_lineno` was left to the opaque `getattr_fn` residual while its three neighbours on the same walk (`tb_next`, `tb_frame`, `f_code`) fold to guarded inline field reads. Measured at 207 ns per read against 0 for `tb_next`; a 2M-read loop drops from 0.587s to 0.087s, the cost of the loop alone. The slot is an Int, so the fold reads it with `getfield_gc_i` and reboxes through `wrapint` the way the unboxed mapdict read does. `get_lineno` maps `LINENO_NOT_COMPUTED` to -1, so the read is only the getter's value once the slot is pinned against that sentinel: a node already carrying it declines before recording anything, and every other node emits `int_eq` + `guard_false` so a replay that meets one deopts instead of reporting `i64::MIN`. `tb_lasti` stays residual: its getter reports `lasti * 2`, not the slot. Assisted-by: Claude
The FOR_ITER inline gate admitted a `CalleeReplaySafety::DeferredCall` body
from every entry, including the two binop dunder-dispatch specializers. That
admission rests on the abort rewinding to the enclosing CALL and re-executing
it; a dunder dispatch enters from a `BINARY_OP`, which is not a boundary the
rewind can name, so a residual that failed to fold resumed one operand short
and dropped the whole iteration's contribution.
Gate the deferred arm on `arg_class_guard.is_none()`, which is `Some` at
exactly those two entries. `Clean` bodies keep their admission there — nothing
in one can abort.
Witness, wrong before this commit:
class C(int):
def __add__(self, o):
return len(str(int(self)))
def fold(acc, r):
return acc + r
acc = 0
for i in range(20000):
w = i if i % 71 == 0 else C(i)
acc = fold(acc, w + w)
`Traces aborted: 0 -> 1` is the only counter that moves; the output is short by
exactly one iteration.
Assisted-by: Claude
`fbw_callee_body_replay_safety` accepted a `binary_op` residual only when both operands were proven exact-numeric, and answered `Dirty` otherwise. A `LOAD_ATTR` result never carries that proof — its own arm is deferred and clears numeric provenance — so a callee as small as `return self.v + i` made the whole call residualize inside a `for` body. Add `BinaryOp` / `CompareOp` to the deferred-call helper list. Which `__add__` runs is a runtime property of the operand's class, the same thing the `CallFn` / `LoadAttr` entries already defer; the walker's numeric specialization erases the residual once the attribute read folds to a mapdict slot with a concrete int shadow, and an operand pair that stays opaque leaves a residual that reaches `fbw_abort_nested_unjournaled_residual` before the helper runs. N=400000, min-of-3, both binaries in target/release: o.v + i, plain function 0.60s -> 0.10s o.v + i, global receiver 0.59s -> 0.10s o.v + 1 0.58s -> 0.11s o.v + o.v 0.58s -> 0.08s stored bound method m(i) 0.36s -> 0.07s Assisted-by: Claude
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit aa4100d). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
|
Warning Review limit reached
Next review available in: 33 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 Plus Run ID: 📒 Files selected for processing (8)
WalkthroughThe PR restricts custom-metaclass classmethod binding, adds a metaclass benchmark, specializes JIT traceback line numbers, changes residual and inline safety handling, and enforces GC-owned virtual-reference allocation after type registration. ChangesMetaclass method binding
JIT trace dispatch
Virtual-reference allocation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant main
participant compute_load_method_bound
participant classmethod_lookup
participant custom_metaclass
main->>compute_load_method_bound: call method on type receiver
compute_load_method_bound->>classmethod_lookup: check receiver metaclass
classmethod_lookup->>custom_metaclass: decline implicit binding for custom metaclass
custom_metaclass-->>main: resolve metaclass-provided callable
sequenceDiagram
participant traceback_walker
participant pytraceback_lineno_descr
participant PyTraceback
participant destination
traceback_walker->>pytraceback_lineno_descr: resolve tb_lineno descriptor
pytraceback_lineno_descr->>PyTraceback: locate lineno field
traceback_walker->>PyTraceback: read validated line number
traceback_walker->>destination: box and write Python integer
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 |
![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-interpreter/src/eval.rs (1)
2991-3009: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the already-resolved metatype pointer instead of calling
r#type(obj)twice.Line 2991 already resolves
crate::typedef::r#type(obj)to computemetatype_is_type. Line 3004 callscrate::typedef::r#type(obj)again inside theNonearm of the match to walk the metaclass MRO. Store the resolved metatype pointer from line 2991 and reuse it at line 3004 instead of resolving it a second time. This function runs on the LOAD_METHOD resolution path, so avoiding the repeated dereference chain inr#typereduces per-call overhead.♻️ Proposed fix to reuse the resolved metatype
- let metatype_is_type = crate::typedef::r#type(obj) - .is_some_and(|meta| std::ptr::eq(meta.as_ptr(), crate::typedef::w_type())); - if !metatype_is_type { + let metatype = crate::typedef::r#type(obj); + let metatype_is_type = + metatype.is_some_and(|meta| std::ptr::eq(meta.as_ptr(), crate::typedef::w_type())); + if !metatype_is_type { return PY_NULL; } let raw = crate::baseobjspace::lookup_in_type(obj, name); match raw { Some(d) if pyre_object::is_classmethod(d) => obj, Some(_) => PY_NULL, // found in own MRO → no binding None => { - match crate::typedef::r#type(obj) - .and_then(|meta| crate::baseobjspace::lookup_in_type(meta.as_ptr(), name)) + match metatype + .and_then(|meta| crate::baseobjspace::lookup_in_type(meta.as_ptr(), name)) {🤖 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-interpreter/src/eval.rs` around lines 2991 - 3009, Store the metatype pointer obtained from crate::typedef::r#type(obj) before computing metatype_is_type, then reuse that stored pointer in the None arm when calling lookup_in_type. Remove the second r#type(obj) resolution while preserving the existing validation and method-descriptor binding behavior.
🤖 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-interpreter/src/eval.rs`:
- Around line 2982-2995: Extract the exact-metaclass predicate into a shared
helper such as has_exact_type_metaclass in
pyre/pyre-interpreter/src/eval.rs#L2982-L2995, preserving the existing
pointer-identity behavior, and call that helper from the local check. Update
pyre/pyre-interpreter/src/baseobjspace.rs#L8925-L8936 to use the same helper
instead of duplicating crate::typedef::r#type and std::ptr::eq logic.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 2684-2694: Move FBW_FORITER_DEFERRED_DENY and its accessors,
including fbw_foriter_deferred_call_denied, out of thread_local! into
interpreter/JIT-session-owned shared state so denials for a CodeObject are
visible across tracing threads. Update all reads and writes, including the
foriter_deferred_admit calculation, to use the owner-scoped registry and
preserve consistent DeferredCall replay behavior.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/eval.rs`:
- Around line 2991-3009: Store the metatype pointer obtained from
crate::typedef::r#type(obj) before computing metatype_is_type, then reuse that
stored pointer in the None arm when calling lookup_in_type. Remove the second
r#type(obj) resolution while preserving the existing validation and
method-descriptor binding behavior.
🪄 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: 083668ed-c6cc-40f2-9484-143b7689f997
📒 Files selected for processing (8)
majit/majit-metainterp/src/virtualref.rspyre/bench/synth/type_metatype_method_call.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/eval.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs
| // | ||
| // `is_type` reports the physical layout every type object shares, | ||
| // not the metaclass, so read the metaclass and require it to be | ||
| // `type`. The shape inferred below is what | ||
| // `type.__getattribute__` produces; a custom metaclass can | ||
| // override `__getattribute__` or define a data descriptor of the | ||
| // same name, and either one produced `attr` in place of the | ||
| // class's own MRO entry — binding `cls` onto that value would | ||
| // pass the class to something that never asked for it. | ||
| let metatype_is_type = crate::typedef::r#type(obj) | ||
| .is_some_and(|meta| std::ptr::eq(meta.as_ptr(), crate::typedef::w_type())); | ||
| if !metatype_is_type { | ||
| return PY_NULL; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate exact-metaclass check across two files. Both sites independently implement the same "receiver's actual metaclass is exactly type" predicate (crate::typedef::r#type(obj) followed by std::ptr::eq(..., crate::typedef::w_type())). This predicate is the core correctness fix for this PR; keeping it in two places risks future divergence.
pyre/pyre-interpreter/src/eval.rs#L2982-L2995: extract this check into a shared helper (for examplecrate::typedef::has_exact_type_metaclass) and call it here.pyre/pyre-interpreter/src/baseobjspace.rs#L8925-L8936: call the same shared helper here instead of reimplementing the pointer-identity check.
📍 Affects 2 files
pyre/pyre-interpreter/src/eval.rs#L2982-L2995(this comment)pyre/pyre-interpreter/src/baseobjspace.rs#L8925-L8936
🤖 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-interpreter/src/eval.rs` around lines 2982 - 2995, Extract the
exact-metaclass predicate into a shared helper such as has_exact_type_metaclass
in pyre/pyre-interpreter/src/eval.rs#L2982-L2995, preserving the existing
pointer-identity behavior, and call that helper from the local check. Update
pyre/pyre-interpreter/src/baseobjspace.rs#L8925-L8936 to use the same helper
instead of duplicating crate::typedef::r#type and std::ptr::eq logic.
| // The deferred promise rests on the abort REWINDING to the | ||
| // enclosing CALL and re-executing it from scratch. A binop | ||
| // dunder dispatch (the only entry carrying an | ||
| // `arg_class_guard`) reaches this lever from a `BINARY_OP` | ||
| // instead, and that opcode is not a call boundary the rewind | ||
| // can name: the flush resumes one operand short and the whole | ||
| // iteration's contribution is dropped, silently. A `Clean` | ||
| // body is still admitted from there — it has nothing that can | ||
| // abort. | ||
| foriter_deferred_admit = | ||
| arg_class_guard.is_none() && !fbw_foriter_deferred_call_denied(callee_code_key); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs --items all
rg -n -C 8 --type rust \
'FBW_FORITER_DEFERRED_DENY|fbw_foriter_deferred_call_denied|foriter_deferred.*den' \
pyreRepository: youknowone/pyre
Length of output: 13081
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
fbw = Path('pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs').read_text()
inline = Path('pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs').read_text()
needles = {
'FBW_FORITER_DEFERRED_DENY': None,
'FBW_FORITER_DEFERRED_INLINE': None,
'FBW_HAZARDOUS_INLINE_DENY': None,
}
for m in re.finditer(r'(?s)\s*const\s+(\w+)_STORAGE', fbw):
for var_name in needles:
if var_name in fbw[m.end():]:
needles[var_name] = (
m.start(),
m.group(1),
re.search(r'(?s)\b' + var_name + r'\s*=', fbw[m.end():]).group(0)
)
break
print('TLS/storage declarations:')
for name, result in needles.items():
if result:
start, storage_prefix, ref = result
# Print the containing const storage declaration if the variable belongs to it
prev = fbw[:start].rfind('pub(crate) const ')
decl = fbw[prev:start+ref.find(name)+1+len(name)]
print(f'--- {name}')
print(decl)
else:
print(f'--- {name}: not found')
print('\ndeferrals functions context:')
for fn in [
'fbw_foriter_deferred_call_denied',
'fbw_foriter_deny_deferred_call',
'fbw_foriter_deferred_inline_outermost',
]:
i = fbw.find(f'fn {fn}')
if i != -1:
print(f'--- {fn}')
print(fbw[i:i+fbw.find('\npub(crate) fn', i+14)-i] if fbw.find('\npub(crate) fn', i+14) != -1 else fbw[i:i+500])
print('\ninline usage context:')
i = max(inline.find('fbw_foriter_deferred_call_denied'), inline.find('ForiterDeferredInlineGuard'), inline.find('CalleeReplaySafety'))
print(inline[max(0,i-800):i+900])
checks = [
('FBW_FORITER_DEFERRED_DENY declared as TLS_STORAGE', any('TLS_STORAGE<' in (ne or fbw) and 'FBW_FORITER_DEFERRED_DENY' in ne for ne,(_,_,fbw) in [] )), # placeholder
]
PYRepository: youknowone/pyre
Length of output: 2778
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1060,1210p' pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
printf '\nTop-level FBW_* declarations (near file start):\n'
rg -n --type rust 'pub *static|static .*FBW_|TLS_STORAGE|thread_local' pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs | sed -n '1,220p'
printf '\nTLS_STORAGE type definitions:\n'
rg -n --type rust 'macro_rules! TLS_STORAGE|struct TLS_STORAGE|pub *struct TLS_STORAGE|thread_local!|const .*TLS_STORAGE' pyre/pyre-jit-trace/src -C 6Repository: youknowone/pyre
Length of output: 40878
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type rust 'fbw_foriter_deferred_deny_deferred_call|fbw_foriter_deferred_call_denied|CalleeReplaySafety::DeferredCall|fn fbw_callee_body_replay_safety|CalleeReplaySafety' pyre/pyre-jit-trace/src/jitcode_dispatch -C 5
printf '\nRelevant replay safety implementation:\n'
sed -n '1800,1930p' pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
printf '\nRelevant inline admission and sub-walk gating:\n'
sed -n '2620,2710p' pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs
sed -n '2710,2785p' pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rsRepository: youknowone/pyre
Length of output: 32222
**Move the deferred-callee deny registry out of thread_local!. **
FBW_FORITER_DEFERRED_DENY stores CodeObject keys and changes whether CalleeReplaySafety::DeferredCall can inline. A denied callee observed on one tracing thread stays visible only on that thread, so the same callee can replay as Clean later on another thread and produce different JIT behavior under the interpreter semantics invariant. Store this registry with the interpreter/JIT-session owner instead of per-thread state, with an upstream citation if per-thread scope is intended.
🤖 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 2684 -
2694, Move FBW_FORITER_DEFERRED_DENY and its accessors, including
fbw_foriter_deferred_call_denied, out of thread_local! into
interpreter/JIT-session-owned shared state so denials for a CodeObject are
visible across tracing threads. Update all reads and writes, including the
foriter_deferred_admit calculation, to use the owner-scoped registry and
preserve consistent DeferredCall replay behavior.
Source: Coding guidelines
1 participant
Six commits on top of
main, all found while chasing the exception-bench gapagainst pypy3. Two of them turned out to be general call-inlining work rather
than exception work, and one is a silent wrong-code fix that was already on
main.The two new commits
jit: decline a deferred callee at the binop dunder entry— wrong-code, pre-existingThe FOR_ITER inline gate admitted a
CalleeReplaySafety::DeferredCallbodyfrom every entry, including the two binop dunder-dispatch specializers. That
admission rests on the abort rewinding to the enclosing CALL and re-executing
it. A dunder dispatch enters the lever from a
BINARY_OP, which is not aboundary the rewind can name, so a residual that failed to fold resumed one
operand short and dropped one whole loop iteration's contribution, silently.
Traces aborted: 0 → 1is the only counter that moves.Reproduces on
maintoday:The fix gates the deferred arm on
arg_class_guard.is_none(), which isSomeat exactly those two entries.
Cleanbodies keep their admission there —nothing in one can abort.
jit: defer an unproven binop in the callee replay scan— 5–7xfbw_callee_body_replay_safetyaccepted abinary_opresidual only when bothoperands were proven exact-numeric, and answered
Dirtyotherwise. ALOAD_ATTRresult never carries that proof — its own arm is deferred andclears numeric provenance — so a callee as small as
return self.v + imadethe whole call residualize inside a
forbody.BinaryOp/CompareOpnow join
CallFn/LoadAttron the deferred list: which__add__runs is aruntime property of the operand's class, the walker's numeric specialization
erases the residual once the attribute read folds to a mapdict slot, and an
operand pair that stays opaque leaves a residual that reaches
fbw_abort_nested_unjournaled_residualbefore the helper runs.N=400000, min-of-3, both binaries in
target/release/:o.v + i, plain functiono.v + i, global receivero.v + 1o.v + o.vm(i)b.at(i)in LOAD_METHOD form is not covered — a separate gate(
method_form_callee_body_supported) declines any method-form callee whosebody reads an attribute. Lifting it gives another 14x but returns wrong output
on
synth/sre_pattern_methods, so it is left for follow-up.The four earlier commits
interp: pin the metatype before LOAD_METHOD binds cls— plus atype_metatype_method_callbench.virtualref: drop the host-box fallback after registration.jit: decline the PopJumpIfNone callee inline instead of aborting— excfamily
loops_aborted208 → 75,guard_failures39330 → 21448.jit: fold tb_lineno like the traceback chain hops— a 2M-read micro goes0.587s → 0.087s, matching the existing
tb_nextfold.Verification
cargo test --release: pyre-jit-trace 313, pyre-interpreter 475,pyre-object 292, majit-gc 213 — all green.
check.py --backend dynasm350/350,--backend cranelift350/350.after on the benches whose wall clock moved, and min-of-5 timings match — the
new commits do not touch that family, they just do not regress it.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
tb_lineno.Performance