bench/synth: refit four pypy ceilings the startup subtraction moved, and retire a fifth - #1666
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughFive Pyre benchmark fixtures update their ChangesBenchmark threshold configuration
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This updates synthetic benchmark performance guard thresholds to reflect the revised measurement basis, without changing benchmark behavior or outputs. The current changes are ready to merge. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit ad15102). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
Contributor
Merging this PR will degrade performance by 31.25%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | fib_recursive |
2.6 s | 4.5 s | -42.72% |
| ❌ | fannkuch |
3.3 s | 4.7 s | -29.75% |
| ❌ | nbody |
1.9 s | 2.5 s | -25.7% |
| ❌ | fib_loop |
1.2 s | 1.6 s | -25.29% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing virtualizable (ad15102) with main (9f81966)
Footnotes
-
6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
… subtraction moved `6aabe927ce1` made every pyre backend subtract pypy's startup rather than its own, so the startup a pyre process spends above pypy now stays in the numerator. 4.1 was fitted before that, and this fixture's pypy execution is 0.05s-0.09s -- one to two times FLOOR_GATE_MIN_BASELINE_S -- so the 0.053s-0.110s that lands on it is comparable to the whole baseline it divides. It was not among the 41 fixtures that commit lengthened or the 8 whose ceilings it refitted. Readings on run 33651833695, which measures it under the new subtraction for the first time -- `main`'s own run at 6aabe92 was cancelled: ubuntu cranelift 5.0x (median of 3; the failure) wasm 4.4x dynasm 3.3x macos cranelift 3.6x dynasm 2.9x windows dynasm 5.0x (marked `?`: the floor declines this baseline) 6.5 covers the widest with 1.3x to spare, and the floor it derives -- capped at parity, since 6.5/6 is above 1 -- sits 2.9x under the fastest reading. Not lengthened. Doubling the trip count takes pypy's execution from 0.05s to 0.10s, which is the 2x bar rather than past it, and leaves a predicted 4.7x that needs this refit anyway; a length that clears the bar with margin puts the windows dynasm leg over a second for one fixture. The recorded jit-stats are not what argues against it -- measured at both 7840000 and 15680000 on dynasm and cranelift, loops_compiled=1, bridges_compiled=2 and guard_failures=402 are identical, and both match the committed baselines. The same subtraction leaves siblings in the same position: `gate` and `jit-gc-resume-gaps` both fail this job on `for_iter_conditional_store_bridge` at 4.8x and 4.5x against a ceiling of 4, with pypy at 0.04s and 0.06s. Those are not touched here. Assisted-by: Claude
… and retire a fourth `exception_loop_warmup` was not alone in the position `6aabe927ce1` left it in, and refitting it alone moved the failure rather than removing it: the same job then failed `for_iter_conditional_store_bridge`, which is what `gate` and `jit-gc-resume-gaps` had been failing on all along. Fitted from 12 CI runs created after that commit -- readings from before it subtracted a different quantity and are not evidence about these gates. Only unmarked rows are used. A `?` row carries a granularity buffer worth EXEC_TIME_FLOOR_S over its baseline, which is what lets it print a ratio far above its ceiling without failing, so counting one would fit the gate to a reading that cannot fail it; every one of these fixtures has its widest reading on such a row, on windows dynasm or ubuntu wasm. Each ceiling has to cover the widest unmarked reading and derive a floor (ceiling/6, capped at parity) under the narrowest, which fixes a window: for_iter_conditional_store_bridge 1.6x-5.1x n=55 window 5.1-9.6 4 -> 7 condexpr_heap_const_merge 2.2x-6.6x n=54 window 6.6-13.2 4.2 -> 9 for_iter_nested_method_inline 0.9x-3.6x n=61 window 3.6-5.4 3.3 -> 4.4 `for_iter_method_branch_inline` loses its ceiling instead. Its unmarked readings span 0.8x to 4.5x over 60 observations, a factor of 5.6 against a PERF_GATE_FLOOR_DIVISOR of 6, which leaves the window 4.5-4.8 -- six percent wide, with under three percent of margin at either end. A bound that narrow over that many samples reports the runner rather than the code. The spread is the denominator's: pypy executes this loop in 0.30s on macos and 0.10s on ubuntu, and a ratio is scale-invariant so a longer trip count cannot close it. `synth_perf_gate` documents an absent ceiling as a full exemption, which 252 of the 525 synthetic fixtures already are. `exception_loop_warmup`'s 6.5 from the previous commit is left alone: the wider sample puts its window at 4.9-12.0, and run 33693600706 measured it at 4.5x under the new ceiling, where it had failed at 5.0x under the old one. No trip count, jit-stats baseline or fixture output changes. pyre/check.py --check-headers: 527 headers read. Assisted-by: Claude
youknowone
force-pushed
the
virtualizable
branch
from
September 3, 2026 01:46
121cb76 to
ad15102
Compare
youknowone
changed the title
This was referenced Sep 3, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
6aabe927ce1(#1653) made every pyre backend subtract pypy's startup rather than its own, so what a pyre process spends above pypy to reach the first bytecode now stays in the numerator. It refitted 8 ceilings and lengthened 41 fixtures; these five were in neither list, and each sits where 0.053s-0.110s lands on a baseline of comparable size.Refitting one at a time does not converge — it relocates.
exception_loop_warmupwas fixed first, and the same job then failedfor_iter_conditional_store_bridge, which is whatgateandjit-gc-resume-gapshad been failing on all along.Method
Every number here is read from CI. Nothing was measured locally, and readings from runs created before
6aabe927ce1are excluded: they subtracted a different quantity and are not evidence about these gates. That leaves 12 runs, pooled across ubuntu/macos/windows and dynasm/cranelift/wasm.Only unmarked rows are fitted. A
?row carries a granularity buffer worthEXEC_TIME_FLOOR_Sover its baseline, which is precisely what lets it print a ratio far above its ceiling without failing. Every one of these fixtures has its widest reading on such a row — windows dynasm or ubuntu wasm — so counting them would fit each gate to a reading that cannot fail it, inflating all five.A ceiling must cover the widest unmarked reading and derive a floor (
ceiling/6, capped at parity) under the narrowest. That fixes a window:exception_loop_warmupfor_iter_conditional_store_bridgecondexpr_heap_const_mergefor_iter_nested_method_inlineThe fifth is retired rather than refitted
for_iter_method_branch_inlinereads 0.8x to 4.5x over 60 unmarked observations — a factor of 5.6 against aPERF_GATE_FLOOR_DIVISORof 6. That leaves the window 4.5–4.8: six percent wide, with under three percent of margin at either end. A bound that narrow over that many samples reports the runner, not the code.The spread is the denominator's. pypy executes this loop in 0.30s on macos and 0.10s on ubuntu, and a ratio is scale-invariant, so a longer trip count cannot close it.
synth_perf_gatedocuments an absent ceiling as a full exemption, which 252 of the 525 synthetic fixtures already are.Not changed
No trip count, no jit-stats baseline, no fixture output. Lengthening was measured and rejected for
exception_loop_warmup: doubling it takes pypy from 0.05s to 0.10s, the 2x bar rather than past it, and a length that clears it with margin puts the windows dynasm leg over a second. Its counters are identical at both lengths (loops_compiled=1,bridges_compiled=2,guard_failures=402) and match the committed baselines.pyre/check.py --check-headers: 527 headers read.Summary by CodeRabbit