Skip to content

jit(fbw): drop the unwired PendingInlineFrame vref residue; record _MULTIFRAME's measured corpus coverage #795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 1 commit into from
Jul 25, 2026
+36 −19
Merged

jit(fbw): drop the unwired PendingInlineFrame vref residue; record _MULTIFRAME's measured corpus coverage #795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions pyre/gate-triage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ upstream lines:
| gate | orthodox side | outcome |
|---|---|---|
| PYRE_FBW_VABLE_SCALAR_CA | **OFF** | **RETIRED** — the ON design contradicts upstream |
| PYRE_FBW_MULTIFRAME | **ON** | keep; the ON path is the port, and it is unfinished |
| PYRE_FBW_MULTIFRAME | **ON** | keep; the ON path is the port, it is unfinished, and §1 measures it as never reached by the corpus |
| PYRE_FBW_CALLEE_VSTACK | NEITHER | keep OFF; see §5 |

The walker's default-ON `PYRE_FBW_*` cluster was retired separately in #757.
Expand Down Expand Up @@ -187,7 +187,29 @@ keeps inlined callee frames unmaterialized (`fbw_strict_fold_frame_reg`,
upstream does not have — a consequence of pyre's virtual-callee-frame inlining —
plus per-frame vable binding, since `PyjitplBlackholeFrameConfig` stamps one
shared `virtualizable_ptr` onto every frame in the chain and the adopt writes
only `last_instr`.
only `last_instr`. A third item sits below both: `try_adopt_multi_frame_blackhole`
(`pyre-jit-trace/src/trace.rs`) declines outright when the recovered chain is not
rooted at the walked frame, and names the `jit.virtual_ref` emit at the inline
push as the prerequisite. That emit does not exist — `opimpl_virtual_ref` /
`_finish` are ported in both `majit-metainterp/src/pyjitpl.rs` and
`pyre-jit-trace/src/state.rs`, and **neither has a caller outside a `#[test]`**,
so `virtualref_boxes` is empty and no live trace records a `VIRTUAL_REF`.

**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
Comment on lines +199 to +200

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Rerun the coverage census on the rebased corpus

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 👍 / 👎.

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

@coderabbitai coderabbitai Bot Jul 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

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.
Comment on lines +198 to +212

@coderabbitai coderabbitai Bot Jul 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.


## §2 — Not gates (11): Rust identifiers, not env vars

Expand Down Expand Up @@ -229,7 +251,7 @@ OFF path is a needed safety net. Retire at the listed trigger (A7).

| var | subsystem | retire when |
|---|---|---|
| PYRE_FBW_BLACKHOLE_RESUME | single-frame resume-past-escape (#754) | flipped default-ON 2026-07-25; retire once the multi-frame twin (`_MULTIFRAME`) lands and the escape/replay fallback is no longer the only other leg |
| PYRE_FBW_BLACKHOLE_RESUME | single-frame resume-past-escape (#754) | flipped default-ON 2026-07-25; retirement was conditioned on the multi-frame twin (`_MULTIFRAME`) landing, but §1 now measures that twin as having zero corpus coverage, so the condition is unevaluable — keep the gate and re-open the question only once a benchmark reaches `inline_subwalk=true` at a vable escape |
| PYRE_TWO_PHASE_RTYPE, PYRE_TUPLE_PER_SHAPE_CLASSDEF | rtyper prepass / per-shape tuple classdef | WS2 / #346 rtyper epic |
| PYRE_ORIGINAL_BOXES | greens++reds original_boxes index shape | box-identity #202 / resume F1 |
| PYRE_MIR_FRAMESTATE | framestate-threaded MIR lowering | MIR front-end #176/#181/#346 |
Expand Down
27 changes: 11 additions & 16 deletions pyre/pyre-jit-trace/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5491,7 +5491,12 @@ impl PyreSym {
/// records VIRTUAL_REF(box, cindex), and pushes
/// [virtualbox, vrefbox] onto virtualref_boxes.
///
/// Called from metainterp push_inline_frame (executioncontext.enter parity).
/// Upstream's caller is `executioncontext.py:89 enter`, which the tracer
/// reaches by tracing through the interpreter's own frame-entry code. The
/// pyre walker builds its inline levels itself and never traces `enter`, so
/// this has no caller yet and `virtualref_boxes` stays empty in every live
/// trace. Wiring it is a prerequisite for the multi-frame blackhole adopt
/// (`try_adopt_multi_frame_blackhole`, `trace.rs`).
pub(crate) fn opimpl_virtual_ref(
ctx: &mut TraceCtx,
sym: &mut PyreSym,
Expand Down Expand Up @@ -11953,13 +11958,11 @@ mod tests {
/// (`rpython/jit/metainterp/pyjitpl.py`) for pyre. RPython constructs
/// and pushes the callee `MIFrame` directly inside `perform_call`; pyre
/// returns this struct from the trace step so the framestack mutation
/// happens in `MetaInterpreter::push_inline_frame` after the trace
/// handler releases its borrow on `MetaInterpFrame`. No upstream
/// counterpart.
/// happens after the trace handler releases its borrow on
/// `MetaInterpFrame`. No upstream counterpart.
pub struct PendingInlineFrame {
pub sym: PyreSym,
pub concrete_frame: pyre_interpreter::pyframe::PyFrame,
pub drop_frame_opref: Option<OpRef>,
pub green_key: u64,
/// Raw `(code_ptr, target_pc)` greenkey components for element-
/// wise recursion-depth comparison. `green_key` above is the u64
Expand Down Expand Up @@ -12038,8 +12041,9 @@ pub(crate) fn recover_inline_callee_globals(code_ptr: *const ()) -> pyre_object:
/// the bridge's root concrete frame.
///
/// `parent_frames` is the OUTER chain (immediate parent first) the drain
/// builds from the framestack; `push_inline_frame` stamps
/// `parent_frames.first().pending_result_*` with the caller result slot.
/// builds from the framestack. The caller result slot travels in this
/// struct's own `caller_result_stack_idx` / `caller_result_type`; nothing
/// stamps `parent_frames.first().pending_result_*`, which stays `None`.
pub(crate) fn assemble_bridge_inline_pending(
ctx: &mut TraceCtx,
recipe: &ReconstructRecipe,
Expand Down Expand Up @@ -12149,10 +12153,6 @@ pub(crate) fn assemble_bridge_inline_pending(
PendingInlineFrame {
sym,
concrete_frame,
// No virtual_ref for a reconstructed frame: the forward trace's
// virtual_ref was already finished/encoded; None skips the
// opimpl_virtual_ref emission in push_inline_frame.
drop_frame_opref: None,
// The reconstructed frame represents the same inlined call the
// forward trace pushed at function entry; match its (code, 0)
// greenkey identity for recursion-depth + inline-position tracking.
Expand Down Expand Up @@ -12315,11 +12315,6 @@ pub(crate) fn setup_reconstructed_callee_frame(
Some((pending, argboxes_r))
}

pub enum InlineTraceStepAction {
Trace(TraceAction),
PushFrame(PendingInlineFrame),
}

pub fn execute_inline_residual_call(
frame: &mut pyre_interpreter::pyframe::PyFrame,
nargs: usize,
Expand Down
Loading