jit: #366 default the multi-frame carrier resume to the P2 drain#538
jit: #366 default the multi-frame carrier resume to the P2 drain#538youknowone wants to merge 9 commits into
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 4b6545a). 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 adaptations
|
![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: 2b7b519c02
ℹ️ 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".
| let default_hash = | ||
| crate::baseobjspace::lookup_in_type(crate::typedef::w_object(), "__hash__"); | ||
| if default_hash != Some(method) { | ||
| return hash_call_normalize(method, obj); |
There was a problem hiding this comment.
When a typed-payload subclass such as class D(deque): __hash__ = lambda self: 123 reaches this new branch, hash(D()) now honors the override, but baseobjspace::hash_w_strict still falls through to hash_value and ignores typed-payload overrides. Callers like _random.W_Random.seed use hash_w_strict for non-integer seeds, so the same object is hashed by pointer identity there instead of by __hash__, breaking the ObjSpace hash contract this branch is trying to restore. Please route the strict hash path through the same dispatch or share the helper.
Useful? React with 👍 / 👎.
try_hash_value: for a non-instance object whose type resolves a __hash__ other than the inherited object.__hash__ (identity), call and normalize it instead of falling through to the identity-hash fallback, so a subclass override (class D(deque): __hash__ = ...) is honored while the base builtin payload still routes to hash_value. Extract the shared call-and-normalize into hash_call_normalize. W_Deque.__init__: clear via store (which bumps the iteration lock) instead of resetting state to 0, and validate maxlen before the clear so a rejected maxlen leaves the deque untouched. Assisted-by: Claude
… docs The walk-level operand-stack box mirror (`vstack_boxes`) is the sole kept-stack source at a branch guard; the flat `stack_slot_color_map` static-color read it once fell back to is retired, so a slot the mirror does not cover is omitted and resume re-materializes it. Update the vstack-mirror doc/comments that still describe a "legacy read" / "legacy fallback" the overlay defers to (`vstack_boxes`, `reconcile_vstack_at_boundary`, `classify_vstack_opcode`, `reseed_vstack_from_shadow`, `step_vstack_mirror`, `seed_vstack_mirror`) to state the mirror-only behavior, and drop the stale `(SLICE 1, INERT)` status labels — the mirror is now the authoritative source, not inert side-data. `PYRE_VSTACK_DIAG` now logs the per-op reconcile trace, not a mirror-vs-legacy compare. Comment-only; no runtime change. Assisted-by: Claude
`bridge_semantic_maps_at_with_jitcode_pc` and `const_ref_slots_at_pc_at` sourced depth/pcdep/const-slots from the predecessor-keyed jitcode-pc twins and, on a twin miss, re-inverted the carried `jitcode_pc` through `python_pc_for_jitcode_pc`. The twin-miss arm carried the `M73_FALLBACK` census (0 across pyre/bench + synth on both backends) and the `PYRE_PCMAP_BRIDGE_AUDIT` twin-equality asserts. Replace the re-inversion with `decode_resume_pc(pc).0` — the same merge-target fallback the non-decodable and `NO_JITCODE_PC` arms already use. A colored jitcode's carried coordinate is always an op-start or block-head offset the codewriter seeds a twin for, so the miss arm is unreachable there; a portal bridge is uncolored, so its empty `pcdep_color_slots` / `depth_at_py_pc` / `const_ref_slots_at_pc` yield the same `(0, empty)` / empty-slot result under either fallback. The now-dead census and audit asserts are removed with the arm. check.py: dynasm 180/180, cranelift 180/180. Assisted-by: Claude
drive_bridge_framestack_walk miscompiles a branchy inlined-callee continuation: a module-scope hot loop inlining a two-level call chain whose middle function has a data-dependent branch resumes the caller frame at the callee coordinate and compiles a wrong bridge. Route a multi-frame carrier resume to the drive_bridge_carrier_walk drain, which sub-walks the deepest callee then aborts to the blackhole safety floor (every shape aborts while PYRE_P2_COMPILE is unset, so the guard failure re-interprets correctly). PYRE_P2_DRAIN=0 restores the framestack walk, kept reachable pending the orthodox multi-frame reconstruction (#343). Gate the [p2-drain] sub-walk diagnostics behind PYRE_P2_DIAG now that the drain is the default. Add a module-scope branchy-inline regression test. Assisted-by: Codex Assisted-by: Claude
Formatting-only: wrap the depth/pcdep table lookups to satisfy `cargo fmt --check`. Assisted-by: Claude
resolve_bridge_walk_entry_at went through resolve_resume_pc_with_jitcode_pc, whose pc_map fallback translates the stored Python pc when the carried direct jitcode_pc is unusable. Its two callers gate on jitcode_pc != NO_JITCODE_PC, and a kept-stack branch guard's carried word is always a `-live-`-anchored startpoint, so the carried path is always taken and the Python pc is never consulted at this seam (census: 1923 hits across 170 synth progs, 0 fallback). Add resolve_bridge_walk_entry_pc (carried-only, no raw_pc/fallback), call it from the now 2-arg resolve_bridge_walk_entry_at, and drop the unused py_pc from select_recipe_entry and its three call sites. Assisted-by: Claude
hash_w_strict duplicated a subset of try_hash_value's unhashable type ladder and fell through to hash_value, dropping user and typed-payload __hash__ overrides (e.g. `class D(deque): __hash__ = ...`). Delegate to try_hash_value — the dispatch builtin_hash already uses — so the strict hash path (_random seed, dict-strategy lookups) honors the overrides. Assisted-by: Claude
deque.__init__ re-initialization clears the backing list (bumping the iteration lock) only when the deque is non-empty, matching interp_deque.py init (`if self.len > 0: self.clear()`). Re-initializing an empty deque leaves the iteration lock untouched so a lock-aware scan of it yields StopIteration rather than a spurious mutation RuntimeError. Assisted-by: Claude
branch_arm_resume_ref_liveness read the kept-stack not-taken arm's Ref register colors by inverting the carried jitcode-pc target to a Python opcode pc (python_pc_for_jitcode_pc + skip_python_trivia_forward) and passing that as the resolve fallback. frame_liveness_reg_indices_by_bank_from_pc resolves the same liveness window directly from target, which resolve_branch_target_through_trampoline already lands on a decodable startpoint, so the inversion and the trivia skip are dropped. Assisted-by: Codex
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28c31fcc76
ℹ️ 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".
| /// reachable pending the orthodox multi-frame reconstruction fix (#343). | ||
| pub(crate) fn p2_drain_enabled() -> bool { | ||
| std::env::var_os("PYRE_P2_DRAIN").is_some() | ||
| std::env::var_os("PYRE_P2_DRAIN").as_deref() != Some(std::ffi::OsStr::new("0")) |
There was a problem hiding this comment.
When PYRE_P2_DRAIN is unset, this now sends every multi-frame carrier into drive_bridge_carrier_walk. That path still runs drive_bridge_carrier_subwalk with is_authoritative_executor: true (jitcode_dispatch.rs:3051), then with PYRE_P2_COMPILE unset falls through to ctx.cut_trace, fbw_store_journal_reset, and TraceAction::Abort (trace.rs:954, trace.rs:1016-1020). For a guard failure where the reconstructed callee executes a side-effecting residual or journaled store before the drain aborts, the concrete effect is left standing while the caller restores the guard state and resumes via the blackhole, so the same callee effect is executed again. Please keep this path opt-in or make the drain non-authoritative/rolled back before making it the default.
Useful? React with 👍 / 👎.
1 participant
#366 / WS1 F1 — fix a module-scope branchy-inline silent miscompile
A module-scope hot loop that inlines a two-level call chain whose middle
function has a data-dependent branch (
while … acc += mix(i, acc&255)withdef mix(a,b): if a&1: return add3(a,b,7); return add3(b,a,-3)) produced asilent wrong answer by default (
7197540756vs7215477652). The same loop atfunction scope is byte-correct, which is why the synthetic corpus never caught it.
Root cause. On a guard failure the multi-frame inline resume ("carrier") is
decoded by
drive_bridge_framestack_walk, which mis-reconstructs the branchyinlined-callee cross-frame continuation and compiles a wrong bridge.
Fix. Default the carrier decode to
drive_bridge_carrier_walk(the P2 drain):it sub-walks the deepest reconstructed callee and, with
PYRE_P2_COMPILEunset(the default), aborts to the blackhole safety floor for every shape — so the guard
failure re-interprets correctly.
PYRE_P2_DRAIN=0restores the framestack-walkbridge, kept reachable pending the orthodox multi-frame reconstruction (#343),
which would make that cross-frame bridge correct for branchy callees and regain
its (currently unmeasurable on the corpus) perf. The leftover
[p2-drain]sub-walk diagnostics are gated behind
PYRE_P2_DIAGnow that the drain is thedefault path. Adds a module-scope branchy-inline regression test.
Verification. dynasm 181/181 and cranelift 181/181 (
python pyre/check.py),repro correct on both backends, stderr clean, escape hatch verified. Per-bench
timing is neutral-to-better (
fib_recursiveunchanged,nested_loopfaster).Branch also carries
#369droppython_pc_for_jitcode_pcfrom the two decode seamscollections.dequesubclass__hash__+ lock-aware reinitNote
A pre-existing rustfmt difference around
state.rs:1584(from the#369commit'sresolve_resume_pcregion) is present on the branch and is not part of the #366diff; flagging it for a separate formatting pass.
🤖 Generated with Claude Code