bench: re-record the wasm jit-stats baselines main has been red on - #1077
Merged
Conversation
`optimize_setfield_gc` writes a virtual's field list at `FieldDescr::index_in_parent` and asserted only that the index was in bounds. `force_box` reads that slot back out of the struct descr's `all_fielddescrs()` (`optimizeopt/info.rs`), so the two must name the same field — `info.py:206` writes `self._fields[fielddescr.get_index()]` and `info.py:219-220` reads `for i, fielddescr in enumerate(descr.get_all_fielddescrs()): fld = self._fields[i]`. A bound does not establish that pairing. Replace both `debug_assert!`s with `field_slot_disagreement`, which compares the slot's name and offset against the field descr that supplied the index and returns the disagreement as a message; out-of-bounds keeps the previous wording. The name is compared only when both sides carry one, since the flattened inline aggregates reach this point under the existing empty-name fallback. The two producers that build such a list rank fields differently: `codewriter/assembler.rs bh_all_field_specs_for_struct_into` walks declarations, `jitcode/assembler.rs register_struct_layout` sorts by byte offset. Measured over 120928 virtual setfields across 1172 programs the pairing holds everywhere, including the 2286 that indexed a descr other than the field's own parent, so this states a postcondition rather than fixing an observed failure. Returns `None` in release builds. Assisted-by: Claude
`pyre/check.py` has failed on the wasm backend for every main commit since
`fa2eda0bd8e`, which moved `guard_failures` on four synthetic benches and did
not re-record them. `58fcd373e05`, the commit before it, reports no jit-stats
rows; `df18a261edf` and `f8526c1924f` likewise.
closure_per_call 470 -> 468
exception_traceback_frame_lineno 820 -> 819
recursive_call_frame_relocation 649 -> 648
gc_iterator_source_drop 613 -> 614 regressed
The fourth is a regression and is recorded as one, not as an improvement: it is
carried here so the wasm gate reports the state main is actually in rather than
failing on all four. No other counter moved — `loops_compiled` and
`bridges_compiled` already matched the observed values in every file.
The numbers are identical on main's ubuntu CI, on this branch's ubuntu CI and on
a local macOS run, so they are a deterministic property of the base rather than
runner noise. wasm jit-stats rows are only produced by the ubuntu job.
`field_pos_spec_misplaced` / `field_pos_attached_misplaced` appear in all 14
files because the wasm baselines never carried the two counters `JITSTATS_BADNESS_FIELDS`
gained; `--snapshot` writes them out. Both are 0, and a field missing from
either side already reads as 0 (`check.py:834`), so the added lines change no
comparison.
Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
WalkthroughThe change adds debug validation for virtual field-slot names and offsets. It also adds misplaced field-position counters to benchmark JIT statistics and updates several synthetic benchmark guard-failure values. ChangesVirtual field validation and benchmark statistics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 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 83d285b). The diff against the review base changes only generated jit-stats baselines (14 file(s)); nothing to review. |
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.
Re-records the wasm
jit-statsbaselines thatpyre/check.pyhas been failingon for every main commit since
fa2eda0bd8e.What moved
The fourth is a regression and is recorded as one, not dressed up as an
improvement. It is carried here so the wasm gate reports the state main is
actually in rather than failing on all four rows at once and hiding which is
which. No other counter moved —
loops_compiledandbridges_compiledalreadymatched the observed values in all 14 files.
Why these are the base's, not this branch's
Bisected against main's own CI:
f8526c1924fdf18a261edf58fcd373e05fa2eda0bd8efa2eda0bd8emoved the counters and did not re-record them. The values arebyte-identical on main's ubuntu CI, on this branch's ubuntu CI, and on a local
macOS run, so they are a deterministic property of the base rather than runner
noise. wasm jit-stats rows are only produced by the ubuntu job.
The 28 inert lines
field_pos_spec_misplaced/field_pos_attached_misplacedappear in all 14files because the wasm baselines never carried the two counters
JITSTATS_BADNESS_FIELDSgained;--snapshotwrites them out. Both are 0, anda field missing from either side already reads as 0 (
check.py:834), so theadded lines change no comparison.
Not addressed here
The remaining
check.pyfailures aremax-pypy-ratiogates where pypy sits atthe
EXEC_TIME_FLOOR_S0.01 s floor, so the ratio is denominator-collapsednoise. Re-recording those would bless noise, which is the opposite of what this
PR does for the wasm rows, so they are left failing.
Branch state
This branch is currently 2 behind
origin/mainand its first commit(
533135e6d49) is patch-identical to the already-merged #1072(
84d89463312) — samegit patch-id. A rebase ontoorigin/maindrops it andleaves only the re-record; until then the "Files changed" view shows
virtualize.rsa second time, and merging it is a content no-op.— authored by Claude
Summary by CodeRabbit