descr gate: close the wasm hole, and derive index_in_parent from the parent that gets indexed - #892
Conversation
WalkthroughJIT descr-set counters are centralized in the runtime, exported through wasm-host functions, emitted in runner snapshots, and parsed by shared checker logic for equality, diffs, and regression floors. ChangesJIT statistics counters
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant JITRuntime
participant WasmExports
participant WasmRunner
participant CheckPy
JITRuntime->>WasmExports: expose descr-set counter fields
WasmExports->>WasmRunner: return counter values
WasmRunner->>CheckPy: emit normalized [jit-stats] snapshot
CheckPy->>CheckPy: parse fields for comparisons and regression floors
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 8d96dc8). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
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.
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 448-454: Add explicit return type annotations to both
_parse_jit_stats and _jit_stats_fields_equal, using syntax supported by the
repository’s minimum Python version and matching each function’s actual return
value.
🪄 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: 1038d0b9-a358-4314-b4c5-f14e13806200
📒 Files selected for processing (5)
pyre/check.pypyre/pyre-jit-trace/src/jitcode_runtime.rspyre/pyre-jit/src/lib.rspyre/pyre-wasm-runner/src/main.rspyre/pyre-wasm/src/lib.rs
| def _parse_jit_stats(snapshot): | ||
| """Split a normalized jit-stats snapshot into its `field -> value` map. A | ||
| snapshot that was never taken reads as no fields at all, so every caller can | ||
| treat "absent" and "absent from this side" the same way.""" | ||
| if snapshot is None: | ||
| return {} | ||
| return dict(line.split("=", 1) for line in snapshot.splitlines()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add return annotations to satisfy Ruff.
Ruff reports ANN202 for _parse_jit_stats and _jit_stats_fields_equal; add explicit return annotations consistent with the repository’s supported Python version.
Also applies to: 457-461
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 448-448: Missing return type annotation for private function _parse_jit_stats
(ANN202)
🤖 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 448 - 454, Add explicit return type annotations
to both _parse_jit_stats and _jit_stats_fields_equal, using syntax supported by
the repository’s minimum Python version and matching each function’s actual
return value.
Source: Linters/SAST tools
`_jit_stats_fields_equal`, `_jit_stats_diff` and `_jit_stats_regression_floor` each defined the same nested `parse`, splitting snapshot lines on the first "=" and reading `None` as no fields. Move it to `_parse_jit_stats` and call it from all three. Assisted-by: Claude
The wasm module exported only `loops_aborted` and `internal_compile_panics`, so
the three descr-universe members of `JITSTATS_BADNESS_FIELDS` never reached the
`[jit-stats]` line on that backend. `_jit_stats_regression_floor` reads a field
missing from the current run as 0, so a wasm-only `descr_set_absent`,
`descr_set_ambiguous` or `descr_set_stale_absent` passed the hard gate.
Add `pyre_jit_descr_set_{absent,ambiguous,stale_absent,resolved}` guest exports
and print them from the runner's MAJIT_STATS line. Both they and
`descr_set_jit_stats` now read `descr_set_counts`, so the gated numbers cannot
drift from the printed ones.
Assisted-by: Claude
`get_field_descr` took `index_in_parent` from its caller. Upstream has no such argument: `heaptracker.py:60-72 get_fielddescr_index_in` is the only numberer and shares its skip set with `:96-112 all_fielddescrs`, so `all_fielddescrs(S)[i].get_index() == i` holds by construction. pyre numbers fields in several places and they disagree. The analyzer's `field_pos` and the assembler's `specs.len()` both count the two header words at offsets 0 and 8 that the runtime publish skips, because the skip sets drop only `typeptr` and pyre spells that header `ob_type` / `w_class`. `all_fielddescrs()[index_in_parent]` is load-bearing at `optimizeopt/info.rs` `force_box`, which `.expect`s it in release builds, so a field carrying one convention under a parent numbered by another is either an out-of-range panic or a store emitted against a different field. Derive the index from the parent that will actually be indexed, matching on the cache key. Where there is nothing to derive from — no parent published, or a parent with an empty list — or where the name is absent from a non-empty list, the caller's index stands. Match on the name only. An offset fallback was measured to be unsound while one struct is still reachable under several identity keys: the parent found can belong to a different struct, and a same-offset field of another type then reads as this one (`Const::getint on non-Int variant: Ref(..)`, 22/4/17 synth failures across the three backends). Add `field_position_jit_stats`, because `descr_set_absent` / `descr_set_ambiguous` cannot see any of this: they ask whether a member resolved to some descr, not whether it resolved correctly. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d96dc8a27
ℹ️ 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".
| fd.index_in_parent = | ||
| Self::derive_index_in_parent(parent.as_ref(), field_name, offset, index_in_parent) | ||
| .unwrap_or(index_in_parent); |
There was a problem hiding this comment.
When a runtime descriptor group is the first publisher, make_simple_descr_group_keyed_with_headerless calls get_field_descr before register_keyed_size, so this lookup sees no parent and preserves the header-counting caller index; the later registration only changes the parent backreference, not index_in_parent. The result therefore still depends on publication order and the runtime-first path can retain the out-of-range or wrong-field index this change is meant to eliminate. Fix the independent field-list producers so they use the same upstream numbering rather than repairing only cache misses.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| fd.index_in_parent = | ||
| Self::derive_index_in_parent(parent.as_ref(), field_name, offset, index_in_parent) | ||
| .unwrap_or(index_in_parent); |
There was a problem hiding this comment.
In debug and test builds, whenever this branch actually rederives a caller index, a subsequent lookup from the same producer hits the cache and the earlier debug_assert! compares the normalized cached index against the original caller index through describes_same_field. For example, a first lookup corrected from 2 to 0 makes the next identical lookup assert that cached 0 equals requested 2, so ordinary descriptor reuse panics precisely in the mismatch scenario this code handles. Normalize the expected index before performing the cache-hit validation.
Useful? React with 👍 / 👎.
| let mut counter = |name| { | ||
| instance | ||
| .get_typed_func::<(), u64>(&mut store, name) | ||
| .and_then(|f| f.call(&mut store, ())) | ||
| .unwrap_or(0) | ||
| }; |
There was a problem hiding this comment.
When PYRE_WASM_MODULE points to an older or otherwise incompatible module that lacks one of these new exports, get_typed_func fails and this converts the failure to the healthy value 0. Because _jit_stats_regression_floor also interprets a missing baseline/current field as zero, such a module silently bypasses the descriptor gate this change is intended to close. Under MAJIT_STATS, missing or wrongly typed badness-counter exports should fail the run rather than be reported as zero.
Useful? React with 👍 / 👎.
…e hypotheses survive (#905) * descr: census the fieldless size descrs and whether their layout exists elsewhere `get_size_descr` returns on cache hit (descr.py:108-109), so whoever publishes a key first decides what every later consumer sees. A mint publishing `get_size_descr(key, size, 0, false)` — vtable-less and caller-sized, a shape descr.py:111-116 cannot produce — therefore outranks a real layout published after it. Nothing measured how often that happens or whether the displaced layout existed at all. `GcCache::size_shell_census` reports `[published, fieldless, shadowing, aliased, aliased_multi]`. `shadowing` is a fieldless parent for which `_cache_field` holds fields under the SAME key; `aliased` is a shadowing parent whose struct also reaches the cache under another key that did get a populated list. On dynasm/int_loop (identical on fannkuch and nbody): size_shell_published=1106 size_shell_fieldless=693 size_shell_shadowing=693 size_shell_aliased=572 size_shell_aliased_multi=190 `fieldless == shadowing` exactly: every fieldless parent in the cache has fields cached under its own key. 572 of them have a shape-matching populated twin under a different key; 190 of those match on two or more slots. `aliased` matches on shape — equal `size()` and every cached `(offset, field_size)` present in the candidate — not on owner name. An earlier name-based version reported 0: `get_field_descr` falls back to a `T<type_id>.` stand-in for mint sites carrying only the numeric struct identity, so all 693 shell owners are `T<id>` strings that cannot match a real `Owner.field` spelling. `size_shell_owner_sample` prints both sets under `PYRE_SIZE_SHELL_OWNERS=1` so that zero cannot be read as a finding again. `aliased_multi` restricts to shells carrying two or more cached fields, since a one-field shell matches any same-size struct owning that slot. These are diagnostic: check.py narrows snapshots to `JITSTATS_SNAPSHOT_FIELDS`, so no baseline records them. Also restores three doc comments that 8d96dc8 detached from their items by inserting between the comment and what it documented — `GcCache`, `GcCache::get_field_descr` and `descr_set_counts` — and drops the sentence in `get_field_descr` describing an offset fallback that commit did not keep. Assisted-by: Claude * descr: normalise the cache-hit check, and stop reading a missing wasm export as 0 Two findings from the review of #892. `get_field_descr` normalises `index_in_parent` against the parent, so the cached descr holds the derived number while callers keep handing in their own. The cache-hit `debug_assert!` compared the cached value against the raw argument, so the SECOND lookup of any rederived field asserted that (say) cached 0 equals requested 2 — firing on exactly the fields the normalisation exists for. Only debug builds carry `debug_assert!`, so the release gate could not see it; `field_pos_rederived=21` on int_loop is how many fields were eligible. The check now runs the caller's index through the same parent lookup first. `find_index_in_parent` is that lookup with no census side effect, because the census measures what producers hand in and a cache hit is one producer's answer read back, not a second producer disagreeing. `derive_index_in_parent` keeps the counters and delegates. Its unused `offset` parameter goes with it — the offset fallback it was for is not coming back while one struct is reachable under several identity keys. `a_rederived_field_survives_a_second_lookup` covers it; it panics without the change. Second: the wasm runner resolved every `MAJIT_STATS` counter with `unwrap_or(0)`. Zero is the healthy value for all six, and `_jit_stats_regression_floor` also reads a field missing from a run as zero, so a module built before these exports existed would have gated on nothing while looking perfectly green — the same vacuous-gate hole this PR set out to close, reintroduced one level down. Missing or wrongly typed exports are now named and the run exits non-zero. Assisted-by: Claude * descr: check the two invariants descr.py gets for free, and name what fails The shell census said 693 parents list nothing while holding fields. That says where the layout is, not whether anything is currently wrong, so add the two checks upstream never needs and read them. `positional_invariant_census` checks `all_fielddescrs(S)[i].get_index() == i`. heaptracker.py:60-72 and :96-112 are one walker sharing one skip set, so upstream this holds by construction; pyre builds the list from cache-or-mint results, so a descr minted earlier by a header-counting producer could sit at a position it disagrees with. Measured on int_loop: positional_slots=1794 positional_misplaced=0. The lookup `optimizeopt/info.rs force_box` depends on is intact. `identity_collision_census` checks whether one key carries more than one struct, by comparing a cached field's offset against the same field in its parent's list. descr.py keys on the lltype STRUCT object, so a key means one struct; pyre keys on `path_hash(<some spelling>)` minted by several producers. Measured: key_compared=1766 key_conflicting=0. A field cannot be at two offsets in one struct, so zero here is zero collisions among fields both sides name. That leaves `field_pos_unresolved` — fields cached under a key whose parent lists other fields but not them. `size_shell_owner_sample` now names them, and they are one shape: the cached key is a bare `__pos_0` while the parent lists `__pos_0.flags`, `__pos_0.ob.w_class`, `__pos_0.deque`. 178 distinct. That is the key SPELLING split, not two structs sharing a key. Assisted-by: Claude
Follow-up to #876 (merged as
8f0ead8492). Three commits: two close reviewfindings raised on that PR after it landed, one closes a defect the review
pointed at that turned out to be reachable in release builds.
1. The wasm backend passed a gate it could not fail
#876 put the descr-universe invariants in
JITSTATS_BADNESS_FIELDS. On wasm themodule exported only
pyre_jit_loops_abortedandpyre_jit_internal_compile_panics, so the threedescr_set_*counters neverreached the
[jit-stats]line — and_jit_stats_regression_floorreads a fieldmissing from the current run as 0. A wasm-only regression passed silently.
Adds
pyre_jit_descr_set_{absent,ambiguous,stale_absent,resolved}guest exports,printed from the runner. Both they and
descr_set_jit_stats()read one producer,descr_set_counts(), so the gated numbers cannot drift from the printed ones.Observed rather than assumed:
No baseline re-record needed: the committed wasm baselines carry only the two old
keys, the run emits the new ones as 0, and missing-as-0 compares them equal. A
future 0 -> N still fires, because a field missing from the baseline also reads
as 0.
2. The snapshot parser was defined three times
_jit_stats_fields_equal,_jit_stats_diffand_jit_stats_regression_flooreach carried the same nested
parse. Extracted as_parse_jit_stats.3.
index_in_parentcame from the caller, and callers disagreeUpstream has no such argument.
heaptracker.py:60-72 get_fielddescr_index_inisthe only numberer and shares its skip set with
:96-112 all_fielddescrs, soall_fielddescrs(S)[i].get_index() == iholds by construction.pyre numbers fields in several places and they disagree: the analyzer's
field_posand the assembler'sspecs.len()both count the two header words atoffsets 0 and 8 that the runtime publish skips, because the skip sets drop only
typeptrwhile pyre spells that headerob_type/w_class.This is not cosmetic.
all_fielddescrs()[index_in_parent]is load-bearing atoptimizeopt/info.rsforce_box, which.expects it in release builds — soa field carrying one convention under a parent numbered by another is either an
out-of-range panic or a store emitted against a different field. The existing
field_descr_identity_censuscannot see it: it compares Arc identity only, and aminted field publishes its own parent, so it reports
Converged.get_field_descrnow derives the index from the parent that will actually beindexed. Where there is nothing to derive from — no parent published, or an empty
list — or where the name is absent from a non-empty list, the caller's index
stands.
Match on the name only. An offset fallback was tried and measured unsound: one
struct is still reachable under several identity keys (
intvalunder three,PyFrameunder two), so the parent found can belong to a different struct and asame-offset field of another type reads as this one. That produced 22/4/17 synth
failures across the three backends with
Const::getint on non-Int variant: Ref(..)and SIGSEGV. Closing that key aliasing is separate work.Also adds
field_position_jit_stats, becausedescr_set_absent/descr_set_ambiguouscannot answer this question — they ask whether a memberresolved to some descr, not whether it resolved correctly, and they read 0
throughout the above.
Verification
cargo test -p majit-ir -p majit-metainterp: 1414 + 295 + … passed, 0 failed.Deliberately not in this PR
Keying
_cache_fieldon the bare field name.descr.py:218-233keys onfieldnameand uses the qualified spelling only for the displaynameat :227;several pyre groups spell the struct into the key itself. Stripping the prefix
alone breaks the build:
assembler.rs bh_field_namequalifies a name only when itdoes not already contain a
., so the prefixed runtime keys and the serializedspec keys were agreeing through that heuristic. Both sides have to move in one
commit.
Declining to publish a fieldless parent.
mint_fieldpublishes a parent viaget_size_descr(key, size, 0, false)— vtable-less and caller-sized, whichdescr.py:111-116makes impossible — and nothing fills itsall_fielddescrs.The census shows the state is real and large (
field_pos_parent_empty), butwhether any of it reaches a consumer is unmeasured, and declining would raise
descr_set_absentoff zero. Consumer-side counters first.🤖 Generated with Claude Code