Repository navigation
jit: hand every abort_permanent the value stack the resumed opcode reads; gc: arena range cache; check.py measurement fixes - #992
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 Plus Run ID: 📒 Files selected for processing (8)
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 ccf1242). 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
|
…aseline `measure_startups` took the median of STARTUP_SAMPLES empty-program user-CPU runs; it now takes the minimum. Every error source on that fixed per-process cost is one-sided -- contention, cache pressure and page faults only add user CPU -- so under sustained load every sample inflates together and the median inflates with them, which is why raising the sample count did not help. That estimate is subtracted from every timed run and is the divisor for a short bench, so an inflated one drives the baseline's exec time onto EXEC_TIME_FLOOR_S and the reported ratio becomes `pyre_exec / floor`. `_gate_fail_detail` now appends a marker when the baseline exec sits on the floor, so such a line reads as a clamp rather than as a measurement. Assisted-by: Claude
`ArenaCollection::contains` ran a binary search over `arena_ranges` on every
call; it now re-tests the range it matched last before searching. `free_arena`
clears the cache -- that is the only way a cached range can go stale, since
inserting an arena leaves every existing range where it was.
Measured by retired instructions, min of 7, both arms built from one LLBC
extraction (whole-process totals; the same binary measured twice under two
labels differs by 0.010%):
polymorphic_binary_receiver 10.477G -> 10.071G
recursion_memo_branch 2.712G -> 2.665G
polymorphic_slot_retype 2.758G -> 2.726G
call_loop_local_function 896.5M -> 881.5M
Those arms also carried a null-root skip in `OwnerRootGuard::new`, which is
dropped: the save side stores a livevar without testing its value
(`shadowcolor.py`'s `expand_one_push_roots` emits a `gc_save_root` per live
GCREF and marks unused slots through the translation-time `make_bitmask`), so
skipping the acquire moved a null test that belongs on the walk to the save.
Recorded the walk-side rule on `walk_roots` instead.
Assisted-by: Claude
… a closes_block marker `abort_permanent/` stores `last_instr = py_pc - 1` into the vable, so the interpreter resumes at the marker's own opcode and runs it. Two arms reached the marker with a frame that could not run it. The `Call` nargs > 14 arm ran `emit_popvalue_ref!` over the callable, null_or_self and every argument first; each of those nulls its stack slot and syncs the lowered `valuestackdepth` into the vable. Resuming a 17-argument CALL then hit the frame's own `report_stack_underflow` at depth 0. Move the arity test above the pop loop and bail there, and drop the `nargs > 14` legs that the earlier `continue` leaves unreachable. `push_and_bump!` keeps a pushed value in its SSA register and syncs only the depth, so a slot last written that way still reads PY_NULL out of the vable; the resumed CALL read seventeen NULLs. Mirror every live stack slot with `setarrayitem_vable_r` before the terminal marker. The `LoadFastCheck` unbound arm walked its depth back on the abstract state only, leaving the vable one slot above the frame's real top. Assisted-by: Claude
`abort_permanent/` stores `last_instr = py_pc - 1`, so the interpreter resumes AT the marker's opcode and re-runs it. `pyframe.py`'s `pushvalue` lowers every push to a `setarrayitem_vable_r` (`jtransform.py`'s `do_fixed_list_setitem`), but `push_and_bump!` keeps the pushed value in its SSA register and syncs only `valuestackdepth`, so a stack slot last written that way still reads PY_NULL out of the vable. A CALL_KW past the per-arity ceiling (nargs > 13) resumed against empty argument slots and raised `UnboundLocalError` inside the callee for its `*args` parameter. Write the stack back at the marker. The snapshot taken as the opcode is entered, not `current_state`: an aborting arm has usually already modelled the opcode's stack effect, and the frame the interpreter needs is the one from before the opcode ran. `pre_opcode_stack` is declared beside the macro because a `macro_rules!` body resolves a non-parameter identifier in its definition scope. This subsumes the `closes_block`-only flush, which used the post-arm state, and the `LoadFastCheck` unbound arm's separate `emit_vsd!`; both are removed. The depth is now published from the snapshot at every marker. Adds `synth/wide_callkw_resume`. Assisted-by: Claude
`measure_startups` goes back to the median of STARTUP_SAMPLES empty-program user-CPU runs. The minimum recovers the fixed per-process cost for an idle machine, but that estimate is subtracted from bench runs that were not idle: a loaded bench carries an inflated startup inside it, and taking the unloaded minimum away leaves the inflation in the reported exec time. On one CI job the minimum came in 15ms under the median for dynasm and 13ms for cranelift, and nine short benches whose baseline exec was pinned to EXEC_TIME_FLOOR_S crossed their gates in that run. The floor-clamp marker on a gate-failure line is unchanged. Assisted-by: Claude
![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: ccf12421a7
ℹ️ 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_setarrayitem_ref_graph_args( | ||
| frame_var.into(), | ||
| v_idx.into(), | ||
| value.clone().into(), | ||
| ), |
There was a problem hiding this comment.
When a wide CALL abort is reached from a pre-call resume, pre_opcode_stack can contain the symbolic ConstRef(0) null-or-self sentinel while the tracer-seeded vable slot contains a real method receiver—the existing call lowering explicitly handles this at lines 9160-9196 by reading the physical slot instead of const-folding it. This loop writes the symbolic constant back before abort_permanent, replacing the receiver with null; the interpreter then re-runs the wide call without self, producing wrong arguments or a TypeError. Preserve tracer-seeded slot values here (or restore the upstream per-push vable mirroring) rather than blindly flushing symbolic constants.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
The defect
A call past the per-arity helper ceiling, reached after a hot loop, miscompiles.
Both backends, and in both shapes:
abort_permanent/publisheslast_instr = py_pc - 1, so the interpreter resumesat the marker's own opcode and runs it. Every operand that opcode reads has
to still be readable out of the frame. Two independent reasons it was not:
The wide
Callarm consumed its operands before testing the arity. It ranemit_popvalue_ref!over every argument,null_or_selfand the callablefirst; each of those nulls the slot and lowers
valuestackdepth, so theframe reached the marker at depth 0. The arity test is now hoisted above the
pop loop —
nargsis known on the arm's first line.push_and_bump!never mirrors the pushed value into the vable. It doescurrent_state.stack.push+current_depth += 1+emit_vsd!and nothingelse, so a slot last written that way still reads PY_NULL.
LoadSmallIntandLoadConstpush through it; onlyemit_load_fast_ref!/emit_pushvalue_ref!/emit_pushvalue_ref_const!write the array. After (1)the resumed call read seventeen NULLs. Fixed by flushing the value stack with
setarrayitem_vable_rin the sharedemit_abort_permanent!path, so everymarker — not only the
closes_blockones — hands back a real frame.The flush uses a per-PC snapshot of the entry stack, not
current_state: anarm that aborts has usually already modelled the opcode's stack effect, so at the
marker
current_statedescribes the state after an opcode that has not run.Nothing on a run that keeps executing pays for the stores — the walk reaches a
marker only on a run that is bailing.
Also: the
LoadFastCheckunbound arm walked its depth back on the abstract stateonly, leaving the vable one slot above the frame's real top.
Why it surfaced now
Before the blackhole rollback was removed, the deopt restored
locals/valuestackdepth/last_instrfrom a pre-blackhole snapshot, which overwrotethe published coordinate and hid all of this. Removing the rollback was correct —
it double-executed side effects — and made the coordinate load-bearing.
The deviation this compensates for
pyframe.py'spushvaluelowers every push to asetarrayitem_vable_r(
jtransform.py'sdo_fixed_list_setitem); the optimizer then elides theredundant ones and forces at a
may_forceboundary.push_and_bump!omittingthe store is the deviation, and the abort-site flush compensates for it. Making
push_and_bump!mirror is the orthodox fix, but it puts a store after everycomputed push on the hot path and moves jit-stats broadly — a separate, measured
change.
Regression coverage
bench/synth/wide_callkw_resume.py, verified against a binary built without thefix, where it fails with exactly the
UnboundLocalErrorabove.nargs <= 13(no abort) is the passing control.
It carries no
max-pypy-ratio: the CALL_KW is refused every time so theenclosing frame falls back to the interpreter, and pypy's own execution time here
sits on the harness measurement floor, so no ratio computed against it is a
measurement. The committed jit-stats and the output comparison are what gate it.
check.pynever caught any of this; the probe scripts did.Also here
gc: cache the lastArenaCollection::containsrange hit, invalidated infree_arena. A doc note onwalk_rootsrecords that null entries are skippedon the walk and nowhere else — the save side stores a livevar untested
(
shadowcolor.py'sexpand_one_push_rootsemits agc_save_rootper liveGCREF with no value test, and marks slots unused through the translation-time
make_bitmask), and the only null test iswalk_stack_root'sif content:inshadowstack.py.check.py: annotate a gate failure whose baseline exec was clamped toEXEC_TIME_FLOOR_Swith[... exec clamped to floor; ratio not a measurement], so a ratio that is an artifact of the clamp says so instead ofreading as a regression. This is the whole behavioural delta against
main;the rest of those two commits is a comment.
The first of them also switched the startup estimator from the median of
STARTUP_SAMPLESto the minimum, and the second puts it back. The minimumdoes recover a fixed per-process cost — on an idle machine. It is subtracted
from bench runs that were not idle, and a loaded bench carries an inflated
startup inside it, so taking the unloaded minimum away leaves that inflation
in
exec. On the previous CI run of this branch the minimum came in 15msunder the median for dynasm and 13ms for cranelift, and nine short benches
whose pypy baseline was pinned to the floor crossed their gates at once, where
mainat the same base had none. The comment records this so it is notretried; note it cannot be reproduced locally, because on an idle box the
median and the minimum agree.
Verification
Locally, on both backends: the 15 probe scripts and a 12-repro comparison
against CPython all pass, and
cargo test --all --no-default-featuresis7372 passed, 0 failed.
CI on the previous push of this branch (base
6fbef5f7c7c) hadcargo testgreen on all three platforms. Its
check.pyred was measured againstmain'sown run at that same base, per platform. Windows is the clean comparison —
its exec floor is a scheduler tick rather than 5ms, so short benches do not sit
on the floor and the ratio-noise class is absent there:
main@6fbef5f7c7cexception_args_virtual×2,list_length_hint_validate×2 — 2 failed, 368 passedZero branch-attributable difference; the extra pass is the new bench. Those two
have since been fixed on
mainby #993 and no longer appear at this branch'scurrent base.
— authored by Claude