majit: node virtualization for aheui linked-list nodes (~1.9x logo)#555
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (27)
WalkthroughThis change adds headerless nursery allocation metadata, IR lowering, runtime descriptor propagation, resume-heap preparation, and Dynasm backend support for x86_64 and AArch64. It also updates guard handling, trace teardown, compilation logging, and related tests. ChangesHeaderless allocation and descriptor propagation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 b905d1b). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
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: a72cd44895
ℹ️ 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".
| // subject to GUARD_GC_TYPE (which reads at `ref - GcHeader::SIZE`). | ||
| // Headerless is only used for monomorphic structs that are never | ||
| // type-guarded. | ||
| self.register_struct_layout(size, type_id, true, headerless, fields); |
There was a problem hiding this comment.
When headerless is true this still registers the layout as is_gc_managed = true, and the CanonicalBhDescr::Size emitted just below does the same. StructPtrInfo::make_guards gates GUARD_GC_TYPE only on sd.is_gc_managed(), so any escaped or loop-carried headerless node that reaches a short preamble can emit a type guard that reads the nonexistent header word at ref - GcHeader::SIZE, causing spurious guard failures or comparing payload bytes as a type id. Headerless nursery objects need either is_gc_managed = false for guard purposes or the guard path must also check headerless().
Useful? React with 👍 / 👎.
| OpCode::CallMallocNurseryHeaderless => { | ||
| unreachable!("headerless nursery alloc is aheui-dynasm-only"); | ||
| } |
There was a problem hiding this comment.
GcRewriterImpl::handle_new emits CallMallocNurseryHeaderless solely from the size descriptor, not from the selected backend, so a user who runs the same headerless_structs JIT with the cranelift backend will hit this unreachable! during compilation instead of compiling or falling back to a helper call. The new macro/configuration is backend-independent, so this needs a real cranelift lowering or an earlier backend-gated fallback before the rewritten op reaches cranelift.
Useful? React with 👍 / 👎.
| BinOp::And(_) => "IntAnd", | ||
| BinOp::Or(_) => "IntOr", |
There was a problem hiding this comment.
Mapping Rust &&/|| to IntAnd/IntOr changes semantics because lower_binary has already lowered both operands before this opcode selection. In a #[jit_interp] expression where the RHS has a residual call, field dereference, division/mod, or other side effect/trap that Rust would skip when the LHS determines the result, the generated JIT trace will evaluate it anyway and can raise or mutate state on paths the interpreter never executes. These operators need control-flow lowering, or should remain unlowered unless the RHS is proven safe to evaluate eagerly.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@majit/majit-backend-dynasm/src/aarch64/assembler.rs`:
- Around line 6141-6230: Extract the duplicated bump-allocation fast path and
slow-path spill/gcmap/restore logic from genop_call_malloc_nursery and
genop_call_malloc_nursery_headerless into a shared helper. Parameterize the
helper for header initialization/offset behavior and the slowpath address,
preserving the existing headerless behavior and the normal nursery header
epilogue while making both methods use the shared implementation.
In `@majit/majit-backend-dynasm/src/x86/assembler.rs`:
- Around line 8006-8081: Update genop_call_malloc_nursery_headerless to detect
zero nf/nt addresses before dereferencing the nursery pointers. When GC is
inactive, bypass the fast path and initialize rcx to the old nursery pointer and
rdx to rcx plus size before branching to the existing slow_path, preserving the
build_malloc_slowpath_headerless contract; retain the current fast-path behavior
for valid nursery addresses.
In `@majit/majit-metainterp/src/jitdriver.rs`:
- Around line 5311-5322: Factor the duplicated fallback allocator and liveness
setup used before both prepare_exit_resume_heap calls into a private helper on
the surrounding type, such as blackhole_allocator_and_liveness(). Update both
the should_bridge and legacy-fallback branches to use that helper while
preserving the existing NullAllocator fallback and liveness source.
- Around line 188-201: Update back_edge_internal’s guard-failure path to reuse
convert_rd_virtuals_for_storage instead of duplicating the
RdVirtualInfo-to-VirtualInfo mapping. Preserve the existing Option::Some
wrapping and pass the same ResumeStorage and raw value count used by the inline
conversion.
- Line 2199: Update the successful compile handling in the
TraceAction::CompileTrace arm to call finish_trace_live() instead of
abort_trace_live(false). Keep the existing abort-side handling for actual
failures and align this success path with the bridge success paths.
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Line 6744: Update the retrace compile-stat calculation around num_ops_before
so it measures the same trace slice represented by num_combined_ops, including
the saved preamble. Ensure the values passed to Logger::log_compile are derived
from consistent trace boundaries so reduction statistics compare matching
operation sets.
In `@majit/majit-metainterp/src/pyjitpl/dispatch.rs`:
- Around line 158-167: Update the make_simple_descr_group_keyed_with_headerless
call in the dispatch logic to pass p.headerless instead of hardcoding false,
preserving the parent layout and preventing incorrect descriptor caching for
subsequent New operations.
🪄 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
Run ID: 8bab0801-adaa-4b41-b007-1a5598fee999
📒 Files selected for processing (27)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/regalloc.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-backend-dynasm/src/x86/cpu_ext.rsmajit/majit-backend-dynasm/src/x86/reghint.rsmajit/majit-gc/src/rewrite.rsmajit/majit-ir/src/descr.rsmajit/majit-ir/src/resoperation.rsmajit/majit-macros/src/jit_interp/codegen_trace.rsmajit/majit-macros/src/jit_interp/jitcode_lower/helpers.rsmajit/majit-macros/src/jit_interp/jitcode_lower/lower_vable.rsmajit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rsmajit/majit-macros/src/jit_interp/jitcode_lower/mod.rsmajit/majit-macros/src/jit_interp/mod.rsmajit/majit-metainterp/src/jitcode/assembler.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/optimizeopt/heap.rsmajit/majit-metainterp/src/optimizeopt/virtualize.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/resume.rsmajit/majit-trace/src/logger.rsmajit/majit-translate/src/codewriter/assembler.rsmajit/majit-translate/src/codewriter/jitcode.rspyre/pyre-jit-trace/src/descr.rs
| /// Headerless fixed-size nursery allocation. `size` excludes any GC | ||
| /// header; result is the old nursery base. | ||
| fn genop_call_malloc_nursery_headerless(&mut self, op: &Op) { | ||
| let size_ref = op.arg(0).to_opref(); | ||
| let size = size_ref.inline_const_bits().unwrap_or_else(|| { | ||
| self.constants | ||
| .get(&size_ref.raw()) | ||
| .map(|c| c.as_raw_i64()) | ||
| .unwrap_or(size_ref.raw() as i64) | ||
| }); | ||
| let (nf_addr, nt_addr) = crate::runner::dynasm_nursery_addrs(); | ||
| if nf_addr == 0 || nt_addr == 0 { | ||
| self.emit_mov_imm64(0, size); | ||
| self.emit_mov_imm64( | ||
| 2, | ||
| crate::runner::dynasm_nursery_slowpath_headerless as *const () as i64, | ||
| ); | ||
| self.emit_malloc_slowpath_helper_call(2); | ||
| self.reload_frame_if_necessary(); | ||
| return; | ||
| } | ||
|
|
||
| self.emit_mov_imm64(2, nf_addr as i64); | ||
| dynasm!(self.mc ; .arch aarch64 ; ldr x0, [x2]); | ||
|
|
||
| if size < 4096 { | ||
| let sz = size as u32; | ||
| dynasm!(self.mc ; .arch aarch64 ; add x1, x0, sz); | ||
| } else { | ||
| self.emit_mov_imm64(1, size); | ||
| dynasm!(self.mc ; .arch aarch64 ; add x1, x0, x1); | ||
| } | ||
|
|
||
| self.emit_mov_imm64(3, nt_addr as i64); | ||
| dynasm!(self.mc ; .arch aarch64 ; ldr x3, [x3]); | ||
| dynasm!(self.mc ; .arch aarch64 ; cmp x1, x3); | ||
|
|
||
| let slow_path = self.mc.new_dynamic_label(); | ||
| let done = self.mc.new_dynamic_label(); | ||
| dynasm!(self.mc ; .arch aarch64 ; b.hi =>slow_path); | ||
|
|
||
| self.emit_mov_imm64(2, nf_addr as i64); | ||
| dynasm!(self.mc ; .arch aarch64 | ||
| ; str x1, [x2] // *nursery_free = new_free | ||
| ; b =>done | ||
| ); | ||
|
|
||
| dynasm!(self.mc ; .arch aarch64 ; =>slow_path); | ||
| let base_ofs = crate::jitframe::FIRST_ITEM_OFFSET as i32; | ||
| dynasm!(self.mc ; .arch aarch64 | ||
| ; stp x2, x3, [x29, base_ofs + 2 * 8] | ||
| ; stp x4, x5, [x29, base_ofs + 4 * 8] | ||
| ; stp x6, x7, [x29, base_ofs + 6 * 8] | ||
| ; stp x8, x9, [x29, base_ofs + 8 * 8] | ||
| ; stp x10, x11, [x29, base_ofs + 10 * 8] | ||
| ; stp x12, x13, [x29, base_ofs + 12 * 8] | ||
| ; stp x19, x20, [x29, base_ofs + 14 * 8] | ||
| ; stp x21, x22, [x29, base_ofs + 16 * 8] | ||
| ); | ||
| if let Some(gcmap) = self.pending_malloc_nursery_gcmap { | ||
| self.push_gcmap(gcmap as *mut usize); | ||
| } else { | ||
| let gcmap_ofs = crate::jitframe::JF_GCMAP_OFS as u32; | ||
| dynasm!(self.mc ; .arch aarch64 ; str xzr, [x29, gcmap_ofs]); | ||
| } | ||
| self.emit_mov_imm64(0, size); | ||
| self.emit_mov_imm64( | ||
| 2, | ||
| crate::runner::dynasm_nursery_slowpath_headerless as *const () as i64, | ||
| ); | ||
| self.emit_malloc_slowpath_helper_call(2); | ||
| self.reload_frame_if_necessary(); | ||
| let gcmap_ofs = crate::jitframe::JF_GCMAP_OFS as u32; | ||
| dynasm!(self.mc ; .arch aarch64 ; str xzr, [x29, gcmap_ofs]); | ||
| let base_ofs_r = crate::jitframe::FIRST_ITEM_OFFSET as i32; | ||
| dynasm!(self.mc ; .arch aarch64 | ||
| ; ldp x2, x3, [x29, base_ofs_r + 2 * 8] | ||
| ; ldp x4, x5, [x29, base_ofs_r + 4 * 8] | ||
| ; ldp x6, x7, [x29, base_ofs_r + 6 * 8] | ||
| ; ldp x8, x9, [x29, base_ofs_r + 8 * 8] | ||
| ; ldp x10, x11, [x29, base_ofs_r + 10 * 8] | ||
| ; ldp x12, x13, [x29, base_ofs_r + 12 * 8] | ||
| ; ldp x19, x20, [x29, base_ofs_r + 14 * 8] | ||
| ; ldp x21, x22, [x29, base_ofs_r + 16 * 8] | ||
| ); | ||
| self.emit_propagate_memory_error_if_null(0); | ||
|
|
||
| dynasm!(self.mc ; .arch aarch64 ; =>done); | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Near-duplicate of genop_call_malloc_nursery; extract shared bump-alloc emitter.
genop_call_malloc_nursery_headerless (6143-6229) repeats the entire fast-path bump/compare sequence and the ~40-line slow-path register-spill/gcmap/restore/null-propagate block from genop_call_malloc_nursery (6006-6139) almost verbatim. The only real deltas are: no header zero (str xzr,[x0])/no header-size offset, and the slowpath helper address (dynasm_nursery_slowpath_headerless vs dynasm_nursery_slowpath). Since this is now a hot path for the aheui Node allocation optimization, a future fix to the spill/gcmap sequence in one copy risks silently not being applied to the other.
Consider factoring the common bump-allocate + slow-path-spill/gcmap/restore skeleton into a shared helper parameterized by (zero_header: bool, slowpath_addr: i64) (or a small closure for the header-specific epilogue), called from both genop_call_malloc_nursery and genop_call_malloc_nursery_headerless.
🤖 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 `@majit/majit-backend-dynasm/src/aarch64/assembler.rs` around lines 6141 -
6230, Extract the duplicated bump-allocation fast path and slow-path
spill/gcmap/restore logic from genop_call_malloc_nursery and
genop_call_malloc_nursery_headerless into a shared helper. Parameterize the
helper for header initialization/offset behavior and the slowpath address,
preserving the existing headerless behavior and the normal nursery header
epilogue while making both methods use the shared implementation.
…const-path values - pool_arrays accepts an optional element type `<base> => <getter> -> T`; the getter's ref binding carries T as its struct_type, so field access on a local `let x = <getter>(...)` binding lowers to getfield_gc/setfield_gc instead of aborting. - opcode_for_binop maps logical `&&`/`||` to IntAnd/IntOr (boolean operands hold 0/1, evaluated unconditionally, as lower_match_stmt already does). - lower_value_expr lowers a SCREAMING_CASE symbolic const path to load_const_i via `#path as i64`; a lowercase non-binding path stays unlowerable. Assisted-by: Claude
log_compile was never called on the compile-success path, so the "Traces compiled" counter stayed 0 regardless of how many loops compiled. Wire warm_state.log_compile at both compiled_loops.insert sites (main loop and retrace/FINISH), sourcing op counts from the optimizer input/output op vectors. Bridge-compile success reused abort_trace_live for teardown, which bumped the JIT logger abort counter and cell.abort_count. Add finish_trace_live: same live cleanup as abort_trace_live but clears the TRACING flag via finish_tracing instead of abort_tracing, so no abort accounting fires. Call it from the two BridgeCompileResult:: Compiled teardown edges. summary() now also prints "Traces bridged". Assisted-by: Claude
…-guard Add OpCode::CallMallocNurseryHeaderless. When a New's SizeDescr is marked headerless, the GC rewriter lowers it to a nursery bump of round_up(size) with no GcHeader prefix and no tid init; the aarch64 and x86 dynasm backends emit a base-return 16-byte bump (no header, no field zero), and cranelift carries an explicit unsupported arm. A headerless bit is threaded through SizeDescr, new_struct, and the jit_interp `headerless_structs` macro config; it defaults false so existing (headered) New lowering is unchanged. Add FieldDescr::force_virtual_at_guard, set by the jit_interp `force_at_guard` macro config. In force_lazy_sets_for_guard, a flagged field whose lazy-set RHS is a virtual is materialized at the guard instead of being deferred to rd_pendingfields. Assisted-by: Claude
…aths The bridge and legacy fallback deopt paths applied guard pending fields via materialize_pending_fields, which reads raw_values[slot] directly without materializing rd_virtuals, so a pending field whose value is a never-forced virtual resolved to garbage. Factor the virtuals-then-pendingfields step out of blackhole_from_resumedata into prepare_resume_heap (prepare_virtuals + push_resume_ref_roots + prepare_guard_pendingfields, wrapped in ResumeRefRootsScope) and call it from both paths before recover, so virtual pending-field targets are reconstructed and rooted across a mid-reconstruction collect. Non-virtual pending fields decode to the same value the previous raw_values read produced. Assisted-by: Claude
Address latent robustness findings from the node-virt parity review; no behavior change for the current aheui NodeJit path (byte-id identical). - descr reconstruction: thread the headerless flag through BhSizeSpec so the inverse trace-side path (bh_size_spec_from_descr, make_descr_from_bh cache miss) rebuilds a headerless SizeDescr instead of silently minting a GC-headered one. Route a headerless owner marker through the empty-field branch too. - rewrite: debug_assert the headerless NEW size is nursery-eligible, since headerless has only the inline nursery path (no large-alloc fallback). - force_at_guard: key the producer and consumer on the full canonical struct path rather than the last segment, so distinct same-named structs do not collide. - assembler: correct the stale is_gc_managed comment to describe headerless structs (rooted and traced but carrying no GcHeader, never GUARD_GC_TYPE). Assisted-by: Claude
… non-headerless Address the P1 findings from the PR #555 review. - force_virtual_at_guard: the per-field guard-force flag had no remaining user after the node-virt head-defer landed, so the heap.rs virtual arm always deferred to pendingfields anyway. Remove the flag and its whole plumbing (descr, macro `force_at_guard` config, lower_value/lower_vable emission, dispatch, translate, pyre-jit-trace) so the lazy-set collection matches heap.py exactly: a virtual-valued lazy store is always appended to pendingfields. - GUARD_GC_TYPE: StructPtrInfo.make_guards gated the guard only on is_gc_managed, so a headerless struct (is_gc_managed = true for rooting, but no GcHeader) could emit a type guard that reads a type word at `ref - GcHeader::SIZE`. Gate on `is_gc_managed() && !headerless()`. - Document why the headerless nursery-alloc branch safely skips clear_gc_fields (every ref field is initialized inline before any can-collect op; the fast path emits no gcmap). Assisted-by: Claude
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00baa91f6c
ℹ️ 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".
| 16, | ||
| 0xCD, | ||
| false, | ||
| &[(0, false, "value", false), (8, true, "next", false)], |
There was a problem hiding this comment.
In tests that build new_struct with fields, these entries now have four tuple elements, but JitCodeBuilder::new_struct still accepts fields: &[(usize, bool, &str)]. This makes cargo test -p majit-metainterp fail to compile before any tests run; the same mismatch appears again in the inline-const test below, so either drop the extra bools here or thread the new field through the builder API.
Useful? React with 👍 / 👎.
| if let Some(base) = try_headerless_nursery_bump(gc, size) { | ||
| return base; |
There was a problem hiding this comment.
When a trace using headerless_structs is compiled with the cranelift backend, this helper bypasses GcAllocator::alloc_nursery_headerless and directly bumps whatever active GC exposes nursery pointers. With the normal headered MiniMark/GcHandle this silently places a headerless payload in a nursery the collector later scans as headered, and after a failed post-collection bump it even falls back to alloc_zeroed outside the GC; use the headerless-aware allocator path (or decline/fail) so misconfigured or full nurseries do not produce untraceable objects.
Useful? React with 👍 / 👎.
The x86 and aarch64 genop_call_malloc_nursery_headerless fast paths raw-bump dynasm_nursery_addrs() inline, which is correct only when the active dynasm GC is headerless-aware (its nursery yields a raw base with no GcHeader). Document that invariant at both emit sites, matching the overflow slowpath which already enforces it via alloc_nursery_headerless's panicking default. Comment-only. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80808d2e84
ℹ️ 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".
| self.emit_mov_imm64(2, nf_addr as i64); | ||
| dynasm!(self.mc ; .arch aarch64 ; ldr x0, [x2]); |
There was a problem hiding this comment.
On aarch64, MALLOC_NURSERY_CLOBBER only reserves x0/x1, so regalloc may keep live boxes in x2 or x3 across CallMallocNurseryHeaderless. This fast path overwrites x2 with nf_addr here and later overwrites x3 before the overflow branch; on the slow path those scratch values are then saved into the jitframe as if they were the live registers, and on the fast path the live values are simply lost. Any headerless allocation with a live value assigned to x2/x3 can therefore corrupt state or GC roots; use non-allocatable scratch registers or include x2/x3 in the clobber set for this op.
Useful? React with 👍 / 👎.
The jitcode_new_then_field_round_trip and jitcode_setfield_gc_i_c dispatch tests passed 4-element field tuples `(0, false, "value", false)` to JitCodeBuilder::new_struct, whose `fields` parameter is `&[(usize, bool, &str)]`. Drop the stray 4th element so the tuples match the 3-element signature (as the production jitcode_lower macro and the other call sites already do), fixing an E0308 that broke `cargo test --all` (dynasm and cranelift) on all platforms. Assisted-by: Claude
1 participant
Summary
Node virtualization for the aheui tracing JIT: the transient push/pop linked-list
Nodeallocations that never escape a trace are folded away instead of emitting aresidual allocation call per push. On the logo benchmark this is a ~1.9x
wall-clock speedup (independently corroborated by a −47.6% IR reduction,
39210 → 20564 optimized ops), byte-identical output vs the naive interpreter.
The branch carries the 3-commit epic plus two supporting majit commits.
The node-virt epic (3 commits)
The aheui
Node(#[repr(C)] { value @0, next @8 }, 16B, headerless — noGcHeader) is allocated on everyOP_PUSH/OP_DUP/OP_MOV. Previously each was aresidual ref-returning call; now the trace emits an
OpCode::New+ field storesthat the optimizer folds when the node does not escape.
Three pieces of majit machinery make this correct:
Headerless nursery-alloc opcode (
1d8db58) — a newOpCode::CallMallocNurseryHeaderlessthreaded through the descr/rewriter/regallocand the aarch64 + x86 dynasm backends: a 16B inline nursery bump that returns the
raw base (no
GcHeader, no header-zeroing), because the aheui copying collectorassumes headerless 16B node slots. The stock headered
CallMallocNurseryreservesand returns
base + GcHeader::SIZE, which would corrupt the node layout. cranelift(non-default backend) lowers the tagged op as an ordinary call. A per-field
force_virtual_at_guardflag lets a flagged field's virtual RHS be materialized ata guard rather than deferred.
Bridge/legacy deopt-path reconstruction (
a5f8117) — the bridge and legacyfallback deopt paths applied guard pending fields via
materialize_pending_fields,which reads
raw_values[slot]directly with nord_virtualsmaterialization,so a pending field whose value is a never-forced virtual resolved to garbage. The
virtuals-then-pendingfields step is factored out of
blackhole_from_resumedatainto
prepare_resume_heap(prepare_virtuals+push_resume_ref_roots+prepare_guard_pendingfields, wrapped inResumeRefRootsScope) and called fromboth paths before recover, so virtual pending-field targets are reconstructed and
GC-rooted across a mid-reconstruction collect. Non-virtual pending fields decode to
the same value the previous
raw_valuesread produced (behavior-neutral for pyre).Parity-review robustness fixes (
a72cd44) — preserve the headerless flagthrough the inverse descr-reconstruction path (
BhSizeSpec.headerless), keyforce_at_guardon the full canonical struct path (producer + consumer),debug_assertthat a headerlessNEWsize is nursery-eligible, and correct astale
is_gc_managedcomment. No behavior change (see Parity review below).The concrete aheui-side activation (the
NodeJitstruct literals +struct_allocs/headerless_structsconfig) lives in the separate aheui backend repo; this PR is thepyre-side infrastructure.
Supporting commits
307e8bc— jit_interp lowering: pool-array element type, logical&&/||, andconst-path values.
f02d628— fix theMAJIT_STATStrace counters (compiled / aborted / bridged) sothe diagnostics report real trace outcomes.
Parity review
A scoped Codex parity review (
b820a15d823..HEADvs upstream RPython/PyPy) foundno parity regressions (§1 = 0). Of 6 new mismatches: 4 fixed in
a72cd44, 1false positive (headerless slowpath has a dedicated base-returning path), 1
reclassified as an intentional cranelift adaptation. One pre-existing mismatch
(
force_from_resumedatalacks aResumeRefRootsScope) is tracked as a follow-up; itis not on any path this change introduces. The 3 structural adaptations (new
headerless opcode / explicit resume-cache rooting / per-field force-at-guard flag) are
valid RPython↔Rust/GC-model gaps and documented in-code.
Verification
pyre/check.py: dynasm 181/181, cranelift 181/181, wasm 180/180.7fcdbfff0af449c4283c008e3ca317ce,jit output identical to
--no-jit(release + debug + small-NURSERY_SIZEforced-collect).across all deopt paths.
majit-backend-dynasmtest corpus green;cargo fmtclean.— commented by Claude
Summary by CodeRabbit
New Features
Bug Fixes
ANDandORlowering behavior.