check.py: give the non-Windows perf ceiling its denominator term - #1121
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (1)
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 |
![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: c062cb1504
ℹ️ 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 Some(op) = self.ops.get((r.raw() - num_original_inputargs) as usize) else { | ||
| return false; | ||
| }; | ||
| if !op.opcode.has_no_side_effect() { |
There was a problem hiding this comment.
When a snapshot-only box comes from a mutable read such as GETFIELD_GC_I, this predicate admits it because has_no_side_effect() includes loads even though replay is not value-preserving. For example, if a field is read, changed before the cut, and the later guard snapshot retains the old box, the emitted prefix rereads the changed field; guard-failure resume then writes the new value into the frame instead of the captured value. Allocations have the same problem because replay produces a fresh, potentially uninitialized object. Thread these snapshot-only values through the cut's live input set rather than deciding replayability from side effects.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a2b1d2b). 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)None. 4. Structural adaptations
|
The ceiling comparison `pyre_exec > baseline_exec * limit + buffer` amplifies the baseline's own measurement error by `limit`, but only the Windows path carried a term for it (`2q * limit`); off Windows the buffer was just the left-hand `BENCH_COMPARE_BUFFER_S`. A baseline near EXEC_TIME_FLOOR_S is a difference of two medians granular to about one floor, so the budget it sets swings by `limit * EXEC_TIME_FLOOR_S`. Add that term off Windows, making the effective ceiling `limit * (1 + EXEC_TIME_FLOOR_S / baseline)`: the grace is `floor / baseline`, real only near the floor and vanishing for a baseline that is real work. exception_traceback_loop_forms read 35.6x on one runner and 37.1x on another against an unchanged ~2.3s exec, a 0.06s baseline wobbling +-0.004s across the 36x line. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 773ba81a07
ℹ️ 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".
| if inputarg_consts.get(r.raw() as usize).is_none() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
When a snapshot-only pre-cut result depends on an original input that has no entry in inputarg_consts—the production shape observed in test.test_urlparse—returning false leaves that live snapshot slot mapped to OpRef::NONE; if the post-cut guard fails, resume therefore restores an uninitialized value rather than the captured input. Avoiding the virtualizable-layout assertion by retaining this corrupt fallback is a workaround rather than a correctness fix; the cut entry layout must carry the missing live value. The newly added test_cut_trace_from_declines_a_snapshot_root_needing_an_extra_inputarg is fresh evidence beyond the existing comment because it explicitly asserts the NONE fallback for this missing-constant path.
AGENTS.md reference: AGENTS.md:L252-L254
Useful? React with 👍 / 👎.
youknowone
changed the title
1 participant
Summary
One commit: the non-Windows perf ceiling was missing the denominator term its
Windows counterpart already carries.
Fixes
pyre/check.py (ubuntu-24.04)—cranelift synth/exception_traceback_loop_forms 36.3x > gate 36x.check.py: buffer the non-Windows perf ceiling against denominator noiseThe ceiling test is
pyre_exec > baseline_exec * limit + compare_buffer. Theright-hand side amplifies the baseline's measurement error by
limit, andonly the Windows path carried a term for it (
2q * (1 + limit)); off Windows thebuffer was just the left-hand
BENCH_COMPARE_BUFFER_S, a numerator term. Abaseline near
EXEC_TIME_FLOOR_Sis a difference of two medians each granular toabout one floor, so the budget it sets swings by
limit * EXEC_TIME_FLOOR_S.That term is added off Windows now. The effective ceiling becomes
limit * (1 + EXEC_TIME_FLOOR_S / baseline): the grace isfloor / baseline,real only where the denominator cannot be trusted and vanishing for a baseline
that is real work (≤1% at a 1s baseline). No per-fixture ceiling is changed and
no baseline is re-recorded.
Not a regression in pyre:
exception_traceback_loop_formsread 35.6x on onerunner and 36.3–37.6x on others against an unchanged ~2.2–2.4s exec — a 0.06s
pypy denominator wobbling ±0.004s across the line. A retry-by-median run failed
too, so it does not clear itself.
Complementary to #1086, which refits per-fixture ceilings to twice the slowest
observed ratio and reasons about the effective bound as
limit + compare_buffer/exec_baseline; #1086 did not refit this fixture.Verification
the added term,
main's 35.6x verdict is unchanged, and a synthetic 2xregression still fails.
pyre/check.pypasses on ubuntu-24.04 andwindows-latest. The macOS
check.pyjob andCPython suite (gate)fail, andboth are inherited from
main— same two rows, verbatim, inmain's own runat
0c3c4777ba9:test.test_pickletools PASS -> FAIL (errors=1)andtest.test_re PASS -> FAIL (errors=303). Neither is a perf row; the perf suiteitself is 410/410 on dynasm and 410/410 on cranelift in that same job.
test_reis the defect jit: re-emit a cut-trace ref held only by a guard snapshot, or decline the cut #1122 fixes.
⚠ Note this run is not a demonstration of the fix:
main's ubuntucheck.pyis green in the control too, so the intermittent ceiling failure simply did not
recur. The argument rests on the arithmetic and the recorded failing readings.
Measured scope, and the one thing this loosens
Counted from the comparison table of the macOS
check.pyjob on this branch(377 fixtures timed against pypy):
~, gate not applied (baseline clamped to the floor)exec_baseline ≥ FLOOR_GATE_MIN_BASELINE_S(0.05s)The clamp boundary sits exactly at pypy raw
0.020s(max among~rows = minamong armed rows), which puts pypy startup at about
0.015s.The added grace is
floor / exec_baseline, so it is ≤10% only for the 67fixtures whose baseline clears
FLOOR_GATE_MIN_BASELINE_S, and it approaches100% — an effective ceiling of roughly
2 × limit— for the ~69 sitting justabove the clamp.
FLOOR_GATE_MIN_BASELINE_Scurrently guards only the floor(
check.py:2038); the ceiling path has no such requirement, so this wideninglands on fixtures whose ratio the same constant already declares unmeasurable.
I am not changing that here: the motivating fixture is in the trustworthy group
(
exception_traceback_loop_forms, pypy raw0.09s⇒ exec ≈0.075s⇒ grace≈ 6.7%), and disarming or re-scoping ceilings on ~206 fixtures is a coverage
decision, not a bug fix. The coherent follow-up is to make the ceiling decline to
arm below
FLOOR_GATE_MIN_BASELINE_Sexactly as the floor does — flagged here sothe choice is explicit rather than implied by this patch.
Withdrawn from this PR: the two cut-trace commits
This PR originally also carried
jit: seed the cut-trace escape closure from post-cut guard snapshotsandjit: decline a snapshot root whose cone needs an extra inputarg. Both aredropped — #1122 fixes the same defect with a sounder design, and the two
should not race each other into the same function:
has_no_side_effect()is_always_pure()NONEmapping and compilehas_no_side_effect()is the wrong boundary: the band runs to_NOSIDEEFFECT_LASTand so admits every mutable load (GETFIELD_GC,GETARRAYITEM_GC,GC_LOAD,RAW_LOAD) and every allocation (NEW*). Prefixre-emission re-executes the definition at the cut, so a replayed load returns
memory as it is at the cut rather than the value the snapshot named, and a
replayed allocation mints a blank second object.
is_always_pure()is thecorrect predicate. Credit to the Codex parity review on this PR for naming it.
One thing does not carry over to #1122, and is left here for whoever picks it
up: #1122 still treats a pre-cut original inputarg as an unconditional leaf,
with the comment that phase 4 maps it to its pool constant. Phase 4 has a second
arm — it appends the value as an extra inputarg — and
patch_new_loop_to_load_virtualizable_fieldsrequires the run after the red argsto be exactly the virtualizable's expansion, asserting
i == len(inputargs)(
compile.py:458). Detail and a reproduction are in a comment on #1122.🤖 Generated with Claude Code