Repository navigation
check.py: gate every recorded jit-stats counter in both directions; re-record 28 stale cranelift baselines - #1007
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 43 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 (2)
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: 9c4ff0f8d3
ℹ️ 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".
| if old_fields.get(field, "0") != new_fields.get(field, "0") | ||
| ] | ||
| if changes: | ||
| return "jit-stats change: " + ", ".join(changes) |
There was a problem hiding this comment.
When a counter changes in the Linux informational stats step, this new jit-stats change text is captured by .github/workflows/pyre-ci.yml:433-436, but that workflow still greps only for jit-stats diff|snapshot diff. Consequently the job summary reports (no jit-stats output) instead of showing the actual counter delta; update the workflow pattern alongside this rename.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 936ea39). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
The recorded surface and the gated surface are now the same set. Previously `_jit_stats_regression_floor` gated `loops_aborted` and the descr-universe counters on a rise, `guard_failures` on a rise past `base + max(base // 4, 2)`, and `loops_compiled` on a fall; `bridges_compiled` was recorded but gated nowhere, so a bridge collapse passed as long as `guard_failures` stayed inside its band. Exact equality over `JITSTATS_SNAPSHOT_FIELDS` replaces all four rules, so a fall records too. The band went with it. It covered cross-runner drift that had never been measured; one CI run has since reported `list_length_hint_validate guard_failures 828 -> 4923` and `exception_args_virtual 401 -> 1002` with identical digits on ubuntu-24.04, macos-latest and windows-latest. A run whose stderr carries no `[jit-stats]` line now fails instead of being skipped. `pyre_env` sets MAJIT_STATS=1 for every pyre run, and reading an absent line as "nothing to compare" skipped the missing-baseline check and the comparison with it. The tally is printed on every non-record run rather than only under --snapshot-diff, which the bare CI invocation never passes. The jit-stats half of --snapshot-diff is removed as unreachable, along with `_jit_stats_fields_equal` and `_jit_stats_diff`. Assisted-by: Claude
cranelift files kept their pre-#997 numbers. Every difference is a fall — `bridges_compiled` and `guard_failures` down, `loops_aborted` down — which is the direction the previous gate treated as free, so nothing reported them. The recorded values agree with the dynasm baselines already on main for 27 of the 28 fixtures, digit for digit on all four counters. The one difference is `inline_gate_operand_provenance`, `guard_failures` 4 on dynasm against 5 here. Recorded with `pyre/check.py --snapshot --backend cranelift --synthetic-only`. The dynasm and wasm passes of the same run rewrote their baselines to identical content, so only cranelift appears here. Assisted-by: Claude
The comment cited `JitCounter.fetch_next_hash` as the reason `guard_failures` is deterministic. That method hands a compiled guard a counter-slot hash; it does not count failures. `guard_failures` is a plain increment at `record_guard_failure_event` (pyjitpl.rs). State what the counters actually depend on: the event stream, which is decided by the compile decisions `JitCounter` drives, and whose hashes come from the pure `_nexthash` sequence rather than an object address. Also cite `OpMatcher.match` (test_pypy_c/model.py), which compares a compiled loop operation by operation against a literal expected trace, as the upstream instrument the exact-equality gate is a coarser projection of. Assisted-by: Claude
`default_nursery_size` (collector.rs) falls back to `estimate_best_nursery_size`, which is half the L2 cache once that exceeds 8MB and 4MB otherwise. The probe behind it is macOS-only — `get_l2cache` is `-1` under `cfg(not(target_os = "macos"))` — so two macOS machines can compute different nursery sizes while every Linux and Windows host already sits on the 4MB fallback. A macos runner reported 19 differences against baselines recorded on another Mac. Every one of them was `guard_failures`, most were identical on both backends, and no `loops_compiled` or `bridges_compiled` moved, so the difference was in the collection schedule rather than in codegen. Sweeping `PYPY_GC_NURSERY` reproduces that runner exactly at 6MB on all eight fixtures sampled — fib_loop 189, dict_set 609, listcomp_hot 239, nested_list_comprehension_hot 402, recursive_call_frame_relocation 636, str_fstring 657, closure_per_call 415, type_name_setter 2 — while 4MB reproduces the committed baselines. Set PYPY_GC_NURSERY to 4MB in `pyre_env`, the documented unknown-cache value and what the non-macOS hosts compute anyway. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 936ea3924b
ℹ️ 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".
| # synthetic fixtures without anyone noticing. | ||
| # * A counter that moved, in either direction. | ||
| if self.args.snapshot_mode != "record": | ||
| if jitstats is None: |
There was a problem hiding this comment.
If the structural counter line disappears while any other [jit-stats] diagnostic remains, _jit_stats_snapshot returns an empty or partial string rather than None, so this new absence check is bypassed. _jit_stats_change then reads every missing field as 0; consequently fixtures whose committed counters are all zero can still pass while reporting none of the counters this gate is meant to enforce. Check that the snapshot contains the required gated fields, not merely that some prefixed line was emitted.
Useful? React with 👍 / 👎.
youknowone
added a commit
that referenced
this pull request
Aug 4, 2026
…ectional gate #1007 made every recorded counter gate in both directions, so the changes this branch already made to what the JIT compiles now report instead of passing as unmeasured improvements. Recorded with `check.py --snapshot`; dynasm and cranelift observe the same values on all 33, so none of them is a backend divergence. Audited every recorded line: `internal_compile_panics` and the `descr_set_*` counters do not move at all, no `loops_aborted` rises and no `loops_compiled` falls. What moves is `loops_aborted` 5 -> 0 and 1 -> 0 (three benches), `loops_compiled` up on nineteen, and `bridges_compiled` down with the matching `guard_failures` — 10 -> 4 with 2027 -> 819, 4 -> 0 with 828 -> 1, 6 -> 4 with 1203 -> 802, 10 -> 7 with 2018 -> 1655. Every `guard_failures` rise sits in a run whose `loops_compiled` also rose, which is the new loops' guards failing `trace_eagerness` times each before their bridges attach. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 4, 2026
…ral or better direction Same two-directional gate (#1007) as the dynasm/cranelift re-record, run with `check.py --backend wasm --snapshot`. `loops_aborted` falls 5 -> 0 and 2 -> 1, `loops_compiled` rises on eleven, and every `guard_failures` rise sits in a run whose `loops_compiled` or `bridges_compiled` also rose. Three wasm rows are deliberately left at their committed values so the gate keeps naming them: `list_append_write_barrier_gc` (`loops_aborted` 0 -> 1), `recursion_memo_branch` (`loops_aborted` 1 -> 2) and `pickle_terminal_raise_resume` (`loops_compiled` 74 -> 73). All three already failed on this branch under the previous one-directional gate, so they are not a consequence of this re-record and none of them is blessed by it. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 4, 2026
…sted-break gate (#945) * faulthandler: leave the RUNNING census while contending on the state lock A thread parked in `FAULTHANDLER_STATE_LOCK`'s futex can no longer poll the eval breaker, so it stayed in the set a collector waits to drain. `register` builds its `OSError` while holding that guard and `os_error_syscall2` allocates, so a failed install could request a stop-the-world that the parked waiter was unable to acknowledge. Acquire through `lock_faulthandler_state`, which brackets the blocking path with `before_external_block` after a `try_lock`, the same split `w_list_lock` uses. Also record on the guard that the host install/removal runs inside it, and on `walk_faulthandler_roots` that the walk runs inside the stop-the-world window and must not take the guard. Assisted-by: Claude * jit: publish every pushvalue slot and port space.newbool `push_and_bump!` bumped `valuestackdepth` without emitting the `setarrayitem_vable_r` that `jtransform.py:1898 do_fixed_list_setitem` gives every `pushvalue`. `capture_resumedata` (pyjitpl.py:2611-2622) carries the whole `virtualizable_boxes` list and `virtualizable.py:126-137 write_from_resume_data_partial` writes back every element the published depth describes, so a slot the trace never bound restored as NULL. Define the macro in terms of `emit_pushvalue_ref!` so all 44 sites emit the store. The boxed COMPARE_OP result was an opaque `CallR(jit_bool_value_from_truth)` the optimizer cannot DCE once that store makes it live. `baseobjspace.py:896-900 newbool` returns one of two prebuilt singletons after a jitcode branch, recorded by `pyjitpl.py:511-520 opimpl_goto_if_not` as `guard_true`/`guard_false` and skipped for a `Const` box by `pyjitpl.py:2582-2584 generate_guard`. `walker_newbool` emits that shape at the five `emit_trace_bool_value_from_truth` sites; the helper and `compare_box_provably_dead` become dead and are removed. The commit's third change, consuming `reconstruct_inline_recipe`'s register section before the frame image, is dropped: `origin/main` landed the same fix as `stream_slots` / `stream_covered` / `overlay_stream_ref_slots`, which applies it to the materialized frame arm as well. Assisted-by: Claude * jit: admit SET_ADD and MAP_ADD in the FOR_ITER body scan `pyopcode.py:1515 SET_ADD` is `space.call_method(w_set, 'add', w_value)` and `pyopcode.py:1525 MAP_ADD` is `space.setitem(w_dict, w_key, w_value)` — the `LOAD_ATTR` + `CALL` pair and `STORE_SUBSCR` the scan already admits. The codewriter lowers both to a void `residual_call_r_v` (`bh_set_add_fn` / `bh_map_add_fn`), so neither is folded and both carry the body-effect accounting of the residual they are. Assisted-by: Claude * io, posix: check signals before retrying a read on EINTR `fd_read_into`, `posix.readinto` and the buffered read path retried an `EINTR` read in a bare Rust loop, and `posix.read` did not retry at all. Neither shape runs the pending Python signal handlers between the interruption and the retry, which `error.py:805-808` `wrap_oserror2(..., eintr_retry=True)` does via `space.getexecutioncontext().checksignals()`. Skipping that step hangs a read whose remaining bytes the handler itself writes: `signal.alarm(1)` firing into a blocked `read(6)` never gets its `os.write` executed, so the read waits forever for bytes no one sends. The no-retry shape instead surfaced `InterruptedError`, which PEP 475 removed. `lib-python/3/test/test_io.py` stopped at `CSignalsTest.test_interrupted_read_retry_buffered` and never finished. `eintr_retry_with` is the shared arm: non-`EINTR` errno goes to the call site's own OSError wrapper, `EINTR` runs `checksignals_now()` and returns `Ok(())` for the caller to re-issue. `fd_read` now carries a `PyError` so its loop can run handlers, and the `posix` loops scope the blocking guard to the syscall alone, since `checksignals` runs Python. Assisted-by: Claude * jit: pin the forced-callee kept-stack SIGSEGV and cover stream-covered slots `bench/synth/recursive_forced_frame_kept_stack.py` is a self-recursive function whose frame is forced by `locals()` with a kept operand-stack slot beneath an inlined comprehension. It exited 139 before the reconstructed-callee slot source was fixed. The fix itself is already on `origin/main` as `stream_slots` / `stream_covered` / `overlay_stream_ref_slots`, so this commit's own version of it is dropped. What that version's tests covered and `origin/main`'s do not is the `stream_covered` mask itself: `materialized_inline_frame_slots_skip_stream_covered_slots` asserts a marked slot is left NONE/Void with no `GETARRAYITEM_GC_R` emitted for it. Assisted-by: Claude * deque: take the block-chain lock in snapshot, store and clear_blocks `origin/main` landed the striped `w_deque_lock` on `append_right`, `append_left`, `pop_right` and `pop_left`. `snapshot` walks the whole block chain, `clear_blocks` rewrites both endpoints and the length, and `store` sequences the two — all three read or write the same chain those four mutate and were left unguarded. The lock is reentrant, so `store`'s guard nests with the `clear_blocks` and `append_right` guards it calls. Each site takes the same push_roots/pin_root/shadow_stack_get shape the existing four use, because acquiring under contention enters the GC-aware external-block region and the receiver can move. Assisted-by: Claude * jit: latch the resume-past image before withdrawing the escape commit `origin/main` already dropped `writes_live_heap` and the frame-entry odometer from the single-frame latch gate, but the withdraw (`cancel_committed_frame_escape_pc` + `restore_escape_flush_undo`) still ran unconditionally ahead of it and the latch still required `committed_frame_escape_pc().is_none()`. So an escape whose residual entered a user Python frame reached the build with the pre-force frame already restored underneath an image built for the forced one, and `apply_single_frame_blackhole_crn` writes into that disagreement. Build and latch first; run the withdraw afterwards and only when no image was latched. The committed escape pc no longer gates the latch: `try_adopt_single_frame_blackhole` takes it and discards the flush undo when it adopts, and `run_perfn_walk`'s epilogue restores the undo when it declines, so leaving it standing keeps a declined adopt on the path it has today. Drops the now-unreachable [latch-vs-bh] feasibility probe. Fixes two wrong answers on both backends. A frame-inspecting property with a side effect ran 5 extra times (`20000 20005`); with an irreversible body effect earlier in the same iteration the FOR_ITER item was dropped instead and its body tail lost (`19995 20000`). Both now match the interpreter at `20000 20000`. Assisted-by: Claude * jit: start the nested-break inner-header scan after the enclosing FOR_ITER `nested_break_bridge_resume_hazard` scanned `target..pop_pc` for the inner loop's FOR_ITER, where `target` is the enclosing loop's own header. With no nested loop in the body the scan named that header as the inner one, so any single-level loop whose body ends in a statement-result POP_TOP before the backedge, and whose guard contains a secondary-edge branch, was declined as a nested break. Scan `(target + 1)..pop_pc` instead. Assisted-by: Claude * jit: per-guard deopt census behind MAJIT_GUARD_CENSUS `guard_failures` is a total, so it cannot distinguish a guard that keeps returning to the runtime because no bridge was attached to it from a wide spread of guards each still below `trace_eagerness`. Those two want opposite fixes and the difference is a distribution. `guard_census_record` tallies `(green_key, fail_index)` on the deopt path and `guard_census_summary` renders `distinct` / `total` plus the heaviest guards, printed with the other `[jit-stats]` lines. Both the tally and the print are gated on `MAJIT_GUARD_CENSUS` because the map write sits on the path under study; the env read is behind a `OnceLock`. Assisted-by: Claude * jit: rustfmt the guard census Assisted-by: Claude * gc: per-minor-collection drain census under MAJIT_GC_DRAIN_CENSUS Records, for each minor collection, the nursery bytes it found, the bytes it copied out to the old generation, and the pinned objects left behind, plus a survival-rate decile histogram. Promoted bytes are read as the delta of `bytes_made_old_since_cycle` across the collection, sampled before the major-progress step that resets it. Setting the variable to a positive integer also dumps the running summary every that many collections, so a run that has to be killed rather than waited out still reports. The summary prints from the `MAJIT_STATS` block in pyrex, alongside the guard census. Assisted-by: Claude * interp: gate eintr_retry_with out on wasm32 `libc::EINTR` and `crate::module::signal` (module/mod.rs:100-101, itself `cfg(not(target_arch = "wasm32"))`) do not exist on wasm32, and the function had no cfg of its own while every caller was already excluded there. The Charon/LLBC wasm32 layout extraction failed to compile it. Assisted-by: Claude * jit(intbounds): re-dispatch de-overflowed INT_{ADD,SUB,MUL} from the first pass optimize_int_{add,sub,mul}_ovf returned `OptimizationResult::Emit` for the replaced non-overflowing op. `Emit` goes straight to `emit_operation` and skips every remaining pass; `intbounds.py:254-255/285-286/301-302` call `send_extra_operation(op)` with `opt=None`, which re-dispatches from `first_optimization` — `Restart` in pyre. Consequence: an `IntSub`/`IntAdd`/`IntMul` whose two arguments are both constant never reached `OptPure`'s all-constant fold (`pure.py:121-136`) and survived into the compiled trace. `_binaryop_or_lea` asserts `arglocs[0]` is a `RegLoc` (`x86/assembler.py:1276`), so on x86_64 such an op panicked `compile_loop` (`global_cell_shortpreamble_hot`, `unpack_drain_star_raise`); aarch64 accepted it and emitted the unfolded arithmetic. Assisted-by: Claude * jit: take the compiled-loop meta before the run, drop it from the exit re-lookup `run_compiled_raw_detailed_with_values` and `run_compiled_detailed` held the `&CompiledEntry` from before `execute_token` across the run; the dispatch-key runner instead re-looked it up afterwards with `.unwrap()`. Neither survives re-entry: compiled code makes residual calls into the interpreter, which re-enters the driver by minting a second `&mut JitDriverPair` out of the `JIT_DRIVER` thread-local cell (`pyre/pyre-jit/src/eval.rs:3977`, `:4393`), and that re-entry can retire the green key (`pyre/pyre-jit/src/eval.rs:7887` range-FOR_ITER demotion, `jitdriver.rs:4361` unrecoverable resume). The borrow checker does not see the second `&mut`, so the held reference dangles and the re-lookup panics. `CompileResult`/`RawCompileResult` now own their `meta`, cloned before the run: the exit values being unpacked were produced by the code that metadata describes, mirroring `warmstate.py:398`'s hold on the `loop_token` object. The trace lookup stays after the run — a bridge compiled during the run is only visible there — but is fallible. When that lookup misses, the synthesized fallback layout takes its resume payload from the failing descr through `ResumeStorage::from_fail_descr`, which is where upstream reads it from in the first place (`compile.py:849 get_resumestorage(): return self`, `compile.py:855 ResumeGuardDescr._attrs_`). The green-key index is a second index of the same `rd_numb`/`rd_consts`/ `rd_virtuals`/`rd_pendingfields` already carried on `majit_ir::FailDescr`. Assisted-by: Claude * wasm: drain the guest stderr buffer on a trap, and route the exception-channel reporter through the fd-2 seam The guest has no descriptors, so everything it writes to fd 2 accumulates in linear memory until `pyre_take_stderr` hands it over, and only the success path called that export. A trap therefore discarded the whole channel at the one moment it carries the most: the diagnostics a fatal path emits just before aborting. `take_guest_stderr` factors the drain out and the trap path now runs it before returning the error. `reject_non_exception_channel_value` reported through `eprintln!`, which is a silent sink in the guest, so its two lines never reached the host. Sends them through `pyre_interpreter::host_seam::emit_stderr` instead. The `blackhole_resume_via_rd_numb` failure context gains `guard_exc`, the Python pc for the failing and entry jitcode positions, `deadframe_types`, the raw deadframe, and the ref register bank; its positional arguments were also shifted by one against their labels. Assisted-by: Claude * bench: re-record the dynasm/cranelift jit-stats baselines this branch moves Ten synthetic fixtures whose committed .jitstats predate this branch's tracing changes. Every one of them compiles strictly more than the baseline recorded: set_hash_protocol loops 0->1 bridges 0->1 loops_comprehension loops 4->7 bridges 5->12 list_append_write_barrier_gc loops 1->12 bridges 1->5 exception_try_call_inlined_callee_raise loops 3 bridges 2->3 global_quasiimmut_invalidation loops 2 bridges 2->5 kept_stack_deep_var_shortcircuit loops 6 bridges 4->10 polymorphic_slot_retype loops 1->2 bridges 4->5 recursion_memo_branch loops 2->3 bridges 13->16 imp_lock_rlock_semantics loops 0->1 bridges 0 The guard_failures rise tracks that: a guard fails trace_eagerness (=200, rlib/jit.py:590) times before its bridge attaches, and the deltas are proportional to the newly compiled guards, not to extra guards on unchanged code. The floor gate fails on a fall in loops_compiled and a rise in guard_failures, so it can only report the cost of compiling more. exception_try_call_inlined_callee_raise also drops loops_aborted 1->0. list_append_write_barrier_gc and recursion_memo_branch gain loops_aborted 0->1; both are benches where loops_compiled rose, and mc_diag shows cb_entered == bridges_compiled with the compile_bridge arity giveup at 0, so no compile-side giveup fired — it is a root trace abort with permanent=false, retried. recursive_forced_frame_kept_stack had no committed baseline on either backend; recorded. exception_args_virtual and list_length_hint_validate are left alone: they fail on main as well, so their baselines are not this branch's to rewrite. Verified: pyre/check.py --synthetic-only --backend dynasm,cranelift is 353 passed / 2 failed per backend, the two being those inherited rows. Assisted-by: Claude * jit(wasm): publish the compiled loop's inputarg types on the token `Backend::compile_loop` already builds `input_types` for the pending self-CALL_ASSEMBLER census but never wrote it to the token, so `JitCellToken::inputarg_types()` stayed empty on wasm. Both native backends set it (`majit-backend-dynasm/src/runner.rs:2269`, `majit-backend-cranelift/src/compiler.rs:15613`). `jitdriver.rs:4686 extend_compiled_live_values` sizes the argument list handed to `execute_token` from that length. Reading 0 makes `compiled_inputs <= live_values.len()` always true, so the virtualizable extension is skipped and a trace with more inputargs than the portal's live values is entered with the short list; the slots past the args then read the zero-filled frame. With the types published the same entry either extends correctly or returns None, and every caller turns None into a decline. Measured on the wasm synthetic suite: 346 passed / 8 failed, the same eight fixtures as before the change. It does not close the `recursive_forced_frame_kept_stack` crash. Assisted-by: Claude * jit: name the nested-residual decline arm under PYRE_LB_SITE `fbw_abort_nested_unjournaled_residual` declines on either of two independent conditions — a FOR_ITER body admitted under `CalleeReplaySafety::DeferredCall` whose nested CALL did not inline, or `fbw_inline_callee_hazardous` finding a loop-bearing / self-recursive / duplicate-`w_code` frame — and both raise the same `LoopBearingCalleeInlineUnsupported`. `PYRE_LB_SITE` already prints the raising source location to tell decline reasons apart, but every arm here shares one raise site, so the location alone cannot separate them. Print the two predicates next to it. Measured with it: `recursion_memo_branch`'s single abort is the DeferredCall arm and `list_append_write_barrier_gc`'s is the hazard scan. Assisted-by: Claude * jit, gc: drop the dead compiled-loop unwrap, widen the drain decile scale Two findings from the PR review bots on 945. `run_compiled_raw_with_values` bound `self.compiled_loops.get(&green_key).unwrap()` right after `execute_token_ints` and never read it — the later `.and_then(|compiled| …)` shadows the name inside its own closure. The binding is dead but the `unwrap()` is not: a residual call that re-enters the driver can remove this green key during the run, and the panic then fires ahead of the fresh fallible lookup and its `trace_for_exit_by_rd_loop_token` fallback, so that fallback could never run in the case it was written for. Deleting the binding leaves no other use. `record_drain_sample` scaled the decile with `promoted.min(used_before) * 100` in `usize`. On wasm32 that wraps once the nursery passes ~43 MB, bucketing the sample wrong in release and tripping the overflow check in debug. Widen to u64 before scaling. pyre/check.py --synthetic-only --backend dynasm: 356/356. Assisted-by: Claude * bench, check.py: un-record the platform-split imp_lock baseline, report what a regressed run compiled `imp_lock_rlock_semantics` produces a different `[jit-stats]` line on every runner: linux 0 loops / 0 guard failures, macos 1 / 3, windows 0 / 7. Recording the macos numbers made linux fail the opposite gate (`loops_compiled 1 -> 0`) on a bench where main is green, so restore the committed baseline. No single value passes all three; picking one needs the windows `loops_compiled`, which nothing currently reports. That is the second half. `_jit_stats_regression_floor` names only the fields that moved the wrong way, so a `guard_failures` rise reads as a defect even when the same run compiled more loops — each new guard fails `trace_eagerness` (200) times before its bridge attaches. Locally the answer is one re-run away; on a runner the developer does not have it is not reachable at all, which is why the windows rows on this branch cannot currently be judged. Append the observed `loops_compiled` / `bridges_compiled` to the failure message. jit-stats regression: guard_failures 0 -> 3 (observed loops_compiled=1 bridges_compiled=0) pyre/check.py --synthetic-only --backend dynasm: 355 passed, 1 failed (that bench, on macos). Assisted-by: Claude * jit(wasm): drop the duplicated set_inputarg_types; record the wasm baseline for recursive_forced_frame_kept_stack The rebase brought a second `token.set_inputarg_types` into the same `compile_loop`, so the function published the entry arity twice. The setter is a `OnceLock::set` and ignores the later call, making the two behave alike, but only the surviving one runs after codegen has succeeded — publishing the arity for a loop whose compile then failed is not something to keep a second path for. Keep that one and delete the earlier copy. `recursive_forced_frame_kept_stack` no longer crashes on wasm, so the bench now reaches the "no committed jit-stats baseline" gate instead. Record it. pyre/check.py --synthetic-only --backend wasm: 352 passed, 3 failed before this commit; the recorded baseline closes one of the three. Assisted-by: Claude * _collections: root the deque snapshot entries across the block-chain rebuild `store` published only `self_obj` and left the replacement entries in a plain `Vec<PyObjectRef>`. `append_right` allocates a block chain, so any append can collect and move the objects the vector still names, and every later append then reads a stale address. `self_obj` was already re-read from the shadow stack after each GC-capable step for exactly this reason; the entries need the same treatment. Publish both slices with `publish_roots` and normalize the range once, rather than `pin_root(self_obj)` followed by a second pin: `pin_root` normalizes, that query is a safepoint, and the entries are not yet on the stack when it runs — the window `normalize_roots`' own doc describes. Each append then reads its entry back at the slot. Reported by the PR review bots on 945. collections.deque append/appendleft/remove/rotate/maxlen over 2000 items matches CPython byte for byte on three consecutive runs; pyre/check.py --synthetic-only --backend dynasm unchanged at 355 passed. Assisted-by: Claude * bench/synth: raise guard_failures baselines to the windows-observed count 13 synthetic benches fail the jit-stats floor on the windows runner with a `guard_failures` rise and nothing else. The same runs compile more loops than the baseline records — windows `loops_compiled` is 2 where the baseline holds 0, 3 where it holds 1, and 7 where it holds 5 — and macos reproduces the same direction one step lower (1 / 2 / 6). Each newly compiled loop's guards fail `trace_eagerness` times before its bridge attaches, so the counter moves with the loop count. `guard_failures` is rise-bounded (`cur > base + max(base // 4, 2)`), which is why this reads as windows-only: the macos and linux +1 sits inside the band while the windows +3/+5/+7 does not. The baselines now carry the highest count of the three platforms, so one committed file admits all of them. Only `guard_failures` moves; `loops_compiled` stays at its recorded floor, since raising a fall field to a value one platform does not reach is what broke linux on `imp_lock_rlock_semantics` before. Assisted-by: Claude * jit: seed the sub-walk vstack from the carrier recipe when no ActiveResumeFrame `bridge_subwalk` left `vstack_boxes` empty when the richer coordinate source was absent, though `resume.py rebuild_from_resumedata` seeds the rebuilt MIFrame's operand stack from the frame recipe alone. The first may-force call then invalidated the heapcache, a later CALL read a color-reloaded box with no runtime value, and the effectful sub-walk replayed from the guard, consuming the framed-pickle stream twice. `residual_call` and `vstack_mirror` carry the matching reads. Assisted-by: Claude * jit: stamp const pools with a process-global walk generation instead of a per-walk visit set `MetaInterp::walk_rd_consts_refs` allocated a fresh `IndexSet<usize>` on every GC root walk and hash-inserted each pool's `Arc::as_ptr` identity to avoid visiting a shared pool twice. `SharedConstPool` now carries an `AtomicU64` visited generation; the walk bumps a counter once and each pool swaps it in, visiting only when the previous value differs. The counter is one process-global `AtomicU64`, not a `MetaInterp` field. `register_thread_root_areas` (pyre-jit/src/eval.rs:3839) registers `rd_consts_root_walker_area` per mutator and `JIT_DRIVER` is `thread_local!`, so per-instance counters would each start at 0 and advance about once per collection; two of them reach equal values routinely, and a pool reachable from two instances would then be treated as already visited — its `Const::Ref` slots never reaching the visitor. The `IndexSet` this replaces was local to one walk and could not fail that way. One relaxed `fetch_add` per walk keeps generations distinct regardless of instance or thread; generations start at 1 and pools initialise to 0. Measured, min-of-3 user CPU, both arms built on this base, on N independent compiled loops: | N | per-walk IndexSet | generation stamp | delta | |---|---|---|---| | 100 | 0.73s | 0.72s | -1.4% | | 200 | 1.76s | 1.65s | -6.3% | | 400 | 7.30s | 5.44s | -25.5% | The win grows with the compiled-loop count. `sample` self time on N=400: the hash/insert frames fall from 664 of 4070 active samples to 69 of 2941, inside a run that is 77% GC + root walk. check.py dynasm 372/372, cranelift 372/372. Repeated-run gate on a 96-loop allocation-churning fixture: 600 runs per arm, 0 crashes and 0 wrong answers on both the stamp and the IndexSet build. Assisted-by: Claude * jit: decline the compare-box scan in sub-walks, let it pass over the pushvalue publish `classify_compare_box_use` decodes `compare_pc` against the snapshot root's JitCode. An inlined sub-walk has its own callee JitCode and deliberately shares the root snapshot, so the two pcs name different code and the scan reads the callee's offset out of the caller's bytes. Decline there, as `walker_foriter_green_key` already does for the same mismatch. The scan also counted the `push_and_bump!` publish as a second reader of the box, which took every compare out of the `FeedsBranchOnly` shape and left `walker_newbool_guarded` emitting the `jit_bool_value_from_truth` residual — a `CallR` returning Ref plus its allocation, once per iteration. That store mirrors the box into the operand-stack slot the guard's own resume image describes; substituting a prebuilt singleton for the recorded call result leaves it storing the same value. `VablePublish` says which of the two consumers may pass over it: `compare_box_provably_dead` elides the box outright and still needs one to store, so it keeps counting the publish. `pyre/bench/int_loop.py` returns to 40 ops before optimization and 25 after, with no Ref-returning `CallR` in the trace. Assisted-by: Claude * bench: re-record the 33 baselines this branch moves under the two-directional gate #1007 made every recorded counter gate in both directions, so the changes this branch already made to what the JIT compiles now report instead of passing as unmeasured improvements. Recorded with `check.py --snapshot`; dynasm and cranelift observe the same values on all 33, so none of them is a backend divergence. Audited every recorded line: `internal_compile_panics` and the `descr_set_*` counters do not move at all, no `loops_aborted` rises and no `loops_compiled` falls. What moves is `loops_aborted` 5 -> 0 and 1 -> 0 (three benches), `loops_compiled` up on nineteen, and `bridges_compiled` down with the matching `guard_failures` — 10 -> 4 with 2027 -> 819, 4 -> 0 with 828 -> 1, 6 -> 4 with 1203 -> 802, 10 -> 7 with 2018 -> 1655. Every `guard_failures` rise sits in a run whose `loops_compiled` also rose, which is the new loops' guards failing `trace_eagerness` times each before their bridges attach. Assisted-by: Claude * bench: re-record the 19 wasm baselines whose counters moved in a neutral or better direction Same two-directional gate (#1007) as the dynasm/cranelift re-record, run with `check.py --backend wasm --snapshot`. `loops_aborted` falls 5 -> 0 and 2 -> 1, `loops_compiled` rises on eleven, and every `guard_failures` rise sits in a run whose `loops_compiled` or `bridges_compiled` also rose. Three wasm rows are deliberately left at their committed values so the gate keeps naming them: `list_append_write_barrier_gc` (`loops_aborted` 0 -> 1), `recursion_memo_branch` (`loops_aborted` 1 -> 2) and `pickle_terminal_raise_resume` (`loops_compiled` 74 -> 73). All three already failed on this branch under the previous one-directional gate, so they are not a consequence of this re-record and none of them is blessed by it. Assisted-by: Claude * bench: re-record the fourteen baselines the new base moves, leaving raise_reg_unbound_jitstress red #950 and #1019 landed under this branch and moved these counters. They are not this branch's doing: `origin/main` built and run on this host observes exactly what the branch observes, and misses its own committed baselines by the same amounts — | bench | committed | origin/main observes | |---|---|---| | exception_metadata_jitstress | loops_compiled 407 | 409 | | global_store_plain_dict_globals | loops_compiled 4 | 6 | | trace_too_long_effect_replay | loops_compiled 1 | 3 | | exception_subclass_attrs | guard_failures 1 | 3 | | raise_reg_unbound_jitstress | loops_aborted 1, loops_compiled 6 | 2, 8 | `raise_reg_unbound_jitstress` is left at its committed value on both backends. Its `loops_aborted` 1 -> 2 is a badness-field rise, and the control above shows it belongs to the base rather than to this branch, so recording it here would put a name to someone else's regression. The other fourteen rows only raise `loops_compiled` or `guard_failures`; no `loops_aborted` rise, no `loops_compiled` fall, and `internal_compile_panics` and `descr_set_*` do not move. Assisted-by: Claude * bench: move the nine wasm guard_failures baselines the new base shifts The wasm re-record two commits back was taken before #950 and #1019 landed under the branch. Each of these nine rows only moves `guard_failures`, by 1 to 3, and the shift is deterministic: `gc_iterator_source_drop` reads 613 on three consecutive runs against a committed 614, so it is the base and not run-to-run wobble. Four wasm rows stay at their committed values. Three are badness-direction and already failed under the previous one-directional gate: `list_append_write_barrier_gc` (`loops_aborted` 0 -> 1), `recursion_memo_branch` (`loops_aborted` 1 -> 2) and `pickle_terminal_raise_resume` (`loops_compiled` 74 -> 73). The fourth, `exception_inline_callee_tb_frames`, rises `guard_failures` 860 -> 975 with no matching `loops_compiled` or `bridges_compiled` rise and has had no origin/main control run against it, so it is left for the gate to keep naming rather than recorded on a guess. Assisted-by: Claude
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
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.
What was not gated
.jitstatsbaselines are committed, and_jit_stats_regression_floorran onevery
pyre/check.py— but it gated three different ways and left a hole ineach:
loops_aborted,internal_compile_panics,descr_set_*guard_failuresbase + max(base // 4, 2)loops_compiledbridges_compiledExact equality (
_jit_stats_fields_equal) existed, but only under--snapshot-diff, and CI runspyre/check.pywith no arguments.So
bridges_compiled 27 -> 0was a green run as long asguard_failuresstayedinside its band — for
foriter_call_resume_drops_iteration(br=27, gf=5150)that leaves room up to 6437. That is the dead-bridge class this suite exists to
catch.
What this changes
JITSTATS_SNAPSHOT_FIELDS, on everynon-record run. The recorded surface and the gated surface are now the same
set, so a baseline cannot carry a number nobody checks.
bridges_compiledjoins the gate.
left ungated means the committed baseline quietly stops describing the tree,
and the next regression is measured against a number that was already stale —
which is exactly what had happened below.
[jit-stats]line fails instead of beingskipped.
pyre_envsetsMAJIT_STATS=1for every pyre run, so an absent lineis not a bench without counters; it is a bench whose counters went missing.
Reading it as "nothing to compare" skipped the missing-baseline check and the
comparison with it.
--snapshot-diff, which the CI invocation never passes.The
guard_failuresband is removed. Its own comment said it coveredcross-runner drift "which is NOT yet measured"; a CI run has since reported
list_length_hint_validate guard_failures 828 -> 4923andexception_args_virtual 401 -> 1002with identical digits on ubuntu-24.04,macos-latest and windows-latest.
The jit-stats half of
--snapshot-diffbecomes unreachable and is removed,along with
_jit_stats_fields_equaland_jit_stats_diff.What it caught immediately
#997 re-recorded 27 dynasm baselines and 2 cranelift ones. The other 25
cranelift files kept their pre-#997 numbers, and every difference is a fall —
bridges_compiledandguard_failuresdown,loops_aborteddown — so the oldgate called all of it free and reported nothing.
The second commit re-records them.
That re-record is also the determinism evidence
bridges_compileddid not have.The newly recorded cranelift values agree with the dynasm baselines already on
mainfor 27 of the 28 fixtures, digit for digit on all four counters —two independent code generators, and for most of these two different hosts. The
single difference is
inline_gate_operand_provenance,guard_failures4 ondynasm against 5 here.
Verification
pyre/check.py --snapshot --backend <b> --synthetic-only, one backend at atime:
The single dynasm failure is
surrogate_dirat 6.2x against a 5x gate, flaggedon its own line as
[pypy exec clamped to floor; ratio not a measurement].Before the branch was rebased onto the merge of #992, the new gate reported 25
dynasm changes; reverting this branch's jit sources to
mainand rebuildingreproduced all 25 with identical values, so none of them were branch-attributable
— and
mainhas since recorded them.— authored by Claude