jit, front: a retrace's closing JUMP keeps its own args; the swap element proves it is an object pointer - #1370
Merged
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (9)
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)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe change separates slice element-type extraction from object-array identity validation. Slice swaps now preserve object-array descriptors only when the element type is positively proven to be ChangesObject-array identity validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized fix is merge-ready after normal checks; no actionable merge-blocking risk remains. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit efef295). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptationsNone. |
youknowone
changed the title
Owner
Author
|
Added a second commit, Short version:
— commented by Claude |
…object array `ffd81546376` gated `PYOBJECT_GCARRAY_TYPE_ID` on the element's `ValueType` coming out `Ref`. `Ref(None)` is also `tyref_to_value_type`'s fallback for every shape it cannot resolve, so on the `slice::swap` arm that condition read "I could not tell" as "this is the object block". Charon runs `monomorphize:false`. A `core::slice::<Impl>::swap` call inside a generic body therefore carries a `TypeVar` in `generics.types[0]`, and `listsort::sort_with<T: Copy, L: SortLt<T>>` reaches `swap` through `reverse_slice` — so the `i64`, `f64` and `usize` sort paths all arrive unresolved. Stamping the object-array identity on them aliases them onto the real object array and gives them a pointer itemsize, which on wasm32 is 4 bytes where those banks stride 8. The swap arm now gates on `output_type_is_objectptr`, a positive proof that the type is a raw pointer onto `PyObject` — the same probe that recognises `w_list_new`'s `PyObjectRef` return. `FixedObjectArray::swap` (`&mut [PyObjectRef]`) resolves and keeps the identity; the `array` module's `Vec<u8>` buffer, the scalar banks and every unresolved element carry `array_type_id: None`, whose `arraydescrof_concrete` arm mints locally without a cache publish and so cannot alias. `slice_swap_elem_value_type` becomes `slice_swap_elem_tyref` so the item kind can keep using the fallback — what the decomposition assumed unconditionally before `ffd81546376` — while the identity requires the proof. The workspace `Index` arm keeps its item-kind condition. Extending the same proof to it costs 9 recorded counters — `pickle_terminal_raise_resume` `guard_failures` 298 -> 338, `list_append_write_barrier_gc` `loops_compiled` 13 -> 12, and `global_quasiimmut_invalidation`, each on all three backends — so some site behind that arm resolves under `tyref_deref_value_type` but not under `output_type_is_objectptr`, and drops an identity it was using. Which site that is has not been identified, so this commit does not change that arm. This also corrects `ffd81546376`'s message: it claimed the sort path monomorphizes over the `int_items` and `float_items` backings. Those monomorphizations exist in Rust, but `monomorphize:false` means the front end never sees them at this call site. Gates: check.py dynasm 442/442, cranelift 442/442, wasm 435/435. Assisted-by: Claude
`normalize_closing_jump_args` replaces a JUMP argument that names no definition in the trace with the trailing LABEL's argument at the same index. That correspondence exists only while the JUMP closes onto that LABEL. `compile_retrace` also reaches the pass with a JUMP retargeted at another TargetToken (`unroll.py:156/171 jump_to_preamble`), whose argument order comes from that token's virtual state, so slot `idx` names a different live value in each list. On `retrace_accumulator_type_flip` with `binary_op_int` and `binary_op_float` suppressed, the trailing LABEL is LoopTargetDescr(2) with state order [0,1,3,5,6,7,8,9,10] and the JUMP targets LoopTargetDescr(1) with order [0,1,5,7,8,...]. Slots 2, 3 and 4 were rewritten from positions 5, 7, 8 to positions 3, 5, 6, and the builtins dict reached a Python local. Compare the two descrs and return the trace unchanged when they differ. `assemble_peeled_trace_with_jump_args` already passes a redirected JUMP through untouched for the same reason. Assisted-by: Claude
![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: 4cc536bcfc
ℹ️ 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
+7561
to
7564
| let elem_array_type_id = elem_tyref | ||
| .as_ref() | ||
| .is_some_and(|ty| output_type_is_objectptr(ty, self.llbc)) | ||
| .then(|| PYOBJECT_GCARRAY_TYPE_ID.to_string()); |
There was a problem hiding this comment.
When listsort::sort_with<T> leaves the element as a TypeVar, this now sets array_type_id to None for all four synthetic operations. arraydescrof_concrete responds to None by minting a fresh descriptor for every operation, while OptHeap::arrayitem_submap keys its caches by descriptor identity; consequently, the two writes do not invalidate values cached by either read when this swap site is retraced or repeated in an optimized loop, so a scalar sort can reuse a pre-swap element. Keep the non-object array separate from PYOBJECT_GCARRAY_TYPE_ID, but give all four operations a shared per-array descriptor identity.
AGENTS.md reference: AGENTS.md:L12-L15
Useful? React with 👍 / 👎.
A census of `PYRE_FBW_SPEC_CENSUS=1` over all 432 `pyre/bench/synth/*.py` fixtures (24192 rows = 432 x 56) records zero fires for `zip_two_tuple_iters`, `builtin_zip`, `compare_op_long_int`, `truediv_op_long`, `binary_op_long_int_pow`, `binary_op_long_int_shift`, `math_frexp` and `math_ldexp`. Suppressing exactly those eight with `PYRE_FBW_NO_SPECIALIZE` leaves `check.py` at 442/442, 442/442, 435/435 with no counter moved. Their call paths are exercised, not dead: `math_frexp` and `math_ldexp` are consulted 2272 times each and `builtin_zip` 17 times. A fixture that calls `zip`, `zip(..., strict=True)`, long/int compare, bigint truediv, `**`, `<<`, `>>`, `math.frexp` and `math.ldexp` in hot loops makes seven of the eight fire; `builtin_zip` still declines on the exact `zip(tuple0, tuple1, strict=True)` shape its comment names. On that fixture the retired build prints the same values as CPython. Removes the eight `spec_gate` sites, the eight `try_walker_specialize_*` bodies, and their `SPEC_FOLD_ROWS` entries (56 -> 48), and rewords the four doc comments that named a removed function. Assisted-by: Claude
The eight folds retired in the previous commit were retirable because no fixture in the corpus ever made them fire; the shapes behind them now reach the generic residual with no gate on them at all. Exercises `zip` over two tuples positionally and with `strict=True`, a long/int comparison, a two-bigint true-divide, `bigint ** int`, `bigint << int` / `bigint >> int`, and `math.frexp` / `math.ldexp`, each in a hot loop. Every result is reduced to an integer checksum, the float legs through `int(round(x * 1e6))`, so a backend's float formatting cannot enter the comparison. Output matches CPython. Assisted-by: Claude
Owner
Author
|
Two more commits,
A
Removes the eight
That covering fixture, added to the corpus. Without it the now-generic paths would carry no gate at all — which is precisely the hole that let these eight sit unexercised in the first place. Float legs are folded to Gates
One thing worth flagging for anyone recording a baseline: Not done, deliberately. Retiring the remaining 48 is not free and I did not attempt it. Measured: suppressing all 56 moves jitstats on 198 benches (~99 fixtures on both native backends), including — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efef29551a
ℹ️ 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".
| // exactly this reason). Slot `idx` then names a different live value in | ||
| // each list, and rewriting it lands the trailing label's value in an | ||
| // unrelated target slot. | ||
| if jump.getdescr().map(|descr| descr.index()) != label_descr_index { |
There was a problem hiding this comment.
When the LABEL and JUMP carry distinct LoopTargetDescr allocations with the same numeric token ID, this gate treats them as the same target and performs the positional rewrite that the new comment says is valid only for this exact LABEL. TargetToken::token_id is explicitly local to a JitCellToken.target_tokens list and debug-only (history.rs), while actual target identity is the descriptor Arc; therefore IDs can collide across loop/replacement boundaries and recreate the Ref-slot shuffle this change is intended to prevent. Compare descr_identity (and make the same-target test share one descriptor) instead of comparing Descr::index().
AGENTS.md reference: AGENTS.md:L184-L185
Useful? React with 👍 / 👎.
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
…a dead reference `FrameBox::new` cited `gate-triage.md` for the measurement that rejected making frame allocation uniform. That file is the env-var table and carries no such entry, and the docstring also disclaimed the ratio it quoted. Re-measured: `PYFRAME_DESCR_GROUP.size_descr.set_non_moving(true)` — which routes `NewWithVtable` through `gen_malloc_fixedsize` to the old generation — takes `bench/fib_recursive.py` from 0.39s to 1.43s user CPU, five samples per arm, arm64 darwin. Record the numbers, and that `GcAllocator::pin` refuses a `PyFrame` outright (`gctypelayout.py:88-92 q_cannot_pin`). This commit also restated `w_pytraceback_new`'s caller obligation. #1370 superseded that by reading every input back out of the bracket (`frame: roots.get(inputs + 2)`), which discharges the obligation inside the function, so that half is dropped. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
…, and settle gc.collect()'s answer (#1385) * gc, object array: report a forwarded header as a moved object and name the holder `validate_type_id` panicked with only the out-of-range type id, which reads the same whether the header is a corpse marker or genuinely corrupt. Move the panic out of line; when the header is `FORWARDED_MARKER`, report the forwarding address, the live copy's type id, the nursery bounds and the offset into them, and the minor/major collection counts. The ordinary arm gains the header word, nursery membership and the same counts. `FixedObjectArray::set_ref` already loads that header for the write-barrier flag test, so it now also tests it for forwarding and aborts there, naming the array and the copy it became. A `pyre-object` hook lets `pyre-interpreter` supply the holder: the scan walks `f_backref` and prints each frame's address, locals array, GC ownership, nursery membership, type id, forwarding state and `TRACK_YOUNG_PTRS`. It stops rather than dereference an `f_backref` read out of a frame the GC does not own. A `majit-gc` test roots a nursery object, collects, and puts the pre-collection address through the barrier. Assisted-by: Claude * majit-translate: seed the collecting census with the real entry points `COLLECTING_SEEDS` named `gc_hook::try_gc_alloc_collecting`, which does not exist in the tree, and `gc_hook::try_gc_alloc_collecting_rooted`. Running the example reported `collecting-alloc seeds: 0 seed function(s)`, so the collecting half of the reachability closure was seeded by nothing. Replace it with the same external symbols `lib.rs` already marks `canmallocgc`. `liveness::scan` counted bodies with an unparsable terminator but not bodies holding a statement that fails to parse or reads as `StmtKind::Unknown`; both shrink the computed live set the same way. Add `unparsed_statement_bodies`. The example discarded the conservative scan's `ScanStats` and printed the resolved scan's figures beside the conservative finding count. Print each scan's own accounting. Assisted-by: Claude * gc: root the descriptor operands get_and_call_function re-reads after __get__ `get_and_call_function`'s slow path calls `get(w_descr, w_obj, w_type)`, which dispatches `__get__`: a `property` getter and a user descriptor are application-level Python. It then passed `w_obj`'s siblings — `args_w`, a slice into the caller's Rust stack that no root reaches — to `call_function_impl_result`. Pin the three descriptor operands and every argument across the lookup and rebuild the argument list from the shadow stack. Both slices are published before the first `normalize_roots`. The argument rebuild uses the same stack-array-or-`Vec` split as the fast path above it. Assisted-by: Claude * jit, object array: re-read the portal frame, and name the caller of a stale store `portal_runner_dispatch` reused its raw `frame` argument after `try_function_entry_jit`, which runs compiled code and returns `None` afterwards while rooting the frame only for its own body. Its sibling `eval_with_jit_inner` re-reads `frame_root.frame()` at the same pair. Take the `FrameRoot` and re-read. Measured: this does NOT fix the `re_jit_call_resume.py` abort. At `PYPY_GC_NURSERY=262144` under load the pre-fix binary aborted 10/10 and the patched one 24/25. `stale_array_abort` now captures a backtrace. Built with `CARGO_PROFILE_RELEASE_DEBUG=line-tables-only` it resolves the inlined frames and names the store: set_locals_w pyframe.rs:1445 <- push pyframe.rs:3010 <- push_value eval.rs:2280 <- opcode_build_list shared_opcode.rs:119 <- execute_opcode_step pyopcode.rs:3584 <- eval_loop_jit eval.rs:8936 `opcode_build_list` allocates in `handler.build_list(&items)` and then writes through the same pre-collection `&mut H`. Without line tables the frames from `execute_opcode_step` down carry no symbols at all. Assisted-by: Claude * jit: re-read the frame in the exit-exception delivery and the bridge walk `deliver_exit_frame_exception` took `next_instr`, wrote the handler pc and handed the frame to `handle_jitexception` through the raw argument it was called with. `handle_exception` in between materialises the exception, records a traceback and can run a trace function, all of which allocate; the frame on this path is the one `emit_new_pyframe_inline_with_params` built in the nursery, so the argument can name the pre-collection copy by then, and `handle_jitexception` roots that address. Take a `FrameRoot` and read the frame back at each use. `trace_and_compile_from_bridge` computed `live_frame_addr` once outside the `jit_merge_point_keyed` closure. The closure re-reads `bridge_frame_root.frame()` for its snapshot but passed the hoisted address to `trace_bytecode`, which publishes it through `sym.set_live_vable_frame_addr` as the virtualizable the walk writes back through. The driver re-enters the closure per merge point and `snapshot_for_tracing` allocates. Compute the address inside the closure. Assisted-by: Claude * interpreter: anchor the frame across the opcode steps that run Python `FrameAnchor` covers the `SharedOpcodeHandler` helpers and `PyFrame::call`. Extend the same treatment to the rest of the dispatch surface a movable frame reaches. `handle_exception_with_context` writes through its `&mut PyFrame` after `err.to_exc_object()`, after `ec.bytecode_trace_after_exception` (which runs application code) and after `record_application_traceback`. Seven re-reads: the `w_f_trace` restore, the frame pointer `w_pytraceback_new` copies into the node it allocates, the `exception_trace` argument, the `pycode` read that feeds the exception-table lookup, and the `push` of the materialised exception — which is the `opcode_build_list` shape. 34 handlers in `impl OpcodeStepExecutor for PyFrame` take an anchor and push through it: the import, format, conversion, containment, truth, match, set, async and attribute opcodes, plus `send_value`, whose `w_yielding_from` store and write barrier also ran through the pre-collection pointer. Selected by whether the intervening step can execute Python-level code or allocate through the collecting allocator. `load_method` anchors only its `getattr_str` arm, leaving the `load_method_fast_path` return allocation-free; `check_exc_match`, `build_slice`, `build_string`, `load_deref`, `load_closure`, `load_fast_and_clear`, `is_op`, `match_mapping` and `match_sequence` keep the plain `push`. This crate is extracted to LLBC, so an anchor is an operation in every trace that records the handler. `re_jit_call_resume.py` at `PYPY_GC_NURSERY=262144`, eight concurrent runs: 0 aborts in 64. The site that abort named is `opcode_build_list`, which `FrameAnchor` already covers, so the run is a regression check rather than attribution for these sites. Assisted-by: Claude * interpreter: record what rejects a non-moving JIT frame, in place of a dead reference `FrameBox::new` cited `gate-triage.md` for the measurement that rejected making frame allocation uniform. That file is the env-var table and carries no such entry, and the docstring also disclaimed the ratio it quoted. Re-measured: `PYFRAME_DESCR_GROUP.size_descr.set_non_moving(true)` — which routes `NewWithVtable` through `gen_malloc_fixedsize` to the old generation — takes `bench/fib_recursive.py` from 0.39s to 1.43s user CPU, five samples per arm, arm64 darwin. Record the numbers, and that `GcAllocator::pin` refuses a `PyFrame` outright (`gctypelayout.py:88-92 q_cannot_pin`). This commit also restated `w_pytraceback_new`'s caller obligation. #1370 superseded that by reading every input back out of the bracket (`frame: roots.get(inputs + 2)`), which discharges the obligation inside the function, so that half is dropped. Assisted-by: Claude * majit-translate: ask the collection census about the running frame `liveness::scan` took the `PyObjectRef` type ids as its only pointer kind, so the second thing a minor collection leaves a body holding a corpse of -- the `&mut PyFrame` the interpreter carries -- was outside the census. `frame_ptr_type_ids` reads the three spellings off `install_current_frame`, `handle_exception`, `force_frame` and `enter_recursive_frame`, and the example runs the same scan a second time with them. No bracket set is passed for that kind: re-reading the frame out of a `FrameAnchor` kills the stale local at the call, so the liveness answer already separates a reloaded frame from a carried one. `GC_FRAME_SCAN` prints the rows, marking which the resolved graph finds and which only the conservative one does. The `movable_use` ranking column becomes a `scan` parameter, `MOVABLE_GC_MARKERS`, because its list/dict callee names answer for a managed reference and for nothing else. Two seeds were not matching anything: `pyframe::execute_frame` and its resumed twin are inherent methods, so charon carries the `impl` block as an opaque path segment and the bare spelling could never match -- `seed_report` had been naming it as unmatched since it was added. `COLLECTING_SEEDS` named only `majit_gc` symbols and `gc_hook::try_gc_alloc_collecting_rooted`. None of them appear in a pyre-interpreter artefact, which reported `0 seed function(s)`. The requested collections do appear and are added; the plain host allocator deliberately is not, and the comment now records why -- `try_gc_alloc` routes to `alloc_nursery_typed`, which falls back to old-gen rather than collect, so seeding it would report every allocating body in the interpreter. `GC_NAME_GREP` and `GC_PATH_FROM` print the name table and a reachability chain, which is what settles whether a seed's absence is a spelling or a fact. Assisted-by: Claude * interpreter: re-read the frame at every site the census reports it carried The frame census (`gc-root-reachability`, frame kind) reports 44 calls that can collect with a `PyFrame` pointer live across them. Five are `eval::walk_pyframe_roots_area`, which is the root walker itself and runs inside the collection it would be reported for; the rest are here. None are in `impl OpcodeStepExecutor for PyFrame` -- that surface was already closed. `FrameAnchor::from_raw` is the constructor these sites need: they hold `*mut PyFrame` read off the `f_backref` chain or a thread's execution context, so minting the `&mut` `new` takes would claim an exclusivity none of them has. `executioncontext::_trace` runs the trace callback, and after it writes through the frame's debug block and calls `locals2fast`; the profile arm below is handed the frame as well. `normalize_exception`, `fast2locals` and `wrap_trace_frame` sit between the earlier reads. Its callers `leave`, `call_trace`, `run_trace_func` and `bytecode_trace_after_exception` each use the frame again on the far side of it. `bytecode_trace` is on the per-tick path, so neither of its anchors is taken unconditionally: the thread-hook one rides `all_thread_hooks_current` and the tracer one rides `w_tracefunc.is_null()`, which are the same tests that decide whether the call below can run Python at all. `action_dispatcher` hands the same frame to each action in turn and a `perform` may deliver a signal, which runs the handler at app level. `builtins::exec_or_eval` consults the caller's frame after `ensure_*_builtins` (a dict subclass's `setdefault` / `__setitem__`), after the function and frame objects are built, and after the builtin module is picked. `call::call_with_kwargs_in_ctx_impl` marshals the arguments between reading the profiled frame and handing it over. `pyframe::capture_coroutine_origin` reads `fget_f_code` and `fget_f_lineno` on either side of the filename object. The `f_lineno` / `f_trace_lines` / `f_trace_opcodes` setters write through the frame after `int_w` / `is_true`, which reach `__index__` and `__bool__`. The `force_frame` cluster -- `gettopframe`, `force_all_frames`, `topframe_for_locals`, the `f_locals` getter, `sys._getframe`, `sys._getframemodulename` and `thread._current_frames` -- reaches the JIT's virtualizable writeback through a backend hook whose callee the census cannot follow. `force_pyframe` reaches no seed by resolved edges, so this is the conservative reading of an undecided call rather than a demonstrated defect; all seven are cold introspection paths. Assisted-by: Claude * interpreter: hand the frame back from bytecode_only_trace, and root the leaving vref `bytecode_only_trace` runs the line/opcode tracer, which is Python, so the frame its two callers use afterwards -- `bytecode_trace`, which passes it to `action_dispatcher`, and `bytecode_trace_after_exception`, which passes it to the pending-action residual -- can name the abandoned copy. It now answers the frame instead, which puts the reload on the arm that can collect: this is the per-opcode path, and an anchor taken in the caller would be a shadow-stack push and pop for every bytecode with no tracer installed. `ExecutionContext::leave` reads `topframeref` into `frame_vref` and forces it after `get_f_back` has forced the caller's; at interp level a chain vref is the frame pointer itself, so that slot goes stale exactly as the frame does. `_trace`'s failed-callback arm calls `settrace` before writing `w_f_trace` through the frame. The frame census over pyre-interpreter now reports one call in one function: `eval::walk_pyframe_roots_area`, which is the root walker and runs inside the collection it would be reported for. Assisted-by: Claude * majit-translate: seed the JIT layer's routes back into interpretation A `pyre-jit` artefact carries only part of `pyre-interpreter`, so 15 of the 20 dispatch seeds do not exist in it and the collect-closure came out at 37 / 7603 -- 0%. A scan over that reports nothing because it can see nothing, which reads exactly like a clean result. The portal and blackhole entries that artefact does carry are named, which takes the closure to 77 and the frame scan from 3 bodies to 11. That is still thin: pyre-jit reaches Python mostly through `global_hook!` function pointers, which stay unresolved, so a null frame result there is weak evidence rather than a clean verdict. Assisted-by: Claude * majit-translate: join artefact call graphs by name, and print every unrooted call `gc-root-reachability` answered per artefact. `pyre-jit.ullbc` carries the portal and the blackhole but only 572 of pyre-interpreter's bodies, so its closure came out at 77 / 7603 and the liveness scan over it reported nothing. `Joined` merges several `CallGraph`s on `item_meta.name_path()` and projects the result back onto one artefact's ids; `GC_JOIN_WITH` names the donors, whose `Llbc` is dropped once the graph is built. With `GC_JOIN_WITH=build/llbc/pyre-interpreter.ullbc` pyre-jit reaches 338 / 7603 and the scan reports 7 PyObjectRef and 3 frame findings. `GC_LIVENESS_SHOW` printed one row per function. It now prints every call with a non-argument live pointer; the second row in `bh_normalize_raise_varargs_with_frame` was hidden behind that. Assisted-by: Claude * jit: re-read the residual operands the joined census reports carried - `bh_load_global_fn`, `bh_load_from_dict_or_globals_fn`: the builtins leg reads the frame and the globals again after `finditem_str`, which reaches `__getitem__` on a non-dict mapping. Anchor the frame; re-derive the globals from it, or from the promoted `w_code`, which does not move. - `bh_call_fn_impl`: the `PYRE_BH_NULL_ARG` diagnostic re-derefs the frame each iteration across `descr_repr`, which fsdecodes the filename and reaches a registered decode error handler. - `bh_normalize_raise_varargs_with_frame`: `exc` is live across both the cause class-call and the exc class-call, and the normalized cause across the second. Bracket each call. - `frontend_global_object`: the globals are read again for `__builtins__`. - `create_self_recursive_callee_frame_impl_1_boxed`: read the logged operand address before `alloc_callee_frame`. `pyre_object_eq_w_trampoline` and `pyre_object_hash_w_trampoline` hand a possibly stale pointer to `signal_eq_error` / `signal_hash_error`, which store it as a presence token `take_*_error` only tests for null. Left unchanged, with the reason recorded at the call. `FrameAnchor::from_raw` documents that a null frame is accepted: the residual helpers take the caller frame as a raw operand the emit site may not have, and the root walker skips a null slot. Assisted-by: Claude * interpreter: rustfmt the frame anchor sites Assisted-by: Claude * majit-translate: do not join two functions that share a spelling `ItemMeta::name_path` renders an inherent `impl` block as the opaque segment `<Impl>`, so `PyFrame::new` and `FrameDebugData::new` are both spelled `pyframe::<Impl>::new`. The previous commit joined on that spelling, merging them into one node and inventing an edge from every caller of one to every callee of the other: `getorcreatedebug`, which only allocates, came out reaching `call_user_function_with_args`, and the interpreter grew 7 frame findings that do not exist. A name one artefact carries more than once is no longer a join key; each occurrence keeps its own node. Of 47839 nodes, 1341 spellings are merged across the two artefacts and 2430 are held apart, reported on the join line. Corrected figures: pyre-jit reaches 240 / 7615, not 338; joining pyre-jit into pyre-interpreter admits 0 more, so that census was never vacuous and its 5 residual frame rows remain the `walk_pyframe_roots_area` opaque ones. The four callees the previous commit's repairs rest on still reach a collection under this join: `pyframe::<Impl>::descr_repr` through `call_registered_decode_error_handler`, `alloc_callee_frame` and `baseobjspace::finditem_str` through `get_and_call_function`, and `normalize_raise_cause` through `normalize_raise_value`. `frontend_global_object` now carries `w_code` across its first lookup; `PyCode` is minted through `malloc_typed_stable` and keeps its address, recorded at the re-read. Assisted-by: Claude * gc: assert in stdlib_gc only what cpython, pypy and pyre answer alike The snippet asserted that `gc.collect` answers an int, that an out-of-range generation raises ValueError, and that `gc.get_objects` takes a generation. pypy 7.3.22 answers None from `gc.collect`, accepts every integer generation and raises NotImplementedError for any non-None `get_objects` generation, which is what `interp_gc.py:7-26` and `referents.py:117-126` specify and what pyre implements; `bench/synth/gc_pypy_frontend.py` pins all three against the pypy oracle. The file carried no `gate=1` marker and has failed at its first line since it landed in #822, so CI never ran it. It now asserts the argument binding, the `__index__` conversion, the TypeError cases and `get_objects()`, which cpython 3.14.6, pypy 7.3.22 and both backends answer alike, and carries `gate=1`. Assisted-by: Claude * replace code eprintln
youknowone
added a commit
that referenced
this pull request
Aug 21, 2026
…ld len(bytes) (#1384) * bench: gate the throughput of eight specialization folds, and drop a refuted wasm claim Eight folds had no fixture that could see them regress. Each one now rides in the existing gated fixture for its own subject, and each of those ceilings is re-derived from the two measured states rather than left at the loose value it carried: fold host fixture folded unfolded ceiling math_isqrt math_sqrt_hot 0.1x 4.2x 11->1 subscr_specialised_pair pure_tupleload 2.6x 198x 87->6 unary_invert_int unary_int_loop_carried 2.5x 15.8x 14->6 builtin_type_getattr class_attrs_methods 3.7x 87x 32->8 builtin_divmod divmod_long_int_pair 1.4x 7.1x 8->4 binary_op_long_int_div divmod_long_int_pair builtin_dict_get dict_update_hot 11.4x 25.3x 15 instance_next foriter_call_body 29x 975x 20->60 The unfolded column is measured with `PYRE_FBW_NO_SPECIALIZE=<label>`. Three of the new ceilings were run against that suppression to confirm they go red: dict_update_hot 25.3x > 15, divmod_long_int_pair 7.1x > 4, math_sqrt_hot 4.2x > 1. `dict_update_hot` has the thinnest margin, ~1.3x either side, because the whole `builtin_dict_get` effect is 1.8x; no loop size widens that. `foriter_call_body` is the one ceiling that rises: pypy folds that iterator into a tight int loop and pyre does not, so 29x is a real gap rather than a fold loss. Four fixtures carried a claim that the wasm backend runs guard-exit re-entry through an uncollected interpreter allocation path, so its wall grows super-linearly in ITERS while native stays linear via bridge chaining, and their N/ITERS were held small on that basis. It does not reproduce. Over a 256x range of work under check.py's own GC pins, wasm's per-iteration cost is flat (205, 189, 192, 183us) and each 4x step costs 3.68x, 4.06x, 3.83x; dynasm is the same shape at a constant 1.4x less. The premise is also wrong: the same script reports loops_compiled=3 bridges_compiled=1 guard_failures=201 on both wasm and dynasm, so wasm chains bridges too. check.py:768 records why -- the guest/native split in interpreter allocation the claim rests on is gone, the gate is default-on everywhere and the allocation model shared. The claim is removed from assert_in_loop, set_update_hot, dict_update_hot and while_is_none; their sizes are left alone. Assisted-by: Claude * jit-trace: restore the eight long/math/zip specializations 1370 retired Reverse-applies 01b740a over jitcode_dispatch/ and trace_opcode.rs, bringing back these spec_gate labels: binary_op_long_int_shift binary_op_long_int_pow truediv_op_long compare_op_long_int math_frexp math_ldexp builtin_zip zip_two_tuple_iters SPEC_FOLD_ROWS goes 48 -> 56. Without them each of these operations leaves a CallMayForceR + GuardNotForced + CallMallocNursery residual in the trace; measured against pypy on dynasm, `x << k` / `x >> k` ran 28.4x, `x ** k` 14.4x, and the long/int comparisons 34x. With the folds restored the same legs are 1.7x, 1.7x and 4.0x with zero forcing residuals. The folds match the vendored pypy sources they specialize: _int_lshift raises on a negative count then wraps W_LongObject(lshift), _int_rshift goes through newlong so the result can demote, descr_pow keeps a W_IntObject exponent unwrapped (exp_bigint = None) and routes a negative exponent to the float path, and _make_descr_binop hands int_w(space) straight to int_func. unspecialized_long_math_zip_paths.py had no max-pypy-ratio header at all, so only its jitstats gated it and a throughput regression here was invisible. It now carries max-pypy-ratio=6 and per-leg sizing (N_CMP 40000000, N_ZIP 50000, N_POW 125000, ...) chosen so each leg drives its own fold and the whole fixture stays inside wasm_ratio_gate's 4x cap (measured 2.55x). Suppressing the eight labels puts the fixture at 12.3x, over the ceiling. Assisted-by: Claude * jit-trace: fold len() on an exact bytes object to its length field try_walker_specialize_builtin_len covered list, str, tuple and range, so `len(b)` fell through to a residual call: 45 CallMayForceR and 16 GuardNotForced in the trace, and 1.44s against pypy's 0.0086s where len(str) over the same loop is 0.014s. Adds a BYTES_TYPE arm that guards w_class against get_instantiate the same way the STR_TYPE arm does, so a bytes subclass with its own __len__ still declines, and a BuiltinLenSource::BytesField that lowers to a getfield on the precomputed count. W_BYTES_DESCR_GROUP describes W_BytesObject's five fields with len marked immutable; the offsets come from offset_of! in bytesobject.rs rather than being written out by hand. bytesobject.py's len answers len(self._value) off the RPython string; pyre keeps that count in a field, so the read has the same shape as the str arm. str_getitem_len_hot.py grows a bytes leg (bn = 12000000 over ascii_b and short_b). Its ceiling stays at 19 — suppressing builtin_len takes the fixture to 29.0x and drops bridges_compiled 1 -> 0. bytearray is deliberately not covered. W_BytearrayObject has no length field, only `data: *mut Vec<u8>`, and Rust does not guarantee Vec's field layout, so reading a length out of it through a descr would add a layout assumption nothing else in the tree makes. len(bytearray) keeps its residual call. 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.
Follows up the P1 Codex raised on #1365 after it merged (
dc8ee02f15a), so the defect is currently inmain.The defect
#1365 gated
PYOBJECT_GCARRAY_TYPE_IDon the element'sValueTypecoming outRef.Ref(None)is alsotyref_to_value_type's tail — the fallback for every shape it cannot resolve — so on theslice::swaparm that condition read "I could not tell" as "this is the object block".Charon runs
monomorphize:false(stated atmir.rs'sslice_index_callnote andfront::slice_index). Acore::slice::<Impl>::swapcall inside a generic body therefore carries aTypeVaringenerics.types[0], andlistsort::sort_with<T: Copy, L: SortLt<T>>reachesswapthroughreverse_slice— so thei64,f64andusizesort paths all arrive unresolved and got the object-array identity. That aliases them onto the real object array and gives them a pointer itemsize: 4 bytes on wasm32, where those banks stride 8.This is strictly worse than the pre-#1365 state, where those sites carried
array_type_id: Noneand so could not alias at all.The fix
The swap arm gates on
output_type_is_objectptr— a positive proof that the type is a raw pointer ontoPyObject, the same probe that recognisesw_list_new'sPyObjectRefreturn.FixedObjectArray::swap(&mut [PyObjectRef]) resolves and keeps the identity; thearraymodule'sVec<u8>buffer, the scalar banks, and every unresolved element carryNone, whosearraydescrof_concretearm mints locally without a cache publish.slice_swap_elem_value_typebecomesslice_swap_elem_tyrefso the item kind can keep using the fallback — what the decomposition assumed unconditionally before #1365 — while the identity requires the proof.What this deliberately does not change, and why
The workspace
Indexarm keeps its item-kind condition. I tried extending the same proof to it and measured the cost:pickle_terminal_raise_resumeguard_failures298 → 338list_append_write_barrier_gcloops_compiled13 → 12global_quasiimmut_invalidationSo some site behind that arm resolves under
tyref_deref_value_typebut not underoutput_type_is_objectptr, and loses an identity it was using.strip_ty_wrapperspeelsRefbut notRawPtr, so a plain&PyObjectRefOutputsatisfies both probes — the divergent site is something else, most likely apyre_-pathedindex/index_mutwhose element does not resolve. I have not identified it, so I am not touching that arm on a guess. Named as follow-up work rather than folded into this fix.Gates
pyre/check.py— dynasm 442/442, cranelift 442/442, wasm 435/435, 3/3 backend runs.Summary by CodeRabbit
Second commit:
jit: leave a closing JUMP alone when it targets another TargetTokenUnrelated to the swap fix above; it lands here because it was found while
suppressing the hand-written trace-time specialization folds and it is the
last thing standing between that corpus and a clean all-folds-off run.
The defect
normalize_closing_jump_args(majit-metainterp/src/compile.rs) repairs aJUMP argument that names no definition in the trace by taking the trailing
LABEL's argument at the same index. That correspondence exists only while
the JUMP closes onto that LABEL.
compile_retracealso reaches the pass with a JUMP retargeted at anotherTargetToken (
unroll.py:156/171 jump_to_preamble). Its argument order comesfrom that token's virtual state, so slot
idxnames a different livevalue in each list.
MAJIT_LOG=1prints the same trace on both sides of thepass:
LABEL descr 2 carries state order
[0,1,3,5,6,7,8,9,10]; the target, descr 1,carries
[0,1,5,7,8,...]. Slots 2, 3, 4 were rewritten from positions 5, 7, 8to positions 3, 5, 6 — and positions 3 and 6 are the frame's globals and
builtins dicts, so a Python local came back holding a
dict:The pass cites
optimizer.py:651-652for this, but that upstream loop isop.setarg(i, self.force_box(op.getarg(i)))— it resolves an argument throughits forwarding chain and never substitutes a LABEL argument. The producing
site,
assemble_peeled_trace_with_jump_args, already documents the rule thispass was breaking: "A JUMP redirected to another token already carries the
arity produced by that token's virtual state and must pass through
unchanged."
The fix
Compare
jump.getdescr().index()with the trailing LABEL's and return thetrace unchanged when they differ. Same-token repair is untouched — the added
test pins both halves.
Why it only shows with two folds off
ddmin over the 56-label suppression mask gives a minimal set of exactly
{binary_op_int, binary_op_float}; neither alone reproduces. With the foldson, the accumulator and induction variable are unboxed
IntLABEL arguments,where a mismap is caught by the type check. With both off they are boxed
Refs, and aRef↔Refshuffle is invisible to every type check in thepipeline.
mc_diagis byte-identical between the failing and passing runs(
retrace_entered=1,close_hdr_fallback=1,all_descrs=9668), so the JIT'sdecisions matched and only the trace content differed.
The bug is not fold-specific — any retrace whose closing JUMP is retargeted
while its live values are Refs can hit it.
Localisation, before any rebuild
PYRE_JIT=0PYRE_NO_UNROLL=1retrace_limit=0/=1Gates
pyre/check.py:dynasm 442/442,cranelift 442/442,wasm 435/435— nocounter moved, so no baseline re-record.
cargo test -p majit-metainterp --lib --features dynasm: new test passes.cargo test --all --features dynasm: one failure,gc_stress::class_body_and_exec_loops_keep_the_vable_shadow_synchronized,panicking at
compiler.rs:1701"no JITFRAME type id published". Not fromthis change — 3/3 in isolation and 2/2 (34/34) as its own suite; it is the
cross-suite race under load already seen on this branch.
pyre/bench/synth/*.py:TOTAL=432 CONTROL_NONZERO=0 ALLFOLDSOFF_NONZERO=0. Before this commit thesame sweep read
431/432withretrace_accumulator_type_flipas the singleholdout.