jit: keep attach_tb inside the frame that re-raised, and merge near-duplicate synth benches - #829
Merged
Conversation
blackhole_resume_via_rd_numb cleared attach_tb on the error handed to the
interpreter when the exiting frame's last opcode was a bare reraise. That
frame's own record had already been skipped a few lines above by the same
bare_reraise test, and a compiled frame never runs handle_exception, so
the cleared flag was first read by the interpreter caller, which then
skipped its own traceback node. handle_exception restores the flag after
one frame for the same reason.
A frame holding a try block it does not match falls through to the
except-cleanup RERAISE, so an exception escaping such a compiled frame
lost the caller's node:
before ('d_nonmatching_except', 'mid', 'leaf')
after ('<module>', 'd_nonmatching_except', 'mid', 'leaf')
Add exception_escape_caller_frame_tb_node.py covering the non-matching
except, finally, while and no-intermediate-frame shapes, plus a no-try
control that escapes through the guard-exception exit, which never
touched the flag.
Assisted-by: Claude
…r traceback names Reading tb_frame.f_code.co_name off every traceback node and retaining the resulting tuples, in a hot try/except taking two exception classes through an inlined frame, aborts cranelift with GC BUG: invalid type_id=... site=object_total_size from gc_alloc_nursery_shim. dynasm and PYRE_JIT=0 are clean. The header lists the ingredients each of which stops the abort when removed. Lives under _pending/ so check.py's non-recursive glob skips it. Assisted-by: Claude
20 benches that differed only in a value, an ordering or a chain length are
folded into 6 new files and 6 existing ones, 317 -> 303 files. Each merged
file keeps one driver loop per original shape rather than parameterising a
single loop, so every call stays the direct call it was and the traced shape
is unchanged.
callee_return_side_effect <- const_int_/float_return_side_effect
global_reassign <- + global_reassign_obj
inline_chain_depth_typeflip <- depth2_/depth3_/depth7_inline_chain_typeflip
inline_subwalk_mutating_residual <- ..._abort / ..._noexc
inlined_helper_mutation <- + inlined_mutation_before_abort
closure_freevar_branch_resume <- closure_freevar_branch_{list_cell,readonly,nonlocal}
kept_stack_deep_var_shortcircuit <- + ..._mutate
call_star_forms_inlined_callee <- call_kw_star / call_function_ex_star
unary_negative <- + unary_negative_min
inline_multiframe_branchy_carrier <- ..._drain_journaled_store / ..._module_branch_deopt
foriter_in_while <- + nested_foriter_range
foriter_call_body <- + nested_foriter_call
inline_chain_depth_typeflip halves N: three drivers at the original count made
it the slowest synthetic bench by 4x, and it now costs about what the slowest
of the three did alone.
The `hot`/`jitstress` pairs are left alone; the jitstress twin lowers the
trace thresholds to 1 so it covers the recording path rather than the steady
state, which is a different shape despite identical output.
Update the two places that named a merged file: the wasm codegen test's
bench list and a churn-guard comment in the trace walker.
Assisted-by: Claude
ResidualFrameChainGuard::enter stores f_backref into an old-gen concrete frame without a write barrier, matching the shape its sibling in the walker carries one for. Adding the same barrier there does not change the abort rate: 18/20 runs abort with it, 8/10 without. Note it in the header so the reading is not repeated. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (38)
✨ 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 |
![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: 3722a07ab8
ℹ️ 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
+71
to
+73
| print(depth2()) | ||
| print(depth3()) | ||
| print(depth7()) |
There was a problem hiding this comment.
Running depth2() first warms the shared g/h function-entry counters; because the function-entry threshold (1619) is only slightly above the loop threshold (1039), those helpers compile during depth3() warmup before its loop is traced. The depth-3 trace can therefore call compiled g instead of inlining the intended three-frame chain, and the same contamination reduces the later depth-7 chain. Separate helper chains or processes are needed to preserve the coverage these former standalone benchmarks provided.
Useful? React with 👍 / 👎.
Comment on lines
+59
to
+60
| append_first() | ||
| store_first() |
There was a problem hiding this comment.
After append_first() runs, the shared push and bump entry counters are already close to the function-entry threshold. They consequently compile early in store_first() before that loop becomes hot, so its trace calls compiled helpers rather than inlining the STORE_ATTR followed by the deliberate list.append abort. That removes the specific committed-inline-mutation-then-abort coverage supplied by the deleted standalone benchmark; use distinct helpers or otherwise reset/isolate JIT state.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 3722a07). 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)None identified in the changed runtime paths. 4. Structural adaptations
|
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
`FBW_INLINE_CHAIN_DEPTH`'s doc cited `depthN_inline_chain`, which no longer names any file: #829 deleted `depth2_`/`depth3_`/`depth7_inline_chain_typeflip.py` and added `inline_chain_depth_typeflip.py` in the same commit. Cite the surviving file and record that the ~2.0-2.3x number was taken before the consolidation. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
…d deny through the warm state (#1364) * jit: name the fixture the inline-chain depth cap was measured on `FBW_INLINE_CHAIN_DEPTH`'s doc cited `depthN_inline_chain`, which no longer names any file: #829 deleted `depth2_`/`depth3_`/`depth7_inline_chain_typeflip.py` and added `inline_chain_depth_typeflip.py` in the same commit. Cite the surviving file and record that the ~2.0-2.3x number was taken before the consolidation. Assisted-by: Claude * majit: publish the preview short-preamble export as one PreviewShortState Replace the four OptContext fields `exported_short_boxes`, `exported_short_inputargs`, `exported_short_inputarg_refs` and `exported_short_args_state` with a single `Option<PreviewShortState>` holding the three vectors plus an `Option` args_state. `preamble_end_args` stays a separate `Option<Vec<OpRef>>`. optimizer.rs binds `create_short_inputargs`, `create_short_inputarg_refs`, the args-state tuple and the filtered short boxes to locals and assigns the struct once, after the majit_log dump. `force_box_for_end_of_preamble` and its `preamble_end_args` write are unmoved. unroll.rs::export_state_with_bounds reads the args-state through `preview_short_state.and_then(|p| p.args_state)`, and takes the short inputargs / inputarg refs / short boxes from one match on the `Option` instead of an `is_empty()` test on a defaulted vector. The `debug_assert_eq!` cross-checking `exported_short_inputarg_refs` against `exported_short_inputargs` is dropped; the two are now built and published together. The length check against the export-site `label_args + virtuals` recompute is kept. unroll.rs tests gain `publish_preview_short_state` and `mint_short_inputargs` helpers; the four fixtures that wrote the ctx fields directly now construct the struct. Assisted-by: Claude * docs: correct the jit.virtual_ref emit note and two fbw fixture headers gate-triage.md claimed `opimpl_virtual_ref` / `_finish` have no caller outside a `#[test]`. They do: `walker_ec_enter` / `walker_ec_leave` call them on the live inline-push path in `pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`, which the same file already says 100 lines later. The parenthetical now states the narrower residue instead -- `vrefs_before_residual_call` / `vrefs_after_residual_call` iterate zero times over a level the walker inlines without seeding a frame -- and points at `jitcode_dispatch/mod.rs` item (a), where that residue is recorded. It also notes that `mod.rs`'s upstream citation for the residue is wrong: `perform_call` (pyjitpl.py:2445-2449) is `newframe` + `setup_call` and never touches `virtualref_boxes`; upstream's vref comes from `ExecutionContext.enter` (pypy/interpreter/executioncontext.py:88-89), traced through on an inlined call. getframe_root_loop_force_blackhole_crn.py said "this file adopts it five times". Its committed baselines record fbw_blackhole_adopted_single_frame=0, loops_aborted=0, loops_compiled=1 on all three backends; ca9edf7 (#1096) moved them from 5 / 5 / 0. Header now states the recorded numbers and points at the `_declined` sibling, which records 5 / 5 / 0. blackhole_inlined_callee_local_after_escape.py opened "Guard for what an adopted multi-frame blackhole chain owes its inner levels". fbw_blackhole_adopted_multi_frame is 0 in all three of its baselines and was 0 before #1096 as well; the five adopts it used to take were single-frame, and today it takes none (0 / 0 / 2). Header now says so, names the eight fixtures' worth of corpus that does pin the multi-frame arm (getframe_inline_subwalk_multiframe, getframe_while_inlined_callee_subwalk et al., all nonzero on three backends), and points at the `_declined` sibling. No baseline was re-recorded; no Rust and no executable Python changed. Assisted-by: Claude * docs: separate the MIFrame perform_call builds from the frame a vref is taken of The previous commit's gate-triage note called `pyjitpl.py:2445-2476` a wrong citation and said `mod.rs` held the last copy. Both overreach. The range spans `newframe` (:2455-2476), which does build a fresh frame per inlined call -- an `MIFrame`, the tracer's register frame. `pyre-jit-trace/src/helpers.rs` cites it for exactly that and is correct; stripping it there would remove a right citation. The same sentence also lives in `inline_call.rs`, so `mod.rs` was never the only copy. What is actually wrong at the two vref sites is conflating that `MIFrame` with the app-level frame `ExecutionContext.enter` takes `jit.virtual_ref` of. State that at both, and say in gate-triage which of the two claims the range supports. Also: the emit note said `virtualref_boxes` is populated for every seeded level, but `entered_ec` adds a non-null concrete frame and a non-null `execution_context` on top of seeding -- necessary, not sufficient. And the crn fixture header's inserted paragraph left "Its effects are idempotent" pointing at the `_declined` sibling instead of the drive. Assisted-by: Claude * majit: lower the int/float list capacity read as arraylen_gc `list.int_capacity` / `list.float_capacity` emitted `getfield_gc_r(<strategy>_items.block)` followed by a struct `getfield_gc_i(ItemsBlock.capacity)`. Upstream's capacity read is `len(l.items)` on `l.items: Ptr(GcArray(ITEM))` (rpython/rtyper/lltypesystem/rlist.py:251 in `_ll_list_resize_hint`, rlist.py:286 in `_ll_list_resize_ge`), which the rtyper lowers to `getarraysize` and `jtransform.py:808 rewrite_op_getarraysize` rewrites to `arraylen_gc`. `list.obj_capacity` already emitted `ArrayLen`. Both arms now emit `OpKind::ArrayLen` on the backing block, matching the object-strategy arm. The two unit tests are renamed and assert the new op shape. Assisted-by: Claude * majit: narrow the PreviewShortState alignment claim to the two vectors that share an index space The struct doc and the publication comment both said the three published vectors are "index-aligned by construction". Only two of them are: `short_inputargs` and `short_inputarg_refs` get one entry per `add_short_input_arg`. `short_boxes` is a different population -- the surviving produced short ops, after `short_boxes_exported`'s `filter_map` drops every one whose `canonical_result` is constant -- so its length is unrelated to the other two and `short_boxes[i]` pairs with nothing. What one publication site does buy is that a reader cannot see one vector from this evaluation of the preview beside another that was never written; both comments now say that instead. The unroll.rs test helper restates the refs/inputargs length invariant as a `debug_assert_eq!`. The production publisher gets it from `create_short_inputarg_refs`, which asserts internally; a fixture builds the two vectors by hand and had no check between a short refs vector and the failed `Weak` upgrade it causes past the peel boundary. Assisted-by: Claude * docs: correct eight comments that assert a capability the code already has Each of these states an absolute negative -- "never emits", "never calls it", "cannot be added and go unnamed" -- that its own call site contradicts. - `residual_call.rs`: `vrefs_after_residual_call` is called by the walker, under the `is_may_force` gate mirroring `pyjitpl.py:2007`. Its loops are empty because no `jit.virtual_ref` producers exist, which is a fact about the vref list, not about the call site. - `branch.rs` `decode_side_other_target`: the fused `goto_if_not_<cmp>` forms do reach the walk dispatch, minted by `majit-translate`'s jtransform as `ExitSwitch::Fused` for the LLBC-lowered graphs. What is true is narrower: the sole caller passes a `PyJitCode`, built per Python CodeObject by pyre's own codewriter, and `ExitSwitch::Tuple` -- the only path to a fused goto there -- has no producer outside `flatten.rs`'s own unit test. A fused form would be declined, not mis-decoded. - `inline_call.rs` / `fbw_state.rs`: `callee_body_contains_raise` caps a raising callee at TWO multiframe levels, not at the top inline level, and the cross-frame unwind bridge is built. The measurement beside `effective_multiframe_depth` is what bounds it: two levels green, a third taking `selfrec_tail_exception_unwind` from 937 to 7408 guard failures. - `state.rs` (two sites) / `helpers.rs`: `perform_call` (`pyjitpl.py:2445-2449`) is `newframe` + `setup_call` + `raise ChangeFrame`, and `newframe` (`:2455-2476`) builds an `MIFrame` and nothing else. Upstream has no recording-time app-level frame to hand out at that point; it gets one from tracing the interpreter's own frame construction, which pyre does not do. - `state.rs` / `trace.rs`: the pointer to `perform_call (trace_opcode.rs)` is dead -- no such function there. Callee sym state is set by `inline_call.rs`'s `setup_call` port. - `diag.rs` `SPEC_FOLD_ROWS`: the table cannot become a complete census by adding rows. It names a fold by its function, and two shapes have no name to give: a fold whose emit is inlined into a `match` arm has no function, and a registry-dispatched fold grows by one entry with no new call site. Also: `specialize.rs` drops line numbers from an in-repo file reference, and `tupleobject.rs` records that the `w_tuple_new` interception is the sole reason the `_ff` layout has no producer -- so restoring the upstream shape also makes the walker's `ff` specialisation arm live. Assisted-by: Claude * fbw: print all fourteen fbw_diag slots from both readers and namespace the escape/force keys The two readers of the same counter array printed disjoint index sets: the native reader (pyre/pyrex) printed {1, 6..13} and the wasm host (pyre-wasm-runner) printed {0..5, 11..13}, so slots 0 and 2..5 were bumped on the native backends and readable only through the wasm export, and slots 6..10 the other way round. One key per tally slot is now declared beside the counters as `pyre_jit_trace::trace::fbw_diag::LABELS` (length `RING_BASE`, so rustc rejects an unnamed slot), re-exported as `pyre_jit::FBW_DIAG_LABELS`, and joined against `get(i)` by both readers into a single `[jit-stats] fbw_diag` line carrying the same keys in the same order. The runner mirrors the array positionally, as it already does for `MC_DIAG_LABELS`, since it links no pyre crate. The MIDBODY_LATCH doc said the native corpus reaches neither leg "so these say whether the wasm target does"; it now says that a nonzero native value is itself the news, which is why both readers print it. On wasm the tally line moves out of the PYRE_WASM_JIT_STATS block — check.py never sets that variable — into the MAJIT_STATS block, the gate the native reader prints under. The `[fbw-census]` ring stays where it was. The four gated keys (fbw_rolled_back_with_effects, fbw_store_journal_rollback_failed, fbw_blackhole_adopted_single_frame, fbw_blackhole_adopted_multi_frame) keep their spelling and move from the counter line onto that fbw_diag line; the single `pyre_fbw_diag` lookup still feeds the missing-export refusal. The two `subset/total` fractions become named keys, the hazardous subset spelled `fbw_midbody_latch_new_unjournaled` and `fbw_escape_plain_fallback_unclean`. The five bare keys portal_only, published_callee_only, portal_and_published_callee, by_portal and by_callee_only are renamed fbw_escape_portal_only, fbw_escape_published_callee_only, fbw_escape_portal_and_published_callee, fbw_force_by_portal and fbw_force_by_callee_only: check.py's `_jit_stats_merged` folds every `[jit-stats]` line into one flat map, in which an un-namespaced key is a collision hazard. No committed .jitstats baseline carries any of the five under either spelling, so nothing is orphaned by the rename. They are left out of JITSTATS_SNAPSHOT_FIELDS, i.e. deliberately ungated, and check.py now records why: they are workload counts with no healthy value and no measured polarity (the reason `bridges_compiled` sits in neither regression list), and listing one would make every baseline that lacks it compare 0 -> N and fail until re-recorded. That re-record is a decision to take deliberately, with a polarity in hand. Checked with `cargo check -p pyrex`, `cargo check -p pyre-wasm-runner` and `cargo fmt --check`. No pyre binary was built or run, and no .jitstats snapshot was re-recorded. Assisted-by: Claude * fbw: make the decline census process-wide, as its own doc already claimed `FBW_DECLINE_CENSUS` was a `thread_local!` while the comment above it called it a "Per-process census". pyre installs `_thread` (`pyre-interpreter/src/importing.rs`), so Python threads are real OS threads and each traces on its own; the dump therefore reported only whichever thread happened to print it and silently dropped every decline the others took. Now a `static Mutex<BTreeMap>` behind a `census_map()` accessor that recovers from poisoning -- a map of counters has no invariant a panicking writer can leave broken, and a diagnostic that goes silent after an unrelated panic is worse than one that keeps counting. The lock costs nothing at this rate: the map is touched only on the cold decline path, never on the hot trace path. Pinned by `the_decline_census_counts_a_record_from_another_thread`, shown to fail on the `thread_local!` storage first: assertion `left == right` failed: a decline recorded off-thread never reached the census left: 0 right: 1 No gate exposure: `fbw_census` appears in no check.py field, no pyrex path and no committed baseline, so this changes a diagnostic only. Assisted-by: Claude * fbw: pin the wasm runner's fbw_diag label mirror against the slot constants `pyre-wasm-runner` links no pyre crate, so it restates `pyre_jit_trace::trace::fbw_diag::LABELS` as a positional array. rustc length-checks each side against its own constant (`RING_BASE` / `FBW_SLOTS`), but neither compiler sees the spellings, so a rename drifts silently and every tally from the divergence onward is printed under the wrong key -- and check.py folds every `[jit-stats]` line into one flat map, so a wrong name is compared against the wrong baseline rather than reported as missing. Four checks, following `majit-metainterp/tests/mc_diag_mirror.rs`: the parser is validated against the compiler-enforced count before being used to diagnose drift, the two declared counts must agree, and the two arrays must agree entry by entry. A positive control injects both drift shapes into the real runner source in memory -- a rename, caught by the entry comparison, and a dropped last slot, which leaves every surviving entry correctly named and so can only be caught by the length check. Perturbing the real text rather than a fixture is what makes the control cover the anchors. A fifth check closes what a two-array diff structurally cannot see: `LABELS` shifting against the slot CONSTANTS moves both arrays together, renaming every tally on both backends at once. Each label is bound to its own constant, and the bound slots are required to be exactly `0..RING_BASE` so a new slot cannot go unbound. The bindings are written out rather than derived from the constant names because two of them break the mechanical reading: `ESCAPE_FORCE_BY_PORTAL` is `fbw_force_by_portal`, not `fbw_escape_force_by_portal`. Assisted-by: Claude * docs: name the third fold shape SPEC_FOLD_ROWS structurally cannot hold The table names a fold by its function. Two shapes with no name to give were already recorded; a sweep of `vable_ops.rs` found a third. An ELISION fold recognises a shape and emits nothing, so a census keyed on "what IR did this fold emit instead" has nothing to key on. Three arms are this, all guarded by `fbw_strict_fold_frame_reg`: a store to the current inline level's own unseeded portal frame is a virtual-field write, folded away with no SETFIELD_GC recorded. Their recognisers (`fbw_strict_fold_frame_reg`, `folded_store_is_observable_local`) are predicates -- they cannot emit, because the eliding is the arm. This is distinct from the functionless-replace shape already listed: `bool_box_truth_lookup`'s arm has no function but does write a result. Assisted-by: Claude * majit-translate: re-anchor pyre-side comment refs to symbols Replace `file.rs:NNN` citations in codewriter/ and annotator/ comments with the symbol that owns the cited code, verified by opening each target. Upstream `.py` line citations are untouched. String-literal occurrences (assertion and panic messages) are untouched. Also drop the internal tracking labels `Z2.5 Path C`, `Phase I3` and `F2 followup`, the filename-less `(line ~3273)` pointer, and replace insns.rs's "documented at the const-table site above" with the fact that byte 18 now houses `BC_GOTO_IF_NOT`. Four citations are left as-is because their target no longer exists: `build_flow.rs:215` (file deleted with the syn-AST front-end) in call.rs and codewriter.rs, and `parse.rs:314-318` (parse.rs shrank from ~2000 to 98 lines) twice in call.rs. Assisted-by: Claude * pyre-jit: re-anchor comment references from line numbers to symbols Replace every `file.rs:NNN` citation in pyre/pyre-jit comments with the symbol that lives at the cited location, or with the bare filename where the surrounding text already names the symbol. Upstream `.py` citations (rpython/pypy/lib-python) are untouched. Also replace directional cross-references ("see below", "see comment above", "see the deferral below") with the named symbol they point at, drop the internal "Slice α-2" and "Phase L2" markers, and drop stale self-file line refs ("line 1891", "line 2120-2122", "at line 1495"). Comment-only; no code, string literal or test data changed. Assisted-by: Claude * optimizeopt: re-anchor pyre-side comment refs to symbols Replace `file.rs:NNN` / `symbol:NNN` citations in optimizeopt comments with the symbol that lives there. Upstream `.py:NNN` citations are unchanged. - `propagate_from_pass_range:3336-3339` and `Optimizer::emit_operation:3524-3528` / `:3527` drop their line ranges; both symbols had moved (4582 / 4816). - `dispatch_emit:2631/2766` in heap.rs and virtualize.rs names no existing symbol; replaced with `emit_residual_call` / `handle_side_effects`. Strip internal tracking labels from comments: `Cat-2.2`, `Path A`, `Post-S0`, `S11`, `S7`, `S8`, `E5b`. GitHub references (`#9`, `#115`, `#160`, `#175`) and the `PYRE_S9_PROBE` env-knob name are kept. Make cross-reference pointers self-contained: "see comment above", "see doc comment above", "see the closure above", "see field doc", "same evidence as the args loop above", "for the reason given in the field loop above", "same rationale as raw fields above", "see the Virtual arm above", "the arms below", "the guard below" now state the load-bearing fact or name the owning symbol. Repair two sentences left dangling by previously stripped refs (`optimizeopt/mod.rs` setinfo_from_preamble, `virtualstate.rs` visit count). Comment-only: no code, string literal, or test data changed. Assisted-by: Claude * pyre-jit-trace: re-anchor pyre-side comment refs to symbol names Replace `<file>.rs:NNN` line citations in pyre-jit-trace comments with the symbol that lives at the cited location, after opening each target. Upstream `rpython/`, `pypy/` and `lib-python/` `.py:NNN` parity citations are left unchanged. Where the cited pyre-side location holds no nameable symbol, or the file/function no longer exists, only the `:NNN` is dropped and the filename kept. Strip internal session tracking labels (`B3`, `C3 S1`, `E1`, `G0`/`G1`/`G2`, `Epic G`, `gap 10 slice 2b`, `P2 drain`, `P3`, `Route C`, `Task 8`, `increment 2b`, `STEP 5`) from the comments that carried them; GitHub issue references (`#32`, `#73`, `#171`, `#203`, `#215`, `#62`/`#23`) are kept. Also make four cross-reference comments self-contained by stating the fact instead of pointing at another comment ("see the module preamble", "see above", "the `current`-frame pattern", "see `history.rs`"). Comment-only: no code, string literal or test data is modified. Assisted-by: Claude * docs: correct three comments refuted by their own call sites Each claimed a capability was missing; each is contradicted by the code it sits next to. `descr.rs`'s tag block says the Field tag is load-bearing for a synthetic `FieldIndexDescr` that unpacks offset/size/type/signed out of the index bits. That descriptor and its helpers were deleted — `majit-ir`'s descr module records the removal — and `VirtualizableFieldState.fields` is keyed by `FieldDescr::index_in_parent()` now (`info.py:203-206`). Nothing decodes the tag; what it still buys is disjoint index ranges so two descr kinds cannot collide on one `HeapCache` key. The `ptr_eq/rr>i` opcode-table row says the `b1 is b2` fast path is omitted, "same rationale as int comparisons". Both handlers implement it: `binop_ref_to_int_record` answers an identical operand pair out of `fastpath_same_boxes` without recording, and so does `binop_int_record`. The `raise` arm says resume-data capture is omitted, pointing at the `goto_if_not/iL` arm, which carries no such comment. The guard this arm emits calls `walker_capture_snapshot_for_last_guard(ctx, op.pc)` twelve lines below, which is `generate_guard`'s `resumepc=orgpc`. Assisted-by: Claude * descr: mark PyCode.co_firstlineno immutable, per _immutable_fields_ `pycode.py:95-106` lists `co_firstlineno` in `_immutable_fields_`; the PyCode descr group marked every field mutable because its spec builder hard-coded the flag. Give the builder the flag as a parameter and set it from the upstream list: only `co_firstlineno` changes. `co_name` and `hidden_applevel` are absent from that list and stay mutable — `w_name` is realized lazily by `w_code_name_obj` and does go null -> non-null after construction — and `code_ptr` is the raw body pointer with no upstream slot. The slot really is write-once: `box_code_constant_with_firstlineno` writes it onto an object `box_code_constant` has just boxed out of a fresh `Box`, so no caching lets a reader see it first, and `code.replace` reads it and builds a new code object rather than writing this one. No behaviour change is expected or observed. The only trace-side reader of a field descr's `is_immutable` is the replay-cleanliness rule in `fbw_state.rs`, which fires on a `setfield_gc` into a freshly allocated object, and traced Python never constructs a PyCode. `check.py --no-build --backend dynasm`: 441/441, no jitstats delta. Assisted-by: Claude * majit-metainterp: replace pyre-side line refs in comments with symbol names Comment-only change across `majit/majit-metainterp/src/` (excluding `src/optimizeopt/`) and `majit/majit-metainterp/tests/`. - Rewrite `<file>.rs:NNN` citations of pyre's own Rust sources to name the symbol that lives there, or drop the line number when the surrounding prose already names it. Line refs inside string literals and inside ```text panic transcripts are left untouched. - Upstream `.py:NNN` citations (rpython/, pypy/, lib-python/, lib_pypy/) are unchanged. - Strip session-local tracking tags (Slice X-D/X-G/X3-E/QQ-n/P3/T-final, Sub-slice B/C.x, F.n-orthodox, M2 Step n, Box Identity Phase E Step n, #19 Step n, Step 2e.2b, P1.5) from comment prose. - Replace "see above"/"see the header"/"same rationale as" pointers with the fact plus the symbol that holds the rest. Assisted-by: Claude * majit-translate: replace pyre-side comment line refs with symbol names Strip `file.rs:NNN` / `file.rs:NNN-MMM` line numbers from comments in majit/majit-translate/src (excluding codewriter/ and annotator/) and majit/majit-translate/tests, keeping the symbol name the comment already cited or naming the enclosing item where the citation had none. Upstream `.py` citations are untouched. Also: - qualify ambiguous bare `model.rs` references to `flowspace/model.rs` or `annotator/model.rs` where the named symbol resolves there - drop self-referential filename parentheticals in rclass.rs, rpbc.rs, rtyper.rs, mir.rs, flowspace_adapter.rs, cutover.rs and rbuiltin.rs - point flowspace_adapter.rs's exc_from_raise cross-reference at the "TODO: `Constant` SSA carrier shape" section that exists in that module preamble - remove the "slice A" / "Slice C" tracking labels from flowspace_adapter.rs and llinterp.rs Comment-only: no code, string literal or test data changed. Assisted-by: Claude * pyre-interpreter: replace pyre-side line-number comment refs with symbol names Rewrite `<file>.rs:NNN` citations in comments under pyre/pyre-interpreter to name the symbol at the cited location instead of a line number, or drop the line number where the symbol was already named. Citations against the pinned rustpython-compiler-core and rustpython-sre_engine snapshots (`oparg.rs`, `bytecode/instruction.rs`, `string.rs`, `engine.rs`) are left as they are. Also replace directional cross-references ("see above", "see below", "the note above", "as noted above") with the fact or the owning symbol, and drop the "B1" tracking prefix from a jit_fnaddr comment. Comment-only; no code, string literal or test data changed. Assisted-by: Claude * Re-anchor pyre-side comment references to symbols Replace `<file>.rs:NNN` line citations in comments across majit-backend-{dynasm,cranelift,wasm}, majit-macros, majit-ir, majit-gc and pyre-object with the file name alone, or with the symbol that the cited line's enclosing item defines where the cited location still matches the comment's claim. Upstream `rpython/`, `pypy/`, `lib-python/` and `lib_pypy/` `.py:NNN` citations are unchanged, as is the `compiler.rs:12884` reference inside the `bridge_cache_addrs` expect string. Symbols named where verified: `bh_call_r` / `bh_call_f` / `bh_call_v` default trait impls, `gc_rewriter`, `do_compile`, `emit_guard_exit`, `cranelift_realloc_frame`, `dynasm_typeid_subclass_range`, `generate_state_fields_jit_state`, `generate_trace_fn`, `handle_new`, `gen_malloc_nursery`, `gen_write_barrier`, `handle_write_barrier_setfield`, `do_collect_nursery`, `rescan_major_nonstack_roots_and_drain`, `register_active_hooks`, `CompiledLoopToken`, `next_op_can_accept_cc`, `AbstractVirtualPtrInfo`. Replace three cross-reference pointers with the fact they pointed at: the `write_float_at_mem` "see read sibling above", the `reg_write_audit` "see the module doc", and the wasm `stamp_and_publish_label_targets` "the comment below". Drop the internal tracking ids `S-11`, `Phase E.3+`, `Slice 80-G.7` and `Pre-A.2.3` from the comments they appeared in. Assisted-by: Claude * fbw: record the measured mechanism behind the loop-bearing blackhole handoff decline The `walk_abort_adopted` deny-list arm for `LoopBearingCalleeInlineUnsupported` carried a comment saying its second blocker was open and that dropping the arm produces wrong code, without naming a cause. Measured it on both failing fixtures and replaced that paragraph with the mechanism. `bhimpl_jit_merge_point` treats a frame that has a `nextblackholeinterp` as the recursive portal level: it takes `bhimpl_recursive_call_*`, parks the result in `tmpreg_*` and raises `LeaveFrame`. A multi-frame image stacks the callee above its caller, and this decline reports that the callee bears a loop, so the callee reaches its own loop-header merge point before any `*_return` and the caller below it receives `tmpreg_*` as the callee's return value. Both fixtures leave at that opcode with `ret_type=Ref`: `inline_subwalk_user_iterator` on `[run@1054, step@260]`, and `list_append_write_barrier_gc` on `[big_live_len_regrow@1191, churn@162]`. Comment only; no behaviour change. Assisted-by: Claude * fbw: name the caller-image refusal, the vref bracket's size, and the two qmut decline causes Three diagnostics, all gated on fbw_debug_abort_enabled, no behaviour change. capture_inline_parent_blackhole answered None from its three liveness-pass early returns without printing anything, so the downstream "parent.blackhole None (capture missing)" could not say which bank, which color, or whether the walk's shadow was merely too short. Added report_caller_image_decline and wired it into the int, ref and float sites. Each site changed from `...get(color).copied()?` to an explicit `let Some(..) = got else`, which separates an out-of-range color from one whose shadow holds a different concrete kind; the `?` conflated them. The vref bracket's two halves iterate virtualref_boxes and nothing reported its length, so its size was only ever restated from which call sites populate it. Print the pair count above vrefs_before_residual_call. The qmut flush leg printed one decline message for both WalkEndResume variants that can be unprovable. Split it: RewindUnproven means no opcode-entry sample was taken, a still-unprovable Rewind means the opcode had already applied an effect. Assisted-by: Claude * fbw: correct two docs that call the vref bracket's loops empty Both said the bracket's loops are empty because no jit.virtual_ref producers exist. A producer does exist and runs: walker_ec_enter takes a vref of every seeded callee frame through TraceCtx::opimpl_virtual_ref, paired with opimpl_virtual_ref_finish when the frame leaves. Measured with the [vref-bracket] report over 431 synth + 93 parity fixtures: 5487 bracket entries, 686 of them (12.5%) with at least one pair, 66 of the 316 emitting fixtures reaching a nonzero count, maximum 7 pairs. Assisted-by: Claude * docs: name the produced-view source PreviewShortState::short_boxes `produced_short_boxes_from_exported_boxes`'s header still cited `ctx.exported_short_boxes`, a field removed when the preview export was collapsed into `PreviewShortState`. Point it at the surviving field. Assisted-by: Claude * descr: state PyCode field purity per field in its spec test `pycode_field_descrs_share_parent_and_preserve_specs` asserted `!descr.is_always_pure()` for every PyCode field. Marking `co_firstlineno` immutable made that field answer true — `is_always_pure()` returns the `immutable` flag — so the test has been failing since that change; its commit verified with `check.py` only, which does not run crate unit tests. Add the expected purity to the per-field tuple and compare it, so each field states its own answer and a move in either direction fails. Also re-point one comment in optimizer.rs at `exported_short_boxes`, the name the local kept after the preview-export collapse. `cargo test --release -p pyre-jit-trace -p majit-metainterp --features dynasm`: rc=0. Assisted-by: Claude * fbw: answer a caller image's unstamped ref color instead of refusing the image `capture_inline_parent_blackhole`'s liveness pass demanded a `ConcreteValue::Ref` for every ref color live at the resume pc and returned `None` for the whole image otherwise. The innermost-frame fill `build_single_frame_miframe` (residual_call.rs) runs the same pass and answers the two ways that demand fails: * a live color whose register holds no box is skipped — a `-live-` set is the union over the paths INTO its coordinate, so a color can be live there and undefined on the path walked, and `_copy_data_from_miframe` (`blackhole.py:1711-1730`) likewise leaves a `None` box unset; * a color whose shadow is `ConcreteValue::Null`, the walker's untracked sentinel that `write_ref_reg` stamps for every recorded-but-unobserved result, is recovered through `TraceCtx::recover_ref_value`. Port both. The image is still refused when neither applies. Measured with a report added at the refusal site, over 431 synth + 93 parity fixtures (dynasm, darwin): 21 refusals across 10 fixtures, all bank `r` with a `Null` shadow, partitioning as 11 no-box and 10 recoverable and 0 neither. After the change the corpus reports none. The report itself stays, extended with the box and its recoverability, and the header's decline count is corrected: it cited the downstream `[s2-build-decline]` symbol, which prints only when a multi-frame build was attempted and so undercounted tenfold. `check.py --no-build`: dynasm 441/441, cranelift 441/441, wasm 434/434, no jitstats delta. `cargo test --release -p pyre-jit-trace -p majit-metainterp --features dynasm`: rc=0. Assisted-by: Claude * fbw: route the hazardous-inline deny through disable_noninlinable_function `fbw_abort_nested_unjournaled_residual` names the callee an abort is attributable to and denies it, but the deny wrote only the walker-local `FBW_HAZARDOUS_INLINE_DENY` thread-local, so the callee's JitCell never carried `JC_DONT_TRACE_HERE` and no warm-state reader saw it. It now also calls `disable_noninlinable_function` on `make_green_key(callee_code, 0)`, the function-entry key `inline_call.rs` already uses for that callee — the same answer `pyjitpl.py:2818-2828` gives for the callee `find_biggest_function` names. The consuming half — `warmstate.py:485-496`, where a `JC_DONT_TRACE_HERE` cell that has never seen a procedure token retraces at once instead of waiting out the counter — is already carried by `WarmEnterState::maybe_compile_decision`. Measured (dynasm): the three fbw witness fixtures now mint the denied callee's cell, cells 3 -> 4 on each. `get_stats` counts the `BaseJitCellState` enum rather than the flag, so its `dont_trace_here` reads 1 only on `wasm_ca_trampoline_decline`; on the two `foriter_exempt_*` fixtures the new cell has already moved on to tracing (tracing 0 -> 1) and the state no longer names the deny its flag still records. `list_append_write_barrier_gc` gains a compiled loop (loops_compiled 12 -> 13), re-recorded on all three backends; the wasm baseline is from a measured wasm run. Assisted-by: Claude * fbw: name which hazard clause denied the nested-residual inline `fbw_inline_callee_hazardous` fires on three clauses and returned only the callee's code key, so the `PYRE_LB_SITE=1` `[lb-arm]` line could say `hazard=true` and nothing more. It now returns the clause name alongside the key and the report prints it: `hazard=repeat`, `hazard=for-iter`, `hazard=self-recursive`, or `hazard=false`. The clauses are not equally tight. `repeat` and `self-recursive` name the frame that is actually recursing; `for-iter` is `code_has_for_iter`, which fires on any code object whose bytecode contains a `FOR_ITER` anywhere, whether or not an iterator is in flight at the decline point. Census over 441 synth + 83 parity fixtures: for-iter 8 fires / 6 fixtures, self-recursive 2 / 2, repeat 2 / 2. Only two of the six `for-iter` fixtures are the witnesses that clause documents. Same denial set as before — the three clauses are checked in the same order and return the same key. check.py --no-build --backend dynasm: 441/441. cargo test --release -p pyre-jit-trace: rc=0. Assisted-by: Claude * warmstate: drop the DontTraceHere state and count the denial off its flag `JC_DONT_TRACE_HERE` had two representations: the flag, and a `BaseJitCellState` variant. Every real decision already read the flag — `can_inline_callable`, `counter_tick_checked`, `should_start_dont_trace_here_trace`, and `should_remove_jitcell` — while `is_compiled` and `is_tracing` read the token and `JC_TRACING`. The state variant reached only `get_stats`, and the two answers disagreed: `disable_noninlinable_function` set the state only when `JC_TRACING` was clear, so a cell denied on its way into a trace carried the flag but never took the state, and the census counted zero denials on every fixture that reaches the fbw hazard arm. The flag is now the only representation. `get_stats` counts it directly and independently of the lifecycle state, which is what makes a denied-then-tracing cell visible; `is_dont_trace_here` reads it; the two `state == DontTraceHere` tests in `counter_would_fire` and `counter_tick` were unreachable behind the flag test on the line above and are gone. warmstate.py has no such state either: `JC_DONT_TRACE_HERE` is orthogonal to the lifecycle — a denied cell still traces, compiles, and is invalidated, and `warmstate.py:485-496` retraces it once its procedure token dies. So the abort paths now leave `BaseJitCellState::NotHot` and set the flag alone, which also collapses `abort_tracing`'s three branches into the single condition `abort_tracing_for_key` already used. cargo test --release -p majit-metainterp --features dynasm: rc=0. Assisted-by: Claude * fbw: record that narrowing the for-iter hazard clause is wrong code `fbw_inline_callee_hazardous`'s `for-iter` clause is deliberately loose — it fires on any callee whose bytecode contains a `FOR_ITER`, in flight or not — and the census this branch added shows it carrying 8 of the 32 declines across 441 synth + 83 parity fixtures for 2 witnesses. Narrowing it to "a consume already ran in this frame" is measurable and wrong. `FBW_FORITER_INFLIGHT` answers that question without the per-frame Python pc `InlineFrame` lacks, since its `Jit` entries carry the `jitcode_index` each consume ran in, and it does cut the clause to 3 fires with both witnesses still declining. But `foriter_exempt_shared_generator` then produces wrong output on all three backends, `inline_subwalk_user_iterator` regresses (loops_aborted 1 -> 5, fbw_rolled_back_with_effects 0 -> 5, loops_compiled 3 -> 2) and `list_append_write_barrier_gc` loses its compiled loop again (13 -> 12). The witness still declined, just at pc 533 instead of 261: inlining the residual is what carries the walk to the consume, so a test conditioned on the consume having happened is always one step late. The clause has to stay forward-looking, and a real narrowing needs FOR_ITER reachability from the frame's current position — which is where the missing per-frame pc actually bites. 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.
attach_tbescaped the frame that re-raisedAn exception escaping a compiled frame that holds a
tryblock it does not match lost the caller's traceback node:attach_tbis a per-frame decision — it suppresses the traceback record of the frame that performed the re-raise, and only that frame.handle_exceptionrestores it right after the frame's record decision so the caller still records its own node. The blackhole's_exit_frame_with_exceptionarm did the opposite: it cleared the flag on the error handed to the interpreter. That frame's own record had already been skipped a few lines above by the samebare_reraisetest, and a compiled frame never runshandle_exceptionitself, so the first frame to read the cleared flag was the interpreter caller, which then dropped its node.A non-matching
exceptfalls through to the except-cleanupRERAISE, so this fired for shapes nobody would call a re-raise — a non-matching clause, a barefinally,forandwhileforms alike. Probe output at the moment of the bug (driveris the compiled frame and never appears):exception_escape_caller_frame_tb_node.pycovers the four affected shapes plus a no-trycontrol that escapes through the guard-exception exit, which never touched the flag.named_reraise_sibling_hotreaches the changed line at all, and it is byte-identical to pypy3 both before and after (its caller sits inside the same blackhole chain, so the flag never escapes to an interpreter frame). A green corpus was not evidence of correctness here — hence the new gate bench.Synthetic bench consolidation
20 benches that differed only in a value, an ordering or a chain length are folded into 6 new files and 6 existing ones, 317 → 303. Each merged file keeps one driver loop per original shape rather than parameterising a single loop, so every call stays the direct call it was and the traced shape is unchanged.
inline_chain_depth_typefliphalvesN: three drivers at the original count made it the slowest synthetic bench by 4x, so it now costs about what the slowest of the three did alone.The
hot/jitstresspairs are deliberately not merged — the jitstress twin lowers the trace thresholds to 1 so it covers the recording path rather than the steady state, which is a different shape despite identical output.Pending repro
_pending/gc_bug_bridge_flavor_traceback_names.pyfiles a pre-existing cranelift GC abort (GC BUG: invalid type_id … site=object_total_size, 8/10–9/10 runs; dynasm andPYRE_JIT=0clean). It is independent of the fix above. The obvious culprit — the unbarrieredf_backrefstore inResidualFrameChainGuard::enter— is refuted: adding the barrier gives 18/20 aborts versus 8/10 without, and the header records that so the reading is not repeated.Verification
Re-run from scratch after a mid-session rebase, with a fresh LLBC extraction and both binaries rebuilt:
pyre/check.py --backend dynasm317/317pyre/check.py --backend cranelift317/317cargo test --workspace --features dynasmgreen (100 test binaries)Two intermediate runs failed on boundary perf benches (
raise_catch1.6x, thennested_loop2.4x) while sibling worktrees drove the box to load 167. Each run failed on a different bench and the third passed 317/317 at load 30; startup-subtracted,raise_catchmeasures 0.83–1.08x against its 1.5x gate.