Repository navigation
jit: two exception-path fixes (callee is-None inline, too-long Ref image) - #980
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds indexed lazy JIT-code loading, cached ChangesIndexed lazy JIT-code runtime
Multiframe exception dispatch
Cached code names
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RuntimeLookup
participant JitCodeIndex
participant LazyJitCodeCell
participant JitCodeDecoder
RuntimeLookup->>JitCodeIndex: resolve indexed JitCode
JitCodeIndex->>LazyJitCodeCell: access requested entry
LazyJitCodeCell->>JitCodeDecoder: decode and patch body
JitCodeDecoder-->>RuntimeLookup: return JitCode
sequenceDiagram
participant SubRaise
participant CarrierFrames
participant CatchTargetLookup
participant TracebackNodes
participant RootRaiseContinuation
SubRaise->>CarrierFrames: validate middle frames
CarrierFrames->>CatchTargetLookup: resolve catch targets
CatchTargetLookup-->>SubRaise: accept or decline chain
SubRaise->>TracebackNodes: record crossed frames
SubRaise->>CarrierFrames: close frames deepest-first
SubRaise->>RootRaiseContinuation: resume root raise
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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 4559376). 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
|
![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/residual_call.rs`:
- Around line 667-690: Move BOOL_BOX_TRUTH ownership from process-global TLS
into walker state scoped to each walk lifetime. Add the association map to the
walker context/state, pass the same state through nested walks, and update
bool_box_truth_lookup and its writers, including the residual_call
from_bool_marker path, to use that state. Remove the TLS registry and preserve
existing bool-box reconstruction 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: 7fd8acf2-aaa1-4634-ab6b-302821414ce8
📒 Files selected for processing (4)
pyre/bench/synth/exception_catching_frame_tb_node.cranelift.jitstatspyre/bench/synth/exception_catching_frame_tb_node.dynasm.jitstatspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_compiled=4 | ||
| loops_compiled=3 |
There was a problem hiding this comment.
do not giveup this
![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: f466674534
ℹ️ 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".
| majit_metainterp::record_inline_application_traceback_for_recording( | ||
| exc_ptr as usize as i64, | ||
| w_code as usize as i64, | ||
| w_globals as usize as i64, | ||
| recipe.jitcode_index, | ||
| recipe.jitcode_pc, | ||
| ); |
There was a problem hiding this comment.
When a carrier exception crosses a middle frame but the subsequent root full_body_walk_trace aborts or declines, this call has already prepended a concrete traceback node. Unlike record_bridge_handler_entry_traceback, it does not add that mutation to FBW_TRACEBACK_STORE_JOURNAL, so the non-commit rollback cannot remove it and blackhole/interpreter replay can attach the same middle frame again, producing duplicate traceback entries.
Useful? React with 👍 / 👎.
| let from_bool_marker = opref | ||
| .filter(|&value| value != OpRef::NONE) | ||
| .filter(|&value| bool_box_truth_lookup(value) == Some(value)) | ||
| .and_then(|value| match ctx.trace_ctx.lookup_opref_concrete(value) { |
There was a problem hiding this comment.
This new recovery path makes BOOL_BOX_TRUTH thread-local state determine the reconstructed Ref value and therefore observable control flow after ABORT_TOO_LONG; it is no longer a disposable cache whose loss can only affect optimization. Carry the marker on the equivalent value/forwarded representation so the blackhole image does not depend on an independently reset TLS side table.
AGENTS.md reference: AGENTS.md:L148-L162
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 2946-2950: Update the effective_multiframe_depth calculation so
the raising-callee depth of 2 is clamped to fbw_max_multiframe_depth(). Preserve
the configured depth for non-raising calls and ensure the environment-configured
maximum remains an upper bound for both paths.
In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 1936-1943: Update the raise middle-frame loop around
record_carrier_crossed_frame_traceback and carrier_ec_leave so each carrier
leave also finishes and pops/advances its matching virtual-ref pair via the
established walker_ec_leave flow. Ensure subsequent iterations select the next
innermost virtual frame, allowing every middle frame to reach a recordable leaf
while preserving deepest-first ordering.
- Around line 1924-1929: Guard the middle-frame catch check in the surrounding
reconstruction logic against ReconstructRecipe.jitcode_pc being NO_JITCODE_PC
before casting it to usize. When the value is NO_JITCODE_PC, decline the drain;
otherwise preserve the existing carrier_catch_target call using the valid
offset.
🪄 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: 4b609cf2-21e4-44ab-b364-c36de6dbcfc7
📒 Files selected for processing (4)
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/trace.rs
| let effective_multiframe_depth = if contains_raise { | ||
| let bounded = crate::jitcode_dispatch::inline_chain_catches_a_raise(ctx.session) | ||
| || inline_chain_unrolls_recursion(ctx); | ||
| if bounded { 1 } else { 2 } | ||
| 2 | ||
| } else { | ||
| fbw_max_multiframe_depth() | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clamp the raising-callee depth to the configured multiframe depth.
fbw_max_multiframe_depth() reads PYRE_FBW_MULTIFRAME_DEPTH and clamps it to [1, 7]. If an operator sets that variable to 1, a raising callee still gets 2, so the raising path exceeds the configured cap while the non-raising path honours it. Take the minimum so the environment variable remains an upper bound for both paths.
🛠️ Proposed fix
let effective_multiframe_depth = if contains_raise {
- 2
+ 2.min(fbw_max_multiframe_depth())
} else {
fbw_max_multiframe_depth()
};📝 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.
| let effective_multiframe_depth = if contains_raise { | |
| let bounded = crate::jitcode_dispatch::inline_chain_catches_a_raise(ctx.session) | |
| || inline_chain_unrolls_recursion(ctx); | |
| if bounded { 1 } else { 2 } | |
| 2 | |
| } else { | |
| fbw_max_multiframe_depth() | |
| }; | |
| let effective_multiframe_depth = if contains_raise { | |
| 2.min(fbw_max_multiframe_depth()) | |
| } else { | |
| fbw_max_multiframe_depth() | |
| }; |
🤖 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 2946 -
2950, Update the effective_multiframe_depth calculation so the raising-callee
depth of 2 is clamped to fbw_max_multiframe_depth(). Preserve the configured
depth for non-raising calls and ensure the environment-configured maximum
remains an upper bound for both paths.
| if carrier_catch_target( | ||
| middle_pjc.jitcode.code.as_slice(), | ||
| middle.jitcode_pc as usize, | ||
| "middle", | ||
| ) | ||
| .is_some() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the sentinel value and whether the catch scanners bounds-check their pc argument.
set -euo pipefail
rg -nP -C4 'NO_JITCODE_PC' --type=rust
rg -nP -C25 '\bfn\s+catch_target_after_resume_live\s*\(' --type=rust
rg -nP -C25 '\bfn\s+find_catch_before_resume_live\s*\(' --type=rustRepository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "repo root:"
git rev-parse --show-toplevel
echo
echo "tracked files matching trace.rs:"
git ls-files | grep -F 'pyre/pyre-jit-trace/src/trace.rs' || true
echo
echo "search sentinel identifiers:"
rg -n "NO_JITCODE_PC|jitcode_pc|catch_target_after_resume_live|find_catch_before_resume_live|carrier_catch_target" . || true
echo
echo "outline pyre/pyre-jit-trace/src/trace.rs around relevant function:"
if [ -f pyre/pyre-jit-trace/src/trace.rs ]; then
wc -l pyre/pyre-jit-trace/src/trace.rs
rust-analyzer outline pyre/pyre-jit-trace/src/trace.rs 2>/dev/null | grep -E "pub|fn|ReconstructRecipe|carrier_catch_target|Middle" | head -200 || true
fiRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant constants:"
rg -n -C3 'NO_JITCODE_PC|type.*ReconstructRecipe|struct ReconstructRecipe|jitcode_pc:' pyre/pyre-jit-trace/src pyre/pyre-jit/src/majit_ir 2>/dev/null | head -200
echo
echo "trace.rs around carrier_catch_target caller:"
sed -n '1880,1955p' pyre/pyre-jit-trace/src/trace.rs | cat -n
echo
echo "jitcode_dispatch.rs catch scanners:"
sed -n '2940,3005p' pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs | cat -n
echo
echo "ReconstructRecipe field definitions:"
rg -n -C5 'jitcode_pc.*i32|jitcode_index.*i32' pyre/pyre-jit-trace/src pyre/pyre-jit/src/majit_ir 2>/dev/null | head -240Repository: youknowone/pyre
Length of output: 16679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "ReconstructRecipe definition:"
rg -n -C8 'pub.*struct ReconstructRecipe|type ReconstructRecipe|struct ReconstructRecipe' pyre/pyre-jit-trace/src pyre/pyre-jit/src/majit_ir 2>/dev/null | head -160
echo
echo "trace.rs lines 1880-1945:"
sed -n '1880,1945p' pyre/pyre-jit-trace/src/trace.rs | cat -n
echo
echo "jitcode_dispatch catch scanners lines 2960-3015:"
sed -n '2960,3015p' pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs | cat -n
echo
echo "NO_JITCODE_PC definition lines:"
rg -n -C4 'const NO_JITCODE_PC|NO_JITCODE_PC =|enum|pub.*mod resumedata|NO_JITCODE_PC' pyre/pyre-jit/src/majit_ir pyre/pyre-jit-trace/src/pyjitcode.rs 2>/dev/null | head -140
echo
echo "Static verifier for relevant i32-to-usize cast in carrier_catch_target call:"
python3 - <<'PY'
from pathlib import Path
p = Path('pyre/pyre-jit-trace/src/trace.rs')
text = p.read_text()
needle = 'middle.jitcode_pc as usize'
if needle in text:
idx = text.find(needle)
start = text.rfind('\n', 0, idx-200)
end = text.find('\n', idx+80)
print(text[start+1:end] if start != -1 and end != -1 else '...')
PYRepository: youknowone/pyre
Length of output: 183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All ReconstructRecipe occurrences:"
rg -n -C6 'ReconstructRecipe' .
echo
echo "NO_JITCODE_PC origin:"
rg -n -C5 'NO_JITCODE_PC.*=' pyre/pyre-jit/src pyre/pyre-jit-trace/src | head -160
echo
echo "trace.rs relevant block:"
sed -n '1880,1945p' pyre/pyre-jit-trace/src/trace.rs | cat -n
echo
echo "jitcode_dispatch catch scanners:"
sed -n '2960,3015p' pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs | cat -n
echo
echo "Static verifier of cast and function behavior:"
python3 - <<'PY'
from pathlib import Path
for path in (Path('pyre/pyre-jit-trace/src/trace.rs'), Path('pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs')):
text = path.read_text()
print(f'# {path}')
for needle in ['middle.jitcode_pc as usize', 'catch_target_after_resume_live', 'find_catch_before_resume_live']:
idx = text.find(needle)
print(f'{needle}: {idx}')
PYRepository: youknowone/pyre
Length of output: 37972
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "majit_metainterp ReconstructRecipe field definition:"
sed -n '635,665p' majit/majit-metainterp/src/trace_ctx.rs | cat -n
echo
echo "majit_metainterp NO_JITCODE_PC definition:"
rg -n -C4 'const NO_JITCODE_PC|NO_JITCODE_PC|pub mod resumedata|resumedata' majit/majit-metainterp pyre/pyre-jit/src/majit_ir 2>/dev/null | head -160
echo
echo "pyre-jit-trace catch_target_after_resume_live callers:"
rg -n -C6 'catch_target_after_resume_live|find_catch_before_resume_live' pyre/pyre-jit-trace/src/pyjitcode.rs pyre/pyre-jit-trace/src/jitcode_dispatch/pyjitcode.rs pyre/pyre-jit-trace/src 2>/dev/null | head -220
echo
echo "decode_op_at implementation:"
rg -n -C4 'fn decode_op_at|pub\(crate\) fn decode_op_at|fn can_decode_live_vars' pyre/pyre-jit-trace/src pyre/pyre-jit/src 2>/dev/null | head -220Repository: youknowone/pyre
Length of output: 16191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find resumedata module:"
git ls-files | grep -E '(^|/)resumedata\.rs$|resumedata' | head -80
echo
echo "NO_JITCODE_PC exact:"
rg -n -C3 'NO_JITCODE_PC\s*=' pyre majit/majit/src majit/majit/dependencies majit/majit/pyre-jit . 2>/dev/null | head -200
echo
echo "Reconstructed frame setup/entry uses of jitcode_pc:"
sed -n '13626,13725p' pyre/pyre-jit-trace/src/state.rs | cat -n
sed -n '2084,2135p' pyre/pyre-jit-trace/src/trace.rs | cat -n
rg -n -C5 'recipe\.jitcode_pc|mid\.jitcode_pc|middle\.jitcode_pc' majit/majit-metainterp/src pyre/pyre-jit-trace/src pyre/pyre-jit/src 2>/dev/null | head -240
echo
echo "decode_op_at definition:"
file=$(rg -l 'fn decode_op_at|pub\(crate\) fn decode_op_at' pyre/pyre-jit-trace/src pyre/pyre/src majit || true | head -1)
if [ -n "$file" ]; then
rg -n -C8 'fn decode_op_at|pub\(crate\) fn decode_op_at' "$file"
fiRepository: youknowone/pyre
Length of output: 232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "majit_ir resumedata definition:"
sed -n '1,120p' majit/majit-ir/src/resumedata.rs | cat -n
echo
echo "Reconstructed frame setup uses:"
sed -n '13626,13725p' pyre/pyre-jit-trace/src/state.rs | cat -n
sed -n '2084,2135p' pyre/pyre-jit-trace/src/trace.rs | cat -n
echo
echo "All recipe jitcode_pc reads:"
rg -n -C5 'recipe\.jitcode_pc|middle\.jitcode_pc|mid\.jitcode_pc' majit/majit-metainterp/src pyre/pyre-jit-trace/src pyre/pyre-jit/src 2>/dev/null | head -240
echo
echo "decode_op_at definition:"
rg -n -C8 'fn decode_op_at|pub\(crate\) fn decode_op_at' majit/majit-metainterp/src pyre/pyre-jit-trace/src pyre/pyre-jit/src 2>/dev/null | head -240Repository: youknowone/pyre
Length of output: 28713
Guard middle.jitcode_pc against NO_JITCODE_PC before the usize cast.
ReconstructRecipe.jitcode_pc is majit_ir::resumedata::NO_JITCODE_PC (-1) when the middle resume frame has no direct JitCode offset. Casting it to usize turns it into a huge code offset, and carrier_catch_target reads from that offset as a bytecode position. Reject -1 explicitly before the catch check and decline the drain.
🤖 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/trace.rs` around lines 1924 - 1929, Guard the
middle-frame catch check in the surrounding reconstruction logic against
ReconstructRecipe.jitcode_pc being NO_JITCODE_PC before casting it to usize.
When the value is NO_JITCODE_PC, decline the drain; otherwise preserve the
existing carrier_catch_target call using the valid offset.
9204db0 to
1ba1e12
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pyre/pyre-jit-trace/src/trace.rs (1)
1903-1946: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
middle.jitcode_pcagainstNO_JITCODE_PCbefore theusizecast.
ReconstructRecipe.jitcode_pcismajit_ir::resumedata::NO_JITCODE_PC(-1) when the decoded resume frame carries no direct JitCode offset, per the field's own documentation. The checkmiddle.jitcode_pc as usizecasts-1into a very large offset.carrier_catch_targetthen uses that offset as a bytecode position to decode ops. This can read out of bounds or select an unrelated instruction.A prior review flagged this exact cast on an earlier commit as a stability issue. That flag does not show a resolution marker, and the same unguarded cast is present in this diff. Reject
NO_JITCODE_PCbefore the cast, and decline the drain for that middle frame instead.🛠️ Proposed fix
&& middles.iter().rev().all(|middle| { let Some(middle_pjc) = crate::state::pyjitcode_for_code(middle.code_ptr) else { crate::jitcode_dispatch::census_record("P2Drain::NoMiddlePjc"); return false; }; + if middle.jitcode_pc == majit_ir::resumedata::NO_JITCODE_PC { + crate::jitcode_dispatch::census_record("P2Drain::MiddleNoJitcodePc"); + return false; + } if carrier_catch_target( middle_pjc.jitcode.code.as_slice(), middle.jitcode_pc as usize, "middle", ) .is_some() {Run the following script to confirm the sentinel value and the decoder's bounds behavior:
#!/bin/bash set -euo pipefail rg -n 'NO_JITCODE_PC' majit/majit-ir/src pyre/pyre-jit-trace/src rg -n 'fn decode_op_at' -A 20 pyre/pyre-jit-trace/src🤖 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/trace.rs` around lines 1903 - 1946, Guard middle.jitcode_pc against majit_ir::resumedata::NO_JITCODE_PC before converting it to usize in the middles_ok validation within the carrier drain logic. When the sentinel is present, record the existing decline census event and return false for that middle so the drain is rejected; only call carrier_catch_target with a valid non-sentinel PC.
🤖 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_runtime.rs`:
- Around line 72-94: Deduplicate the jitcodes.bin embedding used by
load_jitcode_index and load_jitcode. Factor the include_bytes! expansion into a
shared helper or constant, then have both load_jitcode_index and load_jitcode
reuse it while preserving the existing length assertion and JIT body loading
behavior.
In `@pyre/pyre-jit-trace/src/runtime_fnaddr_patch.rs`:
- Around line 237-254: Make address-correspondence priming unconditional in the
lazy jitcode table/loading path by invoking prime_address_correspondences()
before load_jitcode() can decode or patch any jitcode. Preserve the existing
setup calls, but ensure the lazy initialization path itself forces both
correspondence tables before resolving patch helpers or loading jitcode.
---
Duplicate comments:
In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 1903-1946: Guard middle.jitcode_pc against
majit_ir::resumedata::NO_JITCODE_PC before converting it to usize in the
middles_ok validation within the carrier drain logic. When the sentinel is
present, record the existing decline census event and return false for that
middle so the drain is rejected; only call carrier_catch_target with a valid
non-sentinel PC.
🪄 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: fced9590-290f-49ba-a2a5-286b896e0820
📒 Files selected for processing (10)
pyre/bench/synth/exception_try_call_inlined_callee_raise.cranelift.jitstatspyre/bench/synth/exception_try_call_inlined_callee_raise.dynasm.jitstatspyre/pyre-jit-trace/build.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_runtime.rspyre/pyre-jit-trace/src/runtime_fnaddr_patch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rs
The multiframe seed declined any callee containing POP_JUMP_IF_NONE / POP_JUMP_IF_NOT_NONE. Its stated premise — that the opcode lowers to an `is`/`is_not` residual whose Ref operand a mid-body guard resume cannot source — no longer holds: the codewriter emits a bare `ptr_eq`/`ptr_ne` with a `Kind::Int` result feeding the exitswitch, so there is no residual call and no boxed bool. The opcode was also never the discriminator, since LOAD_FAST_CHECK emits `ptr_nonzero/r>i` — a Ref register read straight into a branch guard, the same shape — and was never declined. What does break is narrower: a branch that leaves operands on the value stack. Its guard resume has to restore those kept slots, and inside an inline sub-walk the walk mirror does not model them, so the kept Ref reads NULL and `walker_branch_guard` raises BranchGuardUnrestorableKeptStackPermanent — a permanent abort that discards the enclosing loop trace. The scan now asks `LiveVars::stack_depth_at` instead of matching the opcode alone: the depth is taken before the instruction and the branch pops the tested value, so `depth > 1` is exactly "a kept slot survives". A statement-level `if x is None:` or `while x is not None:` reads 1 and inlines; the same test in expression position (`acc += 1 if x is None else 0`, which holds `acc` across the branch) reads 2 and keeps the residual. The new predicate is strictly narrower than the old one, so no callee that inlined before is declined now. Measured on dynasm, interleaved A/B against the same tree without this change, median of 9 pairwise ratios: exception_nested_exc_info_restore 1.72x gc_bug_bridge_flavor_traceback_names 1.32x Both callees are the statement-level shape — `classify(t)` with `if t is None:` and `chain(e)` with `while tb is not None:`. Everything else in the exception set lands inside this box's +-15% noise floor, calibrated on exception_bridge_traceback_head, whose jit-stats are identical in both arms. exception_catching_frame_tb_node compiles one loop fewer, because the callee now inlines into the caller's loop instead of taking its own function-entry trace, and its guard_failures fall 601 -> 401. Its baselines are left alone: the jit-stats floor gates a loops_compiled fall so that someone looks at it, and re-recording would spend that gate. The wasm module is byte-identical with and without this change. Assisted-by: Claude
`fill_trace_too_long_register_banks` copies all three MIFrame banks for the ABORT_TOO_LONG blackhole handoff and skips any Ref color it cannot supply a concrete for, on the premise that an unfilled color is one the interpreter does not read at that pc. That premise does not hold for the dead-`box_bool` marker. A comparison whose box `compare_box_provably_dead` proves invisible to guard resume snapshots writes its RAW TRUTH Int straight into the Ref register instead of boxing it. Neither fill arm can represent an Int in a Ref bank, so the color was skipped and the blackhole read NULL; `is_true(NULL)` is false, so a module-scope `while` took its exit arm and dropped every remaining iteration. Materialize the immortal bool singleton the box would have produced. The marker is recognisable because its sites record the truth against itself, while a genuinely boxed bool records a distinct key whose concrete is a Ref. Reached only when the module-dict NAME cell fold fires, since that is what makes the loop compare specializable, so a module carrying any `try` never showed it — including `trace_too_long_effect_replay`, whose own `try: import pypyjit` unfolds its whole body and left the phase-A contract it documents untested. With the fold reachable, its phase A read `free_total` as 15 instead of 22900 at `trace_limit=8`; 16 and above were unaffected. Also corrects the two cell-fold comments: the handler-free gate is not the can-raise argument they claimed — that argument transfers from the LoadGlobal fold, which shares the emitter and runs ungated. Lifting the gate is held back by a retrace storm instead (`exception_reraise_tb_depth_jitstress` 0.90s -> 5.85s at loops_compiled 305 -> 1802), so both gates stay. check.py dynasm 362 passed / cranelift 362 passed, the same 6 pre-existing jit-stats baselines failing as before the change; JIT-on vs JIT-off identical across all 353 synth fixtures; cargo test --all --features dynasm 97 suites ok. Assisted-by: Claude
`drive_bridge_carrier_walk`'s SubRaise arm was gated on `carrier.recipes.len() == 1`. At depth 2 or more the raise fell through with the trace already cut back, reaching `p2_drain_abort()`; jitdriver.rs then saw a bridge Abort whose op count never passed `bridge_body_start_op_count` and called `record_declined_bridge_guard`, so the guard was permanently blacklisted and every later failure of it short-circuited to a full blackhole resume. On a two-level caught-raise shape that read `mc_entered` 1203 -> 19654 with 19053 short-circuits. The arm now walks `recipes[n-2]..recipes[0]` — the frames between the raising leaf and the root — the way `pyjitpl.py finishframe_exception` pops the framestack. Per crossed frame: the `PyTraceback` node that `pyopcode.py handle_operation_error` records for every frame an exception passes through (without it `gc_bug_bridge_flavor_traceback_names` drops `mid_two` on the deopt iterations), then `carrier_ec_leave`. A middle that CATCHES has to be entered at its own handler and walked on from there, so that shape records `P2Drain::MiddleCatchesRaise` and declines. The whole chain is validated before any `carrier_ec_leave` runs, because that helper performs the concrete leave as well as recording it: a mid-chain decline that had already closed a frame would leave `topframeref` one level short for the blackhole replay the abort epilogue hands the iteration to. `carrier_root_catch_target`'s body is factored into `carrier_catch_target` so the middles ask the same question of their own jitcode. With the drain complete, `effective_multiframe_depth` is a flat 2 for any `contains_raise` chain; `inline_chain_catches_a_raise` and `inline_chain_unrolls_recursion` computed the removed distinction and are deleted. Nine interleaved reps per arm against one binary, median of pairwise ratios with a self-A/B control at 1.00x: exception_escape_caller_frame_tb_node 0.70x, gc_bug_bridge_flavor_traceback_names 1.02x, selfrec_tail_exception_unwind 0.99x. `exception_try_call_inlined_callee_raise` now inlines `shape` -> `mid` -> `leaf` as one trace, so its jit-stats move loops_compiled 3 -> 2, bridges_compiled 2 -> 1, guard_failures 408 -> 201, loops_aborted 1 -> 0, at 0.90x CPU with byte-identical output. Its baseline is NOT re-recorded: the floor gates a loops_compiled fall precisely so someone looks, and lowering it would hide the next one. `bool_box_truth_reset`'s doc claimed two reset sites; there are seven, and the ones it omitted are the sub-walk rollbacks that cut the very ops a recorded key names. Restated, with the reason a reset can only cost a fold and never mis-fold. Assisted-by: Claude
`flush_with_latched_stack` returned false on the first operand slot that did not resolve to a concrete Ref, without saying which slot or why, so a declined `ForceQuasiImmutable` resume was indistinguishable from one that never latched a stack. Report slot index, OpRef and concrete value under `PYRE_FBW_DEBUG_ABORT`, matching the neighbouring `[fbw-flush] DECLINE` line. Rewrite the LoadName cell-fold gate comment. Its retrace-storm numbers (`exception_reraise_tb_depth_jitstress` 0.90s -> 5.85s at loops_compiled 305 -> 1802) were measured before the walk aborted at the namespace write; re-measured with the gate lifted, that fixture and `exception_reraise_tb_depth_hot` both sit at 1.00x. What lifting the gate does reach is a dropped iteration: the fold installs the module dict's `version?` watcher, a later bumping write abandons the walk, the mid-expression flush declines on an unbound mirror slot, and the legacy leg then refuses to re-deliver the in-flight FOR_ITER item — `bench/synth/pickle_terminal_raise_resume` prints 214 under the JIT against 216 interpreted. Record that, and what the fold is worth (1.87us -> 1.32us per iteration over 200k iterations of `bench/synth/exc_info_module_loop_hot`). Assisted-by: Claude
`a03b89c08a7` narrowed the multiframe seed's decline of a callee containing POP_JUMP_IF_NONE / POP_JUMP_IF_NOT_NONE to fire only at `LiveVars::stack_depth_at(pc) > 1`. It miscompiles the unpickler: `test.test_descr` and `test.test_pickletools` both go PASS -> FAIL in the CPython suite gate, the first as `pickle._loads` -> `load_newobj_ex` "Value after * must be an iterable, not int", the second as five read-position errors out of `pickletools._genops`. Bisected to that commit alone; reverse -applying its hunk makes both pass again. The depth rule covers only one of the two hazards the old comment named. The other — when the multiframe inline int-specializes the tested local, the mid-body guard resume cannot source that operand's Ref form from the callee register banks, the encoded liveness stream disagrees with the decoder and the caller frame is corrupted — is about the branch's own operand, not about what the branch keeps, so it does not depend on stack depth at all. A statement-level `if x is None:` reads depth 1 and now inlines, which is exactly the shape that carries it. The measured win it was buying is real (exception_nested_exc_info_restore 1.72x, gc_bug_bridge_flavor_traceback_names 1.32x), so the gate is restored as it was rather than replaced, and redoing it soundly is left to the resume-side rebox the original comment already named. `exception_catching_frame_tb_node` returns to its committed loops_compiled=4, and the wasm loops_aborted rises on exception_inline_callee_tb_frames, exception_raise_caught_same_frame_tb and exception_traceback_lineno_chain go away with it. Assisted-by: Claude
`201889611f2` made the fixture inline `shape` -> `mid` -> `leaf` as one trace and left its baselines alone so the loops_compiled floor would force a look. The look confirms an improvement on every counter it moves: loops_compiled 3 -> 2 and bridges_compiled 2 -> 1 because a callee that needed its own function-entry trace now folds into the caller's, alongside loops_aborted 1 -> 0 and guard_failures 408 -> 201, at 0.90x CPU with byte-identical output. Measured on both backends, which agree field for field. A later fall to 1 is still gated. Assisted-by: Claude
`jitcodes.bin` was one bincode blob holding all 2042 `Arc<JitCode>` entries, deserialized in a single pass the first time anything touched `all_jitcodes()`. Since that first touch is the first JIT compile, a program that compiles one loop paid for the whole build-time universe, including every jitcode it never reaches. build.rs now emits the same entries individually concatenated, plus a `jitcodes_index.bin` carrying the names and the byte offsets that delimit them. The runtime holds a leaked `[OnceCell<Arc<JitCode>>]` and decodes an entry the first time its index is looked up. The three runtime patch passes and the `all_jitcodes[i].index == i` assertion move from whole-table to per-entry, still running while the `Arc` refcount is 1 and before the cell publishes it. Queries that do not need a body no longer decode one: `compute_named_jitcode_index` scans the index's names and `jitcode_count` (replacing `all_jitcodes().len()` in `reserve_build_time_index_space`) reads the offset count. `all_jitcodes()` remains, forcing the full table, and is now only reached from tests. The two build -> runtime address correspondences the patch passes consult are cached, and `reserve_build_time_index_space` now forces both through `prime_address_correspondences`. Without that the caches would be filled at whatever moment the first body happens to decode, which once bodies are lazy is mid-trace, after `init_typeobjects` has retagged the `PyType` singletons. Every later-decoded body would then have its `constants_r` patched against that stale view, and those constants reach a compiled loop's gc_table, so the collector drags a bad root out of it: `GC BUG: invalid type_id`, site `minor_root_target`, reproduced on `synth/gc_bug_bridge_flavor_traceback_names` at 19/45 runs across three nursery sizes where the whole-table loader was 0/45. Forcing them from `reserve_build_time_index_space` restores the instant the whole-table loader took its snapshot, and that fixture is 0/70 over the same sizes. `materialize_str_consts`'s local `interned` map is now per entry rather than per table. That does not split any identity: it is a fast path over `box_str_constant`, which interns by content in a process-wide table, so two jitcodes sharing a literal still resolve to one immortal block. A test pins that across separate calls, and a second test pins that a name query decodes zero bodies. `CODEGEN_CACHE_VERSION` v4 -> v5, since the artifact set gained a file and `jitcodes.bin` changed shape. Measured on dynasm, `scratchpad/oh_3000.py` (compiles one loop) against `oh_3.py` (never reaches the threshold), so the difference is the first-compile step. Peak RSS attributable to that step falls 32.3 MiB -> 25.7 MiB (min of 7 runs each). Wall clock on the compiling script is a -1.97ms median pairwise difference against a +2.84ms self-A/B control on the same script, and -0.34ms on the non-compiling script, i.e. the win is confined to the arm that compiles. This box's noise floor spans ~10ms at p25/p75, so the RSS figure is the tighter of the two. Assisted-by: Claude
`349b86f5e64` reverted the is-None inline widening because it was wrong-code, restoring a blanket decline of any callee containing `POP_JUMP_IF_NONE` / `POP_JUMP_IF_NOT_NONE`. A `while tb is not None:` helper is exactly that shape, so the whole traceback cluster went back to failing to trace. The decline is now conditional on three arms, each naming a distinct mechanism rather than the opcode: 1. Kept operands. A branch that leaves slots on the value stack needs its guard resume to restore them, and the inline sub-walk's mirror does not model them, so the kept Ref reads NULL and `walker_branch_guard` raises `BranchGuardUnrestorableKeptStackPermanent` — a permanent abort that discards the enclosing loop. `stack_depth_at` is the depth before the instruction and the branch pops the tested value, so `depth > 1` is exactly "a kept slot survives". 2. The tested operand itself. When the multiframe inline int-specializes the tested local, the mid-body guard resume cannot source that operand's Ref form from the callee register banks, so the encoded liveness stream disagrees with the decoder and the caller frame is corrupted. This is independent of kept depth: a statement-level `if x is None:` reads depth 1 and still carries it. Bindings are re-classified through `ConcreteValue::from_pyobj` rather than matched on the variant, because a default argument is pushed as a raw `Ref` that never went through that classification — `_read_from_buffer(self, size=-1)` in `_io_app.py` reaches the scan with `size` as `Ref(<int object>)` and is the shape that miscompiled. 3. wasm keeps the blanket decline. Tracing into the helper closes the enclosing trace with a CALL_ASSEMBLER into the helper's own loop, and the wasm backend declines every CALL_ASSEMBLER — each trace is its own module with no inter-module chaining. That decline is the `Err` arm of the compile step, so the enclosing loop is aborted rather than merely left interpreted: `loops_aborted` 0 -> 3 on `synth/exception_traceback_lineno_chain`, 0 -> 5 on `synth/exception_inline_callee_tb_frames`, 0 -> 1/2/3 on three more, each measurably slower than declining (min-of-5 wall clock against a module rebuilt with the widening reverted). `exc_edge_bridge_enabled` and `bridge_rec_root_selfrec` carve wasm out the same way. `synth/exception_catching_frame_tb_node`'s jit-stats baseline is re-recorded on both native backends: `loops_compiled` 4 -> 3 and `bridges_compiled` 3 -> 2, because the helper is now inlined into the caller instead of compiled as its own loop, and `guard_failures` 601 -> 401. Every badness counter stays at 0. Gates: check.py dynasm 370/370, cranelift 370/370, wasm 366/366; pyre-jit-trace 339 unit tests; `test.test_descr` and `test.test_pickletools` — the two oracles for the reverted miscompile — both report no regressions. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/440f40b975a67fbfaaf58bf1bd5b61a1226634e6/pyre-interpreter/src/pycode.rs#L771
For ordinary dynamically created code objects, this cache changes observable PyPy behavior: pypy/interpreter/typedef.py:716 exposes co_name through wrapfn="newtext", and pypy/objspace/std/objspace.py:409-417 constructs a fresh W_UnicodeObject unless the source string is an annotation-time constant. Returning the cached w_name instead makes f.__code__.co_name is f.__code__.co_name true rather than false, so retain the upstream per-read wrapping behavior instead of adding persistent wrapped-object identity.
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".
There was a problem hiding this comment.
Actionable comments posted: 1
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_runtime.rs (1)
36-70: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftCheck whether
get_jitcode_by_index()is intentionally process-global.
JITCODE_CELLS,FORCED_ALL_JITCODES,COMPILED_JIT_DRIVERS, andINDIRECTCALLTARGETSare process-wide state, butget_jitcode_by_index()andindirectcalltargets()currently hand out thread-localArc<JitCode>allocations. That diverges from RPython's process-widemetainterp_sd.jitcodesidentity semantics and can break any future cross-thread identity comparison. If JIT dispatch must be single-threaded, add the required written justification; otherwise make this table shared/synchronized.🤖 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_runtime.rs` around lines 36 - 70, Determine whether get_jitcode_by_index() and indirectcalltargets() must preserve process-wide JitCode identity like RPython’s metainterp_sd.jitcodes. If cross-thread identity is required, replace the thread-local storage and initialization with shared synchronized state so all callers receive the same Arc<JitCode> instances; otherwise document the single-threaded dispatch invariant and why thread-local allocations are intentional.Source: Coding guidelines
🤖 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 3465-3523: The is/None hazard scan in the seed block should cache
its callee-invariant results per jitcode instead of recomputing them for every
inline attempt. Extend the existing sub-jitcode body-facts cache used by
sub_jitcode_body_facts_for_code to store the decoded branch/liveness hazards for
kept-stack depth and wasm, then reuse those facts here; keep
callee_binds_unboxed_local evaluated at the call site and combine it with the
cached hazards before applying the existing try_multiframe and seed behavior.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_runtime.rs`:
- Around line 36-70: Determine whether get_jitcode_by_index() and
indirectcalltargets() must preserve process-wide JitCode identity like RPython’s
metainterp_sd.jitcodes. If cross-thread identity is required, replace the
thread-local storage and initialization with shared synchronized state so all
callers receive the same Arc<JitCode> instances; otherwise document the
single-threaded dispatch invariant and why thread-local allocations are
intentional.
🪄 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: 8bdb2c2b-b3fe-4012-98d0-baee04b380e5
📒 Files selected for processing (14)
pyre/bench/synth/exception_catching_frame_tb_node.cranelift.jitstatspyre/bench/synth/exception_catching_frame_tb_node.dynasm.jitstatspyre/bench/synth/exception_try_call_inlined_callee_raise.cranelift.jitstatspyre/bench/synth/exception_try_call_inlined_callee_raise.dynasm.jitstatspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/pycode.rspyre/pyre-jit-trace/build.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_runtime.rspyre/pyre-jit-trace/src/runtime_fnaddr_patch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rs
| if bound_method.is_none() { | ||
| let liveness = crate::liveness::liveness_for(raw_callee_code); | ||
| let has_is_none_branch = (0..callee_code.instructions.len()).any(|pc| { | ||
| matches!( | ||
| pyre_interpreter::decode_instruction_at(callee_code, pc), | ||
| Some(( | ||
| pyre_interpreter::bytecode::Instruction::PopJumpIfNone { .. } | ||
| | pyre_interpreter::bytecode::Instruction::PopJumpIfNotNone { .. }, | ||
| _ | ||
| )) | ||
| ) && ( | ||
| // Hazard 3 — the wasm backend cannot run the widened | ||
| // shape at all, so it keeps the blanket decline. | ||
| // Admitting a callee here traces into its body, and the | ||
| // helper this widening exists for carries its own loop | ||
| // (`while tb is not None:`), so the enclosing trace | ||
| // closes with a CALL_ASSEMBLER into that loop. Every | ||
| // wasm trace is its own module and there is no | ||
| // inter-module chaining, so the backend declines any | ||
| // CALL_ASSEMBLER outright; that decline is the `Err` | ||
| // arm of the compile step, so the enclosing loop is | ||
| // aborted rather than merely left interpreted. | ||
| // Measured `loops_aborted` 0 -> 3 on | ||
| // `synth/exception_traceback_lineno_chain` and 0 -> 5 | ||
| // on `synth/exception_inline_callee_tb_frames`, each | ||
| // ~5-9% slower than declining. Drop this arm once the | ||
| // backend can chain modules. | ||
| cfg!(target_arch = "wasm32") | ||
| // Hazard 1 — kept operands. A branch that leaves slots | ||
| // on the value stack needs its guard resume to restore | ||
| // them, and the inline sub-walk's mirror does not model | ||
| // them, so the kept Ref reads NULL and | ||
| // `walker_branch_guard` raises | ||
| // BranchGuardUnrestorableKeptStackPermanent — a | ||
| // permanent abort that discards the enclosing loop | ||
| // trace. `stack_depth_at` is the depth BEFORE the | ||
| // instruction and the branch pops the tested value, so | ||
| // `depth > 1` is exactly "a kept slot survives". An | ||
| // unreachable pc has no depth and cannot fire a guard. | ||
| || liveness.stack_depth_at(pc).is_some_and(|depth| depth > 1) | ||
| // Hazard 2 — the tested operand itself. When the | ||
| // multiframe inline int-specializes the tested | ||
| // local, the mid-body guard resume cannot source | ||
| // that operand's Ref form from the callee register | ||
| // banks (`collect_callee_active_boxes` reads a | ||
| // stale/mismatched box), so the encoded liveness | ||
| // stream disagrees with the decoder and the caller | ||
| // frame is corrupted. This is independent of kept | ||
| // depth: a statement-level `if x is None:` reads | ||
| // depth 1 and still carries it. | ||
| || callee_binds_an_unboxed_local | ||
| ) | ||
| }) | ||
| { | ||
| if try_multiframe { | ||
| return Ok(None); | ||
| }); | ||
| if has_is_none_branch { | ||
| if try_multiframe { | ||
| return Ok(None); | ||
| } | ||
| break 'seed; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Cache the static half of the is/is not None hazard scan.
The instruction/liveness scan at 3467-3517 re-decodes callee_code.instructions and recomputes crate::liveness::liveness_for(raw_callee_code) on every call to this inline route that reaches the seed block. Hazards 1 and 3 (kept-stack depth, wasm) do not depend on the call site and are candidates for the same per-jitcode OnceCell cache sub_jitcode_body_facts_for_code already uses for contains_raise, has_abort_permanent, etc. Only hazard 2 (callee_binds_an_unboxed_local) needs to stay call-site scoped. For a hot call site inside a loop, this avoids repeating an O(body size) scan on every inline attempt.
🤖 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 3465 -
3523, The is/None hazard scan in the seed block should cache its
callee-invariant results per jitcode instead of recomputing them for every
inline attempt. Extend the existing sub-jitcode body-facts cache used by
sub_jitcode_body_facts_for_code to store the decoded branch/liveness hazards for
kept-stack depth and wasm, then reuse those facts here; keep
callee_binds_unboxed_local evaluated at the call site and combine it with the
cached hazards before applying the existing try_multiframe and seed behavior.
`co_qualname` has held its wrapped string on the code object since it was added, on the argument that `function.py:54 self.qualname = qualname or self.name` and the getter hand out one immutable value, so realizing it once reproduces that identity instead of minting a `W_UnicodeObject` per read. `co_name` sits directly beside it in the same getter match and had no such slot: every read ran `w_str_new`, which is two allocations, a `String` copy and a full `chars().count()`. The identical argument applies to it — `function.py:51 self.name = forcename or code.co_name` — so give it the identical treatment: a `w_name` slot realized on first demand by `w_code_name_obj`, a verbatim sibling of `w_code_qualname_obj`. `W_TypeObject` already carries the same `w_name`/`w_qualname` pair. Storage, immortality and root-walk handling follow `w_qualname` exactly: `PY_NULL` until realized, `malloc_typed`-immortal so the stored pointer is fixed, and forwarded by `walk_raw_code_roots` beside its sibling. `code_replace` clones the `CodeObject` and builds a fresh wrapper, so a replaced `co_name` cannot read through a stale cache — the same reason the existing qualname cache is sound. Measured on a traceback walk (`tb.tb_frame.f_code.co_name` per hop), against `co_argcount` in the same process as an untouched control, because this box's absolute timings move too much between runs to compare directly: `co_name`'s excess over that control falls from +28.5% to +18.1%. The remainder is the residual `getattr` itself, which costs ~133 ns/hop on any `PyCode` attribute — `co_firstlineno` and `co_argcount` measure the same — and is a separate fold. This change is what makes that fold possible, by giving `co_name` a `PyObjectRef` slot and a `CODE_W_NAME_OFFSET` for a descr to name. Gates: check.py dynasm 371/371, cranelift 371/371 and wasm 367/367. `test.test_code`, `test.test_funcattrs`, `test.test_descr` and `test.test_traceback` all report no regressions. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/455937634f75ac984e7c2d4b2fd3b0b507ee68ed/pyre-jit-trace/src/trace.rs#L1631-L1637
When an inlined middle frame has observable state (for example, it saved sys._getframe() or has locals later inspected through tb_frame.f_locals), this callback reaches record_inline_traceback_for_recording, which creates a fresh frame from only w_code and w_globals. The traceback therefore contains a different frame with unpopulated locals instead of the reconstructed middle frame; previously this depth declined to blackhole/interpreter replay, which preserved that frame. Thread the recipe's per-frame red frame through this unwind and attach the node to it rather than using the fabricating hook.
AGENTS.md reference: AGENTS.md:L32-L42
ℹ️ 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".
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-interpreter/src/pycode.rs`:
- Around line 322-329: Add an alignment check after obtaining code_ptr in both
the current accessor and w_code_qualname_obj, rejecting null or misaligned
pointers before dereferencing CodeObject fields. Use CodeObject’s alignment
requirement with the specified bitmask check, and return the existing null
result for invalid pointers while preserving normal name and qualified-name
handling for valid ones.
🪄 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: 5114f221-bc26-4630-9a2d-f16dcce4d33f
📒 Files selected for processing (2)
pyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/pycode.rs
| let code_ptr = w_code_get_ptr(w_code) as *const crate::CodeObject; | ||
| if code_ptr.is_null() { | ||
| return pyre_object::PY_NULL; | ||
| } | ||
| // `w_str_new` is a collection point, but the wrapper is Box-immortal | ||
| // and so never relocates; the fresh string is stored through the | ||
| // still-valid `w_code` before any further allocation can sweep it. | ||
| let w_name = w_str_new(&(*code_ptr).obj_name); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 '\bfn w_code_get_ptr\b|\bw_code_get_ptr\s*\(' \
pyre/pyre-interpreter/src
rg -n -C 8 'w_code_name_obj|w_code_qualname_obj|co_name|co_qualname|code_ptr_aligned' \
pyre/pyre-interpreter/srcRepository: youknowone/pyre
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pycode.rs relevant sections =="
sed -n '200,340p' pyre/pyre-interpreter/src/pycode.rs
echo
sed -n '660,705p' pyre/pyre-interpreter/src/pycode.rs
echo
sed -n '1640,1700p' pyre/pyre-interpreter/src/pycode.rs
echo
echo "== PyCode struct and code_ptr constructors =="
rg -n -C 6 'struct PyCode|code_ptr:|w_code_new\b|w_code_new_with_hidden_applevel\(' pyre/pyre-interpreter/src/pycode.rs pyre/pyre-interpreter/src/error.rs pyre/pyre-interpreter/src/function.rs
echo
echo "== precise call sites of w_code_get_ptr in pycode.rs with immediate deref/alignment =="
python3 - <<'PY'
from pathlib import Path
p=Path('pyre/pyre-interpreter/src/pycode.rs')
lines=p.read_text().splitlines()
for i,l in enumerate(lines,1):
if 'w_code_get_ptr' in l:
snippet='\n'.join(f'{j}: {lines[j-1]}' for j in range(max(1,i-2), min(len(lines), i+5)+1))
print(f'--- pycode.rs:{i} ---')
print(snippet)
PYRepository: youknowone/pyre
Length of output: 29485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== w_code_new_with_hidden_applevel implementation =="
sed -n '440,545p' pyre/pyre-interpreter/src/pycode.rsRepository: youknowone/pyre
Length of output: 5321
Guard code_ptr alignment before reading obj_name.
w_code_new_with_hidden_applevel stores wrapped co_name table values when code_ptr is non-null but misaligned, so this accessor should not assume any non-null result is a valid CodeObject. Check (code_ptr as usize) & align_of::<crate::CodeObject>() - 1 == 0 before &(*code_ptr).obj_name, and apply the same guard in w_code_qualname_obj.
🤖 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/pycode.rs` around lines 322 - 329, Add an alignment
check after obtaining code_ptr in both the current accessor and
w_code_qualname_obj, rejecting null or misaligned pointers before dereferencing
CodeObject fields. Use CodeObject’s alignment requirement with the specified
bitmask check, and return the existing null result for invalid pointers while
preserving normal name and qualified-name handling for valid ones.
… for the new base The rebase onto origin/main put #980 under this branch, and both sides had moved this fixture's recorded jit-stats. Neither side's number describes the combination, so the value here was measured on the rebased tree rather than merged: | | loops | bridges | guards | |---|---|---|---| | old base | 3 | 2 | 402 | | this branch, on the old base | 3 | 3 | 604 | | new base (#980) | 2 | 1 | 201 | | measured on the rebase | 2 | 2 | 405 | The branch's effect on this fixture is the same on either base — one more bridge and ~203 more guard failures, with `loops_compiled` unchanged — so the recorded move is that effect re-expressed, not a new regression. check.py: dynasm 372/372, cranelift 372/372. Assisted-by: Claude
Two JIT fixes found while attributing the slowest exception benchmarks against pypy.
jit: key the callee is-None inline decline on the kept stackThe multiframe seed declined any callee containing
POP_JUMP_IF_NONE/POP_JUMP_IF_NOT_NONE. Its stated premise — a residualis/is_notwhose Refoperand a mid-body guard resume cannot source — no longer holds: the codewriter
emits a bare
ptr_eq/ptr_newith aKind::Intresult feeding the exitswitch.LOAD_FAST_CHECKemits the same shape (ptr_nonzero/r>i) and was never declined.What actually breaks is narrower: a branch that leaves operands on the value
stack, whose guard resume must restore kept slots the inline sub-walk's mirror
does not model. The scan now asks
LiveVars::stack_depth_atinstead of matchingthe opcode, so a statement-level
if x is None:inlines while the same test inexpression position keeps the residual. Strictly narrower than the old predicate.
Measured on dynasm, interleaved A/B, median of 9 pairwise ratios:
exception_nested_exc_info_restore1.72x,gc_bug_bridge_flavor_traceback_names1.32x. Everything else lands inside this box's ±15% noise floor.
jit: fill the dead-box_bool Ref color in the too-long imageLatent wrong-code on main.
fill_trace_too_long_register_banksskips any Refcolor it cannot supply a concrete for, on the premise that an unfilled color is
one the interpreter does not read at that pc. That fails for the dead-
box_boolmarker, which writes a comparison's raw truth Int straight into the Ref register:
neither fill arm can represent an Int in a Ref bank, so the color was skipped, the
blackhole read NULL, and
is_true(NULL)sent a module-scopewhileto its exitarm after one iteration.
Reachable only when the module-dict NAME cell fold fires, so a module carrying
any
trynever showed it — includingtrace_too_long_effect_replay, whose owntry: import pypyjitunfolds its whole body and left the phase-A contract itdocuments untested.
Also corrects both cell-fold comments: the handler-free gate is not the
can-raise argument they claimed (that argument transfers from the ungated
LoadGlobal fold, which shares the emitter). Lifting it is held back by a retrace
storm instead —
exception_reraise_tb_depth_jitstress0.90s → 5.85s atloops_compiled 305 → 1802 — so both gates stay.
Verification
check.py --backend dynasm362 passed /--backend cranelift362 passed, with the same 6 pre-existing jit-stats baseline failures as before the changecargo test --all --features dynasm97 suites ok;cargo test -p pyre-objectokSummary by CodeRabbit
Bug Fixes
Performance
Documentation