Repository navigation
perf-exc: exception JIT perf (attr fold + loop-header dominance) - #689
Conversation
|
Warning Review limit reached
Next review available in: 40 seconds 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 (7)
WalkthroughThe PR adds folding support for exception ChangesException attribute slot folding
Inline-call dispatch guards
Dominance-based loop-header analysis
Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
![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: 8cfbfa83c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if d.opname.starts_with("residual_call") | ||
| && residual_call_helper_kind_in_body(body_code, &d, callee_descr_refs) | ||
| == Some(majit_ir::PyreHelperKind::CallFn) |
There was a problem hiding this comment.
For an exception __str__/__repr__ override like return helper(x=1) or return helper(*args), the body is still straight-line but the nested Python call is lowered as PyreHelperKind::CallKw/CallFunctionEx (checked in pyre/pyre-jit/src/jit/flatten.rs's call_kw and call_function_ex lowering), not CallFn. This check lets those through to the bounded inline path even though the surrounding comment says nested Python calls need to stay residual, so the sub-walk can still hit the same mid-recording abort/trace discard this commit is trying to avoid.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 7620f6f). 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)None. 4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98ed9367b9
ℹ️ 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".
| Instruction::PopJumpIfFalse { delta } | ||
| | Instruction::PopJumpIfTrue { delta } | ||
| | Instruction::PopJumpIfNone { delta } | ||
| | Instruction::PopJumpIfNotNone { delta } | ||
| | Instruction::JumpForward { delta } | ||
| | Instruction::ForIter { delta } => Some(delta.get(op_arg).as_usize()), |
There was a problem hiding this comment.
When this new CFG is used for coroutine/generator bytecode, SEND is a real conditional forward branch (execute_send jumps via jump_target_forward_from_oparg, and liveness already treats Instruction::Send as a successor), but this match only records the fall-through edge. That under-approximates reachability for target_dominates, so a backward cleanup/rejoin jump reachable through the SEND taken edge can be misclassified as a real loop backedge and get a loop_header marker instead of being filtered out.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/baseobjspace.rs`:
- Line 8465: After updating the interpreter source around ExceptionAttrSlot,
re-extract the corresponding Charon .ullbc files using scripts/extract-llbc.py
before rebuilding the rtyper prepass.
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 13889-13948: Update the forward-branch matching in both
code_successors and find_branch_target_pcs to include Instruction::Send { delta
}, using the same delta-based target calculation as the existing forward branch
instructions. Keep the behavior aligned with liveness::target_pc so SEND targets
are recorded as control-flow successors and branch targets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eaeaf3c7-ce3e-4a18-9299-130d241ab032
📒 Files selected for processing (6)
pyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit/src/jit/codewriter.rs
| @@ -8464,6 +8464,8 @@ pub fn object_setattr(obj: PyObjectRef, name: &str, value: PyObjectRef) -> PyRes | |||
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | |||
| pub enum ExceptionAttrSlot { | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Re-extract Charon .ullbc files.
As per coding guidelines, after changing interpreter, object, or JIT source, re-extract the corresponding Charon .ullbc files with scripts/extract-llbc.py before rebuilding the rtyper prepass.
🤖 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/baseobjspace.rs` at line 8465, After updating the
interpreter source around ExceptionAttrSlot, re-extract the corresponding Charon
.ullbc files using scripts/extract-llbc.py before rebuilding the rtyper prepass.
Source: Coding guidelines
| /// Control-flow successors of every code-unit index: fall-through (except | ||
| /// after an unconditional transfer), forward and backward jump targets, and | ||
| /// the exception edge from each protected pc to its handler landing. Built in | ||
| /// one sequential decode pass so an `EXTENDED_ARG` prefix folds into the | ||
| /// following opcode's argument, matching the edge model of | ||
| /// `find_branch_target_pcs`. | ||
| fn code_successors(code: &CodeObject) -> Vec<Vec<usize>> { | ||
| let num_instrs = code.instructions.len(); | ||
| let mut succ: Vec<Vec<usize>> = vec![Vec::new(); num_instrs]; | ||
| let mut scan_state = OpArgState::default(); | ||
| for pc in 0..num_instrs { | ||
| let (instr, op_arg) = scan_state.get(code.instructions[pc]); | ||
| if let Some(target) = backward_jump_target(code, pc, instr, op_arg) { | ||
| if target < num_instrs { | ||
| succ[pc].push(target); | ||
| } | ||
| } | ||
| let forward_delta = match instr { | ||
| Instruction::PopJumpIfFalse { delta } | ||
| | Instruction::PopJumpIfTrue { delta } | ||
| | Instruction::PopJumpIfNone { delta } | ||
| | Instruction::PopJumpIfNotNone { delta } | ||
| | Instruction::JumpForward { delta } | ||
| | Instruction::ForIter { delta } => Some(delta.get(op_arg).as_usize()), | ||
| _ => None, | ||
| }; | ||
| if let Some(delta) = forward_delta { | ||
| let target = jump_target_forward(code, num_instrs, pc + 1, delta); | ||
| if target < num_instrs { | ||
| succ[pc].push(target); | ||
| } | ||
| } | ||
| let terminates = matches!( | ||
| instr, | ||
| Instruction::JumpForward { .. } | ||
| | Instruction::JumpBackward { .. } | ||
| | Instruction::JumpBackwardNoInterrupt { .. } | ||
| | Instruction::ReturnValue | ||
| | Instruction::RaiseVarargs { .. } | ||
| | Instruction::Reraise { .. } | ||
| ); | ||
| if !terminates { | ||
| let fallthrough = pc + 1; | ||
| if fallthrough < num_instrs { | ||
| succ[pc].push(fallthrough); | ||
| } | ||
| } | ||
| } | ||
| for entry in pyre_interpreter::pycode::decode_exceptiontable(&code.exceptiontable) { | ||
| let start = entry.start as usize / 2; | ||
| let end = (entry.end as usize / 2).min(num_instrs); | ||
| let handler = entry.target as usize / 2; | ||
| if handler < num_instrs { | ||
| for pc in start..end { | ||
| succ[pc].push(handler); | ||
| } | ||
| } | ||
| } | ||
| succ | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Instruction::Send exists with a `delta` field and check whether
# other branch-target helpers in this file (find_branch_target_pcs,
# jump_target_forward) already special-case it, to see if code_successors
# is the only place missing this edge.
rg -n 'Send\s*\{' --type=rust
rg -nP -A3 -B3 '\bfn find_branch_target_pcs\b' pyre/pyre-jit/src/jit/codewriter.rs
rg -n 'Instruction::Send' pyre/pyre-jit/src/jit/codewriter.rs pyre/pyre-jit-trace/src/liveness.rsRepository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant sections before reading them.
ast-grep outline pyre/pyre-jit/src/jit/codewriter.rs --view expanded | sed -n '1,220p'
printf '\n=== codewriter Send / branch-target references ===\n'
rg -n -C 3 'Instruction::Send|find_branch_target_pcs|code_successors|jump_target_forward|backward_jump_is_handler_only_target' pyre/pyre-jit/src/jit/codewriter.rs
printf '\n=== liveness target_pc references ===\n'
rg -n -C 3 'Send\s*\{|target_pc|PopJumpIfFalse|ForIter|JumpBackwardNoInterrupt' pyre/pyre-jit-trace/src/liveness.rs
printf '\n=== file sizes ===\n'
wc -l pyre/pyre-jit/src/jit/codewriter.rs pyre/pyre-jit-trace/src/liveness.rsRepository: youknowone/pyre
Length of output: 23788
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '14299,14335p' pyre/pyre-jit/src/jit/codewriter.rsRepository: youknowone/pyre
Length of output: 1992
Include SEND in the forward-edge scan
code_successors under-approximates control flow here: pyre-jit-trace::liveness::target_pc already treats Instruction::Send { delta } as a forward branch, so omitting it can hide a real bypass path and misclassify a non-header backedge as a semantic loop header. The same arm is also missing from find_branch_target_pcs.
Fix
let forward_delta = match instr {
Instruction::PopJumpIfFalse { delta }
| Instruction::PopJumpIfTrue { delta }
| Instruction::PopJumpIfNone { delta }
| Instruction::PopJumpIfNotNone { delta }
| Instruction::JumpForward { delta }
- | Instruction::ForIter { delta } => Some(delta.get(op_arg).as_usize()),
+ | Instruction::ForIter { delta }
+ | Instruction::Send { delta } => Some(delta.get(op_arg).as_usize()),
_ => None,
};📝 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.
| /// Control-flow successors of every code-unit index: fall-through (except | |
| /// after an unconditional transfer), forward and backward jump targets, and | |
| /// the exception edge from each protected pc to its handler landing. Built in | |
| /// one sequential decode pass so an `EXTENDED_ARG` prefix folds into the | |
| /// following opcode's argument, matching the edge model of | |
| /// `find_branch_target_pcs`. | |
| fn code_successors(code: &CodeObject) -> Vec<Vec<usize>> { | |
| let num_instrs = code.instructions.len(); | |
| let mut succ: Vec<Vec<usize>> = vec![Vec::new(); num_instrs]; | |
| let mut scan_state = OpArgState::default(); | |
| for pc in 0..num_instrs { | |
| let (instr, op_arg) = scan_state.get(code.instructions[pc]); | |
| if let Some(target) = backward_jump_target(code, pc, instr, op_arg) { | |
| if target < num_instrs { | |
| succ[pc].push(target); | |
| } | |
| } | |
| let forward_delta = match instr { | |
| Instruction::PopJumpIfFalse { delta } | |
| | Instruction::PopJumpIfTrue { delta } | |
| | Instruction::PopJumpIfNone { delta } | |
| | Instruction::PopJumpIfNotNone { delta } | |
| | Instruction::JumpForward { delta } | |
| | Instruction::ForIter { delta } => Some(delta.get(op_arg).as_usize()), | |
| _ => None, | |
| }; | |
| if let Some(delta) = forward_delta { | |
| let target = jump_target_forward(code, num_instrs, pc + 1, delta); | |
| if target < num_instrs { | |
| succ[pc].push(target); | |
| } | |
| } | |
| let terminates = matches!( | |
| instr, | |
| Instruction::JumpForward { .. } | |
| | Instruction::JumpBackward { .. } | |
| | Instruction::JumpBackwardNoInterrupt { .. } | |
| | Instruction::ReturnValue | |
| | Instruction::RaiseVarargs { .. } | |
| | Instruction::Reraise { .. } | |
| ); | |
| if !terminates { | |
| let fallthrough = pc + 1; | |
| if fallthrough < num_instrs { | |
| succ[pc].push(fallthrough); | |
| } | |
| } | |
| } | |
| for entry in pyre_interpreter::pycode::decode_exceptiontable(&code.exceptiontable) { | |
| let start = entry.start as usize / 2; | |
| let end = (entry.end as usize / 2).min(num_instrs); | |
| let handler = entry.target as usize / 2; | |
| if handler < num_instrs { | |
| for pc in start..end { | |
| succ[pc].push(handler); | |
| } | |
| } | |
| } | |
| succ | |
| } | |
| let forward_delta = match instr { | |
| Instruction::PopJumpIfFalse { delta } | |
| | Instruction::PopJumpIfTrue { delta } | |
| | Instruction::PopJumpIfNone { delta } | |
| | Instruction::PopJumpIfNotNone { delta } | |
| | Instruction::JumpForward { delta } | |
| | Instruction::ForIter { delta } | |
| | Instruction::Send { delta } => Some(delta.get(op_arg).as_usize()), | |
| _ => None, | |
| }; |
🤖 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/src/jit/codewriter.rs` around lines 13889 - 13948, Update the
forward-branch matching in both code_successors and find_branch_target_pcs to
include Instruction::Send { delta }, using the same delta-based target
calculation as the existing forward branch instructions. Keep the behavior
aligned with liveness::target_pc so SEND targets are recorded as control-flow
successors and branch targets.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs (1)
8257-8262: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix
take()short-circuiting to ensurebridge_entry_merge_pcis cleared on the first crossing.Due to Rust's strict left-to-right short-circuit evaluation of
&&, thetake()call will not execute ifctx.trace_ctx.seen_loop_header_for_jdindex >= 0orctx.is_top_levelis false. This violates the intent stated in the comment ("take() clears on the first crossing regardless of pc") and leaks thebridge_entry_merge_pcstate to subsequent loop crossings, where it could incorrectly skip a valid loop iteration.Extract the
take()evaluation before the conditional to ensure it is cleared unconditionally at the firstjit_merge_point.🐛 Proposed fix
- if ctx.is_top_level - && ctx.trace_ctx.seen_loop_header_for_jdindex < 0 - && ctx.fbw_mode.bridge_entry_merge_pc.take() == Some(next_instr) - { - return Ok((DispatchOutcome::Continue, op.next_pc)); - } + let bridge_entry = ctx.fbw_mode.bridge_entry_merge_pc.take(); + if ctx.is_top_level + && ctx.trace_ctx.seen_loop_header_for_jdindex < 0 + && bridge_entry == Some(next_instr) + { + return Ok((DispatchOutcome::Continue, op.next_pc)); + }🤖 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/mod.rs` around lines 8257 - 8262, Extract the ctx.fbw_mode.bridge_entry_merge_pc.take() result before the conditional in the dispatch logic so it executes unconditionally at the first crossing. Use the captured value in the existing top-level and loop-header checks, preserving the current Continue outcome and next-instruction behavior when it matches next_instr.
🤖 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 1561-1576: Route all direct pointer-to-i64 conversions through
usize for consistency and portability. Update physical_type and w_class_const in
the shown guard logic, and apply the same conversion pattern to other direct
pointer casts such as method as i64 in the affected functions.
- Around line 438-443: Update the residual_call filter in the inline-call
dispatch logic to also return true for PyreHelperKind::CallFunctionEx and
PyreHelperKind::CallKw, alongside CallFn. Ensure all three Python call variants
use the residual dispatch path and are excluded from nested inlining.
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 13978-13995: Update backward_jump_is_handler_only_target to accept
a reference to a precomputed successor graph and use it for target_dominates
instead of calling code_successors. In CodeWriter::transform_graph_to_jitcode,
compute the graph once before the bytecode iteration and pass &succ to both
backward_jump_is_handler_only_target call sites.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs`:
- Around line 8257-8262: Extract the ctx.fbw_mode.bridge_entry_merge_pc.take()
result before the conditional in the dispatch logic so it executes
unconditionally at the first crossing. Use the captured value in the existing
top-level and loop-header checks, preserving the current Continue outcome and
next-instruction behavior when it matches next_instr.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c28a8ac8-bbfa-4f17-8a1a-f0d8753f1cfe
📒 Files selected for processing (7)
pyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rs
| if d.opname.starts_with("residual_call") | ||
| && residual_call_helper_kind_in_body(body_code, &d, callee_descr_refs) | ||
| == Some(majit_ir::PyreHelperKind::CallFn) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject CallFunctionEx and CallKw alongside CallFn to completely block nested Python calls.
The residual_call filter only rejects CallFn, but CallFunctionEx and CallKw are also valid Python calls that will trigger the exact same unsupported multi-frame guard-resume snapshot you are avoiding here. Include them to ensure all nested Python calls fall back to the residual dispatch path.
🐛 Proposed fix to exhaustively cover Python call variants
- if d.opname.starts_with("residual_call")
- && residual_call_helper_kind_in_body(body_code, &d, callee_descr_refs)
- == Some(majit_ir::PyreHelperKind::CallFn)
- {
- return true;
- }
+ if d.opname.starts_with("residual_call") {
+ let kind = residual_call_helper_kind_in_body(body_code, &d, callee_descr_refs);
+ if matches!(
+ kind,
+ Some(
+ majit_ir::PyreHelperKind::CallFn
+ | majit_ir::PyreHelperKind::CallFunctionEx
+ | majit_ir::PyreHelperKind::CallKw
+ )
+ ) {
+ return true;
+ }
+ }📝 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.
| if d.opname.starts_with("residual_call") | |
| && residual_call_helper_kind_in_body(body_code, &d, callee_descr_refs) | |
| == Some(majit_ir::PyreHelperKind::CallFn) | |
| { | |
| return true; | |
| } | |
| if d.opname.starts_with("residual_call") { | |
| let kind = residual_call_helper_kind_in_body(body_code, &d, callee_descr_refs); | |
| if matches!( | |
| kind, | |
| Some( | |
| majit_ir::PyreHelperKind::CallFn | |
| | majit_ir::PyreHelperKind::CallFunctionEx | |
| | majit_ir::PyreHelperKind::CallKw | |
| ) | |
| ) { | |
| return true; | |
| } | |
| } |
🤖 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 438 -
443, Update the residual_call filter in the inline-call dispatch logic to also
return true for PyreHelperKind::CallFunctionEx and PyreHelperKind::CallKw,
alongside CallFn. Ensure all three Python call variants use the residual
dispatch path and are excluded from nested inlining.
| let physical_type = unsafe { (*concrete_arg).ob_type } as i64; | ||
| walker_guard_class(ctx, op.pc, arg, physical_type)?; | ||
| // A builtin subclass / user instance shares its `ob_type` with the base | ||
| // layout, so `ob_type` alone does not pin the class the reflected-op | ||
| // decline (`w_type_issubtype`) was computed against. Guard the live | ||
| // `w_class` too so an arg of a distinct class deopts. Singletons with a | ||
| // null `w_class` (`Ellipsis`/`NotImplemented`) are pinned exactly by | ||
| // `ob_type`, and guarding their null slot against `w_type` would itself | ||
| // be provably false — so guard `w_class` only when it is populated. | ||
| if !unsafe { (*concrete_arg).w_class }.is_null() { | ||
| let live_w_class = crate::state::opimpl_getfield_gc_r( | ||
| ctx.trace_ctx, | ||
| arg, | ||
| crate::descr::w_class_descr(), | ||
| ); | ||
| let w_class_const = ctx.trace_ctx.const_ref(w_type as i64); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consistently cast pointers through usize before i64.
For consistency with line 1591 (callable_guard_value as usize as i64), consider routing direct pointer-to-i64 casts through usize.
let physical_type = unsafe { (*concrete_arg).ob_type } as usize as i64;
// ...
let w_class_const = ctx.trace_ctx.const_ref(w_type as usize as i64);(This also applies to other instances like method as i64 on lines 2611 and 2759).
🤖 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 1561 -
1576, Route all direct pointer-to-i64 conversions through usize for consistency
and portability. Update physical_type and w_class_const in the shown guard
logic, and apply the same conversion pattern to other direct pointer casts such
as method as i64 in the affected functions.
The inlined operator's non-receiver argument guard passed the Python W_TypeObject to walker_guard_class, which emits a GuardClass comparing the object's physical ob_type. For a boxed builtin argument whose ob_type the optimizer already knows, that guard is provably false and the loop is discarded (InvalidLoop), so the operator-inline loop never compiles. Emit the GuardClass against the argument's physical ob_type. When the argument carries a populated w_class (a builtin subclass or user instance, whose ob_type is shared with the base layout), additionally guard the live w_class against the recorded type so an argument of a distinct class deopts, preserving reflected-operator priority. Arguments with a null w_class (Ellipsis/NotImplemented) are pinned by ob_type alone. Assisted-by: Claude
The exception __str__/__repr__ override inline admitted bodies whose only disqualifier was a branch: exception_string_override_straight_line rejected goto_if_not/switch but not a nested call. A body such as `return "c:" + repr(self.args)` therefore entered the bounded inline, and recording the nested call's own residual plus its guard-resume snapshot aborted mid-trace with LoopBearingCalleeInlineUnsupported, discarding the whole loop (loops_compiled=0) rather than declining. Add exception_string_override_has_nested_call and gate the inline on it before any IR is emitted: a CallFn residual (or a cond_call/call_assembler/inline_call) keeps the override on the residual dispatch path, where the interpreter owns the nested frame. Str-concat and attribute reads (BinaryOp/getfield) still inline. Assisted-by: Claude
Extend `exception_attr_slot_fold` to resolve `__context__` / `__cause__` loads to a direct `W_BaseException.w_context` / `w_cause` slot read, the same guarded-inline path already used for `args` / `code` / `errno`. A LOAD_ATTR of these on an exception now lowers to a class + version-tag guarded `GetfieldGcR` + `GuardNonnull` instead of a residual getset-descriptor `__get__` call. Loads only: stores decline, because `descr_setcause` (interp_exceptions.py:166-174) also sets `suppress_context = True` and `descr_setcontext` (:183-190) type-validates, so a raw slot write would drop those effects. A null slot declines and falls back to the residual path (which returns None), matching the getter. Assisted-by: Claude
backward_jump_is_handler_only_target decided whether a JUMP_BACKWARD is a loop backedge or a handler return by checking for an exception handler landing between the target and the source and scanning only the code before it for a competing backedge. A while/for loop whose body contains a try/except lays its own backedge after the handler block, so that scan never saw it and the genuine loop header was excluded from find_loop_header_pcs; eval_loop_jit then skipped JIT entry for the frame. Decide by dominance instead: build a control-flow graph (fall-through, forward/backward jumps, exception-table edges) and treat a backward jump as a loop backedge iff its target dominates its source. A handler return (a break/continue laid out inside an except handler) targets a merge the handler does not pass through, so it stays excluded. find_loop_header_pcs builds the successor graph once and reuses it. Replace handler_break_backward_jump_is_not_a_loop_header (its example had no handler-only backward jump and asserted the for-loop header be excluded) with handler_rejoin_backward_jump_is_not_a_loop_header, and add loop_with_try_except_body_keeps_its_loop_header. Assisted-by: Claude
try_walker_inline_user_binop / _compareop dereference the concrete operand's ob_type and w_class to pin the argument's physical class. A tagged immediate has no heap type object, so those derefs would fault on the immediate under CAN_BE_TAGGED. Decline the inline when either operand is a tagged int, before the derefs, matching typedef::r#type's tagged guard. Inert behind CAN_BE_TAGGED (default false); the residual dispatch already handles a tagged operand. Assisted-by: Claude
for_iter_body_op_is_jit_safe now admits RAISE_VARARGS, PUSH_EXC_INFO, CHECK_EXC_MATCH, POP_EXCEPT and RERAISE, so for_iter_bodies_all_jit_safe no longer declines a for-loop whose body raises or handles an exception in-frame. These opcodes already trace in the while-loop form; the finally-duplicated FOR_ITER hazard stays gated by for_iter_frame_is_finally_duplicated. Adds a positive gate test. Assisted-by: Claude
`except ... as name:` whose handler body itself raises reaches the exceptional cleanup tail `STORE_FAST name; DELETE_FAST name; RERAISE 1` on every visit. That tail lowers to a `last_exc_value` jitcode op which a bridge walk cannot answer, so the walk aborts with `LastExcValueWithoutActiveException`; the abort is not permanent, so the loop is retraced and re-aborts for its whole run. With a `FOR_ITER` in the frame an abort can land while an item is in flight and that iteration is dropped, leaving the result short by exactly one handler visit per abort. Add `for_iter_frame_has_raising_named_handler` and OR it into the `FOR_ITER` frame decline beside `for_iter_frame_is_finally_duplicated`. The predicate fires only when a handler binds a name and a RAISE_VARARGS lies between that binding and the cleanup that clears it, so `except E:` handlers, non-raising `as` handlers, and a raise sitting in the `try` block keep tracing. Assisted-by: Claude
Rebased onto
|
| job | verdict | evidence |
|---|---|---|
cargo test (ubuntu-24.04) |
pre-existing on main |
mapdict.rs cache_attrs is a RefCell on process-global interned map nodes; libtest runs each test on its own thread, so two threads writing attributes on the same global type race. Reproduced locally at --test-threads=32 (6/40). main's own run fails the same way across 10 tests. No commit here touches mapdict.rs, and pyre-interpreter does not even link pyre-jit/pyre-jit-trace. |
pyre/check.py (ubuntu-24.04) |
pre-existing on main |
FAIL wasm synth/delete_negative_open_slice_hot wrong output, byte-identical (1 failed, 239 passed) on the merge-base commit this PR sits on. dynasm/cranelift are 241/241 in the same job. |
pyre/check.py (windows-latest) |
timer quantization, not a regression | check.py subtracts a measured per-interpreter startup; Windows quantizes it to 15.625 ms. raise_catch raw times are identical on main and here (pypy 0.19s / dynasm 0.23s) yet the ratio reads 1.0x vs 2.4x purely from that subtraction. raise_catch_loop.py is a while loop with no as e and no FOR_ITER, so the new gate cannot touch it. The merge-base run fails this job too. |
Known follow-ups (not in this PR)
- The same shape in a
whileframe stays correct but is Θ(N²) — 0.98s/4.05s/16.21s at N=200/400/800 vs a linear interpreter. The compiled trace leavesExecutionContext.sys_exc_valuestale after the handler exits, so the__context__chain grows without bound and 92.5% of runtime lands in_break_context_cycle. - Root cause of the abort itself:
reads_last_exc_before_next_catchis a flat byte-stream scan that does not followgoto/Land does not stop at a block terminator, so it can read a physically-adjacent unrelated block'scatch_exceptionand clear an exception the real successor still needs. Fixing that would remove the aborts outright and make this gate unnecessary. LIST_EXTENDis missing from theFOR_ITERbody allow-list, so a ≥3-element constant list literal or any[*a]declines the whole frame (unrelated to exceptions).
— commented by Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7620f6fd27
ℹ️ 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".
| { | ||
| break; | ||
| } | ||
| Some((I::RaiseVarargs { .. }, _)) => return true, |
There was a problem hiding this comment.
This check only treats an explicit RAISE_VARARGS before the DELETE_FAST cleanup as hazardous. The same exceptional cleanup tail is reached when any admitted handler opcode raises, e.g. except ValueError as e: may_raise(e) leaves only a Call in the handler body, and Call/LoadAttr are allow-listed by the FOR_ITER body scan. Those frames now bypass CurrentFrameOnly even though they can still hit the last_exc_value cleanup abort described above, so a FOR_ITER item can be dropped when the handler call raises.
Useful? React with 👍 / 👎.
Summary
Exception-handling JIT performance work, stacked on
perf-exc.Commits
guard inlined-op arg by ob_type + w_class — a P1 rhs-guard passed a
W_TypeObjecttoGuardClass(which needs a physicalob_type); abuiltin-int RHS was then provably-false and the loop never compiled. Emit a
physical
ob_typeGuardClassplus a conditionalw_classGuardValue.decline str-override inline on nested call — a str
__str__-overrideinline admitted nested-call bodies, whose abort poisoned the whole loop. Add
exception_string_override_has_nested_callso only the single-invoke bodyinlines.
fold
__context__/__cause__exception attr reads — extendexception_attr_slot_foldsoLOAD_ATTRof__context__/__cause__on anexception folds to a guarded field read (
GetfieldGcR) instead of a residualgetset-descriptor call. Stores decline (
descr_setcausesetssuppress_context;descr_setcontexttype-validates). Removes ~3 residualcalls/iter from raising-in-
exceptloops.classify loop backedges by target dominance — the loop-header predicate
excluded a genuine loop header whenever a loop body contained a
try/except:the loop's own backedge is laid out after the handler block, so the prior
landing-pad heuristic never saw it, and same-frame raise-catch loops stayed
uncompiled. Decide by dominance instead — build a CFG (fall-through, forward/
backward jumps, exception-table edges); a backward jump is a loop backedge
iff its target dominates its source. Validated against an iterative-dominator
oracle across 8 try/except-in-loop shapes.
Effect
Same-frame raise-catch loops now compile:
vbase_big@3m JIT 0.04s vsinterp 12.86s (was JIT slower than interp);
context_chaincompiles. Thisalso unblocks commit 3's benefit (a set
__context__requires raise-in-except= the previously non-compiling shape).
Verification
pyre/check.py241/241 on both backends (dynasm + cranelift). Two newcodewriterunit tests (loop_with_try_except_body_keeps_its_loop_header,handler_rejoin_backward_jump_is_not_a_loop_header).— commented by Claude
Summary by CodeRabbit
__context__and__cause__exception attributes, preserving expected setter side effects.for-loop tracing when try/except appears in the loop body.