Repository navigation
translator, jit: number fields through the heaptracker walker, restore per-frame bridge state, execute recorded allocations - #1024
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 39 seconds 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?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (25)
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 febbdad). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9faf7886e
ℹ️ 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".
| size: size.size(), | ||
| type_id: size.type_id() as u64, | ||
| vtable: size.vtable() as u64, | ||
| owner: String::new(), |
There was a problem hiding this comment.
When new/d>r originates from JitCodeBuilder::new_struct(..., headerless=true), this conversion always emits an empty owner. BhDescr::is_headerless recognizes only HEADERLESS_SIZE_OWNER_MARKER, so execute_new_allocation incorrectly sends the object through the header-writing bh_new path; the descriptor contract documents that this shifts the field layout. Recording-time field operations can therefore inspect or mutate different memory than the generated headerless allocation, corrupting the trace. Set the owner marker whenever size.headerless() is true.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
…ling back to 0 `InstancePtrInfo` / `StructPtrInfo` / `ArrayPtrInfo` arms of `make_guards` downcast their descr and substituted a literal `0` for the vtable or the GC type id when the downcast returned `None`. `next_type_id` starts at 1 (`majit-ir/src/descr.rs:766-769`, `gctypelayout.py:328-331`), so no descr owns tid 0, but the runtime header tid 0 is the `rclass.OBJECT` root (`pyre-jit-trace/src/descr.rs:475`). A `GUARD_GC_TYPE` against 0 therefore passes on any plain `object` instance, and `use_box` installs the info the guard was meant to justify. For the vtable arm, 0 is the `is_object() == false` marker and reaches `lookup_subclass_range(0)`, which panics in the backend. `info.py:346-352`, `:360-366` and `:632-639` read `get_vtable()` / `get_type_id()` unconditionally. Unwrap the descr with a message naming the invariant instead, matching `ensure_ptr_info_arg0` (`optimizeopt/mod.rs:8362-8366`) and `deserialize_optheap` (`optimizeopt/heap.rs:3691-3696`), which already assert the same shape on the same value. check.py: dynasm 370/370. cargo test -p majit-metainterp: 1525 passed. Assisted-by: Claude
…carried it `ob_type_descr()` built a `PyreFieldDescr` with `immutable: true` and `parent_descr: None` — the pair that made `protect_speculative_field` fail closed on `str_len_descr` before #989. It reached six production call sites, all of which bound it to an unused `_ob_type_descr` parameter, so no operation was ever recorded against it. `heaptracker.py:60-67` skips `typeptr` before a descr is built for it (`if name == 'typeptr': continue`), and the vtable is read through `cpu.vtable_offset` (`llmodel.py:67`) rather than a field descr; the codewriter's own typeptr handling is `jtransform.py:1004-1010`. Pyre's live typeptr fold keys on `fd.offset() == OB_TYPE_OFFSET` (`jitcode_dispatch/heapcache_ops.rs:516-519`), not on this descr. Remove the factory, the `OB_TYPE_FIELD_DESCR` static and the parameters that threaded it through `box_trace`. `make_vtable_field_descr` (`object.typeptr`, parented via `Arc::new_cyclic`) is untouched and remains the sole producer `Descr::is_typeptr` recognises; its doc is updated to say so. check.py: dynasm 370/370. cargo test on the four touched crates: green. Assisted-by: Claude
…gistry The 0-arg call fold matched the `[core|std, ptr, null|null_mut]` segments literally, so a guest const-global that `flowspace/model.rs` binds to the same `HostObject` (`pyre_object::pyobject::PY_NULL` -> `core.ptr.null_mut`) kept its call instead of folding to `ConstRefNull`. Resolve the call path through `HOST_ENV::import_module`/`module_get` and compare the resulting `HostObject::qualname` against the registered null builtins, so the alias keeps one owner and the codewriter repeats no guest spelling. The test now runs both spellings. Assisted-by: Claude
`fuse_boxing_alloc` lowers `lltype::malloc_typed` to `NewWithVtable` but leaves the by-value Rust aggregate it consumed in the dominator, where `gc_interp`'s stable-allocation arm writes it out with `core::ptr::write`. The aggregate's synthetic constructors and field writes therefore still execute on the ordinary JIT path that no longer reads them. Clone the constructor/field-write template of each fused aggregate -- the outer value plus nested aggregates stored into it -- immediately before every `core::ptr::write` sink under a per-site rename map, and repoint the write at the clone. `remove_simple_mallocs` performs the equivalent escape-sensitive motion. Assisted-by: Claude
…d limit paths A translated builtin gateway is transparent to the Python MIFrame stack and has no blackhole entry point of its own, so two per-frame decisions in `walk()` must not fire inside one. Abort coordinate: the helper consumed the session's one-shot `claim_abort_coordinate`, after which the enclosing Python walk could no longer claim and latch the propagated abort, and the latch built here fed `build_multi_frame_miframe` the helper's r0 args-slice as a Python red frame. Trace limit: the post-step `is_too_long()` split ran at the helper's own opcode coordinate, which would pair a helper register with a Python JitCode's red-frame register. Defer both to the enclosing Python `walk()`, which performs the same checks at its real per-frame coordinate. Assisted-by: Claude
`pyjitpl.py:624-629 execute_new` runs the allocation for its side effect and records the trace op over the resulting pointer. pyre recorded the op but wrote `ConcreteValue::Null` into the destination register, so readers of the result declined instead of seeing a real object. Add `TraceCtx::execute_new_allocation`, which converts a size descr to a `BhDescr::Size` and calls `cpu.bh_new`/`bh_new_with_vtable`, and use it from both opcodes: publish the pointer as the opref's concrete value and as the register's `ConcreteValue::Ref`. `new_with_vtable` additionally stamps `w_class` from the descr. The recorded op is unchanged, so the optimizer remains free to virtualize the allocation. Assisted-by: Claude
`heaptracker.py:64-66` skips `typeptr` by name, and `typeptr` is the only
field of the `OBJECT` substructure every instance embeds first, so recursing
into that header contributes zero slots and the census numbers only value
fields. Pyre embeds `PyObject { ob_type, w_class }` as its first field and
spells neither word `typeptr`, so the existing skip never fired: the walker
counted two header leaves that no runtime publish has, and every value field
was numbered two high.
Worse, the header's own `w_class` matched by name before the outer struct's:
`get_fielddescr_index_in` returns the recursion's result as soon as it is
non-negative, so `Method.w_class` numbered 1 -- the header's slot -- while
`w_function`/`w_self`/`w_module` numbered 2/3/5.
Skip both header words in both walkers, qualifying `w_class` by its owning
struct so `Method.w_class` stays an ordinary value field. Measured against
`W_METHOD_DESCR_GROUP`, whose runtime census is complete, the four fields go
from 2/3/1/5 to 0/1/2/3, matching the publish exactly.
A header word now has no census slot at all, so the mint site answers it with
the `index_in_parent: 0` the runtime already mints it with, where consumers
resolve it through `FieldDescr::is_w_class()` rather than by index.
Assisted-by: Claude
Four comments explained the externally-threaded `all_fielddescrs` and the caller-supplied `index_in_parent` by saying pyre has no heaptracker / no lltype STRUCT walker. `majit-translate codewriter::heaptracker` is a port of `heaptracker.py` and is what numbers those fields; the reason the list is threaded in is that it lives in the crate that depends on this one. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: febbdadcb3
ℹ️ 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
+2659
to
+2663
| let coordinate_belongs_to_this_frame = !ctx.fbw_mode.transparent_helper_subwalk | ||
| && ctx | ||
| .session | ||
| .borrow_mut() | ||
| .claim_abort_coordinate(ctx.fbw_mode.inline_subwalk); |
There was a problem hiding this comment.
When a transparent helper returns a non-fallback DispatchError, run_sub_jitcode_walk propagates it unchanged; this gate leaves the coordinate claim available for the enclosing Python walk, but that walk then calls latch_abort_blackhole(ctx, error.stop_pc()) with the helper's bytecode offset. The resulting image combines caller registers with a helper PC, so blackhole recovery can resume at an unrelated caller opcode or an invalid offset. Remap the error to the enclosing call coordinate before allowing this claim, or decline the outer latch.
AGENTS.md reference: AGENTS.md:L24-L30
Useful? React with 👍 / 👎.
| if crate::codewriter::heaptracker::is_header_word(owner, field_name) { | ||
| return 0; | ||
| } | ||
| let walked = crate::codewriter::heaptracker::get_fielddescr_index_in(cc, owner, field_name, 0); |
There was a problem hiding this comment.
When a translated Rust struct embeds a known by-value struct after an earlier countable field, routing fallback descriptor mints through this walker produces the wrong index_in_parent: the recursive miss already encodes the incoming cur_index, but get_fielddescr_index_in adds -r - 1 to that offset again. For Outer { a, inner: { x, y }, b }, b is numbered 4 while all_fielddescrs places it at 3, so optimizer field slots can alias or miss the actual virtual field. The recursive advancement must use the returned cumulative index rather than add it again, or recurse from zero and add only the nested leaf count.
Useful? React with 👍 / 👎.
youknowone
added a commit
that referenced
this pull request
Aug 4, 2026
The RangeTo recognizer required only that `end` be defined, while RangeFrom required `start_is_const_nonneg`; rename that helper to `bound_is_const_nonneg` and apply it to both bounds. Both recognizers stay dormant. front/mir.rs declared, initialised and invoked `slice_index_rangeto_sites` that nothing ever populates — remove it, matching the already-removed RangeFrom plumbing. A flowspace_adapter test asserted `host.qualname() == "rarithmetic.r_uint"`, which any same-named object satisfies; compare against the `HOST_ENV` module attribute instead. The operand-threading half of this change is dropped: origin/main ce37de6 (#1024) replaced `thread_undefined_op_operands` with `available_on_every_predecessor_path`, which already covers the join case and carries the `SSA_to_SSI` reference. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 5, 2026
…eaded operands; oparg signedness (#1029) * jit(#346): add dont_look_inside to push_roots/increase_root_stack_depth/root_stack_depth These three gc_roots functions call with_shadow_stack internally but lacked the #[majit_macros::dont_look_inside] their shadow-stack siblings carry; the prepass lifted their bodies and failed on the unregistered ROOT_STACK TLS access, poisoning RootScope::new and every root-pushing caller. Same-base #1004 census A/B (via .ullbc re-extraction): phaseA 1229 -> 1215 (-14; 14 graphs cleared, 0 new; phaseB 3 -> 3 unchanged); ROOT_STACK occurrences 215 -> 0. Deep callers re-root to other walls. Assisted-by: Claude * Fix oparg signedness lowering (phaseA 1213→1209) Preserve transparent u32 opargs as unsigned SomeInteger inputs, clearing the signed int ∪ r_uint census failures while phaseB remains 3. Assisted-by: Claude * Carry usize slice bounds through annotation; keep the sub-slice capstone dormant Preserve the checked usize arithmetic tuple's element type through flowspace lowering as r_uint, so a usize-typed slice bound annotates as SomeInteger(unsigned=True) and satisfies check_negative_slice. Add the RangeTo arm (&s[..k] -> getslice(s, 0, k)) alongside the existing RangeFrom recognizer in front/slice_index.rs, with the prune_dead_phis wiring the rewrite needs, and leave both arms dormant. Activating RangeTo emitted a bare getslice/rii>r from split_builtin_kwargs and do_warn_explicit, which no blackhole handler dispatches; that failed production_bh_builder_covers_every_build_emitted_opname and default_bh_builder_unwired_set_matches_task_85_snapshot. Both stops are statically unsigned, so signedness does not separate the sites whose planted op is consumed from an rtyped graph from those that drop to the legacy walker. phaseA 1272, phaseB 3 - unchanged. Assisted-by: Claude * Residualise the prebuilt singleton accessors (phaseA 1272→1242) w_none, bool_singleton and w_not_implemented read a process-global OnceLock<usize> holding an immortal object. sync::once_lock::OnceLock:: get_or_init has no PyreCallRegistry entry, so every caller that tried to lift one of these accessors failed to annotate: 125 census records, 87 of them walled on nothing else. dont_look_inside residualises the call, so the caller no longer lifts the body. All 125 get_or_init occurrences leave the census and 30 graphs stop failing; the remaining 95 re-root onto other walls. phaseB stays 3 and no graph starts failing. Assisted-by: Claude * Gate the RangeTo bound and retire the dormant RangeTo plumbing The RangeTo recognizer required only that `end` be defined, while RangeFrom required `start_is_const_nonneg`; rename that helper to `bound_is_const_nonneg` and apply it to both bounds. Both recognizers stay dormant. front/mir.rs declared, initialised and invoked `slice_index_rangeto_sites` that nothing ever populates — remove it, matching the already-removed RangeFrom plumbing. A flowspace_adapter test asserted `host.qualname() == "rarithmetic.r_uint"`, which any same-named object satisfies; compare against the `HOST_ENV` module attribute instead. The operand-threading half of this change is dropped: origin/main ce37de6 (#1024) replaced `thread_undefined_op_operands` with `available_on_every_predecessor_path`, which already covers the join case and carries the `SSA_to_SSI` reference. Assisted-by: Claude * Remove push_roots residual-call annotation Remove dont_look_inside from the two-word RootScope return and document the residual-token ABI mismatch. Measured phaseA 1242 -> 1261 and phaseB 3 -> 3 after LLBC re-extraction. Assisted-by: Claude * Document the RangeTo bounds-proof blocker Record the StartStop clamp mismatch as the primary dormant RangeTo blocker and non-negativity as the secondary blocker. Measured phaseA 1261 -> 1261 and phaseB 3 -> 3, with zero new failing graphs. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 21, 2026
…l order a defect Verified the three upstream claims these comments make against the vendored tree, and two of them were spelled in a way the repo's porting discipline rejects or the code does not support. `shadowstack.py` has no by-value removal at all -- `push_stack` and `pop_stack` move one `root_stack_top`. So `RootSet::remove`'s out-of-order fallback is pyre's own, and `position` vs `rposition` does not change which roots are scanned: `add` hands out no index, nothing reads `roots` positionally, and both duplicates are the same pointer. The comment now says what the change is actually for -- keeping the stack shape so the following removals stay on the O(1) path. The marshal comment now names `unmarshal_pycode` and `PyCode.__init__`, whose `co_consts_w` is the only constants table upstream keeps, which is the reason pyre's second compiler-level table can hold a placeholder here. `RootScope::normalize` and `RootScope::set` take the wording their siblings already use for this guard, which `pin_root`, `normalize_roots`, `shadow_stack_set` and `RootScope::pin_root` have carried since #1024. Line numbers dropped throughout, per the symbol-only citation rule. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 21, 2026
…l order a defect Verified the three upstream claims these comments make against the vendored tree, and two of them were spelled in a way the repo's porting discipline rejects or the code does not support. `shadowstack.py` has no by-value removal at all -- `push_stack` and `pop_stack` move one `root_stack_top`. So `RootSet::remove`'s out-of-order fallback is pyre's own, and `position` vs `rposition` does not change which roots are scanned: `add` hands out no index, nothing reads `roots` positionally, and both duplicates are the same pointer. The comment now says what the change is actually for -- keeping the stack shape so the following removals stay on the O(1) path. The marshal comment now names `unmarshal_pycode` and `PyCode.__init__`, whose `co_consts_w` is the only constants table upstream keeps, which is the reason pyre's second compiler-level table can hold a placeholder here. `RootScope::normalize` and `RootScope::set` take the wording their siblings already use for this guard, which `pin_root`, `normalize_roots`, `shadow_stack_set` and `RootScope::pin_root` have carried since #1024. Line numbers dropped throughout, per the symbol-only citation rule. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 21, 2026
…l order a defect Verified the three upstream claims these comments make against the vendored tree, and two of them were spelled in a way the repo's porting discipline rejects or the code does not support. `shadowstack.py` has no by-value removal at all -- `push_stack` and `pop_stack` move one `root_stack_top`. So `RootSet::remove`'s out-of-order fallback is pyre's own, and `position` vs `rposition` does not change which roots are scanned: `add` hands out no index, nothing reads `roots` positionally, and both duplicates are the same pointer. The comment now says what the change is actually for -- keeping the stack shape so the following removals stay on the O(1) path. The marshal comment now names `unmarshal_pycode` and `PyCode.__init__`, whose `co_consts_w` is the only constants table upstream keeps, which is the reason pyre's second compiler-level table can hold a placeholder here. `RootScope::normalize` and `RootScope::set` take the wording their siblings already use for this guard, which `pin_root`, `normalize_roots`, `shadow_stack_set` and `RootScope::pin_root` have carried since #1024. Line numbers dropped throughout, per the symbol-only citation rule. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 22, 2026
…s foreign-mutator guard, and root-set shape (#1393) * gc, interpreter: retire the newest duplicate root, and pin box_code_object's ownership transfer `RootSet::remove` answers a matching pop in O(1) and falls back to a scan when the removal is out of order. That fallback used `position`, which finds the *oldest* copy of a root registered more than once by nested host brackets. `shadowstack.py:80-106` decrements the same `root_stack_top` it advanced on push, so duplicates retire newest-first; `rposition` restores that order, which also leaves the following removals on the O(1) path instead of turning each one into another scan. The added test registers `a, b, a, b` and asserts the vector after each removal; with `position` the first assertion already fails. `box_code_object` moves its `CodeObject` into the leaked box rather than cloning it, and nothing pinned that. The added test compares the `source_path` and `instructions` buffer addresses across the call. Assisted-by: Claude * interpreter, object: keep a decoded nested PyCode as its own constant slot, and carry normalize's foreign-mutator guard into RootScope `code_constant_from_value` ran every decoded value through `obj_to_constant_data`, which for a nested PyCode copies the whole child `CodeObject` into the parent's compiler constants table. The wrapped child is then installed in `co_consts_w` by `w_code_fill_wrapped_consts`, so the copy is a second owner of a graph the wrapper already names. Return the shape placeholder for that arm and let the wrapped child stand as the slot's authority, which is where `marshal_impl.py`'s unmarshaller puts the already-decoded `PyCode`. `RootScope::normalize` and `RootScope::set` are the scope-local spellings of `normalize_roots` and `shadow_stack_set`. Both free functions skip the `try_gc_current_object_address` queries when `foreign_mutator_seen` is false -- RPython has one mutator under the GIL, so no root becomes a forwarding stub between publication and the allocation bracket -- and the two methods had not been given that guard. `set` keeps the publish-then- query order its twin documents, so the raw store still happens first. The `PyCode` doc comments describe the nested-body lifetime the wrapper now relies on: a top-level body is `Box::into_raw`'d and a nested body points into one of those owners. Assisted-by: Claude * comments: cite the upstream symbols, and stop calling the root-removal order a defect Verified the three upstream claims these comments make against the vendored tree, and two of them were spelled in a way the repo's porting discipline rejects or the code does not support. `shadowstack.py` has no by-value removal at all -- `push_stack` and `pop_stack` move one `root_stack_top`. So `RootSet::remove`'s out-of-order fallback is pyre's own, and `position` vs `rposition` does not change which roots are scanned: `add` hands out no index, nothing reads `roots` positionally, and both duplicates are the same pointer. The comment now says what the change is actually for -- keeping the stack shape so the following removals stay on the O(1) path. The marshal comment now names `unmarshal_pycode` and `PyCode.__init__`, whose `co_consts_w` is the only constants table upstream keeps, which is the reason pyre's second compiler-level table can hold a placeholder here. `RootScope::normalize` and `RootScope::set` take the wording their siblings already use for this guard, which `pin_root`, `normalize_roots`, `shadow_stack_set` and `RootScope::pin_root` have carried since #1024. Line numbers dropped throughout, per the symbol-only citation rule. Assisted-by: Claude * perf: remove three more pip runtime bottlenecks * perf: match PyPy shutdown and prehash object dicts * extra_tests: pin the decoded nested code object's identity across marshal `unmarshal_pycode` is declared `save_ref=True` and registers the code object before filling it, so a second occurrence of that object in the stream is a back-reference and both spellings owe the same object. Until the preceding commit, pyre's parent kept a copy of the nested body in its own constants table rather than the decoded wrapper, and answered `co_consts` from a wrapper rebuilt out of that copy -- equal to the back-reference's target but not the same object. Measured on two binaries built from the same LLBC inputs and run with PYRE_JIT=0, the interpreter path being the one under test: | arm | result | |---|---| | CPython 3.14.6 | passes | | b728a92, without the change | AssertionError | | with the change | passes | `test_marshal` is not in the CPython-suite baseline, so nothing else in the tree loads a stream that repeats a code object. Assisted-by: Claude * pyrex: drop the line range from finalize_runtime's baseobjspace citation The doc comment already names `ObjSpace.finish()`, which pins the claim without the number. Assisted-by: Claude * marshal: intern a decoded name from its characters, and skip the surrogate scan on an ascii name `make_interned_str` built a `str` for every occurrence of an interned name and handed it to `intern_exact_str`. `w_str_from_wtf8` is `malloc_typed`- immortal, so each occurrence whose value was already interned left that object retained for the process lifetime. Over `lib-python/3`'s 918 pyc files, 495,240 interned-name occurrences carry 37,135 distinct values, so 92.5% of those allocations were abandoned. `intern_str_value` already interns from the characters, but takes `&str` while the reader holds `&Wtf8`. Generalize it to `intern_wtf8_value` and delegate; the table has always been keyed by WTF-8. `str_from_value` reached `to_string_lossy`, which scans for a surrogate on every call. `w_str_get_value_opt` answers from the cached code-point and byte counts when the payload is ascii, which a code object's names overwhelmingly are; a surrogate-bearing name still takes the lossy path. Assisted-by: Claude * regalloc: hash the codewriter's per-variable tables with FxHash The union-find, the interference graph and the coloring key on compiler- internal identities — a flow-graph `Variable`, a jitcode `VariableId` newtype over `u32`. Upstream's dictionaries hash those by object identity, so a probe is a pointer read; `std`'s default `RandomState` runs SipHash-1-3 over the key instead, and `make_dependencies` probes once per operand of every operation in every block. Ordering is unaffected. `getnodes` orders from the `_all_nodes` insertion list; the adjacency sets are only tested for membership or folded into an order-independent accumulator; `UnionFind` takes its order from `link_to_parent`, which stays an `IndexMap`, while `weight` and `root_info` are point-accessed only. The per-kind map keeps `RandomState` — it holds one entry per kind and is not probed in a loop. `Coloring` is exported from both regalloc modules so the passes that build or borrow one name the type instead of the hasher. Assisted-by: Claude * gc_interp: describe the hot path safepoint actually has The doc claimed the function's first act was the `enabled()` early-return. It is not: the two request flags are read ahead of the cached feature switches, and deliberately so, because an explicit request has to be serviced with the routing feature off — which is why `note_eval_activation_exit` tracks nesting unconditionally. A measurement of the ordering on the flag read — relaxed load plus an acquire fence on the observed-set branch, in place of the acquire load — showed no improvement in the safepoint subtree, so the ordering is left alone. The wrong description is what cost time. Assisted-by: Claude * bench: restore fib_recursive's dynasm jit-stats snapshot The branch had re-recorded it as bridges_compiled=7, guard_failures=1501 from a local run. Every CI host observes 8/1645 for it -- macos-latest, ubuntu-24.04 and windows-latest alike -- and the cranelift and wasm snapshots of the same bench carry 8/1645 and match there, so the recorded pair was one machine's reading rather than a backend- or platform-dependent value. Assisted-by: Claude * pyrex: restore the shutdown teardown finalize_runtime performs `perf: match PyPy shutdown and prehash object dicts` deleted `collect_and_run_finalizers`, the newest-first `__main__` global release loop with `release_frees_nothing`, and `clear_shutdown_modules` / `clear_shutdown_module_dict`, leaving `finalize_runtime` at thread join, atexit, stream flush and module shutdown hooks. Without them no `__del__` runs at interpreter teardown, so `test.test_struct` and `test.test_threading` went PASS -> FAIL against `pyre/cpython_tests/baseline.json` on six subtests that read finalization output and got b''. `pyre/pyrex/src/lib.rs` is byte-identical to main again. The teardown costs, on this machine, about 10ms on a bare `pass` and about 33ms once `unittest`, `json`, `argparse`, `email`, `logging` and `http.client` are loaded (min of 12, interleaved). Assisted-by: Claude * shutdown: sweep on entry to the module walk only when the import-cache detach dropped something `release_sys_modules_for_shutdown` detaches every import-cache entry but hands back only the modules it found under a string key, and `clear_shutdown_modules` pins each of those before it sweeps. An entry the list does not carry -- a non-module value, or a module under a key that is not a string -- is the only one the detach can strip of its last referrer, so it reports whether it dropped one and the sweep runs only then. `finalize_runtime` sweeps immediately before the detach and the module walk ends on a sweep, so a run whose import cache holds nothing but named modules reaches both of those with the same heap and now pays one whole-heap mark-and-sweep less. Measured on child CPU time, min of 15 interleaved: -3.1% min / -2.7% median for `import unittest, json, argparse, email, logging, http.client`, and no measurable change for a bare `pass`, whose heap is small enough that a sweep over it is nearly free. Assisted-by: Claude
This was referenced Oct 17, 2026
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
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.
Sixteen commits, rebased onto
a8f3cdb36ed. They fall into four groups.Positional field census (
heaptracker.pyport)majit-translate codewriter::heaptrackeris the port ofall_fielddescrs/get_fielddescr_index_in, and it is what numbersindex_in_parent.Number fields through the walker instead of a caller-supplied index, and
drop the standalone
typeptrfield descr.ob_type_descr()built aPyreFieldDescrwithimmutable: trueandparent_descr: None— the pairthat made
protect_speculative_fieldfail closed before jit: give W_UnicodeObject.len a parent SizeDescr; read the trailing -live- first for after-residual-call guards #989 — and reachedsix call sites that all bound it to an unused parameter, so no operation was
ever recorded against it.
heaptracker.py:60-67skipstypeptrbefore adescr exists for it.
Keep the object header out of the census.
heaptracker.py:64-66skipstypeptrby name, andtypeptris the only field of theOBJECTsubstructure every instance embeds first, so recursing into that header
contributes zero slots. Pyre embeds
PyObject { ob_type, w_class }andspells neither word
typeptr, so the skip never fired: the walker countedtwo header leaves that no runtime publish has, and every value field was
numbered two high.
Worse, the header's own
w_classmatched by name before the outer struct's —get_fielddescr_index_inreturns the recursion's result as soon as it isnon-negative — so
Method.w_classnumbered 1, the header's slot.Measured against
W_METHOD_DESCR_GROUP, whose runtime census is complete:Method.w_functionMethod.w_selfMethod.w_classMethod.w_modulew_classis skipped only when its owner isPyObject, soMethod.w_classstays an ordinary value field.
Four comments claimed pyre has no heaptracker / no lltype STRUCT walker.
Corrected to the real reason the field list is threaded in: the walker lives
in the crate that depends on
majit-ir.Bridge and subwalk frame state
MIFrame.make_result_of_lastopputsa completed call's return value at the top of the caller's post-call operand
stack; derive that semantic slot from the codewriter's after-residual depth
twin rather than inverting a post-regalloc colour. This retires the raw-colour
fallback that was simultaneously load-bearing and wrong (it answered a local
slot where the map named an operand-stack slot). The matching
set_valuestackdepthis what keepsinit_symbolicfrom truncating thejust-injected top slot.
subwalks out of the frame-owned abort and limit paths. A translated builtin
gateway is transparent to the Python MIFrame stack and has no blackhole entry
of its own, so it must not consume the session's one-shot
claim_abort_coordinate(after which the enclosing Python walk can no longerlatch the propagated abort) and must not run the post-step
is_too_long()split at its own opcode coordinate.
Recording and optimizer
new/new_with_vtableallocation while recording.pyjitpl.py:624-629 execute_newruns the allocation for its side effect andrecords the trace op over the resulting pointer; pyre recorded the op but
wrote
ConcreteValue::Null, so readers of the result declined instead ofseeing a real object. The recorded op is unchanged, so the optimizer remains
free to virtualize.
make_guardsinstead of falling back to 0.NEW_ARRAY_CLEARslots with typed zeroes.without dropping iterations.
Translator front-end and GC sync
ptr::writeuses.fuse_boxing_alloclowerslltype::malloc_typedtoNewWithVtablebutleaves the by-value Rust aggregate it consumed in the dominator, where
gc_interp's stable-allocation arm writes it out withcore::ptr::write.remove_simple_mallocsperforms the equivalent escape-sensitive motion.0-arg fold matched
[core|std, ptr, null|null_mut]literally, so a guestconst-global bound to the same
HostObject(
pyre_object::pyobject::PY_NULL→core.ptr.null_mut) kept its call. Nowresolved through
HOST_ENV::import_module/module_get, so the alias keepsone owner and the codewriter repeats no guest spelling.
Verification
cargo test --workspace --no-default-features --features dynasm— RC=0, 101suites ok.
cargo fmt --all --check— clean.python3 pyre/check.py --backend dynasm— 372 passed, 1 failed.The single red is
synth/raise_reg_unbound_jitstress(
loops_aborted 1 → 2, loops_compiled 6 → 8) and it is base-owned, notthis branch's.
origin/mainata8f3cdb36ed, built and run on the same host,misses its own committed baseline by exactly the same amount:
loops_compiledbridges_compiledloops_abortedguard_failuresorigin/maina8f3cdbThe two sides are identical, so the baseline is left at its committed value
rather than re-recorded here.
No
.jitstatsfile is touched by this branch.— authored by Claude