jit: recover the CALL's null_or_self slot on outer-call resume; check.py: arm the jit-stats floor across the synthetic suite - #947
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 1031 files, which is 931 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1031)
You can disable this status message by setting the |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a115ce6). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
![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: c1ebe654f7
ℹ️ 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".
Comment on lines
+606
to
+608
| for field in ( | ||
| JITSTATS_BADNESS_FIELDS + JITSTATS_RISE_BOUNDED_FIELDS + JITSTATS_FALL_FIELDS | ||
| ): |
There was a problem hiding this comment.
On the default pyre/check.py jobs for Ubuntu, macOS, and Windows, this adds guard_failures and loops_compiled to the hard regression floor, so any host that compiles fewer loops than the committed baseline—or exceeds its guard count by the band—now fails CI. The existing pipeline explicitly documents in .github/workflows/pyre-ci.yml:408-412 that these absolute counts were recorded on macOS aarch64, have not been confirmed cross-runner or cross-platform, and must remain informational; it therefore runs their comparison under continue-on-error. Keep these fields out of the default hard gate until those summaries establish stability, or provide platform-specific baselines.
Useful? React with 👍 / 👎.
`intern_liveness` keyed its dedup table on the sorted, deduped form of its three `&[u8]` arguments but took the record's count bytes and encoded bitsets from the raw slices. `encode_liveness` dedups, so a repeated register index made the count exceed the bitset's cardinality. `LivenessIterator` is driven by that count, so decoding such a record read past its own bitset into the following record's bytes and swallowed it. Read the counts and the bytes off the canonical form already computed for the key, matching `majit-translate`'s producer, which counts the `VecSet` it encodes from (codewriter/assembler.rs:1060). Assisted-by: Claude
…ehind them `execute_token` brackets every entry into compiled code with `debug_validate_oldgen_freeblocks`, which read `PYRE_GC_FREELIST_DIAG` with `std::env::var_os` on each call, and it read `PYRE_DYNASM_EXEC_DIAG` the same way — three `getenv`s per trace entry, each taking the environment lock and scanning the array. The two bracket calls also built their `site` string with `format!` before the gate, so a disabled diagnostic still allocated and formatted one per trace entry, and `dynasm_debug_validate_oldgen_freeblocks` one per residual call site. Cache both gates in a `LazyLock<bool>` as `majit_log_enabled` does, take `site` as `Arguments` so the string is materialised only past the gate, and do the same for `MAJIT_LOG` in `do_collect_nursery` / `finish_incremental_cycle` and `PYRE_GC_FREELIST_DIAG` in `dynasm_nursery_slowpath`. On a 20x-scaled `recursion_memo_branch` the startup-subtracted user-CPU exec time goes 0.7609s to 0.6566s (0.863x, min of 7 interleaved rounds, same host), output unchanged; `__findenv_locked` and `std::env::__var_os` fall from 43 of 388 working samples to 0 of 581. Also correct the comment above the `should_disable` application: it cited `trace_opcode.rs:3044-3049` as a second site applying `disable_noninlinable_function`, where no such symbol exists. Assisted-by: Claude
`collect_call_stack_overrides` had no source for the `null_or_self` operand a
`PUSH_NULL` leaves under a `CALL`: the walk holds no box for that slot, no live
color names it, and the virtualizable shadow reads back the same NULL an
unmirrored slot does. The slot stayed absent, so the outer-call flush that
follows a `LoopBearingCalleeInlineUnsupported` abort declined on "stack override
missing for a live slot" and fell back to the legacy rollback+replay, where
`fbw_foriter_inflight_take` refuses in-flight FOR_ITER delivery and the whole
iteration is dropped.
Name the slot from the CALL's own operand layout — `[callable, null_or_self,
arg0 .. arg_{argc-1}]` ends at the caller's stack top, so the sentinel sits
`argc + 1` below it — and push an explicit null override. A coordinate that
does not invert to a plain `CALL` keeps the existing decline.
The added `bench/synth/foriter_call_resume_drops_iteration.py` printed
`491088 8999` before this change and `491130 9000` after, matching CPython,
PyPy and `PYRE_NO_JIT=1`.
Assisted-by: Claude
… baseline _jit_stats_regression_floor now iterates JITSTATS_RISE_BOUNDED_FIELDS in addition to JITSTATS_BADNESS_FIELDS. A badness counter still fails on any rise above its baseline; a rise-bounded counter fails only past `base + max(base // 4, 2)`. A field absent from the baseline keeps reading as 0 for the badness counters, whose healthy value is 0, but leaves a rise-bounded counter unpinned: the wasm [jit-stats] line reports no guard_failures, so every *.wasm.jitstats baseline omits it. guard_failures moves out of the informational snapshot surface into that gate. Assisted-by: Claude
pyre/.gitignore ignored /bench/synth/*.jitstats with three `!` negations, so the regression floor read a committed baseline for 3 of 340 synthetic fixtures and skipped the other 337. Recorded with `pyre/check.py --snapshot` (dynasm 355/355, cranelift 355/355, wasm 351/351): 340 dynasm, 340 cranelift, 339 wasm — getframe_caller_locals_nested_compiled_callee carries skip-backends=wasm. Of the 1019 synthetic baselines, 872 record all-zero badness counters, 147 record a nonzero loops_aborted, and none record a nonzero descr_set_* or internal_compile_panics. Assisted-by: Claude
…tale defaults_reassigned_midloop and del_cellvar_walk_commit arrived from origin/main with no .jitstats baseline, so the regression floor skipped them. type_metatype_method_call moves loops_compiled 4 -> 2, bridges_compiled 47 -> 0 and guard_failures 9480 -> 1 on the native backends, the effect #925 records in its own message. comprehension_object_append_hot.dynasm moves guard_failures 3611 -> 3612. Assisted-by: Claude
… baseline JITSTATS_FALL_FIELDS gates a counter whose defect direction is down. loops_compiled is its first member: a pre-trace decline aborts nothing, so loops_aborted holds, and it removes the compiled guards that used to fail, so guard_failures falls — both existing gates read the loss as an improvement while the hot loop runs interpreted. The fall is gated exactly rather than through a band. Diffing the dynasm and cranelift baselines across the 341 synthetic fixtures, the two code generators agree on loops_compiled for 340 of them (99.7%), against 97.4% for guard_failures. A benchmark that emits a [jit-stats] line but has no committed baseline now fails instead of passing. Every gate above compares against nothing when the file is absent, so "never recorded" and "recorded and clean" print the same PASS — which is how the floor came to cover 3 of 340 synthetic fixtures. Assisted-by: Claude
…nline moved `#942` passes `allow_method_load_attr = true` at `try_walker_inline_user_call`, so an unbound method-form callee whose body reads `self.attr` inlines into the caller's loop instead of running as a residual call. Three fixtures carry that shape (`c.add(i & 7)`, `self.process_word()`) and lose the callee's separately compiled loop: class_attrs_methods loops_compiled 2 -> 1 (dynasm, cranelift) foriter_inplace_immutable loops_compiled 2 -> 1 (cranelift) inline_subwalk_mutating_residual loops_compiled 4 -> 3 (cranelift) Every counter that moves with them falls: `class_attrs_methods` cranelift `guard_failures` 202 -> 1 and `bridges_compiled` 1 -> 0; `inline_subwalk_mutating_residual` cranelift `guard_failures` 12316 -> 603 and `loops_aborted` 1 -> 0, wasm `loops_aborted` 4 -> 2. The measured ratio for that fixture is 13.1x dynasm / 17.2x cranelift, matching the 13.0x / 18.3x `#942` records. `method_reassign_after_warmup` arrived with `#940` and had no baseline, which the missing-baseline gate reported on all three backends. check.py: dynasm 359/359, cranelift 359/359, wasm 355/355. Assisted-by: Claude
…-file list The workflow appends `git diff --name-only upstream/main` to the review prompt as the authoritative definition of "this patch". On a PR that re-records check.py's jit-stats baselines in bulk that list reached 1027 entries and Codex returned no report at all (exit 1); CodeRabbit skipped the same PR for being 927 files over its own 100-file limit. `.jitstats` files are generated golden data with no RPython/PyPy counterpart, so no parity finding can cite one. Exclude them from the list, report the withheld count in the prompt header and in the empty-diff message, and put the same exclusion in the prompt's own scope-discipline instruction, which is what the local `/codex-review` path re-derives from. On this branch the list goes from 1132 to 112 files. Assisted-by: Claude
…e moved
`origin/main` alone reproduces the counters exactly, so this is not our branch:
exception_reraise_tb_depth_hot 40 loops / 0 guards / 30 aborts / 0 bridges
-> 4 loops / 1803 guards / 0 aborts / 9 bridges
exception_reraise_tb_depth_jitstress 900 / 0 / 30 / 0 -> 805 / 1798 / 0 / 1
Both gated directions fired at once (`loops_compiled` down, `guard_failures` up)
and neither is a defect on the native backends. Interleaved user+sys CPU time,
min of 5, `e1e97e61d32` vs `origin/main`, same host: `_hot` 0.750s -> 0.370s
(2.03x faster), `_jitstress` 0.490s -> 0.490s. The 40 specialized loops with 30
aborts were the worse shape.
wasm moves the other way on the same two fixtures: `loops_aborted` 30 -> 44 and
30 -> 1198. Recording those pins the higher value, so the rise is stated here
rather than absorbed silently.
check.py, per fixture: dynasm 1/1, cranelift 1/1, wasm 1/1.
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a115ce65a6
ℹ️ 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".
Comment on lines
+1315
to
+1318
| let (pyre_interpreter::Instruction::Call { argc }, op_arg) = | ||
| pyre_interpreter::decode_instruction_at(code, py_pc)? | ||
| else { | ||
| return None; |
There was a problem hiding this comment.
When the outer inlined call is CALL_KW or CALL_FUNCTION_EX, this match returns None, even though try_walker_inline_user_call explicitly supports both helpers and both stack layouts contain the same null_or_self slot. Consequently, a plain keyword/star call whose callee aborts—for example, a loop-bearing callee during an in-flight FOR_ITER—still omits its null slot from the overrides, declines the forward flush, and reaches the iteration-dropping legacy replay that this patch fixes for positional CALL. Decode these supported forms as well, accounting for the trailing kwnames or star/kwargs operands when calculating the slot.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
This was referenced Aug 1, 2026
Merged
youknowone
added a commit
that referenced
this pull request
Aug 2, 2026
…d closure_freevar_branch_resume Six synthetic fixtures landed without a `.jitstats` file, so check.py's always-on regression floor hard-failed each of them with "no committed jit-stats baseline" on every backend it ran: hot_loop_exit_then_class_stmt, raise_reg_unbound_jitstress (#890) inline_freevar_after_mayforce, math_log_trig_hot, tuple_unpack_array_backed_hot (#934) pypy_dict_primitives_nonbinding (#957) Record all three backends for each. A native-only record desynchronizes the tracked `*.wasm.jitstats` set, because `--backend wasm` is not in the default backend list and so would keep comparing against nothing. `closure_freevar_branch_resume` is re-recorded deliberately. Its committed baseline is `loops_aborted=3 loops_compiled=6 guard_failures=23380 bridges_compiled=0`; it now measures `0 / 3 / 606 / 3`, identically on both native backends. The compiled-unit total is unchanged at 6 — three units that were compiled as separate loops are now compiled as bridges — while the aborts disappear and the guard failures fall 38x. Every gated field moves the tightening way (`loops_aborted` and `guard_failures` down, `loops_compiled` down against a fall-gate that now pins 3 instead of 6), so the re-record arms the floor further rather than disarming it. Only `bridges_compiled` rises, and it is in no gated group. Deliberately NOT re-recorded, so the floor keeps reporting them: exception_args_virtual loops_aborted 0 -> 3 exception_multi_handler_warmup loops_aborted 0 -> 23 exception_reraise_tb_depth_jitstress loops_aborted 0 -> 1198, loops_compiled 805 -> 305 list_length_hint_validate loops_aborted 14 -> 34 These raise a `JITSTATS_BADNESS_FIELDS` counter, whose healthy value is 0. Recording them would switch the floor off for those fixtures permanently, which is the opposite of what the counter is for. They are main's own drift away from the baselines `#947` recorded: every one reproduces on both native backends with byte-identical counters, so the movement is walker-level rather than backend-level. Attribution did not converge on a single commit — the nine commits main landed after `#947` include three whose pre-merge check.py was fully green, and reverting `#954` reproduces the counters exactly. `pickle_terminal_raise_resume` (#845) also has no baseline, and is not recorded here either: it does not fail the floor, it crashes (exit 1) on both native backends. The unpickler's read position desyncs under the low JIT thresholds the fixture sets, dispatching `readline()` payload bytes as opcodes. `PYRE_JIT=0` and `pypy3` both pass, so a baseline would only record the crash. Assisted-by: Claude
This was referenced Aug 2, 2026
youknowone
added a commit
that referenced
this pull request
Aug 7, 2026
…es no host produces `pyre/check.py` has been red on main for binary_int_overflow_local_resume, exc_bridge_entry_guard_not_removed and list_append_write_barrier_gc since 9d2fff9, on ubuntu-24.04, macos-latest AND windows-latest (run 31140634730), and locally on macOS across all three backends. Every one of those hosts observes bridges_compiled/guard_failures 5/647, 4/809 and 5/1345. Those are exactly the values that stood on main before 9d2fff9 (last written by #947 and #1059); 9d2fff9 recorded 6/686, 5/1009 and 6/1562, which reproduce nowhere. The re-record was taken against a base whose behaviour these fixtures no longer had, and the merge replayed it. Re-recorded on dynasm, cranelift and wasm. The counters land back on the pre-9d2fff92649 values; the `field_pos_*` fields 9d2fff9 added are kept. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 7, 2026
…es that have main red on every OS (#1099) * jit: keep find_biggest_function's closed-frame result when the recorder is gone pyjitpl.py:3562 reads `self.history.get_trace_position()` unconditionally, so the `max_key` the closed-frame loop above produced always survives to the return. pyre's recorder is an `Option` and the port spelled that read as `self.tracing.as_ref()?`, which returns `None` for the whole function whenever tracing has ended with an unmatched open entry still in `portal_trace_positions`. Only the open frame is unmeasurable without a recorder, so only its measurement is skipped now. Not reachable from `blackhole_trace_too_long_slow`, which holds `self.tracing` as `Some`; `find_biggest_function` is `pub`. `find_biggest_function_keeps_a_closed_frame_when_the_recorder_is_gone` covers it and fails with the `?` put back. Also corrects blackhole_inlined_callee_local_after_escape_declined.py's second description block, which still called `sys._getframe()` the residual after the file's own header states it folds and the added `.f_locals` read is the force. Assisted-by: Claude * jit: emit sys._getframe's mark_as_escaped as a setfield, and carry the sized frame's jitdriver out of find_biggest_function vm.py:54 `f.mark_as_escaped()` is traced as an ordinary `setfield_gc` on the flag. The constant-depth fold emitted it as a void CallN into a Rust helper instead, which hides the update from the optimizer and its heap cache. Replaced with the read/or/store the `tb_frame` fold in the same file already uses (specialize.rs:2299-2313): getfield_gc_i(flags) + int_or(FLAG_ESCAPED) + setfield_gc + heapcache_setfield_cached. `jit_frame_mark_as_escaped` is deleted. pyjitpl.py:3575 returns `max_jdsd, max_key`, and pyjitpl.py:2821-2824 uses both -- the disable goes through the OWNING driver's warmstate and that driver is what `aborted_tracing_jitdriver` stores. The port dropped the jd_no its own log entries already carry and hardcoded driver 0. It now returns `Option<(usize, u64)>` and the caller stores the index it was given. pyre keeps one WarmEnterState on the MetaInterp rather than one per JitDriverStaticData, so `disable_noninlinable_function` still lands on that single state; the comment names it. No recorded counter moves on dynasm, cranelift or wasm. Assisted-by: Claude * bench: restore the three jit-stats baselines #1063 replaced with values no host produces `pyre/check.py` has been red on main for binary_int_overflow_local_resume, exc_bridge_entry_guard_not_removed and list_append_write_barrier_gc since 9d2fff9, on ubuntu-24.04, macos-latest AND windows-latest (run 31140634730), and locally on macOS across all three backends. Every one of those hosts observes bridges_compiled/guard_failures 5/647, 4/809 and 5/1345. Those are exactly the values that stood on main before 9d2fff9 (last written by #947 and #1059); 9d2fff9 recorded 6/686, 5/1009 and 6/1562, which reproduce nowhere. The re-record was taken against a base whose behaviour these fixtures no longer had, and the merge replayed it. Re-recorded on dynasm, cranelift and wasm. The counters land back on the pre-9d2fff92649 values; the `field_pos_*` fields 9d2fff9 added are kept. Assisted-by: Claude * jit: arm the deferred escape-flush undo when only the locals region flushed `flush_active_frame_escape`'s force arm has three outcomes. A committed full flush publishes a resume pc into `COMMITTED_FRAME_ESCAPE_PC`; an all-or-nothing decline discards the undo capture; the third -- the full flush declines and `flush_locals_region_to_frame` writes slots `0..nlocals` on their own -- did neither. That leg claims no resume pc, so `take_committed_frame_escape_pc` yields nothing and the walk-end block gated on it is skipped in its entirety, including the `restore_escape_flush_undo()` in its `else`. The capture stays armed, `LiveLastInstrGuard::drop` reads an armed capture as a flush owning the frame and declines to put `last_instr` back, and the legacy replay re-enters one opcode past the call on an operand stack no flush wrote: `value-stack underflow: depth=N base=N`, a JIT-only panic with no program output. `mark_escape_flush_undo_pending()` routes the leg to the walk-end deferred restore, which is already conditioned on no continuation having claimed the flushed frame -- so where the walk goes on to adopt a blackhole image the request is consumed without restoring and the adoption keeps the frame it claimed. Restoring earlier is not equivalent: making `LiveLastInstrGuard::drop` test the commit instead removes the crash and returns a stale caller line, because the walk goes on after the residual and nothing else advances `last_instr`. `bench/synth/handler_tb_frame_locals_after_declined_flush.py` reaches the leg: `'i' in tb.tb_frame.f_locals` forces the frame mid-expression, with the `seen.add` receiver and its bound method live below the value being computed. A/B on the cranelift binary that reproduced it: 10/10 panics without the change, 0/10 with it, output `[True]` matching `PYRE_NO_JIT=1`. Assisted-by: Claude * bench: survey a caller's f_lineno and f_lasti from two call sites A callee reading its caller's frame through `sys._getframe(1)` had no coverage of the resume coordinate: `bench/synth` holds ten `_getframe(1)` fixtures, one `f_lineno` fixture (a traceback frame) and no `f_lasti` fixture at all. Both fields resolve off `last_instr`, which compiled code does not store per opcode, so the value only reaches the frame if the force publishes it. Two call sites are what make that observable. One holds the caller's coordinate constant by construction, so a frozen read is indistinguishable from a live one. Surveying every iteration into a set rather than sampling the last one is the other half: the pre-compile iterations are correct, so a miss appears as a changed row count. `f_lasti` is a bytecode offset and so is not comparable against the pypy oracle; only its discrimination is printed. `f_lineno` is compared directly, relative to `co_firstlineno`. Measured by putting a defect back in: with the `flushed` test dropped from `LiveLastInstrGuard::drop`, so the guard restores at the residual's return instead of at walk end, the fixture reports ([(0, 3), (0, 8), (1, 3), (1, 6)], [0, 0, 1, 1], 3) against its ([(0, 8), (1, 6)], [0, 1], 2) -- the pre-call coordinate appears alongside the call-site one on both legs. cpython, pypy, `PYRE_NO_JIT=1`, dynasm, cranelift and wasm all print the latter. The walk-end epilogue gains the negative result measured while looking for a counter to gate the same defect: every walk reaching that point on this fixture reports `armed=false fb=true`, so a leak counter conditioned on the three adoption flags being false reads 0 whether or not the force arm arms its deferred restore. Assisted-by: Claude * check.py: fail the build on a stale LLBC instead of measuring through it `pyre-jit-trace/build.rs` compares each `build/llbc/*.ullbc` against what its crate's sources hash to now and reports a mismatch as `cargo::warning`, which cargo replays only when it re-runs the build script -- so a run whose crates were cached prints nothing at all. Every number check.py produces is read out of a binary whose field offsets come from those artefacts. Measured on this tree: four measurement runs -- a three-backend gate, two A/B arms and a base control -- carried the mismatch, and the string `LLBC STALE` appears in none of their logs, while `cargo check -p pyrex` on the same tree printed it for all three crates. check.py only ever tested for the artefacts being missing. It now exports `PYRE_LLBC_STRICT=1` before every backend build, the promotion build.rs documents for callers that want a gate, and names staleness in the build-failure diagnostics beside the missing-artefact branch. The cost is that a rebase which moves the LLBC crates stops the next check.py until a re-extraction; `PYRE_LLBC_SKIP_FINGERPRINT_CHECK=1` still opts out for an A/B whose only changed crate contributes no field offsets. First use found one: the wasm jit-stats fall on `exception_reused_object_tb_not_doubled` that four arms reproduced was an artefact of the stale artefacts, and the bench passes on all three backends after a re-extraction with nothing re-recorded. 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.
Two lines of work: a silent wrong-answer fix on the outer-call resume path, and
arming the gate that should have caught its neighbours.
1. A dropped
foriteration on outer-call resumeWhen an inline callee owns a compiled loop the walk aborts the enclosing trace
(
fbw_state.rs, mirroringpyjitpl.py:2818-2828) and the caller resumesforward at the CALL.
collect_call_stack_overridesbuilds that caller's operandstack from three sources, and one live slot has no source at all: the
null_or_selfaPUSH_NULLleaves under the CALL. The walk holds no box for it(
vstack_boxis an emptyOpRef), no live color names it (in_pcdep=None), andthe shadow's
Ref(0)is the same NULL an unmirrored slot reads back as.So the slot stayed absent,
flush_walk_end_state_to_frame_innerdeclined theouter-call flush on a slot whose correct value is exactly that null, the legacy
rollback+replay ran instead, and the R1 in-flight
FOR_ITERdelivery was refusedto avoid a double-apply — dropping the iteration entirely.
The CALL's own operand layout names the slot without guessing:
[callable, null_or_self, arg0 .. arg_{argc-1}]ends atstack_end, so thesentinel sits
argc + 1below it. Any coordinate that does not invert to a plainCallkeeps the conservative decline.The collector's comment also asserted that the live sources already emit a
genuine null-or-self sentinel; that is only true where they hold a box for the
slot, which the
PUSH_NULLposition never does. Corrected.New fixture
bench/synth/foriter_call_resume_drops_iteration.pyprints acount alongside the accumulator, so the loss shows up as
8999vs9000rather than as an accumulator that merely looks plausible. All three ingredients
are load-bearing — two distinct recursive callees, a post-warm-up branch flip,
and a trailing statement.
Also here:
majit: read the diagnostic env gates once, and build their messages behind them, andjit: count the deduped set in intern_liveness' liveness record header.2. The jit-stats regression floor was armed on 3 of 340 synthetic fixtures
_jit_stats_regression_flooris the right instrument for a structural JITregression: it runs on every invocation, including the bare
python pyre/check.pyCI runs, needs no flag, and gates only counters whosedefect direction is fixed. It runs only where a committed
<name>.<backend>.jitstatsexists — andpyre/.gitignoreignored/bench/synth/*.jitstatswholesale with three!negations. "No baseline" wasthe default, and at the gate it is indistinguishable from "recorded and clean":
both print PASS.
The
max-pypy-ratioheader cannot substitute. It is one absolute ceiling perfixture, kept loose to survive slow runners:
recursion_memo_branchsits near8.9x under a limit of 20, so it absorbs a 2x regression and still passes. And it
does not exist at all for wasm —
run_synthetic_benchpassesvs_pypy=Nonethere, so the floor is wasm's only gate.
This PR:
.gitignorepolicy flipped; 359 dynasm +359 cranelift + 355 wasm).
guard_failureson a bounded rise pastbase + max(base // 4, 2).loops_compiledon any fall. This is the class the other two miss: apre-trace decline aborts nothing, so
loops_abortedholds, and it removes thecompiled guards that used to fail, so
guard_failuresfalls — both existinggates read the loss as an improvement while the hot loop runs interpreted.
[jit-stats]with no committed baseline, so anewly added fixture cannot silently reopen the hole. It did exactly that during
this branch's rebases:
defaults_reassigned_midloop,del_cellvar_walk_commitand
method_reassign_after_warmupeach arrived frommainwith no baseline andcheck.py printed PASS for them.
Why one counter gets a band and the other does not
Measured, not guessed — diffing the dynasm and cranelift baselines across the
synthetic fixtures, i.e. two independent code generators:
loops_compiledloops_abortedbridges_compiledguard_failuresCross-runner stability: measured, and it holds
The first CI run was the experiment. macos-latest, ubuntu-24.04 and
windows-latest reported the same fixtures with the same numbers, and rerunning
the suite locally on macOS reproduced them byte for byte, down to the per-backend
tallies. So
loops_compiledis stable across 3 OSes × 2 code generators × 2hosts, and a CI disagreement on it means the trees differ, not the hosts —
which is what it meant there: CI builds the PR merge commit, and
mainhad movedunder the branch.
The gate fired three times. Two were intended; one was a false positive.
type_metatype_method_call loops_compiled 4 -> 2guard_failures 9480 -> 1,bridges_compiled 47 -> 0), reproduced against the pre-rebase binary. Re-record.class_attrs_methods,foriter_inplace_immutable,inline_subwalk_mutating_residualguard_failures 12316 -> 603,loops_aborted 1 -> 0), and the measured ratio matches #942's own published 13.0x/18.3x. Re-record.exception_reraise_tb_depth_{hot,jitstress}—loops_compiled 40 -> 4andguard_failures 0 -> 1803That last one is worth stating plainly, because it bounds what these counters
are worth.
origin/mainbuilt alone reproduces the counters exactly, so it isnot this branch. Interleaved user+sys CPU time, min of 5,
e1e97e61d32vsorigin/main, same host:exception_reraise_tb_depth_hotexception_reraise_tb_depth_jitstress40 specialized loops with 30 aborts and zero guard failures was the worse
shape; 4 general loops plus 9 bridges is twice as fast. So
loops_compileddowntogether with
guard_failuresup does not imply a defect, and no combinationof these counters settles it — only an A/B of two binaries does. The gate's value
is that it forces that look; its cost is that the look is sometimes needed.
main, that nothing else could seeThe same two fixtures move the other way on wasm:
loops_aborted30 -> 44 and30 -> 1198. Times, both measured by check.py itself on the same host:
origin/mainexception_reraise_tb_depth_hotexception_reraise_tb_depth_jitstresswasm has no
max-pypy-ratiogate at all, so this floor is the only instrumentthat can see a wasm-only slowdown — and it was armed on 3 fixtures until now.
The baselines here are re-recorded (pinning the higher abort count) so this PR
is not blocked on someone else's defect; the rise is stated rather than absorbed.
Which of
#936/#935/#914/#930causes it is not bisected.Non-vacuity
Each gate was shown to fail on the shape it targets, not merely to exist:
guard_failuresbandnested_break_bridge_resume_hazardnarrowingFAIL … loops_aborted 0 -> 1, guard_failures 1757 -> 2613PASS 0.16s (8.4x)loops_compiledfallFAIL … loops_compiled 3 -> 2FAIL … no committed jit-stats baseline (…)What this gate family cannot see
It watches what the JIT compiles, never how well —
_jit_stats_snapshot's owndocstring says so. Measured: #933 took
type_immutable_rejectfrom ~35x to ~1xby removing a per-iteration
call_may_forceresidual, and all four counters areidentical before and after. That class belongs to
max-pypy-ratio, which#933 tightened 152 -> 15 for the same fixture. The two gates do not substitute
for each other.
3. The baseline commit disabled both automated reviewers
Committing ~1000 baselines put this PR at 1027 changed files. CodeRabbit skipped
it outright ("927 over the limit of 100") and the Codex parity review listed all
1027 names into its prompt and then exited 1 with no report.
.jitstatsfiles are generated golden data with no RPython/PyPy counterpart, sono parity finding can ever cite one.
.github/workflows/codex-review-run.ymlnowbuilds the authoritative list with
git diff --name-only upstream/main -- . ':(exclude)*.jitstats'and reports thewithheld count in the prompt header and in the empty-diff message;
.github/codex-review-prompt.mdcarries the same exclusion for the local/codex-reviewpath. On this branch that is 1132 -> 112 files.CodeRabbit's own
path_filtersis deliberately not added — whether itsfile-count cap applies before or after filtering is unverified, and shipping a
third-party config whose effect cannot be checked is worse than naming the gap.
Verification
pyre/check.py— dynasm 359/359 · cranelift 359/359 · wasm 355/355, runagainst the committed state with all gates armed.
Rebased onto
origin/main(e6e2b25a715). Conflicts: one file across tworebases.
pyre-interpreter/src/baseobjspace.rsresolved by dropping our commitin favour of #933 — both fix the same misjudgement of
type's own__setattr__/__delattr__(theinit_type_typeslot pair forwarding toobject_setattr/object_delattrfor the Carlo Verre hackcheck), but #933 alsoemits the metaclass
version_tagguard the fold needs to be sound againsttype-dict mutation, which ours lacked.
majit-backend-dynasm/src/runner.rsresolved to
mainin all three regions:mainlanded the same cacheddiagnostic-env gates in
lib.rs:76,83, beside themajit_log_enabledour owncomment cited as the model, and carrying the same measured rationale — leaving
only the GC-side gate and a
pyjitpl.rscomment correction from our commit.🤖 Generated with Claude Code