Skip to content

jit: fix two backend-independent jitstats floor reds (catch-landing exc_value, str_len_descr parent) - #995

Closed
youknowone wants to merge 2 commits into
mainfrom
jitstats-handler-liveness
Closed

youknowone wants to merge 2 commits into
mainfrom
jitstats-handler-liveness

Conversation

@youknowone

Copy link
Copy Markdown
Collaborator

Closes the two remaining backend-independent jit-stats floor reds on main, both by fixing JIT code to conform to the committed baselines — no baseline re-recording.

Commit 1 — list_length_hint_validate (catch-landing producerless exc_value)

The catch-landing branch in jit/codewriter.rs pushed the caught exception value with a producerless fresh_ref_value. Regalloc coalesced it onto a body colour (the local list r); on a guard-failure bridge that re-walks the handler, that colour's def sits in the skipped prefix, so the push reads unbound (RegisterReadUnbound{pc:456, reg:3} ×20) and the walk aborts.

Fix: reuse the landing's own last_exception value Variable (which has a generate_last_exc producer and is re-derived on any bridge), mirroring the existing PUSH_EXC_INFO twin.

Measured: list_length_hint_validate loops_aborted 34→14, guard_failures 4923→828 — exact match to baseline.

Commit 2 — exception_args_virtual (parentless const-str getfield)

str_len_descr() minted the len(str) FieldDescr parentless via make_immutable_field_descr. When a trace constant-folds len(const_str), protect_speculative_field fails closed on the missing parent (get_parent_descr() == None), signals InvalidLoop, the bridge is discarded and compile_loop gives up with ABORT_BAD_LOOP.

Fix: add W_UNICODE_DESCR_GROUP (vtable &STR_TYPE, type_id W_UNICODE_GC_TYPE_ID, single positional usize len field), mirroring the W_Int/W_Bool/W_Long single-field groups, and route str_len_descr through field_descr_from_group so the FieldDescr carries a parent SizeDescr whose subclassrange validates the folded constant. This restores PyPy's get_field_descr invariant (descr.py:218-239, parent_descr set unconditionally). No BhDescr/serde change, no LLBC re-extraction.

Measured: exception_args_virtual loops_aborted 3→0 on dynasm, cranelift and wasm.

Verification

  • Full synth jit-stats gate: 353/353 on dynasm, 353/353 on cranelift, zero collateral.
  • wasm: both fixtures verified (exception_args_virtual 3→0). pickle_terminal_raise_resume is provably inert to commit 2 (main and branch both read 59 on macOS; the committed wasm baseline 54 is the Linux-CI value, so the local +5 is a platform read, not a regression).
  • Codex parity review (scoped to the changed files): no regressions, no introduced mismatches; the str descr group's omitted GC slots and unsigned-usize length are intentional Rust-layout adaptations.

— commented by Claude

…sh Ref

The catch-landing exception-value push allocated a fresh Ref Variable with
no defining op. Regalloc treats a producerless Ref as dead-until-first-use
and coalesces it onto a body colour (a local live across the `try`). On a
guard-failure bridge that re-walks the handler, that body colour's def sits
in the skipped prefix, so the pushed value is read unbound
(RegisterReadUnbound), aborting the walk.

Reuse the landing's own last_exception value Variable (the edge pair set by
exception_landing_state, whose colour generate_last_exc writes) as the
pushed value. It is re-derived on any bridge and interferes with the
surrounding live ranges, so it earns a distinct colour. Same fix as the
PUSH_EXC_INFO last_exc_value re-read.

Restores list_length_hint_validate to its recorded jit-stats baseline
(loops_aborted 34->14, guard_failures 4923->828); raise_reg_unbound_jitstress
unchanged. dynasm synthetic gate: 352 passed, only exception_args_virtual red.

Assisted-by: Claude
str_len_descr minted the len(str) FieldDescr parentless via
make_immutable_field_descr. When a trace constant-folds len(const_str),
protect_speculative_field fails closed on the missing parent
(get_parent_descr() == None) and signals InvalidLoop; the bridge is
discarded and compile_loop gives up with ABORT_BAD_LOOP
(loops_aborted and guard_failures rise).

