Skip to content

jit: exception-edge bridge default-on, GC write-barrier fix, and FBW rollout-flag cleanup - #757

Merged
youknowone merged 11 commits into
mainfrom
perf-bridge
Jul 25, 2026
Merged

jit: exception-edge bridge default-on, GC write-barrier fix, and FBW rollout-flag cleanup#757
youknowone merged 11 commits into
mainfrom
perf-bridge

Conversation

@youknowone

@youknowone youknowone commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Summary

Closes out the exception-edge bridge rollout: lands the outstanding exc-edge
bridge fixes, fixes the GC crash they exposed, flips the bridge on by default
on native backends, and removes the whole family of served-their-purpose
default-on FBW rollout flags.

Exception-edge bridge

  • Forward catch-lookup, dynasm exception-cell ops, and stale-exception-seed
    fixes for the exception-guard bridge; guard the exception flavor at
    bridge entry and carry the bridge-entry flavor guard's resume coordinate
    verbatim; clear the standing-exception seed on a no-exception bridge
    failure.
  • exc_edge_bridge_enabled() is now cfg!(not(target_arch = "wasm32")):
    native backends route a caught-in-frame exception-guard bridge resume to the
    in-frame except handler unconditionally. The wasm guest's abort-replay
    exception class (jit: eliminate the guard-fail resume-decode backxlat inverse (jitcode-blackhole Slice 3') #727) is still open, so the bridge stays off there.

Effect: exc_mixed_classes_bridge_flavor 0.415s → 0.166s (2.5×); every
bench's top-level print loop now leaves the pending-exception decline and
compiles. No jitstats drift from the flag removal itself.

GC write-barrier fix

The exc-edge bridge surfaced a latent crash (invalid type_id in
incremental_mark_step, ~50–60% under MAJIT_GC_STRESS=1 on dynasm
reraise loops). JIT vable write-backs box young ints/floats and raw-store
them into a vable frame that runs detached from the walked frame chain, so
the minor collector's root visitor no-ops on the old frame and nothing
re-traces the items; the frame stays alive via a traceback and major
marking then pushes a stale nursery item gray.

Fix: ref-store write barriers at the lowest choke points — the
majit-metainterp write_field / write_array_item ref arms and a
pyre-side frame_array_write_barrier armed in every walk-end / abort-commit
flush and at execute_assembler entry. Non-moving exception carriers now
forward their raw child slots (walk_raw_exception_roots), and the
previously-unrooted BH_LAST_EXC_VALUE TLS cell gets a walker.

Flag cleanup

Every flag below was default-on with an unused =0 opt-out; the gate
machinery is deleted and the enabled behavior is unconditional:

PYRE_FBW_INLINE, _INLINE_MULTIFRAME, _NSVABLE_MULTIFRAME,
_REC_MULTIFRAME, _BRIDGE_REC_INLINE, _REC_MUTUAL_CUTOVER, _REC_CA,
_FORITER_INLINE, _LOOP_CALLEE_CA, _RAISE, _BUILTIN_FOLD,
_LOADATTR_FOLD, _STOREATTR_FOLD, _LOADMETHOD_FOLD, _LOADGLOBAL_FOLD,
_LOADNAME_FOLD, _STORENAME_FOLD, _DELETE_FAST, _INLINE_NSFOLD,
_STACK_LIVEREG, _CALL_ASSEMBLER, _NO_REPLAY_EXIT,
_NESTED_RESID_ABORT, _ABORT_FLUSH, _BRANCH_FLUSH, _END_FLUSH,
_BRIDGE_STAMP, _BRIDGE_LOCAL_SEED, and PYRE_EXC_EDGE_BRIDGE.

check.py drops the --no-fbw-inline-multiframe option and its
PYRE_FBW_INLINE_MULTIFRAME=0 export.

Verification

  • check.py dynasm 301/301, cranelift 301/301.
  • Adversarial exception battery matches the CPython oracle on both backends,
    default and MAJIT_GC_STRESS=1; tryz stress dynasm 8/8 + cranelift 3/3
    clean (was ~4/5 crashing before the barrier fix).
  • Affected-crate unit tests green; both release binaries build clean.
  • The flag removal is compile-behavior-identical — no committed jitstats
    baseline changed.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added coverage for mixed exception classes in hot loops.
    • Enabled more reliable exception handling across compiled and resumed execution paths.
  • Bug Fixes

    • Fixed exception state preservation during save, restore, and bridge transitions.
    • Improved garbage-collection safety for exception data, frame updates, and reference writes.
    • Prevented incorrect bridge cleanup when saved exception values are reused.
    • Improved handling of forced guard failures and exception-handler resumes.
  • Tests

    • Expanded regression coverage for exception bridges and saved exception reuse.
    • Updated recorded JIT benchmark statistics.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@youknowone, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 41 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 478eb4ad-637b-406f-8bac-695dc0ff4ac0

📥 Commits

Reviewing files that changed from the base of the PR and between 1588ad2 and ebb87c5.

📒 Files selected for processing (48)
  • majit/majit-backend-cranelift/src/compiler.rs
  • majit/majit-backend-dynasm/src/aarch64/assembler.rs
  • majit/majit-backend-dynasm/src/x86/assembler.rs
  • majit/majit-gc/src/rewrite.rs
  • majit/majit-ir/src/effectinfo.rs
  • majit/majit-metainterp/src/virtualizable.rs
  • pyre/bench/fannkuch.cranelift.jitstats
  • pyre/bench/fannkuch.dynasm.jitstats
  • pyre/bench/fib_loop.cranelift.jitstats
  • pyre/bench/fib_loop.dynasm.jitstats
  • pyre/bench/fib_recursive.cranelift.jitstats
  • pyre/bench/fib_recursive.dynasm.jitstats
  • pyre/bench/float_loop.cranelift.jitstats
  • pyre/bench/float_loop.dynasm.jitstats
  • pyre/bench/getframe_escape_flush_writethrough_regression.py
  • pyre/bench/inline_helper.cranelift.jitstats
  • pyre/bench/inline_helper.dynasm.jitstats
  • pyre/bench/int_loop.cranelift.jitstats
  • pyre/bench/int_loop.dynasm.jitstats
  • pyre/bench/nbody.cranelift.jitstats
  • pyre/bench/nbody.dynasm.jitstats
  • pyre/bench/nested_loop.cranelift.jitstats
  • pyre/bench/nested_loop.dynasm.jitstats
  • pyre/bench/raise_catch_loop.cranelift.jitstats
  • pyre/bench/raise_catch_loop.dynasm.jitstats
  • pyre/bench/spectral_norm.cranelift.jitstats
  • pyre/bench/spectral_norm.dynasm.jitstats
  • pyre/bench/synth/bridge_recursion_overflow.py
  • pyre/bench/synth/exc_mixed_classes_bridge_flavor.py
  • pyre/bench/synth/foriter_exempt_nested_foriter.py
  • pyre/bench/synth/foriter_exempt_shared_generator.py
  • pyre/check.py
  • pyre/gate-triage.md
  • pyre/pyre-interpreter/src/error.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/vable_ops.rs
  • pyre/pyre-jit-trace/src/state.rs
  • pyre/pyre-jit-trace/src/trace.rs
  • pyre/pyre-jit/src/call_jit.rs
  • pyre/pyre-jit/src/eval.rs
  • pyre/pyre-jit/src/jit/codewriter.rs

Walkthrough

The changes update exception save/restore handling, exception-guard bridge routing, FBW eligibility and lowering, GC root and write-barrier handling, walk-end flushing, benchmark coverage, and recorded JIT statistics across the Cranelift, DynASM, interpreter, and Pyre JIT components.

Changes

Exception and bridge handling

Layer / File(s) Summary
Backend exception state and bridge preservation
majit/majit-backend-*/..., majit/majit-gc/rewrite.rs, majit/majit-metainterp/virtualizable.rs, pyre/bench/synth/exc_mixed_classes_bridge_flavor.py
Backends now save, restore, and classify exception state; Cranelift dispatches bridges before exception staging; bridge-prefix removal checks later operand use; reference writes arm GC barriers; and a mixed-exception bridge benchmark was added.
Exception bridge routing and resume snapshots
pyre/pyre-jit-trace/src/jitcode_dispatch/{bridge_subwalk.rs,mod.rs,resume_snapshot.rs}, pyre/pyre-jit/src/call_jit.rs
Exception bridges select handler targets with resume-aware lookup, preserve carried resume coordinates, handle null-exception arms, and decline forced-guard bridge compilation.
FBW dispatch and lowering eligibility
pyre/pyre-jit-trace/src/jitcode_dispatch/*, pyre/pyre-jit/src/jit/codewriter.rs, pyre/check.py, pyre/gate-triage.md
Multiple environment-controlled FBW gates and opt-outs were removed; inline calls, residual folds, exception lowering, builtins lookup, virtualizable lookup, and DELETE_FAST now use direct eligibility paths.
GC roots and write barriers
pyre/pyre-interpreter/src/error.rs, pyre/pyre-jit/src/{eval.rs,trace.rs}, pyre/pyre-jit-trace/src/{state.rs,trace.rs}
Raw exception children and blackhole exceptions are rooted, while frame, array, virtualizable, and compiled-entry writes re-arm GC barriers.
Walker flushing and validation artifacts
pyre/pyre-jit-trace/src/{state.rs,trace.rs}, pyre/bench/*
Bridge concrete seeding and walk-end abort/flush paths were reorganized, return and raise outcomes use finish payloads, benchmark fixtures and comments were updated, and JIT statistics were refreshed.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Walker
  participant BridgeSubwalk
  participant Backend
  participant GC
  Walker->>BridgeSubwalk: route exception-guard bridge
  BridgeSubwalk->>Backend: preserve exception value and class
  Backend->>GC: retain exception and frame references
  GC-->>Walker: provide rooted state for resume
Loading

Possibly related PRs

Poem

A rabbit hops through bridges bright,
Saving exceptions just right.
GC roots glow, barriers chime,
Walkers flush in proper time.
Gates fall down, the paths grow clear—
JIT carrots for every ear!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main themes: exception-edge bridge rollout, GC write-barrier fixes, and FBW flag cleanup.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf-bridge

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 21142dad3d

ℹ️ 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".

pf.last_instr = body_pc as isize - 1;
}
}
frame_array_write_barrier(frame as *mut u8, arr_ptr);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Re-arm the frame array before allocating during flush

This barrier runs only after the slot-copy loop, but the loop itself can allocate when boxed_slot_value_for_type boxes Int/Float values. When a flushed frame has already stored a nursery Ref into an old frame/array and a later boxing allocation triggers a minor GC, the frame/array is not yet in the remembered set, so the collector will not scan or forward that raw slot; the resumed frame can then contain a stale nursery pointer. Re-arm before the first possible allocation after a raw store, or per store, rather than only after the batch.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 9244406. The barrier now re-arms after every store in the four flush paths (flush_walk_end_state_to_frame_inner, flush_walk_end_state_at_outer_call, write_back_outer_locals, flush_walk_end_state_after_outer_call), and before write_back_outer_locals's first boxing allocation, so a nursery Ref already stored into the detached frame array is in the remembered set when the next iteration's Int/Float boxing can minor-collect. Verified with MAJIT_GC_STRESS=1 (tryz stress dynasm 12/12 + cranelift 5/5 clean) and check.py 301/301 on both backends.

commented by Claude

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 17b603aca1

ℹ️ 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".

pub fn exc_edge_bridge_enabled() -> bool {
static ENABLED: std::sync::OnceLock<bool> = std::sync::OnceLock::new();
*ENABLED.get_or_init(|| std::env::var_os("PYRE_EXC_EDGE_BRIDGE").is_some())
cfg!(not(target_arch = "wasm32"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep routed exception values defined before enabling

Enabling exception-edge bridges by default exposes routed handler bridges where bridge_subwalk.rs records SAVE_EXC_CLASS, SAVE_EXCEPTION, RESTORE_EXCEPTION, then stores the SAVE_EXCEPTION result as last_exc_value for handler code such as except E as e. The GC rewriter still unconditionally drops a leading SaveExcClass + SaveException + RestoreException prefix in majit/majit-gc/src/rewrite.rs:2879-2895 without checking whether value_op is used later, so these default-on bridges can reach GC/codegen with handler ops referring to an undefined exception value. Please make that prefix removal use-aware, or keep this routed path gated, before turning it on for all native backends.

Useful? React with 👍 / 👎.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit ebb87c5).
Updated: 2026-07-24T19:12:47.527Z

Files in the reviewed diff
majit/majit-backend-cranelift/src/compiler.rs
majit/majit-backend-dynasm/src/aarch64/assembler.rs
majit/majit-backend-dynasm/src/x86/assembler.rs
majit/majit-gc/src/rewrite.rs
majit/majit-ir/src/effectinfo.rs
majit/majit-metainterp/src/virtualizable.rs
pyre/bench/fannkuch.cranelift.jitstats
pyre/bench/fannkuch.dynasm.jitstats
pyre/bench/fib_loop.cranelift.jitstats
pyre/bench/fib_loop.dynasm.jitstats
pyre/bench/fib_recursive.cranelift.jitstats
pyre/bench/fib_recursive.dynasm.jitstats
pyre/bench/float_loop.cranelift.jitstats
pyre/bench/float_loop.dynasm.jitstats
pyre/bench/getframe_escape_flush_writethrough_regression.py
pyre/bench/inline_helper.cranelift.jitstats
pyre/bench/inline_helper.dynasm.jitstats
pyre/bench/int_loop.cranelift.jitstats
pyre/bench/int_loop.dynasm.jitstats
pyre/bench/nbody.cranelift.jitstats
pyre/bench/nbody.dynasm.jitstats
pyre/bench/nested_loop.cranelift.jitstats
pyre/bench/nested_loop.dynasm.jitstats
pyre/bench/raise_catch_loop.cranelift.jitstats
pyre/bench/raise_catch_loop.dynasm.jitstats
pyre/bench/spectral_norm.cranelift.jitstats
pyre/bench/spectral_norm.dynasm.jitstats
pyre/bench/synth/bridge_recursion_overflow.py
pyre/bench/synth/exc_mixed_classes_bridge_flavor.py
pyre/bench/synth/foriter_exempt_nested_foriter.py
pyre/bench/synth/foriter_exempt_shared_generator.py
pyre/check.py
pyre/gate-triage.md
pyre/pyre-interpreter/src/error.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/vable_ops.rs
pyre/pyre-jit-trace/src/state.rs
pyre/pyre-jit-trace/src/trace.rs
pyre/pyre-jit/src/call_jit.rs
pyre/pyre-jit/src/eval.rs
pyre/pyre-jit/src/jit/codewriter.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

  • majit/majit-gc/src/rewrite.rs:2897 ↔ rpython/jit/backend/llsupport/rewrite.py:991 — Pyre now retains the SAVE_EXC_CLASS; SAVE_EXCEPTION; RESTORE_EXCEPTION prefix when either result is used later; RPython explicitly removes it unconditionally (“just assume they aren't”). This is a deliberate semantic extension needed for routed except E as e bridges, not an incorrect port.

3. Pre-existing mismatches (already present before this patch)

None identified in the changed-path static audit.

4. Structural adaptations

  • pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs:198 ↔ rpython/jit/metainterp/blackhole.py:396 — Pyre routes an exception-edge bridge walk directly to catch_exception; RPython performs that routing while executing a blackhole frame. The observable handler target and exception payload are equivalent, but tracing-time routing is required by Pyre’s source-translated walker architecture.

  • pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs:2393 ↔ rpython/jit/metainterp/blackhole.py:401 — the forward live → catch_exception lookup mirrors RPython’s direct bytecode decode, with Pyre’s additional backward lookup for its post-residual-call resume-coordinate representation.

  • majit/majit-backend-cranelift/src/compiler.rs:6754 ↔ rpython/jit/backend/x86/assembler.py:690 — Pyre dispatches an attached bridge through a generated indirect tail-call cache; RPython patches the original guard jump to bridge machine code. The changed ordering now matches RPython’s essential property: bridge entry occurs before failure recovery consumes pending exception cells.

  • majit/majit-backend-dynasm/src/x86/assembler.rs:8441 ↔ rpython/jit/backend/x86/assembler.py:1822, and majit/majit-backend-dynasm/src/aarch64/assembler.rs:6477 ↔ rpython/jit/backend/aarch64/opassembler.py:735 — direct Rust loads/stores of backend exception cells replace RPython’s CPU-address abstractions. The implemented SAVE_EXC_CLASS, SAVE_EXCEPTION, and RESTORE_EXCEPTION sequences now match upstream semantics.

  • majit/majit-metainterp/src/virtualizable.rs:1064 ↔ rpython/jit/metainterp/virtualizable.py:101, and pyre/pyre-jit-trace/src/state.rs:3872 ↔ rpython/jit/metainterp/virtualizable.py:126 — explicit remembered-set re-arming follows raw Rust writes into virtualizable/frame arrays. RPython’s translated GC inserts the corresponding barriers around setattr/array writes automatically.

  • pyre/pyre-interpreter/src/error.rs:418 ↔ pypy/interpreter/error.py:225 — Pyre explicitly walks children of a non-moving Rust exception carrier; PyPy’s GC layout traces object fields transitively. This preserves the same traceback/args/context reachability under Pyre’s mixed allocation model.

@youknowone

youknowone commented Jul 23, 2026

Copy link
Copy Markdown
Owner Author

Codex parity review acted on. Two exception-bridge/GC hardening follow-ups landed on top of the rebase:

  • 9244406 — GC write-barrier re-arm. The walk-end / abort flush functions box W_Int/W_Float slots one at a time into the detached vable frame array with the remembered-set barrier armed only once, after the loop. Each minor GC consumes the remembered entry, so a young Ref stored in iteration N is dropped if iteration N+1's boxing minor-collects before a re-arm. Fixed by re-arming frame_array_write_barrier per store in all four flush functions (flush_walk_end_state_to_frame_inner, _at_outer_call, write_back_outer_locals, _after_outer_call), matching the 3-call pattern try_commit_midbody_abort already used.

  • 3f7ace2remove_bridge_exception use-check. rewrite.py:988 strips a leading SaveExcClass + SaveException + RestoreException prefix unconditionally and leaves an XXX should check if the boxes are used later; but we just assume they aren't for now. A routed exception-guard handler bridge (bridge_subwalk.rs) records that same prefix but keeps the SaveException result as last_exc_value for except E as e, so an unconditional strip would drop an operand a later op still references. Implemented the deferred use-check — scan ops after the prefix (args + failargs) for a reuse of the RestoreException class/value operands and strip only when neither is used. Safe by construction: it can only prevent a strip, never cause a wrong one. Regression tests cover strip-when-unused and keep-when-reused.

Verification: adversarial exception battery matches the CPython oracle on both backends (default + MAJIT_GC_STRESS=1); tryz reraise stress clean (dynasm ×6, cranelift ×4); check.py cranelift 301/301, dynasm 300/301 with the single miss being the pre-existing nested_loop gate-sitter (timing identical to the pre-stack binary, no exception paths, unchanged jitstats); 0 jitstats-baseline drift; majit-gc unit tests green.

The check.py CI failures (const_arg_call_resume ×30, residual_raise_except_resume ×10, int_loop ×2, nested_loop ×2) are all boundary/gate-sitter perf benches flaking under runner load — correctness is 100% across all platforms (Windows fully green), and the failing benches are unrelated to this stack's exception/GC changes.

commented by Claude

@youknowone
youknowone force-pushed the perf-bridge branch 3 times, most recently from 81bb774 to 1588ad2 Compare July 24, 2026 03:52

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
majit/majit-backend-cranelift/src/compiler.rs (1)

6819-6834: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove the dead post-staging bridge dispatch for must_save_exception guards.

The info.can_have_bridge branch already emits emit_attached_bridge_dispatch before exception staging; the separate info.can_have_bridge && info.must_save_exception block at Lines 6819-6834 emits the same bridge tail-call again after staging. A bridge hit tail-calls from the pre-staging dispatch, so this later call path is unreachable and must not run the host-call deadframe write barrier twice for any other reason.

🤖 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 `@majit/majit-backend-cranelift/src/compiler.rs` around lines 6819 - 6834,
Remove the post-staging info.can_have_bridge && info.must_save_exception block
that calls emit_attached_bridge_dispatch. Keep the existing pre-staging bridge
dispatch unchanged, and preserve exception staging and deadframe write-barrier
behavior for non-bridge paths.
🤖 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.

Outside diff comments:
In `@majit/majit-backend-cranelift/src/compiler.rs`:
- Around line 6819-6834: Remove the post-staging info.can_have_bridge &&
info.must_save_exception block that calls emit_attached_bridge_dispatch. Keep
the existing pre-staging bridge dispatch unchanged, and preserve exception
staging and deadframe write-barrier behavior for non-bridge paths.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1d6af2d7-6cb1-47a0-8bbb-6ce9b8eba761

📥 Commits

Reviewing files that changed from the base of the PR and between daac3b3 and 1588ad2.

📒 Files selected for processing (48)
  • majit/majit-backend-cranelift/src/compiler.rs
  • majit/majit-backend-dynasm/src/aarch64/assembler.rs
  • majit/majit-backend-dynasm/src/x86/assembler.rs
  • majit/majit-gc/src/rewrite.rs
  • majit/majit-ir/src/effectinfo.rs
  • majit/majit-metainterp/src/virtualizable.rs
  • pyre/bench/fannkuch.cranelift.jitstats
  • pyre/bench/fannkuch.dynasm.jitstats
  • pyre/bench/fib_loop.cranelift.jitstats
  • pyre/bench/fib_loop.dynasm.jitstats
  • pyre/bench/fib_recursive.cranelift.jitstats
  • pyre/bench/fib_recursive.dynasm.jitstats
  • pyre/bench/float_loop.cranelift.jitstats
  • pyre/bench/float_loop.dynasm.jitstats
  • pyre/bench/getframe_escape_flush_writethrough_regression.py
  • pyre/bench/inline_helper.cranelift.jitstats
  • pyre/bench/inline_helper.dynasm.jitstats
  • pyre/bench/int_loop.cranelift.jitstats
  • pyre/bench/int_loop.dynasm.jitstats
  • pyre/bench/nbody.cranelift.jitstats
  • pyre/bench/nbody.dynasm.jitstats
  • pyre/bench/nested_loop.cranelift.jitstats
  • pyre/bench/nested_loop.dynasm.jitstats
  • pyre/bench/raise_catch_loop.cranelift.jitstats
  • pyre/bench/raise_catch_loop.dynasm.jitstats
  • pyre/bench/spectral_norm.cranelift.jitstats
  • pyre/bench/spectral_norm.dynasm.jitstats
  • pyre/bench/synth/bridge_recursion_overflow.py
  • pyre/bench/synth/exc_mixed_classes_bridge_flavor.py
  • pyre/bench/synth/foriter_exempt_nested_foriter.py
  • pyre/bench/synth/foriter_exempt_shared_generator.py
  • pyre/check.py
  • pyre/gate-triage.md
  • pyre/pyre-interpreter/src/error.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/vable_ops.rs
  • pyre/pyre-jit-trace/src/state.rs
  • pyre/pyre-jit-trace/src/trace.rs
  • pyre/pyre-jit/src/call_jit.rs
  • pyre/pyre-jit/src/eval.rs
  • pyre/pyre-jit/src/jit/codewriter.rs
💤 Files with no reviewable changes (2)
  • pyre/gate-triage.md
  • pyre/check.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9d03b3761b

ℹ️ 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".

Comment thread pyre/pyre-jit/src/eval.rs
Comment on lines +7016 to +7020
{
let f = frame_root.frame() as *mut PyFrame as *mut u8;
let arr = unsafe { (*(f as *mut PyFrame)).locals_cells_stack_w };
if pyre_object::gc_hook::try_gc_owns_object(arr as *mut u8) {
pyre_object::gc_hook::try_gc_write_barrier(arr as *mut u8);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Re-arm entry frame before untag allocations

When a compiled loop is entered with tagged-int locals in an old GC-managed frame, untag_tagged_frame_locals runs before this new barrier and w_int_new_unique can allocate while a previous boxed int has already been raw-stored into locals_cells_stack_w. If that allocation triggers a minor GC, the old frame/array has not been remembered yet, so the just-stored young box can be missed and the compiled loop can later read a stale pointer. Move this barrier before untagging, or re-arm inside untag_tagged_frame_locals after each store.

Useful? React with 👍 / 👎.

…ll ops, stale exception seed)

- walker: add find_catch_for_exc_resume — the blackhole
  handle_exception_in_frame forward case (catch_exception directly after
  the resume -live-, blackhole.py:396) tried before the backward scan;
  the exc-edge routing in dispatch_via_miframe now uses it.
- dynasm aarch64/x86: implement SaveExcClass (load pos_exception,
  assembler.py:1817-1818), SaveException (shared
  emit_store_and_reset_exception, assembler.py:1820-1821) and
  RestoreException (assembler.py:1845-1850). The previous
  SaveExcClass/SaveException bodies returned 0 and RestoreException had
  no emit arm.
- walker: seed_standing_exception_for_walk reads BH_LAST_EXC_VALUE
  before the preseeded-sym early return, so the exception published from
  the current guard failure overwrites exception state a previous walk
  left on the persistent sym (_prepare_exception_resumption grabs from
  this failure's deadframe, pyjitpl.py:3125-3126). A preseeded sym is
  kept only when no fresh publish exists.

All three changes are exercised only with PYRE_EXC_EDGE_BRIDGE set.
check.py 293/293 on dynasm and cranelift.

Assisted-by: Claude
A loop trace recorded through a raising iteration carries that iteration's
GUARD_EXCEPTION(class). No-raise iterations chronically fail it WITHOUT a
pending exception, so a bridge is compiled for the no-exception
continuation; a second exception class then enters the same bridge WITH a
pending exception and the recorded continuation runs on the NULL
raised-call result (SIGSEGV in compiled code, both backends, default
mode).

- walker: an exception-guard bridge walk with no standing exception now
  records GUARD_NO_EXCEPTION at bridge entry
  (_prepare_exception_resumption null arm + prepare_resume_from_failure,
  pyjitpl.py:3152-3171), so the pending-exception flavor deopts to the
  blackhole at entry.
- cranelift: attached-bridge in-code dispatch now also runs for
  must_save_exception guards, before the exception staging in
  emit_guard_exit — entering the bridge with the exception cells intact,
  as dynasm's patched guard jump does (patch_jump_for_descr). The
  previous host-loop re-entry consumed the exception before invoking the
  bridge, so the entry flavor guard could not see it.
- call_jit: decline bridge compilation from GUARD_NOT_FORCED failures —
  "Failures of a GUARD_NOT_FORCED are never compiled, but always just
  blackholed" (ResumeGuardForcedDescr.handle_fail, compile.py:950-953).
- bench: add synth/exc_mixed_classes_bridge_flavor covering the
  two-exception-class shape.

check.py 293/293 on dynasm and cranelift.

Assisted-by: Claude
The routed/null bridge-entry flavor-guard captures fed the walk-entry
position — already a post-call resume coordinate — through the
after-residual capture path, whose op-START-keyed twins advanced it a
second time, onto the physically-following except-handler block. The
entry guard's own bridge then resumed inside the handler, and its
other-flavor decode failed the exc-edge catch lookup
(ExcEdgeCrossFrameReturnUnsupported retry loop).

Add GuardCaptureScope::carried_resume_jit_pc: the entry captures carry
position verbatim as the guard's resume word, take the resume py from
the forward twin at that word, and skip the op-START-keyed depth twins
(they read the key opcode's depth, over-publishing valuestackdepth so
the resume read garbage slots as Refs).

Assisted-by: Claude
seed_standing_exception_for_walk kept a preseeded sym exception when the
published cell was empty. For an exception-guard bridge the publish is
the deadframe-grab authority, so an empty cell now clears the seed
(_prepare_exception_resumption null arm, pyjitpl.py:3152-3154).
Previously a no-exception failure of an exception guard walked a stale
exception's handler as the no-exception continuation, and the per-flavor
bridge chain recompiled the same handler indefinitely instead of
converging.

Assisted-by: Claude
…alks

The guard-failure vable sync (write_boxes_to_heap), the walk-end escape
flushes, and the MidBody abort commit stored decoded/boxed refs into a
frame's locals_cells_stack_w raw. The values can be nursery-young while
the frame/array are old-gen and the virtualizable runs detached from the
walked frame chain, so no minor re-traced the items; a traceback-reachable
frame then fed the stale nursery address to major marking, panicking in
incremental_mark_step (invalid type_id; reproducible with
MAJIT_GC_STRESS=1 PYRE_EXC_EDGE_BRIDGE=1 on a reraise loop).

- majit-metainterp write_field / write_array_item: arm the object/array
  in the remembered set after every Ref store (virtualizable.py:101-113
  write_boxes stores run under the translated write barrier upstream).
- pyre-jit-trace flush/commit paths and execute_assembler entry: re-arm
  the frame + array via frame_array_write_barrier.
- Exception root walkers (walk_jit_exc_value, walk_active_sym_exc_roots,
  PyError::walk_gc_refs): forward the non-moving carrier's raw child
  slots so young tracebacks/args parked across a minor stay valid; add
  the missing BH_LAST_EXC_VALUE walker.

Verified: stress battery clean on both backends; adversarial exc battery
matches CPython flag-on and flag-off; check.py 298/298 dynasm and
cranelift.

Assisted-by: Claude
PYRE_EXC_EDGE_BRIDGE becomes opt-out (=0 disables) on native targets; the
wasm guest keeps the opt-in gate (no env plumbing to switch it back off,
and its abort-replay exception class is still open).

Native jitstats baselines regenerated: every bench's top-level print loop
previously hit the pending-exception decline and now compiles (+1 loop,
+1 guard failure); fib_recursive additionally converges two
GuardNoException bridges (bridges 1->3, guard_failures 1->407, absorbed
in warmup). loops_aborted / internal_compile_panics stay 0 everywhere.
wasm baselines unchanged.

A/B (same binary, env toggle, alternating x3): exc_mixed_classes_
bridge_flavor 0.415s -> 0.166s; handler_reraise_second_exc ~6% faster;
no bench regressed. check.py 298/298 on dynasm and cranelift with the
default on; adversarial exc battery matches CPython both with the
default and with =0.

Assisted-by: Claude
Every flag below was default-on with an unused `=0` opt-out. Delete the gate
machinery and make the enabled behavior unconditional; the disabled arms and
their helper code are removed as dead.

Removed env flags (collapsed to always-on):
PYRE_FBW_INLINE, _INLINE_MULTIFRAME, _NSVABLE_MULTIFRAME, _REC_MULTIFRAME,
_BRIDGE_REC_INLINE, _REC_MUTUAL_CUTOVER, _REC_CA, _FORITER_INLINE,
_LOOP_CALLEE_CA, _RAISE, _BUILTIN_FOLD, _LOADATTR_FOLD, _STOREATTR_FOLD,
_LOADMETHOD_FOLD, _LOADGLOBAL_FOLD, _LOADNAME_FOLD, _STORENAME_FOLD,
_DELETE_FAST, _INLINE_NSFOLD, _STACK_LIVEREG, _CALL_ASSEMBLER,
_NO_REPLAY_EXIT, _NESTED_RESID_ABORT, _ABORT_FLUSH, _BRANCH_FLUSH,
_END_FLUSH, _BRIDGE_STAMP, _BRIDGE_LOCAL_SEED.

exc_edge_bridge_enabled() becomes `cfg!(not(target_arch = "wasm32"))`:
native backends run the exception-edge bridge unconditionally; the wasm
guest's abort-replay exception class (#727) is still open, so it stays off
there.

check.py: drop the --no-fbw-inline-multiframe option and its
PYRE_FBW_INLINE_MULTIFRAME=0 export.

Assisted-by: Claude
The walk-end / abort flush functions box each Int/Float slot (an
allocation that can trigger a minor collection) one at a time into the
detached frame array. The array is forwarded by a minor collection only
while it is in the remembered set, and each minor consumes that entry, so
a single barrier after the whole loop left a window: a nursery Ref stored
in one iteration could be dropped by a minor collection triggered by the
next iteration's boxing before the array was re-armed, leaving a stale
pointer in the resumed frame.

Re-arm the barrier after every store (and before the first allocation
that follows a pre-loop nursery store) in flush_walk_end_state_to_frame_inner,
flush_walk_end_state_at_outer_call, write_back_outer_locals, and
flush_walk_end_state_after_outer_call.

Assisted-by: Claude
… result is unused

remove_bridge_exception stripped a leading SaveExcClass + SaveException +
RestoreException prefix unconditionally (rewrite.py:988), leaving its
`XXX should check if the boxes are used later` deferred. A routed
exception-guard handler bridge records that same prefix but keeps the
SaveException result as last_exc_value for handler code (`except E as e`),
so an unconditional strip drops an operand a later op still references.

Scan the ops after the prefix for a use of the RestoreException class/value
operands (args and failargs) and strip only when neither is reused. Add
regression tests for the strip-when-unused and keep-when-reused cases.

Assisted-by: Claude
The rebase renamed `flush_walk_end_state_to_frame` to
`flush_walk_loop_end_state_to_frame` in the end-flush block, pushing the
`else if` condition past the 100-column limit; rustfmt moves the opening
brace to its own line.

Assisted-by: Claude
…ception guards

emit_guard_exit dispatched the attached bridge twice for a
can_have_bridge + must_save_exception guard: once before the
_store_and_reset_exception staging and once after it. The pre-staging
dispatch was widened to cover must_save guards in 618871e, so its
guard set now supersets the post-staging block's. The post-staging
dispatch enters the bridge with the exception globals already cleared,
so the bridge entry flavor guard (prepare_resume_from_failure) reads no
pending exception; a bridge installed between the two probes would take
that wrong-flavor path. dynasm's patched guard jump enters the bridge
before the failure-recovery stub stages jf_guard_exc
(patch_jump_for_descr, x86/assembler.py:987), matching the pre-staging
dispatch. Remove the redundant second dispatch.

check.py 304/304 on cranelift.

Assisted-by: Claude

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ebb87c5758

ℹ️ 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".

pub fn exc_edge_bridge_enabled() -> bool {
static ENABLED: std::sync::OnceLock<bool> = std::sync::OnceLock::new();
*ENABLED.get_or_init(|| std::env::var_os("PYRE_EXC_EDGE_BRIDGE").is_some())
cfg!(not(target_arch = "wasm32"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Decline handlers unless every path rejoins the loop

With this now enabled for every native backend, exception-guard bridges can route into handlers that only partly rejoin the loop: bridge_subwalk admits a catch target when exc_handler_rejoins_loop finds any reachable jit_merge_point, but that helper does not reject a sibling *_return path. For a handler like except E: if cond: return x; ...continue..., the return-taking failure is compiled through the routed bridge instead of declining to the blackhole, even though that path is documented as needing cross-frame resume/caller-frame reconstruction. Require all reachable handler exits to rejoin, or decline when any return path is reachable.

AGENTS.md reference: AGENTS.md:L32-L42

Useful? React with 👍 / 👎.

@youknowone
youknowone merged commit 2360d76 into main Jul 25, 2026
30 of 31 checks passed
@youknowone
youknowone deleted the perf-bridge branch July 25, 2026 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant