Trace w_int_new's allocation again, and cover fuse_boxing_alloc's cross-block walk - #1159
Merged
Conversation
…_ABORT `OrthodoxSubWalkTraceUnsupported` carries the pc but the three decline arms dropped it. The identifier the decline names elsewhere is a symbolic fnaddr minted in `pyre-jit-trace/build.rs`, so a runtime reverse-name registry has nothing to read; the pc is what pairs the decline with a bytecode offset. Assisted-by: Claude
#1131 put a `dont_look_inside` boundary around the int box's allocating tail (`w_int_box_slow`) because the sub-jitcode walk declined on the `SyntheticTransparentCtor` the `malloc_typed` arm lowered to, and named its own exit condition: "Drop the boundary once the fusion resolves a vtable there." #1141 landed that resolution -- `fuse_boxing_alloc` now follows the header pointers across the block boundary each call ends -- and its comment records the condition met, "it does now fire here", while keeping the boundary. `wrapint` (`objspace/std/intobject.py:903-921`) carries no `@dont_look_inside`; its comment reads "this whole function is getting inlined into every caller", and it allocates with `instantiate(W_IntObject)` then `w_res.intval = x`, the alloc-then-init pair the rtyper lowers to `new_with_vtable` + `setfield_gc`. Put the `malloc_typed` arm back in `w_int_new` where the fusion rewrites it into that pair, and keep the collector-heap arm as `w_int_gc_alloc` behind its own boundary: that arm carries a blocker that is still real, the wasm backend not lowering the offset-0 `ob_type` store faithfully. `bench/synth/list_pop_append` does not decide this. It reads the same either way (2.6/2.3/2.6 against 2.5/2.5/2.3, three runs each), and #1141 records a negative control -- boundary removed with the fusion reverted -- that failed to reproduce the regression the boundary was added for, so the bench is uninformative in both directions. The trace shape is what differs and it is observable: with this change the compiled loop carries a `NewWithVtable` whose descr is `W_IntObject.intval` and no residual call to any `w_int_*` boxing symbol. A residual call can never be virtualized. check.py ALL PASSED on all three backends -- dynasm 417/417, cranelift 416/416, wasm 412/412 -- and cargo test --workspace green. The box is still not virtualized away, for a reason outside this change: `orthodox_list_append_commit` deliberately forces the value with a ptr->int->ptr identity pair so the descended sub-walk reads the current iteration's payload, and that force lands before the class guard that would otherwise fold. Assisted-by: Claude
`resolve_addr` steps through `Block.inputargs` and requires every predecessor to agree, but every `fuse_boxing_alloc` case built its cluster in a single block, so neither behaviour was reached by a test: the only case added with the walk is a decline that resolves inside one block. Four rows over the same one-payload `W_FloatObject` cluster, differing only in where the header values come from: one relay block between the producer and the ctor, two relay blocks, two predecessors of a merge block naming one type, and two naming different types. Each asserts the fused count, the address stamped on the `NewWithVtable`, and whether the `malloc_typed` survives as a residual. Measured by ablation on this tree: returning `None` for a phi fails the "one link crossing" row, and dropping the disagreement arm fails the "predecessors naming two types" row. The six pre-existing `fuse_boxing_alloc` tests pass under both ablations. Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe change separates GC integer allocation from fallback allocation, updates the JIT function-address binding, adds boxing-fusion regression coverage for split clusters, and logs unsupported list sub-walk locations during specialization fallback. ChangesInteger allocation and JIT behavior
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 |
![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
https://github.com/youknowone/pyre/blob/6d24dfebe251a293d4a28b4e03f56627b46be91b/pyre-object/src/intobject.rs#L157-L158
With the normal configuration (PYRE_GC_INTERP unset), gc_interp::enabled() returns true by default (gc_interp.rs:188-219), and the JIT installs the stable-allocation hook (pyre-jit/src/eval.rs:4114-4120), so this branch calls the #[dont_look_inside] w_int_gc_alloc and ordinarily returns its non-null result. Consequently, the newly inline malloc_typed block is reached only with the rollback flag or allocation failure and default JIT integer boxing remains unvirtualizable; compared with the previous single residual w_int_box_slow, it now also residualizes the enabled() check. The default collector path itself must lower to the traceable allocation shape for this parity change to take effect.
AGENTS.md reference: AGENTS.md:L231-L233
ℹ️ 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".
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 6d24dfe). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
This was referenced Aug 12, 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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three commits on top of
origin/main.jit: print the orthodox list sub-walk decline pc under PYRE_FBW_DEBUG_ABORTOrthodoxSubWalkTraceUnsupportedcarried a pc that never reached a log line, sothe two list sub-walk declines were indistinguishable from every other abort.
Two
[decline-why]prints behind the existingPYRE_FBW_DEBUG_ABORTgate.intobject: trace w_int_new's allocation again, as wrapint doesw_int_newrouted both its boxing arms through a#[dont_look_inside]w_int_box_slow, which residualises the call and so makes the boxunvirtualizable by construction. The oracle,
intobject.py:903-921 wrapint,carries no
@dont_look_inside— its comment at:908-910says the function"is getting inlined into every caller" — and allocates then initialises
(
:913-920), which isnew_with_vtable+setfield_gc.The boundary's own stated blocker was that the fusion could not resolve the
header pointers across the block boundary each call ends. #1141 fixed exactly
that, so the arm is traced again. The collector arm keeps its boundary as
w_int_gc_alloc, scoped to the one deviation that is still real (the wasmbackend's offset-0 field store).
Measured on this base: the compiled
lst.append(i); lst.pop()loop emitsNewWithVtable(size 24,type_id1) withSetfieldGc … descr=W_IntObject.intvaland no residual
w_int_*boxing call.synth/list_pop_append2.6x / 3.3x /2.4x — the bench does not discriminate this mechanism in either direction, and
the commit message says so; the decision rests on the shape.
majit: test fuse_boxing_alloc across the links a split cluster crossesresolve_addr(added by #1141) steps throughBlock.inputargsand takes ananswer only when every predecessor agrees. Neither behaviour was reached by a
test: the case added with it, and all six that predate it, build their cluster
in a single block, so they resolve without entering the phi arm.
Four rows over one
W_FloatObjectcluster, differing only in where the headervalues come from — one relay block, two relay blocks, two predecessors of a
merge naming one type, two naming different types. Each asserts the fused count,
the address stamped on the
NewWithVtable, and whether themalloc_typedsurvives as a residual. Asserting the address is what separates "walked to the
predecessor" from "read some other constant in the graph".
Non-vacuity measured by ablation on this tree:
resolve_addrreturnsNonefor a phiThe diff is a 196-line addition inside
mod testswith no deletions;resolve_addris unchanged.Verification
check.py: dynasm 417/417, cranelift 416/416, wasm 412/412 (HEADpinnedidentical before and after the run).
cargo test --workspace: 106 suites, 7623 passed, 0 failed.An earlier commit on this branch ported locale-aware
'n'formatting; it wasdropped on rebase because #1147 landed a superset of it.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Diagnostics