jit(fbw): drop the unwired PendingInlineFrame vref residue; record _MULTIFRAME's measured corpus coverage - #795
Conversation
…ULTIFRAME's measured corpus coverage `PendingInlineFrame::drop_frame_opref` was constructed at one site, always `None`, and never read. Its comment said `None` "skips the opimpl_virtual_ref emission in push_inline_frame"; `push_inline_frame` (`majit-metainterp/src/compile.rs`) is a depth guard that pushes a `(code_ptr, pc)` greenkey and contains no vref code, and the emission the comment names was never built. `InlineTraceStepAction` — the enum that would have carried a `PendingInlineFrame` into such a push — is declared and referenced nowhere in the tree. Remove both. Three comments corrected against the call graph rather than restated: `MetaInterpreter::push_inline_frame` (the identifier `MetaInterpreter` occurs exactly once in the tree, inside that comment); `opimpl_virtual_ref`'s "Called from metainterp push_inline_frame" (its only callers, in both the `majit-metainterp` and `pyre-jit-trace` ports, are `#[test]`, so `virtualref_boxes` stays empty and no live trace records a VIRTUAL_REF); and `push_inline_frame stamps parent_frames.first().pending_result_*` (`pending_result_stack_idx` is never assigned a `Some` value anywhere). gate-triage §1 records a measurement of the multi-frame path: the vable-escape latch site was instrumented and all 318 benchmarks (`pyre/bench` + `pyre/bench/synth`) run under `PYRE_FBW_MULTIFRAME=1`. The site is reached in 3 benches, 5 events each, and all 15 have `inline_subwalk=false` — every one takes the single-frame arm. `build_multi_frame_miframe` is never called, so the image is never latched and `try_adopt_multi_frame_blackhole` never sees a candidate. §4's `_BLACKHOLE_RESUME` row is amended: its retirement was conditioned on `_MULTIFRAME` landing, which that measurement makes unevaluable. Gates from the workspace root: `pyre-jit-trace` + `majit-metainterp` unit tests green (1399 / 294 / …), `check.py --backend dynasm` 313/313. Assisted-by: Claude
WalkthroughThe PR documents multi-frame blackhole coverage and updates related gate decisions. It also simplifies inline pending-frame state by removing ChangesJIT trace and gate updates
Estimated code review effort: 2 (Simple) | ~10 minutes 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 ade259b). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptationsNone. |
![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: ade259b614
ℹ️ 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".
| vable-escape latch site was instrumented and all **318** benchmarks | ||
| (`pyre/bench` + `pyre/bench/synth`) run under `PYRE_FBW_MULTIFRAME=1`. The site |
There was a problem hiding this comment.
This 318-case measurement matches the pre-rebase tree (7b70e1ebff: 299 top-level synthetic scripts plus 19 regular scripts), but the reviewed tree contains 307 top-level synthetic scripts plus the same 19 regular scripts. Eight newly added cases—several involving inlined callees and frame/exception handling—were therefore not measured, so the conclusions that _MULTIFRAME has no coverage and is a corpus-wide no-op are not established for this commit. Rerun the instrumentation on the current base or explicitly qualify the ledger as a pre-rebase result.
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
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/state.rs (1)
12165-12167: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale
drop_frame_oprefreference.That field was deleted from
PendingInlineFrame, so this comment now documents an impossible requirement. Refer to the current replay fields or describe the actual gate instead.Proposed comment update
- // back-edge CALL_ASSEMBLER path requires drop_frame_opref and is gated out for them anyway. + // Reconstructed frames carry no CALL-site OpRefs, so the inline + // back-edge CALL_ASSEMBLER path remains gated out for them.🤖 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/state.rs` around lines 12165 - 12167, Update the comment near the reconstructed-frame handling to remove the obsolete drop_frame_opref reference. Describe the current replay fields or the actual gate that excludes reconstructed frames from the inline back-edge CALL_ASSEMBLER path, without implying PendingInlineFrame still contains that deleted field.
🤖 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/gate-triage.md`:
- Line 207: Replace the nonstandard term “unvalidatable” in the affected
sentence with “not independently validated” or “unverifiable,” while preserving
the sentence’s meaning and surrounding wording.
- Around line 198-212: Update the benchmark coverage section documenting the
318-benchmark measurement to include the exact command used, repository
revision, relevant environment variables (including PYRE_FBW_MULTIFRAME,
PYRE_FBW_DEBUG_ABORT, and _BLACKHOLE_RESUME), and a reference to the preserved
output artifact. Ensure the provenance is sufficient to independently reproduce
and audit the reported results.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/state.rs`:
- Around line 12165-12167: Update the comment near the reconstructed-frame
handling to remove the obsolete drop_frame_opref reference. Describe the current
replay fields or the actual gate that excludes reconstructed frames from the
inline back-edge CALL_ASSEMBLER path, without implying PendingInlineFrame still
contains that deleted field.
🪄 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: 3c97bed4-8dc1-484d-8749-40da33055cbf
📒 Files selected for processing (2)
pyre/gate-triage.mdpyre/pyre-jit-trace/src/state.rs
| **Measured 2026-07-25: the multi-frame path has no corpus coverage.** The | ||
| vable-escape latch site was instrumented and all **318** benchmarks | ||
| (`pyre/bench` + `pyre/bench/synth`) run under `PYRE_FBW_MULTIFRAME=1`. The site | ||
| is reached in **3 benches** (`getframe_escape_flush_writethrough_regression`, | ||
| `synth/getframe_inlined_callee_own_frame`, `synth/getframe_stored_fback_walk`), | ||
| 5 events each, and **all 15 have `inline_subwalk=false`** — every one takes the | ||
| single-frame arm and adopts. `build_multi_frame_miframe` is therefore never | ||
| called, the image is never latched, and the adopt never sees a candidate. So | ||
| flipping `_MULTIFRAME` ON is a no-op across the corpus, none of the three items | ||
| above is exercised, and any port of them would be unvalidatable until a | ||
| benchmark that reaches `inline_subwalk=true` at a vable escape exists. Building | ||
| that benchmark is the prerequisite for the rest. Note the multi-frame latch is | ||
| nested inside `single_frame_blackhole_resume_enabled()`, so it also requires | ||
| `_BLACKHOLE_RESUME` to stay ON. The pre-existing `[s2-gate]` eprintln (under | ||
| `PYRE_FBW_DEBUG_ABORT`) already reports `inline_subwalk` at that site. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial
Record reproducible benchmark provenance.
These measurements determine whether _MULTIFRAME and _BLACKHOLE_RESUME can be retired. Add the exact command, revision, environment, and output artifact so the 318-benchmark result can be independently re-audited.
🧰 Tools
🪛 LanguageTool
[grammar] ~207-~207: Ensure spelling is correct
Context: ...three items above is exercised, and any port of them would be unvalidatable until a ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~208-~208: Ensure spelling is correct
Context: ...that reaches inline_subwalk=true at a vable escape exists. Building that benchmark...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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/gate-triage.md` around lines 198 - 212, Update the benchmark coverage
section documenting the 318-benchmark measurement to include the exact command
used, repository revision, relevant environment variables (including
PYRE_FBW_MULTIFRAME, PYRE_FBW_DEBUG_ABORT, and _BLACKHOLE_RESUME), and a
reference to the preserved output artifact. Ensure the provenance is sufficient
to independently reproduce and audit the reported results.
| single-frame arm and adopts. `build_multi_frame_miframe` is therefore never | ||
| called, the image is never latched, and the adopt never sees a candidate. So | ||
| flipping `_MULTIFRAME` ON is a no-op across the corpus, none of the three items | ||
| above is exercised, and any port of them would be unvalidatable until a |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace nonstandard wording.
Use “not independently validated” or “unverifiable” instead of unvalidatable.
🧰 Tools
🪛 LanguageTool
[grammar] ~207-~207: Ensure spelling is correct
Context: ...three items above is exercised, and any port of them would be unvalidatable until a ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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/gate-triage.md` at line 207, Replace the nonstandard term
“unvalidatable” in the affected sentence with “not independently validated” or
“unverifiable,” while preserving the sentence’s meaning and surrounding wording.
Source: Linters/SAST tools
|
Gate update on the current base ( Gates — green
A first attempt at this re-gate failed differently (two An Superseding finding: the multi-frame path IS reachableThis PR's The §1 text also says there are declines to clear before the adopt can fire, and Five candidate programs, all sharing one shape — a Minimal form: import sys
_gf = sys._getframe
def leaf(x):
_gf()
return x + 1
def main():
total = 0
i = 0
while i < 30000:
total = leaf(total)
i = i + 1
return total
print(main())Three things this establishes:
Why every existing bench misses: all five A depth-3 variant ( Locating the adopt-side decline is in progress. The follow-up commit will carry — commented by Claude |
|
Follow-up to the previous comment: the adopt-side decline is located, and it is The decline
if per_frame.first().map(|&(frame_ptr, _)| frame_ptr) != Some(cf_addr as i64) {
return false;
}The comment above it attributes the decline to "a chain rooted at an
let cf_addr = &*concrete_frame as *const PyFrame as usize;
// "The snapshot stands in for concrete stepping only"
sym.set_live_vable_frame_addr(live_frame_addr);
// "gap 10 slice 2b: set this BEFORE `init_symbolic` so the root vable identity
// (seed_virtualizable_boxes) is baked against the live frame address, not the
// discarded snapshot's."The root vable identity that lands in the blackhole's frame register is the What this means for the ledger text in this PRTwo things in the §1 paragraph this PR adds are wrong and I will correct them in
What the paragraph got right and should keep: the multi-frame path has no corpus Not fixing it in this PRSwapping the comparison to the live address is a one-line change but not A depth-3 variant ( Gate status unchanged from the previous commentUnit tests pass; — commented by Claude |
1 participant
One commit: delete FBW walker residue that a call-graph search shows is
unreachable, correct three comments that name mechanisms which do not exist,
and record a measurement of the multi-frame blackhole path in the gate ledger.
Dead state
PendingInlineFrame::drop_frame_oprefis constructed at exactly one site,always
None, and never read. Its comment saidNone"skips theopimpl_virtual_ref emission in push_inline_frame".
push_inline_frame(
majit-metainterp/src/compile.rs) does exist, but it is a depth guard — itpushes a
(code_ptr, pc)greenkey ontoinline_framesand returns bool. Ittakes no
PendingInlineFrame, mutates no framestack, and contains no vrefcode. The emission the comment names was never built.
InlineTraceStepAction— the enum that would have carried aPendingInlineFrameinto such a push — is declared and referenced nowhere inthe tree. Both go.
Comments corrected against the call graph
MetaInterpreter::push_inline_frame: the identifierMetaInterpreteroccursexactly once in the whole tree, inside that comment. There is no such type.
opimpl_virtual_ref's "Called from metainterp push_inline_frame": its onlycallers, in both the
majit-metainterpand thepyre-jit-traceport, are#[test].virtualref_boxesis therefore always empty and no live tracerecords a
VIRTUAL_REF. The replacement says that, and names upstream's realcaller (
executioncontext.py:89 enter, which the tracer reaches by tracingthe interpreter's own frame-entry code — something the pyre walker, which
builds its inline levels itself, never does).
push_inline_framestampsparent_frames.first().pending_result_*":pending_result_stack_idxis never assigned aSomevalue anywhere in thetree.
Measurement recorded in gate-triage §1
The multi-frame blackhole path has no corpus coverage. The vable-escape latch
site was instrumented and all 318 benchmarks (
pyre/bench/*.py+pyre/bench/synth/*.py) were run underPYRE_FBW_MULTIFRAME=1. The site isreached in 3 benches —
getframe_escape_flush_writethrough_regression,synth/getframe_inlined_callee_own_frame,synth/getframe_stored_fback_walk—5 events each, and all 15 have
inline_subwalk=false, so every one takesthe single-frame arm and adopts.
build_multi_frame_miframeis never called,the image is never latched, and
try_adopt_multi_frame_blackholenever sees acandidate.
Consequences recorded in the ledger: flipping
_MULTIFRAMEON is a no-opacross the corpus; the three items §1 lists as its remaining work are all
unexercised, so porting any of them would be unvalidatable until a benchmark
reaches
inline_subwalk=trueat a vable escape. §4's_BLACKHOLE_RESUMErow isamended — its retirement was conditioned on
_MULTIFRAMElanding, which thatmeasurement makes unevaluable. (The multi-frame latch is also nested inside
single_frame_blackhole_resume_enabled(), so it depends on_BLACKHOLE_RESUMEstaying ON regardless.)
The instrumentation was temporary and is not in this diff; a pre-existing
[s2-gate]eprintln underPYRE_FBW_DEBUG_ABORTalready reportsinline_subwalkat that site.Gates
cargo check -p pyre-jit-trace --no-default-features --features dynasmis clean(one pre-existing
unreachable_patternswarning, unrelated).An earlier full gate on this diff —
pyre-jit-trace+majit-metainterpunittests green (1399 / 294 / …) and
check.py --backend dynasm313/313 — was runagainst the pre-rebase base
7b70e1ebff, so it does not certify the currentbase. The re-gate on
0b706504ddis in flight at the time of writing; CI onthis PR is the authoritative x86_64 check. A first attempt at that re-gate
produced a spurious
E0609: no field frame_value_count_fn— the compile raced amid-session pull of #751, which adds that field to
jitdriver.rsand itsassignment in
unpack_state.rsin the same commit.cargo checkon the settledtree passes, so that error was a torn-tree artifact, not a base break.
The diff is dead-code deletion plus comments plus markdown; nothing in it
changes codegen.
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Refactor