mapdict off-GC side-table values, per-guard aarch64 reach check, paused-call floor on the third recipe path, and a per-return getenv by youknowone · Pull Request #907 · youknowone/pyre

youknowone · 2026-07-30T13:20:44Z

Follow-ups to the review comments on #885, plus one profiling find.

1. mapdict: a side-table value the GC does not own (Codex P1 on #885)

alloc_dict_object falls back to malloc_typed when no allocation hook is
installed. Such a dict carries no header, so do_write_barrier drops it (it
admits only a nursery or old-generation address) and no custom trace reaches
its entries — walk_mapdict_roots_area is the only thing that traces them.
Since #885 a minor walk visits an INSTANCE_DICT entry only while its key is
pending, so a store into an off-GC dict after that first visit went unseen.

The walk now puts the key back when the value is not GC-owned. The weakref
walk stops at the lifeline pointer and is left alone.

Adds unit tests for snapshot_root_entries and apply_root_rekeys over local
tables (the process-global tables are shared with concurrently running tests).

2. dynasm/aarch64: measure each guard branch against its own stub (CodeRabbit)

check_guard_reach compared the whole trace span — body plus every recovery
stub — against BCOND_FORWARD_RANGE, which is no particular branch's
displacement, and reported pending_guard_tokens.len(), drained to zero by
write_pending_failure_recoveries before the check ran.

It now records the offset of the first single-instruction b.cond emitted to
each label and compares it to the stub that label is bound to, so the
diagnostic names the branches that actually overflow. A trace that took the
two-instruction form everywhere records nothing and passes.
trace_start_offset had no remaining reader and is removed.

3. jit(fbw): the paused-call operand floor on the third recipe path (CodeRabbit)

reconstruct_inline_recipe has three exits. #885 exempted the operand region a
paused call consumes on the virtual-array path; the color-inversion fallthrough
rebuilds from the same resumedata colors but demanded the whole region, so an
in_a_call frame reaching it declined once the call took an argument.

The materialized-frame exit is deliberately unchanged: it reads those operands
out of the frame's own locals_cells_stack_w, where they are present.

4. interp: `PYRE_INTERP_RETURN_LOG` read once instead of per return

Found while profiling synth/arith_int_bool. finish_value runs on every
interpreted RETURN_VALUE and probed the variable with std::env::var_os, which
takes a lock and scans the environment array.

Interleaved median-of-5, dynasm, arm64:

fixture	before	after
`calls_closures`	0.470s	0.400s	+14.9%
`closure_per_call`	0.540s	0.470s	+13.0%
`class_attrs_methods`	0.820s	0.730s	+11.0%
`exception_reraise_tb_depth_jitstress`	0.690s	0.630s	+8.7%

Verification

pyre/check.py --backend dynasm,cranelift: 342 passed on both backends. The
one failure is synth/ast_compile_roundtrip, a local cpython/pypy output
mismatch (BASEFAIL) unrelated to these changes; it passes in CI.
cargo test over majit-gc, majit-metainterp, majit-backend-dynasm and
pyre-interpreter: 2218 passed, 0 failed.
GC discriminators — a recycled address must not inherit a dead owner's
__dict__, an indirectly reached owner must not lose it, weakref lifelines
survive with their owner and clear when it dies — all match pypy3 exactly.
mapdict scaling stays linear (N x4 -> x2.9).

The local gate ran before the branch was rebased onto 0135f56 (#901); CI on
this PR is the check against the current base.

Not addressed

The Codex parity note that a value whose owner dies survives one extra major
collection is accurate and left as is: the walk marks table values as roots
before finish_incremental_marking prunes dead owners, so pruning is one cycle
behind. A true ephemeron relationship would need a marking fixpoint.

— commented by Claude

`alloc_dict_object` falls back to `malloc_typed` when no allocation hook is installed. `do_write_barrier` admits only a nursery or old-generation address, so such a dict never enters the remembered set, and no custom trace reaches its entries; `walk_mapdict_roots_area` is the only thing that traces them. Since the walk visits an INSTANCE_DICT entry on a minor only while its key is pending, put the key back when the value is not GC-owned. The weakref walk stops at the lifeline pointer, so it is left alone. Also add unit tests for `snapshot_root_entries` and `apply_root_rekeys` over local tables. Assisted-by: Claude

`check_guard_reach` compared the whole trace span — body plus every recovery stub — against `BCOND_FORWARD_RANGE`, which is neither branch's displacement, and reported `pending_guard_tokens.len()`, drained to zero by `write_pending_failure_recoveries` before the check ran. Record the offset of the first single-instruction `b.cond` emitted to each label and, when `write_pending_failure_recoveries` binds that label to a stub, compare the two. `check_guard_reach` now reports the branches that actually overflow. A trace with no short guard branch records nothing and passes. `trace_start_offset` had no remaining reader and is removed. Assisted-by: Claude

…path `reconstruct_inline_recipe` has three exits. The virtual-array one exempts the operand region a paused call consumes from its mandatory-operand demand; the color-inversion fallthrough rebuilds from the same resumedata colors but demanded the whole region, so an in_a_call frame reaching it declined once the call took an argument. The materialized-frame exit is left as is: it reads the operands out of the frame's own locals_cells_stack_w, where they are present. Assisted-by: Claude

`finish_value` runs on every interpreted RETURN_VALUE and probed the variable with `std::env::var_os`, which takes a lock and scans the environment array. Cache it in a OnceLock like `gc_prebuilt_remember_enabled`. Interleaved median-of-5, dynasm, arm64: class_attrs_methods 0.820 -> 0.730s, closure_per_call 0.540 -> 0.470s, calls_closures 0.470 -> 0.400s, exception_reraise_tb_depth_jitstress 0.690 -> 0.630s. Assisted-by: Claude

coderabbitai · 2026-07-30T13:21:24Z

Warning

Review limit reached

@youknowone, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 46 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6403a354-db35-40df-98be-89759899b546

📥 Commits

Reviewing files that changed from the base of the PR and between 0135f56 and 674a608.

📒 Files selected for processing (4)

majit/majit-backend-dynasm/src/aarch64/assembler.rs
pyre/pyre-interpreter/src/eval.rs
pyre/pyre-interpreter/src/objspace/std/mapdict.rs
pyre/pyre-jit-trace/src/state.rs

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-07-30T14:30:42Z

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 674a608).
Updated: 2026-07-30T14:30:42.160Z

Files in the reviewed diff

majit/majit-backend-dynasm/src/aarch64/assembler.rs
pyre/pyre-interpreter/src/eval.rs
pyre/pyre-interpreter/src/objspace/std/mapdict.rs
pyre/pyre-jit-trace/src/state.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

pyre/pyre-interpreter/src/eval.rs:589-591 ↔ pypy/interpreter/pyopcode.py:239-241 — PYRE_INTERP_RETURN_LOG is now latched on its first RETURN_VALUE; changing the environment during process execution no longer enables/disables return logging. PyPy’s return handler has no equivalent mutable debug switch.

3. Pre-existing mismatches (already present before this patch)

None.

4. Structural adaptations

majit/majit-backend-dynasm/src/aarch64/assembler.rs:4723-4730 ↔ rpython/jit/backend/aarch64/opassembler.py:869-870 — Rust/Dynasm must choose either a direct b.cond or a two-instruction long branch before finalization; PyPy reserves a BRK placeholder and patches it in its mutable machine-code builder. This is a backend/code-buffer adaptation, not a semantic divergence.
pyre/pyre-interpreter/src/objspace/std/mapdict.rs:3761-3821 ↔ rpython/memory/gc/incminimark.py:337-355 — the Rust implementation explicitly snapshots/rekeys process-global compatibility side tables and re-arms off-GC dictionaries; MiniMark instead tracks GC objects/prebuilt roots through collector-owned structures and write barriers.
pyre/pyre-interpreter/src/objspace/std/mapdict.rs:3890-3894 ↔ pypy/objspace/std/mapdict.py:894-902 — non-instance has-dict objects use an address-keyed Rust side table rather than PyPy’s mapdict ("dict", SPECIAL) field. This existing representation requires the added explicit GC-root bookkeeping.

#874 and #907 each added `fn interp_return_log_enabled` to `pyre-interpreter/src/eval.rs`. Each PR built against its own merge base, so neither run saw the other's copy; landing both left the name defined twice and `origin/main` fails to compile with E0428. The two bodies are identical (`OnceLock` over `PYRE_INTERP_RETURN_LOG`, both `#[cfg(not(feature = "sandbox"))]`), so this removes the second and keeps the first, whose doc names the caller on the return path. Assisted-by: Claude

#907 added a second `#[cfg(not(feature = "sandbox"))] fn interp_return_log_enabled` with the same body next to the existing one, so `pyre-interpreter` fails to compile with E0428 on main. Keep the first definition and its doc comment; delete the duplicate. Assisted-by: Claude

#874 and #907 each added the function, in different parts of the file, so neither merge saw a textual conflict and the two definitions landed together. The bodies are identical; this keeps one. Assisted-by: Claude

… resolution `interp_return_log_enabled` was defined twice in `pyre-interpreter/src/eval.rs` — #874 and #907 each added a copy under the same `cfg`, and neither PR's CI saw the other's, so `main` does not compile (E0428). Delete the later duplicate. `register` discarded the file `get_fileno_and_file` resolved while the installed handler kept only its descriptor, so a `register(sig, f)` whose `f` was later collected left the handler writing through a closed or reused fd. `handler.py:125` keeps `user_w_files[signum] = w_file` and `:132` pops it on `unregister`; hold the same per-signal owners in a process-global table walked by the faulthandler root walker, which now covers both it and the fatal-error file. A `fileno()` returning a negative value reached `FAULTHANDLER_FD` and the handler registration unchecked. Measured: PyPy accepts it (`enable()` succeeds), 3.14 raises `RuntimeError: file.fileno() is not a valid file descriptor` — distinct from the direct-int arm's `ValueError: file is not a valid file descriptor`. Follow 3.14. Resolving the file argument runs a user `fileno()` and `flush()`; `enable` and `register` ran it before the support gate, so a build that can only answer `NotImplementedError` executed those side effects first. Move both inside the gate — which also puts `register`'s integer coercions ahead of the file resolution, as its argument parsing does. Not changed, after measuring: a `SystemExit` raised out of `flush` does NOT abort `enable`. `handler.py:44-48` re-raises asynchronous errors (`error.py:62-65`) and PyPy does propagate it, but 3.14 clears the flush error unconditionally and swallows it; 3.14 is the behaviour target. check.py: dynasm 345/345, cranelift 345/345, wasm 341/341 — all three green; `synth/mutate_then_raise_caught`, which panicked in `collector.rs` (`minor_custom_trace_target`) 3/3 before, now passes 5/5. cargo test pyre-interpreter 438. faulthandler probes (enable/register file retention across GC, unregister, negative fileno, missing fileno, flush errors) match python3.14 line for line. Assisted-by: Claude

7a3d16d (#874) and c9b98f6 (#907) each added a `fn interp_return_log_enabled` to `eval.rs` at module scope under the same `#[cfg(not(feature = "sandbox"))]`, with byte-identical bodies and different doc comments. Together they are E0428 and `pyre-interpreter` does not build. Keep #874's, which is the earlier definition and the one the single call site at the RETURN_VALUE path already resolved to before #907 landed. Assisted-by: Claude

`eval.rs` carries two definitions of `interp_return_log_enabled`, at :589 and :617. Both are `#[cfg(not(feature = "sandbox"))]` with the same body — a `OnceLock<bool>` over `PYRE_INTERP_RETURN_LOG` — and only their doc comments differ, so `pyre-interpreter` fails to compile with E0428 and the Charon/LLBC extraction step exits before any other CI job runs. The pair is inherited, not produced by the rebase: `origin/main` `25b2442c4e` holds both. #874 added the first; #907 added the second and merged on top without seeing it, since each PR's CI builds only its own merge commit. Keeps the :589 copy and its comment; the sole call site at :2729 is unchanged. Assisted-by: Claude

* review: act on the Codex and CodeRabbit findings on #887 Drop the `CriticalCodeGuard` from the `bound_reached` compile path (`eval.rs`). It spanned the whole trace-compile-and-dispatch region, so `report_error = 0` covered the walk's residual calls into user code — `stack.c:111` returns that flag in place of the overflow verdict, turning a RecursionError into an unreported native overflow. Upstream marks only regions that run no user code (`compile.py:976` `cpu.force` + `handle_async_forcing`, `pyjitpl.py:3305` `rebuild_state_after_failure`, `resume.py:1318`). Measured on the fixture it was added for, `synth/wasm_ca_trampoline_decline`: with the guard suppressed the overflow no longer occurs at all, so the region is inert. Record why `fbw_inline_recursion_count` matching `w_code` alone is the element-wise greenkey compare of `pyjitpl.py:1396-1401`: `MIFrame.setup` (`pyjitpl.py:74-80`) assigns `greenkey` once, so a frame's greenkey stays its entry greens, and every `InlineFrame` is pushed at a callee entry with the `(w_code, 0)` identity `reconstruct_inline_recipe` also stamps. Write the tuple header before publishing its root (`tupleobject.rs`). `try_gc_alloc_stable_raw` returns uninitialized payload and `tuple_object_custom_trace` reads `ob_header.w_class` and `wrappeditems`, so a collection between `pin_root` and the items-block allocation read garbage. The header now goes in first with a null items block — the state the trace hook returns early on — and the block is stored after. Restore `FAULTHANDLER_FD` when `enable_fatal_handlers` fails; a failed re-enable was redirecting the handlers already installed. Give `OwnerRootGuard` a `PhantomData<*mut ()>`: its `index` names a slot in the acquiring thread's `OWNER_ROOTS`, and a bare `usize` left the guard auto-`Send`. Delete the `type_id == 9` backtrace capture from `remember_young_pointer`. check.py: dynasm 345/345, wasm 341/341. cranelift 344/345 — `synth/mutate_then_raise_caught` panics in `collector.rs` (`minor_custom_trace_target`, invalid type_id) 3/3 both with and without this commit, and passes on the Linux CI runner. Assisted-by: Claude * review: act on the remaining #887 findings `W_TextIOWrapper` published its reference fields with one write barrier at the end of `attach_buffer`, but every `w_str_new` in between allocates and `set_newline` / `getattr_str` / `set_encoder_decoder` run Python, so a collection could land after a store and before the barrier that would have told it to trace the wrapper. Barrier after each reference store instead, through a `publish_refs` helper, in `attach_buffer` and in `set_encoder_decoder`. `attach_stdio_codec` dropped both the codec lookup and the incremental encoder/decoder construction errors, leaving a stream that reports itself unreadable however readable its buffer is. Propagate them: `init_stream_codecs` and `init_importlib_bootstrap` now return the failure, with a bootstrap failure still winning as the more fundamental one. `allocate_stdio` still cannot propagate — it runs inside `sys` module creation, before there is an interpreter to raise into — but it no longer discards the result blind: with a null codec the only fallible step `attach_buffer` reaches is `buffer.seekable()`, so finish the two fields that early return skipped rather than leaving them at `Default` and forcing `STATE_OK` over a half-applied init. `faulthandler.enable` resolved `file=None` to a hard-coded fd 2 and dropped the file object. `get_fileno_and_file` (`handler.py:35-49`) resolves the CURRENT `sys.stderr`, asks it for `fileno()`, and flushes it ignoring an ordinary flush error; `enable` then parks it as `fatal_error_w_file` (`handler.py:145`) and `disable` clears it (`:150`), because the descriptor the installed handler writes to belongs to that object. pyre has no Handler instance, so the owner is a process-global slot walked as a GC root alongside the other interpreter globals — not a module attribute, which would show up in `dir(faulthandler)` where upstream has none. `acquire_owner_root` scanned `OWNER_ROOTS` from 0 for a free slot, so rooting the frames of a recursion `depth` deep cost O(depth²). Released interior slots now go on a free stack; a released tail still shrinks the walked range, and the shrink drops the queued indices it took out of range. check.py: dynasm 345/345, wasm 341/341. cranelift 344/345 — `synth/mutate_then_raise_caught` panics in `collector.rs` (`minor_custom_trace_target`, invalid type_id) with and without this commit, and passes on the Linux CI runner. cargo test: pyre-interpreter 432, majit-gc 215. Assisted-by: Claude * faulthandler: per-signal file owners, invalid-fileno rejection, gated resolution `interp_return_log_enabled` was defined twice in `pyre-interpreter/src/eval.rs` — #874 and #907 each added a copy under the same `cfg`, and neither PR's CI saw the other's, so `main` does not compile (E0428). Delete the later duplicate. `register` discarded the file `get_fileno_and_file` resolved while the installed handler kept only its descriptor, so a `register(sig, f)` whose `f` was later collected left the handler writing through a closed or reused fd. `handler.py:125` keeps `user_w_files[signum] = w_file` and `:132` pops it on `unregister`; hold the same per-signal owners in a process-global table walked by the faulthandler root walker, which now covers both it and the fatal-error file. A `fileno()` returning a negative value reached `FAULTHANDLER_FD` and the handler registration unchecked. Measured: PyPy accepts it (`enable()` succeeds), 3.14 raises `RuntimeError: file.fileno() is not a valid file descriptor` — distinct from the direct-int arm's `ValueError: file is not a valid file descriptor`. Follow 3.14. Resolving the file argument runs a user `fileno()` and `flush()`; `enable` and `register` ran it before the support gate, so a build that can only answer `NotImplementedError` executed those side effects first. Move both inside the gate — which also puts `register`'s integer coercions ahead of the file resolution, as its argument parsing does. Not changed, after measuring: a `SystemExit` raised out of `flush` does NOT abort `enable`. `handler.py:44-48` re-raises asynchronous errors (`error.py:62-65`) and PyPy does propagate it, but 3.14 clears the flush error unconditionally and swallows it; 3.14 is the behaviour target. check.py: dynasm 345/345, cranelift 345/345, wasm 341/341 — all three green; `synth/mutate_then_raise_caught`, which panicked in `collector.rs` (`minor_custom_trace_target`) 3/3 before, now passes 5/5. cargo test pyre-interpreter 438. faulthandler probes (enable/register file retention across GC, unregister, negative fileno, missing fileno, flush errors) match python3.14 line for line. Assisted-by: Claude

* jit(fbw): latch a multi-frame blackhole image on ABORT_TOO_LONG `latch_trace_too_long_blackhole` previously returned false for an inlined sub-walk, so an ABORT_TOO_LONG inside an inlined callee fell back to entry replay. It now builds the multi-frame framestack and latches it behind `multi_frame_blackhole_preflight`, a read-only re-check of every adopter gate that can later reject the image -- the abort runs after the opcode's effects, so publishing is only safe when the handoff cannot decline afterwards. `LatchedMultiFrameBlackhole` gains `publish_root_stack`. ABORT_TOO_LONG stops at an arbitrary post-step coordinate, so frame 0's active operand stack has to cross from the detached tracing snapshot to the live red frame before the blackhole runs; the vable-force path keeps its existing handoff and passes false. `try_adopt_multi_frame_blackhole` roots every MIFrame ref bank and the pending exception across root-locals boxing and copies the forwarding updates back before the banks are read, and it pins the canonical `PyFrame` virtualizable info for the whole drive instead of whichever translator-state vinfo happens to remain installed on `TraceCtx`. Standard-vable writes inside an inlined MIFrame are mirrored onto that frame's own concrete red `PyFrame` (`current_inline_concrete_frame`, `store_live_frame_static_int`), so an inner level no longer depends on a per-level slot side-table to be resumable. Adds `synth/trace_too_long_inline_multiframe`, which drives ABORT_TOO_LONG inside an inlined callee that mutates three containers and, in a second part, unwinds instead of returning. Assisted-by: Claude * interp: drop the duplicate interp_return_log_enabled definition #874 and #907 each added `fn interp_return_log_enabled` to `pyre-interpreter/src/eval.rs`. Each PR built against its own merge base, so neither run saw the other's copy; landing both left the name defined twice and `origin/main` fails to compile with E0428. The two bodies are identical (`OnceLock` over `PYRE_INTERP_RETURN_LOG`, both `#[cfg(not(feature = "sandbox"))]`), so this removes the second and keeps the first, whose doc names the caller on the return path. Assisted-by: Claude

* perf(jit): cache exact jitcode assembly declines * jit: expose manual builtin gateways to translation * jit: preserve runtime hints and float bitcasts * jit: inline stored bound methods through dirty bodies * jit: preserve Python frames through builtin gateways * jit: lower rebuilt Result shells as allocations * jit: preserve nested loop and field identity * jit: restrict the builtin-wrapper fold to positional calls `try_walker_inline_builtin_call` admitted both `PyreHelperKind::CallFn` and `CallKw`, then built the generated wrapper's flat positional argument array from `r_args[2..]`. A `call_kw` residual carries its kwnames tuple at arg index 2 — `Instruction::CallKw` emits `callable, null_or_self, kwnames, arg0..argN-1` and `bh_call_kw_<n>` consumes that same order — so the tuple became the wrapper's first positional value and the array length exceeded the real argument count by one. Keywords reach a generated wrapper as the trailing `__pyre_kw__` marker dict that `split_builtin_kwargs` strips, and this fold builds no such dict. Decline `CallKw` so those calls keep the ordinary residual, as `CallFunctionEx` already does. Assisted-by: Claude * jit: require a positional-only callee scope for the walker inline lever `try_walker_inline_resolved_user_call` admitted a callee on `callee_args.len() == nparams` alone, where `nparams` is `co_argcount` as returned by `resolve_inlinable_callee`. `co_argcount` counts neither `*args` nor `**kwargs` nor keyword-only parameters, while the inline frame seeding stores only `param_boxes[0..nparams]` into a `NewArrayClear` array, so a `*args` local read PY_NULL where `pack_varargs` binds `()`. The positional-defaults fill widened the set of calls that reach the arity test. Consult `fbw_callee_scope_is_positional_only` beside the arity gate in the shared resolved half, which covers every admission path, and drop the now redundant check in the CALL_FUNCTION_EX branch. Assisted-by: Claude * jit: decline bool operands in the math.isqrt specialization `pyre_object::is_int` accepts a bool, but the emitted specialization unboxes through `INT_TYPE` and guards the canonical `int` `w_class`, neither of which holds for a bool singleton. Assisted-by: Claude * jit: re-check the dict keys_version under the dict lock The `dict.get` specialization guarded `W_DictObject.keys_version` with an unlocked field read and then called `jit_dict_nth_value`, which took the dict lock only for the indexed read. A key-set mutation between the two compacts the `IndexMap`, so the promoted index named a different key. Replace the helper with `jit_dict_nth_value_versioned`, which holds one reentrant `w_dict_lock` across the version re-check and the read and returns PY_NULL on a mismatch, and emit a `GuardNonnull` on its result. Assisted-by: Claude * jit: return the accepted binop class from one decode `residual_call_is_specialized_plain_numeric_binop` and `residual_call_is_specialized_plain_int_binop` each decoded the body `BINARY_OP` tag out of the constants window and each carried the `And`/`Or`/`Xor` (+ in-place) operator set, which had to stay in lockstep. Return `Option<SpecializedBinop>` from the first and delete the second. Assisted-by: Claude * jit(descr): rank ExecutionContext field descrs by byte offset `EC_DESCR_GROUP` built both fields through one closure that hardcoded `index_in_parent: 0`. `make_simple_descr_group` copies that value verbatim and binds a parent SizeDescr, and `OptHeap::field_slot_index` prefers `index_in_parent` over `descr.index()` whenever a parent is bound, so `sys_exc_value` and `topframeref` resolved to one `PtrInfo._fields` slot. Sort the specs by offset and stamp `index_in_parent` from that position, and resolve both accessors by offset rather than by declaration order. Assisted-by: Claude * majit: order the builtin-wrapper alias pick totally and memoise the family `builtin_wrapper_indirect_graphs` bucketed aliases by iterating the `function_fnaddrs` HashMap and picked with `sort_by_key(Reverse(segment count))`, which is stable, so two aliases of one address with equal segment counts resolved on iteration order. Compare the segment sequences and demote the `crate` placeholder so the order is total. The family was also rebuilt per IndirectCall op and per drained graph. Memoise it in a `OnceCell`; `function_fnaddrs` and `function_graphs` are written only in the setup phase that precedes every reader. `lib.rs` indexes `jitcodes()` instead of `filter_map`, since `grab_initial_jitcodes` has already inserted every path in the family. Assisted-by: Claude * jit: consult the cached frame-shape classification on the portal entry paths `try_function_entry_jit` and `maybe_compile_and_run` stopped consulting `unsupported_jit_shape` on the premise that `eval_with_jit_inner` classifies every frame first. `portal_runner_dispatch` reaches both without that: `compile_tmp_callback` bakes `portal_runner_adr` as the whole callee body and the `!is_resolved` CALL_ASSEMBLER force leg calls the same shim, so the counter tick could start a trace for an excluded shape. Consult `cached_unsupported_jit_shape`, a pointer-keyed lookup into `CallControl.graph_jit_shapes`, rather than the whole-frame scan that classification cache replaced. Assisted-by: Claude * interp: route the variable-arity argument-count error through a gateway helper The generated wrapper's "expected at least N arguments" branch built its message with `format!` inline in the traced body, while the fixed-arity and no-arg branches call `#[dont_look_inside]` `method_arity_failure` / `method_noarg_failure`. Add `method_min_arity_failure` carrying the same attribute, register its fnaddr aliases, and call it from the macro. Assisted-by: Claude * jit: preserve struct identity across field owner spellings * jit(descr): compare the reconciled field description on a get_field_descr cache hit `GcCache::get_field_descr` mints a descr with reconciled metadata, but its cache-hit `debug_assert!` compared the caller's raw arguments against it. `derive_index_in_parent` re-derives the stored `index_in_parent` from the parent that will actually be indexed, so a caller's own numbering never reaches the cached descr. Pyre reaches one struct through several `all_fielddescrs` walks that number their lists independently: the runtime group over the declared payload numbers `W_IntObject.intval` 0, and the walk that models the inherited `PyObject` header numbers it 2. `heaptracker.py:62-64` / `:102-103` skip `typeptr` in both the list and the index, so the header-free numbering is the upstream one. The assert reported every such split as a disagreement; it now derives the caller's index the same way before comparing. `front/mir.rs` leaves `SemanticProgram::immutable_fields` empty for the whole LLBC pipeline — Charon serializes doc comments but not the `#[jit_immutable_fields]` hint — so a spec built from that side reports `(is_immutable, is_quasi_immutable) == (false, false)` for every field. The hit path already resolves that by keeping the cached descr's flags; the assert now compares those. A caller claiming purity a cached descr denies still trips. Both fired only in debug builds, where they cost 28 `cargo test` failures: one panic plus 27 tests failing on the descr mutex the panic poisoned. Downgrading the assert to a print reports 16 distinct pairs over `W_IntObject` `W_FloatObject` `W_LongObject` `Method` `W_Range` `W_IntRangeIterator` `PyFrame` — 9 index-only, 7 immutability-only — and the suite passes with it downgraded, so the cached descr already won. The cache-hit message also names both descrs' owners, which is what identified the two producers. Assisted-by: Claude * jit(bh): register new/d>r in the production blackhole builder Lowering a rebuilt `Result` shell as an allocation made `OpKind::New` reachable from the codewriter, so `build_emitted_insns()` now records `new/d>r` while `build_inline_call_only_bh_builder`'s curated `setup_insns` map did not carry the byte. `handler_new` was already wired, so this was a registration gap, not an implementation gap: `wire_handler` no-ops without the map entry and the byte stays unwired until a forward resume lands on it and `dispatch_step` panics. The operand shape matches the wired decoder. `assembler.rs OpKind::New` emits a 2-byte little-endian descr index then the 1-byte ref register holding the result; `handler_new` reads exactly that through `read_descr` + `code[pos]`. That is `new_with_vtable/d>r`'s shape, already registered beside it, and both read `bh.cpu`, which this builder sets. `production_bh_builder_covers_every_build_emitted_opname` and `production_bh_builder_overlay_only_gap_snapshot` failed on this opname; the snapshot drops it and records why it left, as it does for `vtable_method_ptr/rd>i`. cargo test --all --no-default-features --features dynasm: 0 failed. Assisted-by: Claude * test(jit): cover nested virtual append payloads * test(jit): select the keyword wrapper's argument slice by descr, not by position `keyword_builtin_wrapper_finds_colored_argument_slice_item_descr` picked the wrapper's argument slice as "the first `arraylen_gc`" and pinned the entry call's result colour to `inline_call_r_r/dR>r`. Both name a shape rather than the property, and both stopped naming it once `split_builtin_kwargs` inlined further: the entry call now yields the leading `args.is_empty()` test by value (`inline_call_r_i/dR>i`) instead of the `(&[PyObjectRef], Option<PyObjectRef>)` pair by reference, and the `args.len()` that inlined body reads off the wrapper's own `r0` is now the first `arraylen_gc`. The property held throughout. Every `getarrayitem_gc_r` carrying the argument-slice item descr reads `r6`, and the only `arraylen_gc` on `r0` is that pre-split `args.len()`. So the item descr is selected directly, the off-`r0` assertion is made about the register that read reaches the slice through, and the length read is required on that same register. Reproducing this needs current LLBC: `build/llbc/` predating the lowering change still yields the old shape, and the test passes against it on every target. Verified with a fresh extraction — `pyre-jit-trace --lib`, 312 passed. Assisted-by: Claude * interp: drop the duplicate interp_return_log_enabled definition `eval.rs` carries two definitions of `interp_return_log_enabled`, at :589 and :617. Both are `#[cfg(not(feature = "sandbox"))]` with the same body — a `OnceLock<bool>` over `PYRE_INTERP_RETURN_LOG` — and only their doc comments differ, so `pyre-interpreter` fails to compile with E0428 and the Charon/LLBC extraction step exits before any other CI job runs. The pair is inherited, not produced by the rebase: `origin/main` `25b2442c4e` holds both. #874 added the first; #907 added the second and merged on top without seeing it, since each PR's CI builds only its own merge commit. Keeps the :589 copy and its comment; the sole call site at :2729 is unchanged. Assisted-by: Claude

…orcing, and zero PyFrame.vable_token on JIT-allocated frames (#902) * jit: root the forced-virtual caches and store them inside handle_async_forcing Two review findings on `MetaInterp::forced_virtuals`, the stand-in for the `AllVirtuals` upstream hides in the deadframe's `jf_savedata` word. Rooting. The ptr half holds what `force_all_virtuals` (resume.py:969-981) materialized until the following GUARD_NOT_FORCED consumes it, and it was in no root set: a major collection inside that window frees any old-generation object without `flags::VISITED` (`OldGen::sweep_arenas_step`), and a virtual named only by a resume frame's ref registers has this Vec as its only referent. Upstream gets the edge from `jf_savedata` being traced as a GCREF field (`majit-backend/src/jitframe.rs:354`; the previous citation, :278, is `JitFrame::init`). Enroll it as the fifth `register_mutator_extra_area` member alongside rd_consts / partial_trace / active_trace / compile_snapshot, walking only the ptr half — the int half is unboxed field values. Register an ephemeron pruner, like `mapdict::prune_dead_owner_entries`, so an entry the guard never consumes goes when its owner frame is swept instead of pinning its objects and leaving a key a recycled PyFrame address could match. `EphemeronPrunerFn` carries no data pointer, so the pruner reaches only the collecting thread's `JIT_DRIVER`. Store placement. `compile.py:996-1000` calls `set_savedata_ref` inside `handle_async_forcing`; pyre did it at the `force_pyframe` hook, so `force_pyframe_vref` — the materializing arm of `virtualref.py:134 force_virtual_if_necessary`, which calls `force_virtualizable_token` as a statement — dropped the cache. `force_from_resumedata` now also returns the virtualizable its vable section named (resume.py:1404) and `handle_async_forcing` keys the store on it, so both entry points are covered and the key comes from the resume data rather than the caller. `force_virtualizable_token` returns nothing, like `force_now`. Adds a `[jit][take_forced_virtuals] hit/miss` counterpart to the existing `handle_async_forcing` log, and two tests for the `all_virtuals = Some(..)` resume: `consume_vref_and_vable` jumps the vable and vref sections (resume.py:1433-1435), and `_prepare_virtuals` zeroes a preloaded cache (resume.py:990-991), which is why the caller passes rd_virtuals as None. Assisted-by: Claude * gc: give owner-keyed mutator tables a pruner with the root walk's reach `MetaInterp::forced_virtuals` was pruned through `register_ephemeron_pruner`, which hands the classifier no way to name a thread, so the pyre side had to read `JIT_DRIVER` from caller TLS and saw only the collecting thread's driver. That contradicts the contract stated on `MutatorExtraWalkFn` -- "must derive every thread-specific address from `data`, never from caller TLS" -- and left another mutator's dead-owner entries pinned. Add `register_mutator_pruner` / `prune_all_mutator_areas` / `prune_my_mutator_areas` next to the extra-area equivalents, stored in the same `MutatorEntry`. The collector calls them from the pre-sweep point it already prunes at, with the same classifier, and picks between all-mutator and own-mutator on `gc_sync::mutators_quiesced()` -- the same predicate `do_collect_nursery` and `enumerate_root_walker_values` use, so a collection's prune reach always equals its own root-walk reach. That branch is load-bearing: the unconditional form tripped the quiescence assertion, because the pre-sweep point does not own STW. pyre registers the pruner on the same `jit_driver` data as `forced_virtuals_root_walker_area`. The mapdict tables stay on the global registration -- they are a process-global `Mutex` map, reachable from any thread. Also assert in `save_forced_virtuals` that the owner is not nursery-resident. The entry is keyed by a bare address, which is only sound because the virtualizable comes from `FrameBox::new` -> `try_gc_alloc_stable_raw` ("stable across minor and major collections"), never from the frames a trace builds virtually. A debug run of the getframe force fixture completes 11 majors and 44 minors with five forces and five cache hits, and the assertion does not fire. Assisted-by: Claude * jit: name the force entry point in the MAJIT_LOG stream `force_pyframe` and `force_pyframe_vref` reach the same `handle_async_forcing`, and nothing downstream distinguished them, so a census of async forcing could not say which hook produced an event. That is how the vref hook silently kept the store the frame hook had. Three lines, all behind `majit_log_enabled`: hook entry for the vref (distinct from the token arm, since a vref built during tracing carries `forced` already set and `virtual_token = TOKEN_NONE` and returns without running the closure -- counting only the closure conflates "never reached" with "reached and short-circuited"), the token arm itself, and the frame hook. Census over the 330 runnable synth fixtures with these: 5 async-force events, all from getframe_caller_locals_nested_compiled_callee, all consumed by their GUARD_NOT_FORCED; 4 of the 5 materialize an empty cache. The vref hook is entered 195 times across 6 fixtures and takes the token arm 0 times. Assisted-by: Claude * jit: name PyFrame.vable_token as a GC field of the frame size descr `rvirtualizable.py:29` declares `('vable_token', llmemory.GCREF)`, so upstream's `gc_fielddescrs` names it and `rewrite.py:498-504 clear_gc_fields` zeroes the slot behind every `new`. pyre types the field `usize` and `PYFRAME_DESCR_GROUP`'s positional census did not list it, so a JIT-inlined `NewWithVtable(pyframe_size_descr())` left it holding recycled nursery bytes — `Nursery::reset` does not zero-fill and the `CallMallocNursery` fast path clears only the header word. `emit_force_virtualizable` (`pyjitpl.py:1148-1158`) then read those bytes with `GETFIELD_GC_R`; the backend spilled the result to a jitframe ref-root slot and marked it live in the call site's gcmap, so the next minor collection dereferenced them and aborted with `GC BUG: invalid type_id` (`synth/mutate_then_raise_caught`, cranelift). Carry the edge through a new `build_object_descr_group_with_extra_gc_edges` lane, the one the inherited `PyObject.w_class` edge already uses, which adds to `gc_fielddescrs` without disturbing the positional `all_fielddescrs` that `field_descr_from_group` indexes. Assisted-by: Claude * jit: store the inline empty list's length instead of assuming a memzero `emit_empty_list_inline` and `w_list_size_descr`'s doc both stated that `NewWithVtable` leaves the untouched slots zero-filled. It does not: the nursery is not zero-filled (`incminimark.py:211 malloc_zero_filled = False`) and the only zeroes an inline allocation gets are the pending stores `clear_gc_fields` derives from `gc_fielddescrs`. `items`, `int_items.block` and `float_items.block` are GC-pointer fields of the descr and so are covered; `length` is a plain int and was not. Store `length = 0` explicitly, as `rlist.py ll_newlist` does, and restate both comments in terms of the mechanism that actually clears the slots. Assisted-by: Claude * fix: drop the duplicate interp_return_log_enabled definition #907 added a second `#[cfg(not(feature = "sandbox"))] fn interp_return_log_enabled` with the same body next to the existing one, so `pyre-interpreter` fails to compile with E0428 on main. Keep the first definition and its doc comment; delete the duplicate. Assisted-by: Claude

#907 added a second `interp_return_log_enabled` to `eval.rs`; #874 had already introduced one under a different doc comment. Two definitions of the same name in one module is an E0428, so the crate did not compile. Keep #874's. Assisted-by: Claude

…n in-trace (#918) * pyre-object: record that the instance-nursery blocker no longer reproduces The allocator note claimed a SIGSEGV in synth/inheritance_dispatch as the reason instances use the stable old-gen allocator. Switching the call to `try_gc_alloc` no longer reproduces it: the fixture passes at the default, 256 KB and 128 KB nursery sizes, check.py passes 344 on both backends, and seven GC-sensitive fixtures produce byte-identical output under a 128 KB nursery. The call is left on the stable allocator. Measured with the flip applied: minor/major collection counts and peak RSS are unchanged and every non-microbenchmark fixture moves within noise, because the host-side nursery allocator is non-collecting and falls back to old-gen when the nursery is full. Assisted-by: Claude * eval: drop the duplicate interp_return_log_enabled definition #907 added a second `interp_return_log_enabled` to `eval.rs`; #874 had already introduced one under a different doc comment. Two definitions of the same name in one module is an E0428, so the crate did not compile. Keep #874's. Assisted-by: Claude * Revert "eval: drop the duplicate interp_return_log_enabled definition" This reverts commit a4978192cd747aaf12e5b9319a206324fa9a5cce. That commit was written against a base carrying two definitions of `interp_return_log_enabled` and deleted one of them. On the current base only one definition exists, so replaying the deletion removed the only one and `finish_value`'s call site no longer resolved (E0425). Assisted-by: Claude * majit: route a non_moving SizeDescr's NEW to the old-gen malloc helper `SizeDescr::non_moving()` is a new default-false opt-in, shaped like the existing `headerless()`. `handle_new` skips `gen_malloc_nursery` for such a descr and takes `gen_malloc_fixedsize`, the same `malloc_big_fixedsize` helper the oversized case already uses; both backends' helpers allocate through `alloc_oldgen_typed`, which does not move an object. `SimpleSizeDescr` stores the bit in an `AtomicBool` with a `&self` setter, so a frontend can stamp it on a descr already behind an `Arc`, as `descr_index` is stamped. Assisted-by: Claude * jit: emit class instantiation in-trace instead of a bh_call_fn residual `resolve_inlinable_callee` requires `ob_type == FUNCTION_TYPE`, so a class callable always declined and every `P()` re-ran `type_descr_call_impl` -> `object.__new__` -> an interpreted `__init__` frame. `try_walker_inline_type_call` (`jitcode_dispatch/inline_call.rs`) emits it the way `typeobject.py descr_call` runs it: guard the class and its version tag, allocate the instance, then feed that instance to the existing user-call inline path as the `__init__` receiver. It declines unless `__new__` is the inherited `object.__new__`, the metaclass is exactly `type`, the type is instantiable and non-abstract, and `hasuserdel` is false. `PyObject.w_class` joins `W_OBJECT_OBJECT_DESCR_GROUP`: every instance shares `INSTANCE_TYPE` as its vtable while its Python class varies, so the vtable-derived `w_class_obj` resolves to `object` and only a field the virtual tracks answers `getfield_gc(w_class)` correctly. The instance size descr is marked `non_moving`. `alloc_instance_object` allocates through the stable old-gen allocator because the instance layer reaches an instance through raw pointers it does not root -- `store_attr_caching` holds one across the allocation of the storage block it installs -- and a nursery-allocated instance moves under those pointers, after which the helper writes `map` / `storage` into the dead pre-move copy. `mapdict::ensure_type_terminator` is exported so the emit can install a terminator on a class no attribute has been read off yet, whose terminator is still null. 2M-iteration loops, user time, empty loop 0.07s: `class P: pass` -> `P()` 0.62s -> 0.07s; `PA(1)` with `def __init__(self,a): self.a=a` 2.04s -> 1.43s. check.py --backend dynasm 348/348. Assisted-by: Claude * majit: add a non_moving ArrayDescr opt-in, drop the malloc_big_fixedsize write-barrier stamp `ArrayDescr::non_moving()` mirrors the existing `SizeDescr::non_moving()`. `handle_new_array` skips both nursery routes for such a descr and `gen_malloc_array` selects an old-generation twin of the typed malloc helper. The twins take the same arguments as the nursery helpers, so they share `malloc_array_descr` / `malloc_array_nonstandard_descr`; both backends register them. `gen_malloc_fixedsize` no longer calls `remember_wb` on its result. rewrite.py:794-796 stamps it because upstream's `malloc_big_fixedsize` returns a young raw-malloced object; pyre's helper allocates in the old generation on both backends, where the stamp drops the barrier on the first `SETFIELD_GC` of a young value into it. Adds `test_non_moving_new_array_declines_nursery_at_small_length`. Assisted-by: Claude * jit: emit the mapdict attribute-add transition in-trace instead of the setattr residual `store_attr_add_fast_path` (mapdict.rs) resolves the `map -> PlainAttribute` transition a STORE_ATTR adding a not-yet-present boxed attribute takes, without performing it; `store_attr_add_commit` applies it for the authoritative walk. `try_walker_specialize_store_attr` guards the receiver's map and its type's version tag, then emits `_set_mapdict_increase_storage1` (mapdict.py:942-959) as NEW_ARRAY_CLEAR + SETARRAYITEM_GC + the `storage` and `map` field stores. The storage block gets its own array descr carrying its leaf GC type id (`W_MAPDICT_STORAGE_GC_TYPE_ID`) and `non_moving`, matching `alloc_mapdict_storage_block`. The LOAD_ATTR fold's block read moves to that descr as well; it read the block through `pyobject_gcarray_descr` before, so a read following the emitted write missed the heap cache and the trace aborted with `InvalidLoop: protect_speculative_field`. Declined, leaving the general residual: an attribute already in the map, a value that takes an unboxed slot, an unboxed slot anywhere in the map chain, a NoDict or Devolved terminator, the `_reorder_and_add` case, the LIMIT_MAP_ATTRIBUTES devolve, and a storage block the collector does not own. Measured on 2M-iteration loops (user time, empty loop 0.06s): `class PA:` with `def __init__(self, a): self.a = a` called with a str argument 1.45s -> 0.05s. An int argument takes an unboxed slot and stays on the residual at 0.91s. Adds `pyre/bench/synth/attr_store_add_transition.py`; it runs 4.0x pypy on dynasm and 4.3x on cranelift against its 12x gate. Assisted-by: Claude * majit(wasm): route a non_moving descr's New/NewArray to an old-gen helper The wasm backend lowers `New` / `NewWithVtable` / `NewArray` / `NewArrayClear` itself instead of running the GC rewrite pass, and its lowering never read the descr's `non_moving` flag: every allocation took the nursery, inline bump or helper. `W_ObjectObject`'s size descr is marked `non_moving` because the mapdict layer reaches an instance through raw pointers nothing forwards. On wasm the flag was dropped, so a JIT-emitted instance was allocated in the movable nursery; after a minor collection the mapdict layer walked a recycled `map` chain. `synth/instance_dict_reassign` trapped there — `node_materialize_dict` recursed until the stack was exhausted, then a `setattr_str` read out of bounds. dynasm and cranelift were unaffected; their rewrite pass honours the flag. Add `wasm_jit_alloc_oldgen` / `wasm_jit_alloc_array_oldgen`, group the four helper addresses into `AllocHelpers`, and have both allocation arms decline the nursery and pick the old-gen twin when the descr asks for it. Assisted-by: Claude * jit: latch the CALL boundary when an inlined __init__ returns non-None `descr_call` rejects a non-None `__init__` result, so the inline gives the callee back to the interpreter. That branch returned the error directly while the sub-walk had already executed the constructor body, so the walk driver replayed the loop from entry and re-ran whatever the body did. The invalid-`str`/`repr`-result branch above it already latches the outer CALL as the forward-resume point under the same three preconditions. Extract that block as `latch_abort_call_resume` and call it from all three sites, including the constructor one. Reported by Codex on #918. Assisted-by: Claude * jit: document the storage-before-map order of the mapdict add emit The emitted transition writes `storage` then `map`. Record why the order matters under free threading — the visible intermediate is a block longer than the map needs, not a map indexing past the block — and that these raw stores do not take the striped `instance_lock`, matching the existing in-trace mapdict fast paths. Assisted-by: Claude * jit: fold the mapdict attribute-add only for an is_unescaped receiver The emitted transition is a pair of raw field stores: unlike the interpreter's it does not hold the striped `instance_lock`, and unlike the single-slot in-place write it publishes two fields, so a concurrent mutator of the same instance could pair one thread's `map` with another's `storage`. Requiring `heap_cache().is_unescaped(obj)` rules that out — the receiver is one this trace allocated and no other thread can name. Measured on 2M-iteration loops: a fresh instance with one str attribute and one with two both stay at empty-loop time (0.06s) and their traces carry no `StoreAttr` residual, so the fold still fires where it pays. Reported by Codex on #918. Assisted-by: Claude

youknowone added 4 commits July 30, 2026 22:18

youknowone merged commit c9b98f6 into main Jul 30, 2026
19 checks passed

youknowone deleted the perf-loop branch July 30, 2026 17:04

youknowone mentioned this pull request Jul 30, 2026

jit(fbw): latch a multi-frame blackhole image on ABORT_TOO_LONG #910

Merged

youknowone mentioned this pull request Jul 30, 2026

review: act on the Codex and CodeRabbit findings on #887 #911

Merged

youknowone mentioned this pull request Jul 31, 2026

review: act on the #904 findings, and scope the descr type_id split out #921

Merged

Navigation Menu

mapdict off-GC side-table values, per-guard aarch64 reach check, paused-call floor on the third recipe path, and a per-return getenv - #907

mapdict off-GC side-table values, per-guard aarch64 reach check, paused-call floor on the third recipe path, and a per-return getenv#907
youknowone merged 4 commits into
mainfrom
perf-loop

youknowone commented Jul 30, 2026

Uh oh!

coderabbitai Bot commented Jul 30, 2026

Review limit reached

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

youknowone commented Jul 30, 2026

1. mapdict: a side-table value the GC does not own (Codex P1 on #885)

2. dynasm/aarch64: measure each guard branch against its own stub (CodeRabbit)

3. jit(fbw): the paused-call operand floor on the third recipe path (CodeRabbit)

4. interp: PYRE_INTERP_RETURN_LOG read once instead of per return

Verification

Not addressed

Uh oh!

coderabbitai Bot commented Jul 30, 2026

Review limit reached

Uh oh!

github-actions Bot commented Jul 30, 2026

🤖 Codex parity review

1. Regressions to PyPy parity introduced by this patch

2. Other mismatches introduced by this patch

3. Pre-existing mismatches (already present before this patch)

4. Structural adaptations

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

4. interp: `PYRE_INTERP_RETURN_LOG` read once instead of per return