check.py: direction-labelled jit-stats gate, per-platform baselines; re-record cranelift and wasm - #1021
Conversation
WalkthroughThe change updates JIT-stat baselines and extends ChangesJIT statistics validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant JITStatFixture
participant BaselineLookup
participant JITStatsChange
participant SnapshotGating
JITStatFixture->>BaselineLookup: Load platform-specific baseline
BaselineLookup->>JITStatsChange: Compare counter values
JITStatsChange->>SnapshotGating: Return regressions and improvements
SnapshotGating->>JITStatFixture: Repeat discrepant fixture
JITStatFixture-->>SnapshotGating: Return stable or unstable result
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 09f6ee0). 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
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b8d377221
ℹ️ 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".
| drifted = next( | ||
| (s for s in repeats or () if s != jitstats), None | ||
| ) | ||
| if drifted is not None: | ||
| moved = _jit_stats_change(jitstats, drifted) |
There was a problem hiding this comment.
If one counter is noisy while another has a deterministic regression, any difference between the initial snapshot and a repeat marks the entire fixture unstable, and the caller records it as passed. For example, loops_aborted can be 0 in the baseline and 1 in all three runs while loops_compiled varies in only one run; this branch then waives the persistent loops_aborted regression. Determine stability per counter and continue gating baseline differences that reproduce across the repeats rather than discarding the whole comparison.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/check.py`:
- Around line 1241-1250: Update the instability branch around _jit_stats_change
to store the unstable reason instead of returning immediately. Continue through
the existing output snapshot and threshold gates, and return "unstable" with the
stored reason only after both gates pass; preserve the existing gate behavior
and unstable JIT tracking.
- Around line 1752-1754: Update the status-label mapping near self._record in
the per-fixture reporting flow so snap_status == "regressed" produces
"REGRESSED", while "fail" remains the only state producing "SNAPDIFF"; preserve
the existing "IMPROVED" mapping and associated color behavior.
- Around line 1149-1155: Update the repeat-validation flow around
`_jitstats_repeats` to accept the already validated initial stdout and compare
every repeat’s stdout against it. For each repeat, reject the result when the
exit code is nonzero, stdout differs, `_jit_panic_reason(stderr)` is present, or
`_jit_stats_snapshot(stderr)` is missing; only append valid snapshots so
nondeterministic repeats cannot be reported as merely `UNSTABLE`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f6a8dfaa-c567-491a-ab46-1f9a24338d50
📒 Files selected for processing (21)
pyre/bench/synth/closure_per_call.cranelift.win32.jitstatspyre/bench/synth/closure_per_call.wasm.jitstatspyre/bench/synth/exception_bridge_traceback_head.cranelift.jitstatspyre/bench/synth/exception_bridge_traceback_head.wasm.jitstatspyre/bench/synth/exception_escape_caller_frame_tb_node.cranelift.jitstatspyre/bench/synth/exception_escape_caller_frame_tb_node.wasm.jitstatspyre/bench/synth/exception_raise_caught_same_frame_tb.cranelift.jitstatspyre/bench/synth/exception_residual_raise_caught_in_frame.cranelift.jitstatspyre/bench/synth/exception_traceback_frame_lineno.cranelift.jitstatspyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstatspyre/bench/synth/gc_bug_bridge_flavor_traceback_names.cranelift.jitstatspyre/bench/synth/gc_bug_bridge_flavor_traceback_names.wasm.jitstatspyre/bench/synth/list_length_hint_validate.cranelift.jitstatspyre/bench/synth/mutate_uncaught_raise_delivery.wasm.jitstatspyre/bench/synth/raise_reg_unbound_jitstress.wasm.jitstatspyre/bench/synth/recursive_call_frame_relocation.cranelift.win32.jitstatspyre/bench/synth/recursive_call_frame_relocation.wasm.jitstatspyre/bench/synth/slots_class_var_conflict.cranelift.jitstatspyre/bench/synth/str_fstring.wasm.jitstatspyre/bench/synth/wide_callkw_resume.wasm.jitstatspyre/check.py
| _output, _elapsed, code, stderr = run_timed( | ||
| [self._pyre(backend), script], | ||
| timeout_s=effective_timeout, env=pyre_env(), | ||
| ) | ||
| if code != 0: | ||
| return None | ||
| snapshots.append(_jit_stats_snapshot(stderr)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate each repeat before reporting UNSTABLE.
Line 1149 discards repeat stdout. Line 1153 checks only the exit code. If the initial execution matches PyPy but a repeat returns different stdout or emits a JIT panic with changed counters, Lines 1237-1250 classify the fixture as unstable. Lines 1741-1746 then record the fixture as passed. This can convert a nondeterministic output failure into a warning.
Pass the validated initial output into _jitstats_repeats. Treat changed stdout, _jit_panic_reason(stderr), and a missing JIT-stat snapshot as an invalid repeat.
Proposed fix
- def _jitstats_repeats(self, backend, script, timeout):
+ def _jitstats_repeats(self, backend, script, timeout, expected_output):
...
- _output, _elapsed, code, stderr = run_timed(
+ repeat_output, _elapsed, code, stderr = run_timed(
[self._pyre(backend), script],
timeout_s=effective_timeout, env=pyre_env(),
)
- if code != 0:
+ snapshot = _jit_stats_snapshot(stderr)
+ if (
+ code != 0
+ or repeat_output != expected_output
+ or _jit_panic_reason(stderr)
+ or snapshot is None
+ ):
return None
- snapshots.append(_jit_stats_snapshot(stderr))
+ snapshots.append(snapshot)
return snapshots
...
- repeats = self._jitstats_repeats(backend, script, timeout)
+ repeats = self._jitstats_repeats(
+ backend, script, timeout, output,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _output, _elapsed, code, stderr = run_timed( | |
| [self._pyre(backend), script], | |
| timeout_s=effective_timeout, env=pyre_env(), | |
| ) | |
| if code != 0: | |
| return None | |
| snapshots.append(_jit_stats_snapshot(stderr)) | |
| repeat_output, _elapsed, code, stderr = run_timed( | |
| [self._pyre(backend), script], | |
| timeout_s=effective_timeout, env=pyre_env(), | |
| ) | |
| snapshot = _jit_stats_snapshot(stderr) | |
| if ( | |
| code != 0 | |
| or repeat_output != expected_output | |
| or _jit_panic_reason(stderr) | |
| or snapshot is None | |
| ): | |
| return None | |
| snapshots.append(snapshot) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/check.py` around lines 1149 - 1155, Update the repeat-validation flow
around `_jitstats_repeats` to accept the already validated initial stdout and
compare every repeat’s stdout against it. For each repeat, reject the result
when the exit code is nonzero, stdout differs, `_jit_panic_reason(stderr)` is
present, or `_jit_stats_snapshot(stderr)` is missing; only append valid
snapshots so nondeterministic repeats cannot be reported as merely `UNSTABLE`.
| moved = _jit_stats_change(jitstats, drifted) | ||
| self.jitstats_unstable.append(f"{backend}/{name}") | ||
| return "unstable", ( | ||
| "jit-stats unstable — re-running the same binary moved " | ||
| + ", ".join(moved[0] + moved[1]) | ||
| + ", so this run's counters are not a property of the " | ||
| "tree and the baseline comparison (" | ||
| + ", ".join(regressions + improvements) | ||
| + ") is not gated" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run independent snapshot gates before returning unstable.
Line 1243 returns before the output snapshot check at Lines 1275-1282 and the threshold check at Lines 1288-1299. If --snapshot-diff is enabled, PyPy and Pyre can produce the same changed output while unstable JIT counters cause this fixture to pass at Line 1744.
Store the instability reason. Run the output and threshold gates. Return unstable only if those gates pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/check.py` around lines 1241 - 1250, Update the instability branch around
_jit_stats_change to store the unstable reason instead of returning immediately.
Continue through the existing output snapshot and threshold gates, and return
"unstable" with the stored reason only after both gates pass; preserve the
existing gate behavior and unstable JIT tracking.
| label = "IMPROVED" if snap_status == "improved" else "SNAPDIFF" | ||
| paint = yellow if snap_status == "improved" else red | ||
| self._record(backend, False, name, snap_reason) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report a JIT-stat regression as REGRESSED.
Line 1262 returns regressed, but Line 1752 maps that state to SNAPDIFF. The per-fixture status label then does not distinguish a JIT-stat regression from a generic snapshot failure. Reserve SNAPDIFF for fail.
Proposed fix
- label = "IMPROVED" if snap_status == "improved" else "SNAPDIFF"
- paint = yellow if snap_status == "improved" else red
+ if snap_status == "improved":
+ label, paint = "IMPROVED", yellow
+ elif snap_status == "regressed":
+ label, paint = "REGRESSED", red
+ else:
+ label, paint = "SNAPDIFF", red📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| label = "IMPROVED" if snap_status == "improved" else "SNAPDIFF" | |
| paint = yellow if snap_status == "improved" else red | |
| self._record(backend, False, name, snap_reason) | |
| label = "IMPROVED" if snap_status == "improved" else "SNAPDIFF" | |
| paint = yellow if snap_status == "improved" else red | |
| if snap_status == "improved": | |
| label, paint = "IMPROVED", yellow | |
| elif snap_status == "regressed": | |
| label, paint = "REGRESSED", red | |
| else: | |
| label, paint = "SNAPDIFF", red | |
| self._record(backend, False, name, snap_reason) |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 1754-1754: Boolean positional value in function call
(FBT003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/check.py` around lines 1752 - 1754, Update the status-label mapping near
self._record in the per-fixture reporting flow so snap_status == "regressed"
produces "REGRESSED", while "fail" remains the only state producing "SNAPDIFF";
preserve the existing "IMPROVED" mapping and associated color behavior.
…at do not reproduce, and resolve a per-platform baseline Three changes to the jit-stats gate. Direction. `_jit_stats_change` now returns (regressions, improvements) instead of one string, classified per counter: a rise in the badness fields or in guard_failures is a regression, a fall in loops_compiled is a regression, and the opposite moves are gains. bridges_compiled is in neither list, so both its directions report as regressions — a fall to 0 is the dead-bridge case when guards still fail and a gain when they stopped (list_length_hint_validate fell 4 -> 0 as guard_failures fell 828 -> 1), and a rise is either wider coverage or a guard storm. Both outcomes still fail; the labels are REGRESSED and IMPROVED, and each carries the --snapshot command that re-records it. Reproducibility. A fixture that disagrees with its baseline is now re-run JITSTATS_STABILITY_RUNS times. If a repeat of the same binary reports different counters, the comparison is reported as UNSTABLE and not gated. Measured on this tree: two runs of one pyre-dynasm binary disagreed by loops_compiled +2 on five unrelated fixtures. Instability is measured per invocation, not annotated per fixture. Per-platform baselines. `_jitstats_baseline_path` prefers <name>.<backend>.<sys.platform>.jitstats when it exists. windows-latest cranelift reports closure_per_call guard_failures 416 and recursive_call_frame_relocation 637 where macos-latest and ubuntu-24.04 both report 415 and 638, on two independent runs, with every other counter equal and that host's dynasm leg passing 371/371. Those two overlays are added here; the shared files still gate the other two platforms exactly. Assisted-by: Claude
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/check.py`:
- Around line 1252-1254: Validate the current snapshot in the flow calling
_jit_stats_change before comparing it with the baseline: require every field
listed in JITSTATS_SNAPSHOT_FIELDS to be present, rather than relying on omitted
fields being interpreted as "0". Reject incomplete snapshots, including missing
loops_aborted, so the corresponding gate is not disabled; leave complete
snapshot comparisons unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2a1b2be3-3039-45dc-9775-51c4100bd08f
📒 Files selected for processing (3)
pyre/bench/synth/closure_per_call.cranelift.win32.jitstatspyre/bench/synth/recursive_call_frame_relocation.cranelift.win32.jitstatspyre/check.py
| regressions, improvements = _jit_stats_change( | ||
| jitstats_path.read_text(encoding="utf-8"), jitstats | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject incomplete current JIT-stat snapshots.
_jit_stats_snapshot omits absent fields, and _jit_stats_change reads each omitted field as "0". If a backend stops emitting loops_aborted while its baseline is 0, this comparison reports no change and disables that counter's gate. Validate every JITSTATS_SNAPSHOT_FIELDS member before comparison.
Proposed fix
if not jitstats_path.exists():
...
+ current_fields = _parse_jit_stats(jitstats)
+ missing_fields = [
+ field
+ for field in JITSTATS_SNAPSHOT_FIELDS
+ if field not in current_fields
+ ]
+ if missing_fields:
+ return "fail", (
+ "missing gated [jit-stats] field(s): "
+ + ", ".join(missing_fields)
+ )
regressions, improvements = _jit_stats_change(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/check.py` around lines 1252 - 1254, Validate the current snapshot in the
flow calling _jit_stats_change before comparing it with the baseline: require
every field listed in JITSTATS_SNAPSHOT_FIELDS to be present, rather than
relying on omitted fields being interpreted as "0". Reject incomplete snapshots,
including missing loops_aborted, so the corresponding gate is not disabled;
leave complete snapshot comparisons unchanged.
The overlay recorded guard_failures=637, while the windows cranelift run produces 638 — the value the shared baseline already carries. The overlay only made the fixture fail on windows. Assisted-by: Claude
Follow-up to #1019, which did the dynasm half. Same drift, other two backends,
plus three changes to the gate itself.
check.pyDirection.
_jit_stats_changereturns(regressions, improvements). A risein the badness fields or
guard_failuresis a regression; a fall inloops_compiledis a regression; the opposite moves are gains. Both still fail— the labels only say whether to investigate (
REGRESSED) or just re-record(
IMPROVED), and each line carries the--snapshotcommand that re-records it.bridges_compiledis deliberately in neither list, so both its directionsreport as regressions. It is not interpretable alone: a fall to 0 is the
dead-bridge regression when guards still fail, and a gain when they stopped —
list_length_hint_validatefell 4 → 0 asguard_failuresfell 828 → 1. A riseis either wider coverage or a guard storm.
Reproducibility. A fixture that disagrees with its baseline is re-run
JITSTATS_STABILITY_RUNStimes. If a repeat of the same binary reportsdifferent counters, the comparison is reported
UNSTABLEand not gated —there is nothing to gate on exactly when the number will not reproduce.
Measured on this tree: two runs of one
pyre-dynasmbinary disagreed byloops_compiled+2 on five unrelated fixtures. Instability is measured perinvocation; no fixture is annotated as flaky.
Per-platform baselines.
_jitstats_baseline_pathprefers<name>.<backend>.<sys.platform>.jitstatswhen present. windows-latestcranelift reports
closure_per_callguard_failures416 andrecursive_call_frame_relocation637 where macos-latest and ubuntu-24.04 bothreport 415 and 638 — two independent runs, every other counter equal, and that
same windows host passing its dynasm leg 371/371. This records the host fact
instead of widening the gate for all 371 fixtures on all three backends.
Baselines
Eight cranelift baselines, all three OS legs reporting identical deltas.
Ten wasm baselines taken from the ubuntu CI log rather than recorded
locally: a local macOS run reports
closure_per_callguard_failures471where ubuntu reports 468, and ubuntu is the only host that runs the wasm leg.
Two independent CI runs reported the ten byte-identically.
wide_callkw_resumecarried nobridges_compiled,guard_failuresorloops_compiledat all. Absent on both sides compares equal, so those threewere never gated on it; they are now written explicitly.
Verification
_jit_stats_changedirection mapping checked on eleven cases. TheIMPROVEDand
UNSTABLEpaths were each exercised end-to-end —UNSTABLEwarns and therun still reports
ALL PASSED,IMPROVEDstill fails.Local dynasm and cranelift suites disagree with CI on a further eight fixtures
on this machine only; they reproduce locally, CI's macOS leg reports the
committed values, and the local
descr_set_resolved(1445) matches CI's macOSexactly. Not yet explained, and not touched here.
— authored by Claude
Summary by CodeRabbit
New Features
Bug Fixes