bench: drop the windows closure_per_call overlay - #1043
Merged
Conversation
The counter it recorded is not stable on that host. The overlay was written from three readings of 416 (two named in check.py's overlay comment, one on #1028's CI run); the next windows cranelift run reported 415 against it, so each value fails whenever the other one is recorded. `recursive_call_frame_relocation` moved the same way in the same run and in the opposite direction relative to its shared file — guard_failures 638 -> 637 on windows dynasm, where it had agreed at 638 one run earlier. Two counters disagreeing with themselves across runs, one toward its shared value and one away from it, is not a per-host fact an overlay can hold. The stability re-runs re-run one binary inside one job, so an oscillation between jobs is invisible to them. 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 (1)
💤 Files with no reviewable changes (1)
WalkthroughThe change removes all contents from the Windows Cranelift Changes
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 ec86e94). The diff against the review base changes only generated jit-stats baselines (1 file(s)); nothing to review. |
youknowone
added a commit
that referenced
this pull request
Aug 5, 2026
…failures windows reports guard_failures=637 against a shared 638, on three consecutive runs at three different base shas (#1042 at 4fecf86, #1043 at a8677f4, main at 2f61280), with every other counter identical. check.py's overlay comment names the same value from two earlier runs. macos-latest and ubuntu-24.04 pass their dynasm legs against the shared file, so it keeps describing them. The single contrary reading is one run before that sequence, where windows reported 638 and `closure_per_call` reported 416. Both counters moved down by one at the same point and have held since; the move left `closure_per_call` agreeing with its shared file, which is why the overlay dropped in the previous commit is not needed and this one is. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 5, 2026
Its cranelift guard_failures is not the same on every host. macos-latest and ubuntu-24.04 both report 5980; windows-latest reported 5979 against that recording. #1043 removed the closure_per_call overlay after two counters in that band disagreed with themselves across jobs, so a `.cranelift.win32.jitstats` overlay cannot hold this one either, and a missing baseline is a hard fail rather than an opt-out. The walker guard the file was promoted for therefore has no suite gate. The file stays as the reproduction, with the measurement and the reason in its header. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 5, 2026
…nd token fixes; journal the walk-time traceback attaches (#1050) * jit: carry the virtualizable `debugdata` static as a box, not a recording-time constant `MIFrame::flush_to_frame` republished all six virtualizable statics as constants read out of the live frame. `DELETE_NAME` resolves the name through `debugdata.w_locals`, so the compiled loop deleted through the recording frame's mapping once `exec(code, globals)` reused the same code object with a fresh globals dict, raising NameError. Keep the box `read_boxes` seeded for that field; the remaining pointer statics stay promoted constants. Adds pyre/bench/synth/exec_fresh_globals_delete_name.py with its dynasm, cranelift and wasm jit-stats baselines. Assisted-by: Claude * jit: keep the pointer flag through the BhFieldSpec round trip `BhFieldSpec::from_field_descr` mapped every pointer field to `ArrayFlag::Unsigned`. `field_spec_from_bh` copies that flag into the rebuilt `SimpleFieldDescrSpec`, whose `is_pointer_field()` is `flag == ArrayFlag::Pointer` (descr.py:173), so a pointer field that traversed the round trip reported itself as non-pointer and `handle_write_barrier_setfield` declined its write barrier. The codewriter's other two producers, `value_type_to_field_flag` and `bh_field_flag_from_descr`, already map a pointer field to `ArrayFlag::Pointer`. Assisted-by: Claude * jit: settle the virtualizable token on the top-level raise exit `compile_exit_frame_with_exception` (pyjitpl.py:3261) opens with `store_token_in_vable()`, the same as `compile_done_with_this_frame` (:3223). pyre's value and void arms settle the token via `fbw_force_virtualizable_before_return`; the top-level raise arm only published `last_instr`. Every residual call arms the token in `walker_vable_and_vrefs_before_residual_call`, so the exception exit left the frame naming a jitframe that dynasm frees in `execute_token`, and the next `is_force_token_armed` walked `jf_forward` off freed memory. pyre settles by storing back, which zeroes the slot, rather than by arming the token as upstream does: upstream's FORCE_TOKEN is the heap-allocated GC `JITFRAME`, pyre's is the machine frame pointer. Adds `top_level_raise_settles_the_vable_token`, which asserts the recorded tail op is a `SetfieldGc` of const-0 at the token offset. Assisted-by: Claude * jit: journal the walk-time traceback attaches so an aborted walk undoes them `record_top_level_application_traceback`, `record_inline_application_traceback` and `record_exc_edge_discarded_tracebacks` apply their node to the live exception while the walk records. Only the bridge handler entry wrapped that mutation in the fbw undo log, so a walk that did not commit left the other three sites' nodes attached and the interpreter's replay recorded the same frames again. Move the journaling into the three attach sites and drop the bridge arm's own wrapper, which is now redundant. A freshly raised exception hides this — the discarded walk's object is unreachable — so the observable case is a `raise` that re-uses one exception object: `tb_frame.f_locals` on the catching frame aborts the walk and the handler then reads a four-node chain where the interpreter builds two. Adds `pyre/bench/synth/exception_reused_object_tb_not_doubled.py` and a unit test for the multi-node rollback. Assisted-by: Claude * gc: skip the PyObject-shaped raw walks for a virtual ref in a frame slot `walk_pyframe_roots_area` handed every `locals_cells_stack_w` slot to `walk_raw_exception_roots`, which calls `is_exception` and so dereferences the slot's leading word as an `ob_type`. A slot can hold a `JitVirtualRef`, whose leading word is the `JIT_VIRTUAL_REF_VTABLE` magic, and the dereference then faults. The same function already tested `ptr_is_virtual_ref` on the `f_backref` slot one block earlier; extract that arm as `forward_virtual_ref_forced` and apply it to the value-stack slots too, forwarding `forced` and skipping the raw walks. `walk_raw_immortal_roots` was already safe for this input: `walk_immortal_rec` guards its `ob_type` read behind `try_gc_owns_object`, and a vref is allocated with `alloc_oldgen_typed`. Reproduced on cranelift with a hot `raise`/`except` loop whose handler reads `tb_frame.f_locals` on the catching frame; the collection that faults is a minor one triggered by a nursery allocation from compiled code. dynasm and the interpreter are unaffected. Assisted-by: Claude * bench: park two cranelift crash witnesses under synth/_pending `exception_tb_f_locals_vref_root_walk.py` is the oracle for the virtual-ref root-walk fix: pre-fix it SIGSEGVs 3/3 on cranelift at `15000 head`, post-fix it matches dynasm, `PYRE_NO_JIT=1` and pypy. It stays out of the suite because the same file run with no arguments hits an unrelated open abort. `gc_varsize_item_const_shape_witness.py` is that abort: `GC BUG ... site=minor_varsize_item_target`, cranelift 3/3, dynasm and the interpreter clean. Its docstring records the decoded holder fields and the allocation-layout trigger. Assisted-by: Claude * jit: give the miframe raise test a real exception object `journaled_concrete_traceback_attach` reads the concrete exception's traceback head before the attach, so the walk now dereferences the pointer that `dispatch_via_miframe_mirrors_last_exc_value_back_into_sym` passed as a bare `0xDEAD_BEEF` sentinel. The sentinel survived only because the recording hook is not installed under test; the production path dereferences the same pointer. Build the operand from `w_exception_new` instead. Assisted-by: Claude * gc: report the root path that reached an object in the trace panics `trace_and_update_object` receives a `site` naming the root path it was reached from — `minor_remembered_set`, `minor_jitframe_root`, and the rest — but when it recursed into `copy_nursery_object` or `assert_traced_slot_initialized` it passed a hardcoded site naming only the SLOT KIND, so the invalid-type_id and nursery-poison panics could not say which root path produced the bad holder. Thread the caller's site down as `parent_site` and add it to both messages. The parameter is `&'static str` so no allocation happens on a traced slot. On the `minor_varsize_item_target` witness under `synth/_pending` this reports `parent_site=minor_remembered_set` 3/3, which rules out the jitframe shadow-stack root walk as that abort's path. Assisted-by: Claude * jit: allocate traced traceback nodes non-moving `w_pytraceback_new` allocates a traceback node through the stable old-gen allocator and states the obligation in place: raw `*mut PyTraceback` readers and the exception `w_traceback` chain hold bare pointers no root walker reaches. `PYTRACEBACK_DESCR_GROUP` never stamped `set_non_moving`, so a node built by compiled code was a nursery object a minor collection could relocate. Stamp the group's size descr, as the `W_ObjectObject` group and `mapdict_storage_gcarray_descr` already do for the same reason. `gc_varsize_item_const_shape_witness.py` and four stack-depth variants of it abort with `GC BUG ... site=minor_varsize_item_target` at 18 of 320 (fixture, nursery size) pairs over a 512K-32M `PYPY_GC_NURSERY` sweep before this change, and at none of them after; a finer 256K-step sweep to 51M over two of the fixtures is also clean. Assisted-by: Claude * jit: assert the non-moving stamp on the three raw-pointer descr groups `PYTRACEBACK_DESCR_GROUP`, the `W_ObjectObject` group and `mapdict_storage_gcarray_descr` each carry `set_non_moving(true)` to match an interpreter allocator that is non-moving because raw pointers reach the object without being rooted. Nothing checked the flags, and a missing one shows up only as a GC abort at particular nursery sizes — `check.py` pins `PYPY_GC_NURSERY` to 4MB, which is not one of them. Assert the three flags. Removing the traceback stamp fails the test. Assisted-by: Claude * bench: re-record the guard_failures baselines the traceback descr stamp moved Fourteen `.jitstats` baselines, `guard_failures` only; `loops_compiled` and `bridges_compiled` are unchanged in every one. Assisted-by: Claude * bench: record the three count-valued counters in the wasm delete-name baseline The committed baseline named only the five badness fields. `_jit_stats_change` reads a field absent from either side as 0, so `loops_compiled`, `bridges_compiled` and `guard_failures` compared against 0 and the wasm run of this fixture now reports 1, 0 and 4 — the same three values its cranelift and dynasm baselines carry. Assisted-by: Claude * bench: promote the virtual-ref root-walk fixture out of _pending The file was parked because running it with no arguments hit `GC BUG ... site=minor_varsize_item_target` instead of its own subject. With traced traceback nodes allocated non-moving it is clean at all 64 sizes of a 512K-32M `PYPY_GC_NURSERY` sweep, and cranelift, dynasm and `PYRE_NO_JIT=1` print the same line, so it can gate the walker guard from the suite. wasm is exempted. It loses `e` from the catching frame's `f_locals` over part of the loop and prints a second tuple; that is compiled-code only (clean at N=4000, wrong from N=8000) and reproduces with the whole source tree reset to the merge base, so it is neither this fixture's subject nor this branch's. Same stale-`f_locals`-from-compiled-code class as getframe_caller_locals_nested_compiled_callee, which carries the same exemption. Assisted-by: Claude * bench: park the virtual-ref root-walk fixture back under _pending Its cranelift guard_failures is not the same on every host. macos-latest and ubuntu-24.04 both report 5980; windows-latest reported 5979 against that recording. #1043 removed the closure_per_call overlay after two counters in that band disagreed with themselves across jobs, so a `.cranelift.win32.jitstats` overlay cannot hold this one either, and a missing baseline is a hard fail rather than an opt-out. The walker guard the file was promoted for therefore has no suite gate. The file stays as the reproduction, with the measurement and the reason in its header. Assisted-by: Claude * jit: replay the folded inline-callee locals when a traceback node takes the frame A `STORE_FAST` on a strictly-inlined callee's own fresh frame takes the fold in `setarrayitem_vable_via_metainterp`: it updates the walker shadow, pokes the recording-time frame through `store_live_frame_array_slot`, and records no `SETARRAYITEM_GC`. `disarm_folded_inline_callee_after_escape` is the replay that writes those slots back and disarms the fold, and it had one call site — the branch where a residual call forced the virtualizable. `emit_traceback_node` stores the callee frame box into `PyTraceback.frame`, which is a second escape: the frame outlives the trace with its locals array holding only the parameters `emit_new_pyframe_inline_with_params` seeded. Call the replay from `traceback_node_site` before handing the box out, declining the site on `Err` as both callers already do for an unresolved frame. It is inert at the top level, where there is no inline callee. `exception_inline_callee_tb_frame_locals` reads `tb.tb_frame.f_locals` off the callee's node. dynasm and cranelift both answered `('i',)` on 2758 of 4000 iterations before this and `('i', 'marker')` on all 4000 after, matching pypy3, CPython and PYRE_NO_JIT=1. 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
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.
Reverts the overlay added in #1042. It recorded a value the host does not hold
steady, so it moved the failure rather than removing it.
Whichever number is recorded, the other one shows up.
The same run moved the second fixture that comment names, in the opposite
direction relative to its shared file:
recursive_call_frame_relocationreported
guard_failures=637on windows dynasm against a shared 638, havingagreed at 638 one run earlier. One counter drifting toward its shared value
while another drifts away from it in the same run is not a per-host fact, and a
per-host file cannot hold it.
JITSTATS_STABILITY_RUNSdoes not see this: it re-runs one binary inside onejob, and the disagreement is between jobs.
That leaves the two fixtures gated on a counter that disagrees with itself
across windows runs. Nothing here decides what to do about that — an overlay
just is not the mechanism.
🤖 Generated with Claude Code
Summary by CodeRabbit