bench: restore the forty baselines #945 recorded from a macOS-only run - #1031
bench: restore the forty baselines #945 recorded from a macOS-only run#1031youknowone wants to merge 1 commit into
Conversation
`loops_compiled` for these benches is not one number. The script's own
directory is `sys.path[0]`, so the first import fills
`FileFinder._fill_cache`, whose listing loop is platform-conditional
(`lib-python/3/importlib/_bootstrap_external.py:1419-1437`): linux takes
`set(contents)` and runs no Python-level loop, darwin adds the
`{fn.lower() for fn in contents}` comprehension, and win32 adds the
`for item in contents` suffix-lowering loop on top of it. With 1431
files in `pyre/bench/synth` those loops pass the hot threshold and are
compiled, so one unchanged bench reports 7 loops on linux, 8 on macOS
and 9 on windows; the same file in a directory of 501 entries reports 7
everywhere. Compiling them is the JIT working, not a defect — but it
means the counter carries the runner's platform.
#945 recorded the macOS observation for forty rows that main measured
identically on all three runners, so they went red on the other two: the
ubuntu job moved from 38 to 46 changed rows and windows from 25 to 44,
while macOS moved from 24 to 2. Restore the values main was green with
on every runner.
The macOS and windows counts stay unrecorded because no single value is
green on all three — the same conclusion ae895c30c6c reached for
`imp_lock_rlock_semantics`, which is one of these forty.
Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 35 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 (40)
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: 5d50037687
ℹ️ 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".
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_compiled=8 | ||
| loops_compiled=7 |
There was a problem hiding this comment.
These values deliberately exclude the extra _fill_cache loop observed on macOS and Windows, but the default pyre/check.py invocation compares every recorded counter exactly and returns failure on any difference (_apply_snapshot_gate at pyre/check.py:1129-1148). Both pyre-check-macos and pyre-check-windows reuse that invocation as required CI jobs (.github/workflows/pyre-ci.yml:462-480), so those runners will observe their platform-specific loop/guard counts and fail these benchmarks. The import-side loop must be removed from the measurement, or the gate/baselines must represent platform-specific values, before committing Linux-only counts.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 5d50037). The diff against the review base changes only generated jit-stats baselines (40 file(s)); nothing to review. |
|
Superseded by #1034, which fixed this at the source instead of at the baseline. #1034 sets 7 / 2211 is ubuntu's column, now reproduced on macOS — the platform ladder is All forty files this PR touches are byte-identical to — commented by Claude |
1 participant
loops_compiledfor these benches is not one number, and #945 recorded themacOS one.
What the counter actually measures
The script's own directory is
sys.path[0], so the first import fillsFileFinder._fill_cache. That function's listing loop is platform-conditional(
lib-python/3/importlib/_bootstrap_external.py:1419-1437):_fill_cacheset(contents)— C level+ {fn.lower() for fn in contents}(_CASE_INSENSITIVE_PLATFORMS)+ for item in contents: … .add(…)as wellpyre/bench/synthholds 1435 entries, over the hot threshold, so those loopscompile. Byte-identical source, one dynasm binary:
MAJIT_LOG=1names the eighth loop[bcenc-audit] "_fill_cache". Compiling itunder
PYPY_GC_NURSERY=2Gchanges nothing, so it is not a collection artifact.Splitting
arith_int_bool.pyinto its seven functions and summing gives exactly7 / 10 / 2211 — the linux value — i.e. the bench itself contributes seven loops
on every platform and the eighth is the import machinery.
Not a regression
loops_aborted0 -> 0,internal_compile_panics0 -> 0,bridges_compiled10 -> 10. What moves is
loops_compiledup and the new loop's two guardfailures, which stay under
trace_eagernessso no bridge attaches. Compilingthe loop is
1c41d1a56c4 jit: admit SET_ADD and MAP_ADD in the FOR_ITER body scanworking as intended, measured on that commit and its parent — oneworktree, one
CARGO_TARGET_DIR, the same probe file, and the two revisionsdiffer by that one file's fifteen lines:
loops_compiled/bridges/aborted/guardsce42b0d5dc2(1c41d1a56c4^,SetAddabsent fromeval.rs)1c41d1a56c4(SetAddadmitted)The mechanism:
_fill_cachecarriesSET_ADDin its own codeobject (the comprehension is inlined, no nested code object), and
SetAdddoesnot appear anywhere in
pyre/pyre-jit/src/eval.rsbefore that commit, so theframe was rejected whole by
unsupported_jit_shape— a gate rejection, whichcounts no abort, matching
loops_aborted=0on both sides.What this PR does
Restores the forty rows (twenty benches, dynasm and cranelift) to the values
main measured identically on all three runners. #945 recorded the macOS
observation for them, so they went red on the other two: the ubuntu job moved
from 38 to 46 changed rows and windows from 25 to 44, while macOS moved from 24
to 2.
The macOS and windows counts stay unrecorded, because no single value is green
on all three — the same conclusion
ae895c30c6creached forimp_lock_rlock_semantics, which is one of these forty.Follow-up, not in this PR
Making all three agree means keeping
_fill_cacheout of the measurement — thedirect lever is splitting
pyre/bench/synth/into directories under the hotthreshold, after which every platform runs no Python-level loop there. It also
removes the standing fragility that adding or deleting bench files can move
every baseline at once.
🤖 Generated with Claude Code