Repository navigation
jit: give W_UnicodeObject.len a parent SizeDescr; read the trailing -live- first for after-residual-call guards - #989
Merged
Merged
Conversation
`str_len_descr()` built its FieldDescr through `make_immutable_field_descr`, which leaves `parent_descr` as `None`. `protect_speculative_field` asks the field for that parent before constant-folding a pure length read and fails closed when it is absent, so the fold raised a SpeculativeError, `optimize_peeled_loop` turned it into `InvalidLoop`, and the bridge was rejected. The rejected bridge left a procedure token at the greenkey, and the next `compile_loop` there hit `has_compiled_targets` and aborted. `descr.py:238` sets `fielddescr.parent_descr = get_size_descr(...)` for every field descriptor it builds. Publish the Unicode layout as a `PyreObjectDescrGroup` and take the length field from it, so the field keeps its parent alive. synth/exception_args_virtual: loops_aborted 3 -> 0, guard_failures 1002 -> 401. synth/list_length_hint_validate: loops_aborted 34 -> 14, guard_failures 4923 -> 828. Both back at their committed baselines, 3/3 runs each. Assisted-by: Claude
`walker_capture_snapshot_for_last_guard_impl` picked the resume marker for an after-residual-call guard from the Python-PC metadata twins first, and only fell back to the bytecode's immediately trailing `-live-`. Inside a try block that twin can name a later normal-path marker, past `catch_exception/L`, whose liveness omits the handler-only locals. `pyjitpl.py capture_resumedata(after_residual_call=True)` snapshots the `-live-` that immediately follows the residual call. Read that marker first and keep the metadata twins as fallbacks for bodies that lack it. Extracted as `after_residual_guard_marker` so the selection order is testable. Assisted-by: Claude
WalkthroughThe PR adds cached Unicode object descriptors and makes string length use the canonical descriptor field. It also centralizes residual-guard marker lookup so the immediate trailing ChangesUnicode descriptor canonicalization
Residual guard marker selection
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 672c88d). 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-jit-trace/src/descr.rs`:
- Around line 882-945: Update W_UNICODE_DESCR_GROUP to use
build_object_descr_group_with_extra_gc_edges, supplying w_slots as an additional
GC edge alongside the existing descriptor edges. Add an assertion verifying that
gc_fielddescrs() includes UNICODE_W_SLOTS_OFFSET, ensuring the Unicode object’s
w_slots field is tracked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7a9e8e4a-17ef-4f69-aa89-249f8c6ce15b
📒 Files selected for processing (3)
pyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
Comment on lines
+882
to
+945
| static W_UNICODE_DESCR_GROUP: LazyLock<PyreObjectDescrGroup> = LazyLock::new(|| { | ||
| build_object_descr_group_with_def_path( | ||
| pyre_object::unicodeobject::W_UNICODE_OBJECT_SIZE, | ||
| W_UNICODE_GC_TYPE_ID, | ||
| &pyre_object::pyobject::STR_TYPE as *const _ as usize, | ||
| &[ | ||
| ( | ||
| "value", | ||
| pyre_object::unicodeobject::UNICODE_VALUE_OFFSET, | ||
| std::mem::size_of::<*mut rustpython_wtf8::Wtf8Buf>(), | ||
| Type::Ref, | ||
| false, | ||
| true, | ||
| false, | ||
| ), | ||
| ( | ||
| "byte_len", | ||
| pyre_object::unicodeobject::UNICODE_BYTE_LEN_OFFSET, | ||
| std::mem::size_of::<usize>(), | ||
| Type::Int, | ||
| false, | ||
| true, | ||
| false, | ||
| ), | ||
| ( | ||
| "len", | ||
| pyre_object::unicodeobject::UNICODE_LEN_OFFSET, | ||
| std::mem::size_of::<usize>(), | ||
| Type::Int, | ||
| false, | ||
| true, | ||
| false, | ||
| ), | ||
| ( | ||
| "w_slots", | ||
| pyre_object::unicodeobject::UNICODE_W_SLOTS_OFFSET, | ||
| std::mem::size_of::<pyre_object::PyObjectRef>(), | ||
| Type::Ref, | ||
| false, | ||
| false, | ||
| false, | ||
| ), | ||
| ( | ||
| "index_storage", | ||
| pyre_object::unicodeobject::UNICODE_INDEX_STORAGE_OFFSET, | ||
| std::mem::size_of::<*mut pyre_object::rutf8::Utf8IndexStorage>(), | ||
| Type::Ref, | ||
| false, | ||
| false, | ||
| false, | ||
| ), | ||
| ( | ||
| "hash", | ||
| std::mem::offset_of!(pyre_object::unicodeobject::W_UnicodeObject, hash), | ||
| std::mem::size_of::<i64>(), | ||
| Type::Int, | ||
| true, | ||
| false, | ||
| false, | ||
| ), | ||
| ], | ||
| "W_UnicodeObject", | ||
| "unicodeobject::W_UnicodeObject", | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Register W_UnicodeObject.w_slots as a GC edge.
build_object_descr_group_with_extra_gc_edges adds only W_CLASS_FIELD_DESCR unless the caller supplies extra edges. This group includes w_slots: PyObjectRef but supplies none. A JIT-materialized Unicode object can therefore retain an uncleared w_slots pointer.
Build this group with the extra-GC-edge variant and include the w_slots field. Add an assertion that W_UNICODE_DESCR_GROUP exposes UNICODE_W_SLOTS_OFFSET through gc_fielddescrs().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-jit-trace/src/descr.rs` around lines 882 - 945, Update
W_UNICODE_DESCR_GROUP to use build_object_descr_group_with_extra_gc_edges,
supplying w_slots as an additional GC edge alongside the existing descriptor
edges. Add an assertion verifying that gc_fielddescrs() includes
UNICODE_W_SLOTS_OFFSET, ensuring the Unicode object’s w_slots field is tracked.
This was referenced Aug 2, 2026
youknowone
added a commit
that referenced
this pull request
Aug 4, 2026
…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
youknowone
added a commit
that referenced
this pull request
Aug 4, 2026
…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
youknowone
added a commit
that referenced
this pull request
Aug 4, 2026
…e per-frame bridge state, execute recorded allocations (#1024) * optimizer: read the descr's own type id in make_guards instead of falling 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 * jit: drop the standalone typeptr field descr and the parameters that 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 * optimizer: seed virtual NEW_ARRAY_CLEAR slots with typed zeroes * gc: skip root forwarding queries before mutators are shared * gc: repair sync state after main rebase * jit: restore per-frame ref stack state across bridges * translator: derive field indices from heaptracker * jit: trace unsigned random conversion without dropping iterations * jit: preserve subwalk abort frame coordinates * jit: inline specialised float tuple unpack * translate: resolve the ptr-null call fold through the host builtin registry 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 * translate: rematerialize fused boxing aggregates at raw ptr::write uses `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 * jit: keep transparent helper 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 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 * jit: execute the new/new_with_vtable allocation while recording `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 * translator: keep the object header out of the positional field census `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 * descr: correct the "pyre has no heaptracker" comments 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
2 of 79 tasks
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.
Closes the two
loops_abortedjit-stats regressions that were red onmain:Both trace back to a single parentless field descriptor.
Root cause
The
abort: bad loopis the consequence; the bridge rejection is the cause.Instrumenting
protect_speculative_fieldpinned every failure to one descr:GetfieldGcI off=32 always_pure=true no parent_descr.PyObjectis 16 bytes(
ob_type@0,w_class@8), soW_UnicodeObjectlays outvalue@16,byte_len@24,len@32 — that isstr_len_descr().descr.py:238setsfielddescr.parent_descr = get_size_descr(gccache, STRUCT, vtable)for every field descriptor it builds, unconditionally, and creates thesize descr if it is not cached yet. The pyre side had no parent to hand back,
so a check that cannot fail upstream failed here.
What changed
pyre-jit-trace/src/descr.rs: publish theW_UnicodeObjectlayout as aPyreObjectDescrGroupand take the length field from it, so the field keepsits parent
SizeDescralive.pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs: read the bytecode'simmediately trailing
-live-first when choosing the resume marker for anafter-residual-call guard, keeping the Python-PC metadata twins as fallbacks.
pyjitpl.py capture_resumedata(after_residual_call=True)snapshots thatmarker; inside a try block a fallthrough twin can instead name a later
normal-path marker, past
catch_exception/L, and lose the handler-onlylocals. Extracted as
after_residual_guard_markerso the order is testable.Verification
cargo fmt --all -- --checkclean.cargo test -p pyre-jit-trace --no-default-features --features dynasm: 336 passed, 0 failed, includingtwo new tests —
str_len_descr_keeps_its_unicode_parent_aliveandafter_residual_guard_uses_trailing_live_before_fallthrough_twin.jit-stats, dynasm, 3/3 runs in each direction — fully deterministic, not a
flake. The "before" column was measured in an isolated detached worktree at the
pre-fix commit with its own
CARGO_TARGET_DIRand its own LLBC extraction:synth/exception_args_virtualloops_aborted=3 guard_failures=1002loops_aborted=0 guard_failures=401loops_aborted=0 guard_failures=401synth/list_length_hint_validateloops_aborted=34 guard_failures=4923loops_aborted=14 guard_failures=828loops_aborted=14 guard_failures=828bridges_compiledmoves with them (2→2and3→4), which is the same storyread from the other side: the rejected bridge is restored, and the guard that
kept failing without it stops.
Refuted hypothesis
pyre's
optimizeopt/mod.rs protect_speculative_operationgates the getfieldbranch on
opnum.is_getfield(), whereoptimizer.py:829usesOpHelpers.is_pure_getfield(opnum, descr)— opcode anddescr.is_always_pure(). That is a real parity gap, but it is not this bug:the failing descr probes
always_pure=true, so the upstream gate admits ittoo. Left alone here.
Follow-up not taken
ob_type_descr()(descr.rs) isimmutable: truewithparent_descr: None—the same class, and likewise exposed to a pure fold.
OB_TYPE_OFFSET == 0never reached the probe in these fixtures, so it is dormant rather than live,
and fixing it is out of scope for this PR. The other three
parent_descr: Nonesites (
PYFRAME_VABLE_TOKEN_OFFSET,array_lendescr_at_offset,new_w_class_field_descr) areimmutable: falseand so never pass thepure-getfield gate; the arraylen descr legitimately has no parent upstream
either (
descr.py:265).Summary by CodeRabbit
Bug Fixes
Tests