check.py: raise the wasm/dynasm ratio ceiling to 4x - #1272
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughThe PR raises the default WASM-to-DynASM ratio ceiling from 3.0 to 4.0. It removes ratio directives and performance comments from several benchmarks, and adds diagnostic comments to one synthetic benchmark. Executable benchmark behavior remains unchanged. ChangesWASM ratio and benchmark updates
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: Poem
✨ Finishing Touches📝 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 |
![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: 77ea589914
ℹ️ 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".
| @@ -1,4 +1,5 @@ | |||
| # pyre-check: max-pypy-ratio=34 | |||
| # pyre-check: ungated-jitstats=bridges_compiled,guard_failures | |||
There was a problem hiding this comment.
This exemption permanently removes bridges_compiled and guard_failures from the fixture’s gate even though the accompanying analysis identifies an unresolved JIT defect: the same resume guards continue failing after a bridge is attached and compile additional generations. Consequently, regressions in that bridge/resume path can now land while this fixture stays green; the repository explicitly requires fixing root-cause bugs rather than adding harness workarounds. Resolve the non-converging guard behavior and retain these counter baselines instead.
AGENTS.md reference: AGENTS.md:L309-L311
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 0412642). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
|
weird, because the test now pass on main |
77ea589 to
89d676d
Compare
…wances under it `WASM_MAX_DYNASM_RATIO` was 3x, and four fixtures had been carved out of it one at a time -- fannkuch 3.6, fib_recursive 3.6, raise_catch_loop 3.8, short_circuit_value_kept_stack 3.7 -- each for the reason `wasm_ratio_gate` already documents: the wasm backend reaches CPython-level objects through interpreter round-trips and allocates each on a Rust heap the wasm path does not yet collect, so a loop dominated by that work runs several times dynasm's execution time with no regression behind it. More kept arriving. One main run failed `if_else_jump_forward` at 3.9x and `loop_callee_shared_mutation` at 3.3x, neither of them carrying an allowance. A ceiling most of the fixtures above it are exempted from individually is collecting exemptions rather than measuring the backend. Set it to 4x and remove the four allowances now under it. `global_quasiimmut_invalidation` keeps its 6, the only one still above the gate. This does not clear `short_circuit_value_kept_stack`, which reads 5.2x on ubuntu and 4.9x here: it was over its own 3.7 allowance already and is over 4x too, so it stays as red as it was. Removing its line changes its ceiling from 3.7 to 4x and nothing else; what it is fitted to is a separate question from where the shared gate sits. Assisted-by: Claude
…do not reproduce The same binary on the same source with the same argv, the same child environment and PYTHONHASHSEED all held fixed reads six bridges / 1204 or seven / 1404, run to run -- 1 of 6 runs, then 2 of 6, then 0 of 8 an hour later. CI has seen it as well: the windows runner reported `dynasm/synth/const_arg_call_resume` as `jit-stats unstable (not gated)`. A machine settles on one value and repeats it many times over, so a local red on either counter looks like a property of the tree and is not one. That cost a session's worth of attribution before one configuration was repeated, which is what the note is for. The note also records the two things that do not settle it, so neither is tried again: the trip count reads the same counters at 220 through 400, and re-recording pins whichever side the recording machine was on -- adding this comment alone, text the parser drops, moved a wasm reading from 1404 to 1204 for eight runs in a row. Nothing about the gate changes. `JITSTATS_STABILITY_RUNS` already excuses the fixture whenever its repeats disagree, and in six recent main runs across three hosts the counters were never gated against it. Assisted-by: Claude
89d676d to
0412642
Compare
youknowone
changed the title
1 participant
Two commits: the wasm/dynasm ratio ceiling, and a note on a fixture's counters.
WASM_MAX_DYNASM_RATIO3x → 4x3x was not where the backend is. Four fixtures had already been carved out of it one at a time, each for the reason
wasm_ratio_gatedocuments — the wasm backend reaches CPython-level objects through interpreter round-trips and allocates each on a Rust heap the wasm path does not yet collect, so a loop dominated by that work runs several times dynasm's execution time with no regression behind it:fannkuchfib_recursiveraise_catch_loopsynth/short_circuit_value_kept_stackAnd more kept arriving without one. On main's ubuntu runner,
if_else_jump_forwardfails at 3.9x andloop_callee_shared_mutationat 3.3x today, neither carrying an allowance.A ceiling that the fixtures above it are exempted from individually is collecting exemptions rather than measuring the backend. This sets it to 4x and removes the four allowances now under it.
synth/global_quasiimmut_invalidationkeeps its 6, the only one still above the gate.What this does not fix:
short_circuit_value_kept_stackreads 5.2x on main's ubuntu runner and 5.7x here. It was already over its own 3.7 allowance and is over 4x too, so it stays exactly as red as it is on main. Removing its line moves its ceiling from 3.7 to 4x and changes nothing else — what that fixture should be fitted to is a separate question from where the shared gate sits, and raising a ceiling to cover it is the move that was refused before.A note on
const_arg_call_resumeComment only — no gate change, no baseline change.
Its
bridges_compiledandguard_failuresdo not reproduce. The same binary on the same source with the same argv, the same child environment andPYTHONHASHSEEDpinned reads six bridges / 1204 or seven / 1404, run to run — 1 of 6 runs, then 2 of 6, then 0 of 8 an hour later. CI has seen it as well: the windows runner reporteddynasm/synth/const_arg_call_resumeasjit-stats unstable (not gated).A machine settles on one value and repeats it many times over, so a local red on either counter looks like a property of the tree and is not one. The note also records the two things that do not settle it, so neither is tried again:
JITSTATS_STABILITY_RUNSalready excuses the fixture whenever its repeats disagree, and across six recent main runs on three hosts its counters were never gated against it, so nothing here changes the gate.Verification
Being re-measured on the current branch tip; the numbers will be posted here when the run finishes. (An earlier full run was discarded: the branch was rebased while it was in flight, so its source and its binaries did not come from one tree.)
Assisted-by: Claude
Summary by CodeRabbit
Performance
Documentation
Tests