Add W_UNICODE_DESCR_GROUP (vtable &STR_TYPE, type_id
W_UNICODE_GC_TYPE_ID, single positional usize `len` field), mirroring
the W_Int/W_Bool/W_Long groups, and route str_len_descr through it so
the FieldDescr carries a parent SizeDescr whose subclassrange validates
the folded constant. Register the group in publish_runtime_descr_groups.

exception_args_virtual loops_aborted 3->0 on dynasm, cranelift and
wasm; full synth gate 353/353 on dynasm and cranelift, no collateral.

Assisted-by: Claude
@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Warning

Review limit reached

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

Next review available in: 20 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: b91db938-4a8c-4926-9773-e3dce5caf456

📥 Commits

Reviewing files that changed from the base of the PR and between 6fbef5f and 71bbde0.

📒 Files selected for processing (2)
  • pyre/pyre-jit-trace/src/descr.rs
  • pyre/pyre-jit/src/jit/codewriter.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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

https://github.com/youknowone/pyre/blob/71bbde0fd344dd29fbe721ea5bcca959d308cf0c/pyre-jit-trace/src/descr.rs#L861-L864
P1 Badge Publish the complete Unicode field layout

This registers a one-field SizeDescr as the authoritative parent for W_UnicodeObject, even though the translated object also has value, byte_len, w_slots, index_storage, and hash. Because publish_runtime_descr_groups runs before build-time descriptor rehydration and the nonzero vtable prevents the fuller descriptor from replacing it, those later field descriptors inherit a parent whose all_fielddescrs cannot represent their indices; optimizer paths such as virtual materialization/force_box then enumerate only len and can omit the remaining field stores and GC edges. Build the group from the complete object layout (using extra GC edges where appropriate) rather than relying on the current residual-allocation shortcut.

AGENTS.md reference: AGENTS.md:L194-L196

ℹ️ 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".

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 71bbde0).
Updated: 2026-08-03T01:10:38.923Z

Files in the reviewed diff
pyre/pyre-jit-trace/src/descr.rs
pyre/pyre-jit/src/jit/codewriter.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

  • pyre/pyre-jit-trace/src/descr.rs:861 ↔ rpython/jit/codewriter/heaptracker.py:51: the new W_UNICODE_DESCR_GROUP deliberately exposes only len and omits value, byte_len, w_slots, index_storage, and hash. Upstream get_size_descr() always populates both all_fielddescrs and gc_fielddescrs from the complete struct field census (rpython/jit/backend/llsupport/descr.py:123). Consequently, Pyre’s new parent SizeDescr has only len in all_fielddescrs and only inherited w_class as a GC edge, missing the managed value, w_slots, and index_storage edges.

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

None.

4. Structural adaptations

  • pyre/pyre-jit/src/jit/codewriter.rs:12969 ↔ rpython/flowspace/flowcontext.py:124: reusing the exception edge’s last_exc_value for the Python 3.11 handler value stack is a valid adaptation. Upstream represents exception delivery as last_exception/last_exc_value link extravariables; CPython 3.11 exception-table handlers additionally require the exception value on the operand stack.
  • pyre/pyre-jit-trace/src/descr.rs:866 ↔ pypy/objspace/std/unicodeobject.py:57: Pyre’s W_UnicodeObject uses Rust-owned WTF-8 storage and additional runtime slots, unlike PyPy’s _utf8/_length object. That layout difference is structural, but it does not justify omitting its fields from the SizeDescr census above.

@youknowone

Copy link
Copy Markdown
Collaborator Author

Superseded — both fixes already landed on main independently:

origin/main (949b530) already has both fixtures at their committed baselines, so this branch adds nothing (and would conflict on the duplicate W_UNICODE_DESCR_GROUP). Closing and deleting the branch.

— commented by Claude

@youknowone youknowone closed this Aug 3, 2026
@youknowone
youknowone deleted the jitstats-handler-liveness branch August 3, 2026 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant