#268 bignum nursery/GC representation: kill the malloc_raw leak + bound within-run RSS - #294
Conversation
|
Warning Review limit reached
More reviews will be available in 55 minutes and 42 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughThis PR routes ChangesGC-managed BigInt allocation and JIT trace improvements
CI workflow consolidation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
0af6578 to
1347044
Compare
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 1903f8c). 1. 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 adaptationsNone. |
![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: 0af65788ff
ℹ️ 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".
| let new_op = ctx.record_op_with_descr(OpCode::NewWithVtable, &[], size_descr); | ||
| ctx.heap_cache_mut().new_object(new_op); | ||
| let value_idx = value_descr.index(); | ||
| ctx.record_op_with_descr(OpCode::SetfieldGc, &[new_op, bigint_ref], value_descr); | ||
| ctx.heapcache_setfield_cached(new_op, value_idx, bigint_ref); | ||
| new_op |
There was a problem hiding this comment.
When a JIT-created bigint result escapes and later goes through normal attribute lookup (for example x.bit_length()), baseobjspace.rs uses the raw (*obj).w_class to drive MRO lookup and skips method resolution when it is null. The previous jit_bigint_result_box -> w_long_from_raw path stamped w_class with the canonical int type, but this new inline box only writes value, so materialized longs from compiled bigint ops can lose their int methods even though type(x) still has a fallback.
Useful? React with 👍 / 👎.
| while let Some(obj_addr) = self.young_objects_with_destructors.pop() { | ||
| let hdr_ptr = (obj_addr - GcHeader::SIZE) as *const GcHeader; | ||
| if !unsafe { (*hdr_ptr).is_forwarded() } { | ||
| // Dead: run the destructor before the nursery reset frees |
There was a problem hiding this comment.
For a destructor-bearing nursery object that has been pinned, copy_nursery_object deliberately leaves it in the nursery without installing a forwarding pointer, but this destructor pass treats every non-forwarded entry as dead. Calling GcAllocator::pin on such an object therefore runs its destructor while the object is still live, which can invalidate its payload and then run the destructor again when it is eventually reclaimed; the check needs to exclude pinned survivors before calling the destructor.
Useful? React with 👍 / 👎.
Add an optional per-type destructor the collector runs when an object is reclaimed, mirroring incminimark's young/old_objects_with_destructors (incminimark.py:2884-2912). The copying nursery otherwise abandons dead objects without running Drop, so a payload owning non-GC heap memory leaks it. - trace.rs: DestructorFn type alias; TypeInfo.destructor field (added to all ctor literals); TypeInfo::with_destructor ctor; T_HAS_DESTRUCTOR infobit (0x800000, bit 23); encode_type_shape sets it. - collector.rs: young/old_objects_with_destructors lists; register_destructor_if_needed at the nursery alloc sites and direct old-gen registration in alloc_in_oldgen; run_destructor (type_id -> TypeInfo.destructor lookup); deal_with_young_objects_with_destructors after the young weakref pass in do_collect_nursery (dead -> run, survivor -> promote to old list); deal_with_old_objects_with_destructors before oldgen.sweep in finish_incremental_cycle (VISITED -> keep, else -> run). - 5 unit tests (nursery death, survival-then-oldgen-death, no double-count across minors, direct-oldgen-alloc death, no-destructor).
…raw — no GC header, never freed). Route the payload through the GC so dead bigints are reclaimed by collections instead of leaking. - longobject: alloc_bigint_nursery / alloc_bigint_stable allocate the BigInt payload via the GC hooks (nursery / old-gen stable), falling back to malloc_raw when no hook is installed. w_long_from_raw pins the young payload across the wrapper alloc and write-barriers the store. A BIGINT GC type with a lightweight destructor (drop_in_place) frees the external malachite Vec when the payload dies. - eval: register W_LONG with its value field as a gc-pointer offset; register the BIGINT destructor type and publish its runtime type id before freeze_types. - descr / helpers: W_LongObject.value field descr (gc field) and emit_box_long_inline (NewWithVtable(W_LONG) + setfield_gc(value)), mirroring emit_box_int_inline. - walker binary_op long: guard each operand against LONG, read each immutable value via GetfieldGcPure, call the elidable rbigint op on the bare *const BigInt payloads (jit_bigint_add/sub/mul/and/or/xor, floordiv/mod/ lshift/rshift), then box the result inline behind a GuardFalse(fits_int) demote guard. Passing the payloads (not the W_LongObject wrappers) keeps the elidable call pure on the immutable bigints, so the optimizer forwards the field read and never reorders the call ahead of the boxing setfield_gc that initializes a fresh result wrapper (the function-loop unroll exposed exactly that reorder, reading an uninitialized payload). - raw helpers split into payload-level (jit_bigint_*) used by the walker-emitted call and wrapper-level (jit_w_long_*_raw) used for record-time concrete evaluation and the trait path. The inline NewWithVtable lowers to the collecting CallMallocNursery, the per-iteration safepoint bigint-heavy loops previously lacked; dead wrappers and their nursery bigint payloads are reclaimed at minor collections.
… the nursery once and spilled the rest to old-gen unbounded (a single fib(100000) peaked ~316MB). Route the runtime payload helpers through a collecting nursery alloc and charge each result's external limb-Vec bytes as memory pressure, so minor cadence reflects true footprint instead of only the 48-byte struct the bump pointer tracks. - majit-gc: GcAllocator::charge_memory_pressure (default no-op) + MiniMarkGC impl adds a pressure_since_minor counter and forces a minor when nursery struct-fill plus charged external bytes reach the nursery size; reset at do_collect_nursery (the dead young limb Vecs were freed by the destructors). New collecting-nursery and memory-pressure host hooks (alloc_nursery_collecting_typed / charge_memory_pressure) mirroring the no-collect ones. - dynasm / cranelift: install the collecting-nursery and memory-pressure trampolines (gc.alloc_nursery_typed collecting / gc.charge_memory_pressure) alongside the existing alloc hooks. - pyre-object gc_hook / pyre-jit eval: collecting-alloc and memory-pressure hook plumbing and trampolines. - longobject: alloc_bigint_nursery_collecting (collecting; falls back to no-collect then malloc_raw) charges bigint_external_bytes(value) = ceil(bits/64)*8 before allocating, while the fresh value is only a Rust-stack BigInt and the operands are boxed and gcmap-rooted, so the forced minor holds no unrooted nursery pointer. The 6 wrapper helpers jit_w_long_*_raw read W_LongObject operands and allocate no-collect (record-time, trait path); the 6 payload helpers jit_bigint_* read bare BigInt payloads and allocate collecting. - descroperation: same wrapper/payload split for floordiv/mod/lshift/rshift, with the shared div/mod/shift logic extracted into bigint_*_core(a, b, collecting) plus alloc_result_bigint.
Add a `mimalloc` cargo feature (off by default) installing mimalloc as the global allocator in each binary (pyre, pyre-dynasm, pyre-cranelift). The rbigint path allocates a fresh limb Vec per operation through the global allocator; on the platform default heap (notably the Windows system heap) that per-op cost dominates bignum-heavy workloads. Gated behind the feature so default builds are unchanged; enable with `--features mimalloc`.
A foreign BigInt payload's limb Vec lives in the system heap, invisible to oldgen.total_bytes(), so get_total_memory_used undercounted a promoted bignum's true footprint: the major threshold was never reached in bignum-heavy loops and dead promoted bignums' limbs were never reclaimed. Add an optional per-type external_size fn (TypeInfo, parallel to destructor). Maintain oldgen_external_bytes: increment when a destructor-bearing object is promoted (its value is valid post-copy), recompute exactly from the surviving destructor list at each major cycle's end so the running total cannot drift, and fold it into get_total_memory_used. Register bigint_external_size for the BigInt payload type.
1347044 to
7332dd0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7332dd03d5
ℹ️ 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".
| if (type_id as usize) < self.types.len() && self.types.get(type_id).destructor.is_some() { | ||
| self.old_objects_with_destructors.push(obj_addr); |
There was a problem hiding this comment.
When a destructor-bearing type also has external_size and is allocated directly in old gen, this path records it for later destruction but never adds its external bytes to oldgen_external_bytes. w_long_new now allocates BigInt payloads through the stable old-gen hook, so large interpreter-created longs make the major threshold see only the small BigInt struct until some later major recomputes the total; in workloads that don't otherwise cross the old-gen threshold, the limb memory can grow without triggering collection. Add the external-size charge here just as promoted young objects do.
Useful? React with 👍 / 👎.
A reused trace ran the bare machine shift the int-binop specializer emitted, and x86 SHL/SAR mask the count mod 64 — so 1 << 199 returned 1 << (199 & 63) = 128, a lshift overflowing the machine word silently dropped its high bits instead of promoting to a bignum, and x >> 199 returned x >> 7 instead of 0. - int `<<`: route to the generic (residual BINARY_OP) leg, which carries the full intobject.py descr_lshift semantics (bignum on overflow, ValueError on a negative count). A guarded walker-native specialization is correct on dynasm but crashes the cranelift backend: when the lshift result is the loop variable its box alternates small-int/bignum across the guard's bridge boundary. - int `>>`: specialize behind a runtime uint_lt(count, 64) guard — rshift produces no bignum, so no bridge crash — and a recorded count >= LONG_BIT routes to the generic leg instead of const-folding (which would be baked into the reused trace and be wrong for an in-range count). Validated on dynasm AND cranelift: 1<<199, value-overflow lshift, x>>199, mixed in/out-of-range loops, and `1<<-1` -> ValueError all match CPython; suites green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 653b2ce530
ℹ️ 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".
| ob_type: &LONG_TYPE as *const PyType, | ||
| w_class: get_instantiate(&INT_TYPE), | ||
| }; | ||
| if crate::gc_interp::enabled() { |
There was a problem hiding this comment.
When the JIT has installed the BigInt GC hooks but PYRE_GC_INTERP is off (the native default), jit_w_long_*_raw and w_long_new can still hand w_long_from_raw a GC-managed BigInt payload. This guard skips the only GC-managed W_LongObject allocation, so the fallback malloc_typed wrapper is invisible to the collector and its value edge is never traced; a later collection can run the BigInt destructor while the Python long is still reachable. Allocate/trace the wrapper whenever the payload is GC-managed, not only in interpreter-GC mode.
Useful? React with 👍 / 👎.
| if pinned { | ||
| crate::gc_hook::try_gc_remove_root(&mut slot as *mut *mut u8); | ||
| } | ||
| if let Some(raw) = raw { |
There was a problem hiding this comment.
In PYRE_GC_INTERP mode this successful old-gen wrapper allocation should advance the same gc_interp::note_alloc() counter that w_int_new/w_float_new use; otherwise a workload that allocates mostly longs never reaches the safepoint threshold in gc_interp::safepoint, so dead old-gen long wrappers and BigInt payloads accumulate until some unrelated counted allocation happens.
Useful? React with 👍 / 👎.
| if let Some(raw) = | ||
| crate::gc_hook::try_gc_alloc(tid, BIGINT_PAYLOAD_SIZE).filter(|p| !p.is_null()) |
There was a problem hiding this comment.
This allocator is also used by the record-time jit_w_long_*_raw helpers that are registered as elidable pure calls. When both operands are constants, record_result_of_call_pure cuts the call to a constant pointer/call-pure cache entry, but a nursery BigInt returned here is not a GC root; a later minor collection can drop the payload while compiled code or the pure cache still reuses that folded pointer. Use a non-collectible/stably rooted allocation for trace-time pure results, or prevent those results from folding to unrooted constants.
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: 3
🤖 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-gc/src/collector.rs`:
- Around line 1152-1174: The destructor handling in
deal_with_young_objects_with_destructors() is treating any non-forwarded nursery
object as dead, which breaks live pinned objects. Update the logic to detect
pinned nursery objects before calling run_destructor, using the existing
pin()/forwarding state and TypeInfo::with_destructor-related metadata, so live
pinned values are preserved and only truly dead objects have their destructor
run. Keep the surviving-object path that pushes forwarded copies into
old_objects_with_destructors unchanged.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch.rs`:
- Around line 12429-12442: The IntLshift branch in
jitcode_dispatch::walker_execute_may_force_boxed is reached after
walker_int_specialization_operands, so it still performs the concrete helper
work before falling back to the generic BINARY_OP path. Move the early return
for OpCode::IntLshift ahead of the specialization helper dispatch so `<<` is
only handled by the generic residual leg and not evaluated twice.
In `@pyre/pyre-object/src/longobject.rs`:
- Around line 199-232: The fallback allocation path in the long object
constructor still uses the original bigint pointer after the GC pinning/re-read
logic, so the same refreshed address must be used for both branches. Update the
logic around the `try_gc_add_root`, `try_gc_alloc_stable`, and `malloc_typed`
flow in the `W_LongObject` creation path so the re-read `value` derived from
`slot` is hoisted before the allocation branch and reused whether the stable GC
allocation succeeds or falls back to typed malloc.
🪄 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: 1cf6cf1c-4a06-4528-bd50-71b7255503fc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/pyre-ci.ymlmajit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-gc/src/collector.rsmajit/majit-gc/src/lib.rsmajit/majit-gc/src/trace.rspyre/check.pypyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/gc_hook.rspyre/pyre-object/src/longobject.rspyre/pyrex/Cargo.tomlpyre/pyrex/src/bin/pyre-cranelift.rspyre/pyrex/src/bin/pyre-dynasm.rspyre/pyrex/src/main.rs
| fn deal_with_young_objects_with_destructors(&mut self) { | ||
| while let Some(obj_addr) = self.young_objects_with_destructors.pop() { | ||
| let hdr_ptr = (obj_addr - GcHeader::SIZE) as *const GcHeader; | ||
| if !unsafe { (*hdr_ptr).is_forwarded() } { | ||
| // Dead: run the destructor before the nursery reset frees | ||
| // the bytes. | ||
| self.run_destructor(obj_addr); | ||
| } else { | ||
| // Surviving: track the promoted copy for the major cycle. | ||
| let new_obj = unsafe { GcHeader::forwarding_address(hdr_ptr) }; | ||
| self.old_objects_with_destructors.push(new_obj); | ||
| // The payload is now in old-gen with a valid value; fold its | ||
| // off-heap footprint into the major threshold so the dead | ||
| // promoted bigints' limb memory is reclaimed by a major. | ||
| let tid = unsafe { (*header_of(new_obj)).type_id() }; | ||
| if let Some(external_size) = self.types.get(tid).external_size { | ||
| self.oldgen_external_bytes = self | ||
| .oldgen_external_bytes | ||
| .saturating_add(unsafe { external_size(new_obj) }); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find pin() call sites and whether bigint/destructor payloads can reach them.
rg -nP --type=rust -C3 '\.pin\s*\('
rg -nP --type=rust -C2 'with_destructor|register_destructor_if_needed'Repository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the collector logic and locate any pinning or destructor-related paths.
git ls-files 'majit/majit-gc/src/*.rs'
echo '--- collector outline ---'
ast-grep outline majit/majit-gc/src/collector.rs --view expanded | sed -n '1,260p'
echo '--- pin/destructor search ---'
rg -n -C 3 'pin|pinn|destructor|weakref|forwarded|young_objects_with_destructors|copy_nursery_object' majit/majit-gc/srcRepository: youknowone/pyre
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the specific section around deal_with_young_objects_with_destructors and related helpers.
sed -n '980,1225p' majit/majit-gc/src/collector.rsRepository: youknowone/pyre
Length of output: 12548
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any API that marks nursery objects as pinned or otherwise non-forwarded.
rg -n -C 2 'Pinned|pin_nursery|nursery.*pin|set_pinned|is_pinned|pin\(' majitRepository: youknowone/pyre
Length of output: 26215
Prevent pinning destructor-bearing nursery objects. pin() leaves nursery objects in place without forwarding them, but deal_with_young_objects_with_destructors() treats every non-forwarded entry as dead. A live pinned type registered with TypeInfo::with_destructor() will still run its destructor during minor GC.
🤖 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-gc/src/collector.rs` around lines 1152 - 1174, The destructor
handling in deal_with_young_objects_with_destructors() is treating any
non-forwarded nursery object as dead, which breaks live pinned objects. Update
the logic to detect pinned nursery objects before calling run_destructor, using
the existing pin()/forwarding state and TypeInfo::with_destructor-related
metadata, so live pinned values are preserved and only truly dead objects have
their destructor run. Keep the surviving-object path that pushes forwarded
copies into old_objects_with_destructors unchanged.
| OpCode::IntLshift => { | ||
| let Ok(shift) = u32::try_from(rb) else { | ||
| return Ok(None); | ||
| }; | ||
| if shift >= i64::BITS { | ||
| return Ok(None); | ||
| } | ||
| // intobject.py:207 ovfcheck(a << b) | ||
| let result = la.wrapping_shl(shift); | ||
| if result.wrapping_shr(shift) != la { | ||
| return Ok(None); | ||
| } | ||
| // Don't specialize int `<<`: route to the generic (residual | ||
| // BINARY_OP) leg, which carries the full intobject.py | ||
| // descr_lshift semantics (promote to bignum on overflow, raise | ||
| // ValueError on a negative count). A bare walker-native IntLshift | ||
| // would be wrong — the trace is reused for any operands and x86 | ||
| // SHL masks the count mod 64 — and a *guarded* specialization | ||
| // (range + round-trip guards, bail to bignum) crashes the | ||
| // cranelift backend: when the lshift result is the loop variable | ||
| // its box alternates small-int / bignum across the guard's | ||
| // bridge boundary, and that trips a cranelift bridge bug (works | ||
| // on dynasm). The generic leg handles the alternation correctly | ||
| // on both backends. | ||
| return Ok(None); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the caller flow for try_walker_specialize_binary_op_int and
# whether Ok(None) triggers generic BINARY_OP execution after this function returns.
rg -n -C5 'try_walker_specialize_binary_op_int|walker_int_specialization_operands|write_residual_call_result_to_dst' pyre/pyre-jit-traceRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=pyre/pyre-jit-trace/src/jitcode_dispatch.rs
echo '--- IntLshift branch ---'
sed -n '12420,12460p' "$file"
echo
echo '--- try_walker_specialize_binary_op_int caller ---'
sed -n '15520,15560p' "$file"
echo
echo '--- function signature + nearby docs ---'
sed -n '12340,12370p' "$file"Repository: youknowone/pyre
Length of output: 6103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=pyre/pyre-jit-trace/src/jitcode_dispatch.rs
echo '--- walker_int_specialization_operands ---'
sed -n '12054,12099p' "$file"
echo
echo '--- IntLshift / IntRshift gate region ---'
sed -n '12411,12445p' "$file"Repository: youknowone/pyre
Length of output: 3431
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=pyre/pyre-jit-trace/src/jitcode_dispatch.rs
rg -n -C6 'fn walker_execute_may_force_boxed|walker_execute_may_force_boxed\(' "$file"Repository: youknowone/pyre
Length of output: 6868
Return before the concrete helper for IntLshift
This branch always falls back to the generic BINARY_OP record, so walker_execute_may_force_boxed evaluates << here and then again in the fallback path. Move the IntLshift return above walker_int_specialization_operands.
🤖 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/jitcode_dispatch.rs` around lines 12429 - 12442, The
IntLshift branch in jitcode_dispatch::walker_execute_may_force_boxed is reached
after walker_int_specialization_operands, so it still performs the concrete
helper work before falling back to the generic BINARY_OP path. Move the early
return for OpCode::IntLshift ahead of the specialization helper dispatch so `<<`
is only handled by the generic residual leg and not evaluated twice.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44c9824211
ℹ️ 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".
| } else { | ||
| // Surviving: track the promoted copy for the major cycle. | ||
| let new_obj = unsafe { GcHeader::forwarding_address(hdr_ptr) }; | ||
| self.old_objects_with_destructors.push(new_obj); |
There was a problem hiding this comment.
When a minor collection runs while incr_state.marking_in_progress is true, a live BigInt allocated after the major cycle's root snapshot can be promoted with no VISITED bit set. Pushing that promoted address onto old_objects_with_destructors here lets finish_incremental_cycle classify it as dying, run its destructor, and then sweep it even though a root or old field was just updated to new_obj; bigint-heavy traces can hit this once an incremental major spans multiple minors. Mark/re-grey these newly promoted destructor objects for the current cycle, or defer their destructor tracking until the next major.
Useful? React with 👍 / 👎.
- w_long_from_raw advances gc_interp::note_alloc() on a successful stable wrapper alloc, as w_int_new / w_float_new do. Without it a long-dominated interpreter workload under PYRE_GC_INTERP never reaches the dispatch-loop safepoint, so dead old-gen long wrappers and their bigint payloads accumulate until some unrelated counted allocation trips the threshold. - alloc_bigint_stable charges the bigint's limb-Vec bytes through a new charge_oldgen_external hook (gc_hook -> majit_gc -> MiniMarkGC, registered on dynasm and cranelift) so a directly-old-gen-allocated bignum's footprint enters get_total_memory_used at alloc time, not only at the next major's recompute_oldgen_external_bytes. Unlike charge_memory_pressure it forces no minor, so it is safe on the unrooted host/interpreter path where a moving minor would dangle Rust-stack PyObjectRefs; the promotion path already charges its external bytes, this closes the direct-old-gen gap. - charge_memory_pressure uses saturating_add, matching the other off-heap counters.
Review triage (Codex + CodeRabbit) + one real, unfixed GC issueI triaged every automated-bot finding on this PR against HEAD ( Refuted — bot false positives (verified against current code, no action)
Addressed (fix prepared on the working branch)
|
1 participant
fix #268
Summary
W_LongObject.value(the rbigint payload) wasmalloc_raw'd (Box::into_raw— no GCheader, never freed), so every bignum result leaked.
fib_loopaccumulated ~300k growingbignums → allocator/cache/TLB collapse (the root of the bignum-path slowdown vs PyPy). This
routes the BigInt payload through the GC so dead bignums are reclaimed.
Two parts (the issue's chosen single combined change):
Part 1 — majit-gc lightweight destructors (
9cbdbed1a3)TypeInfo.destructor+ young/old object-with-destructor lists +deal_with_*_objects_with_destructors(incminimark.py:2884-2912 parity,T_HAS_DESTRUCTORbit). Runs
drop_in_place::<BigInt>on dead bignums so malachite's external limbVecisfreed.
Part 2/3 — bignum GC/nursery representation (
0ceef909cf)malloc_rawonly as ano-hook fallback;
W_LONGregistered with itsvaluefield as a gc-pointer offset; aBIGINTGC type carrying thedrop_in_placedestructor.NewWithVtable(W_LONG)+setfield_gc(value)behind aGuardFalse(fits_int)demote guard. TheNewWithVtablelowers to the collectingCallMallocNursery— the per-iteration safepoint bigint loops previously lacked.valueviaGetfieldGcPureand calls theelidable rbigint op on the bare
*const BigIntpayloads (W_LongObject(self.num.add( other.num))parity). Passing the payloads, not the wrappers, keeps the elidable call pureon the immutable bigints so the optimizer never reorders it ahead of the boxing setfield.
Root-caused crash (fixed here)
A JIT-compiled long loop inside a function segfaulted on BOTH backends (top-level loops and
no-loop functions were fine → logic/IR bug, not codegen). The elidable op took the
W_LongObjectwrappers and read.valueopaquely; after the function-loop unroll theoptimizer reordered the elidable
CallRahead of the boxingsetfield_gcthat initializesthe fresh result wrapper → it read an uninitialized BigInt pointer. The payload-based
emission above is the fix.
Verification
fib_loop+ all 8 benchmarks correct on dynasm AND cranelift.majit-gc165 /pyre-object194 tests green (4jitcode_runtimefailures arepre-existing and unrelated —
PopTop#23arm-numbering, fail on a clean tree).fib(200000)→ 3 minors vs 0 before).fib(100000)×5 RSS ≈fib(100000)×1 (5× the dead bignums, +14% RSS) →reclaimed, no cumulative leak.
fib_loop / int_loopratio drops 7.90 → 3.54, i.e. ~2.2×faster on the bignum path.
Part 4 — within-run RSS: collecting alloc + memory pressure (
0af65788ff)Part 2/3's bignum alloc was no-collect → a long bignum loop filled the nursery once and
spilled the rest to old-gen unbounded (
fib(100000)peaked ~316MB,fib(300000)~1.3GB).jit_bigint_*) through a collecting nursery alloc(new host-hook chain mirroring the no-collect one, both backends). The wrapper helpers
(
jit_w_long_*_raw, record-time / trait path) stay no-collect since the tracer holds theiroperands natively; the shared div/mod/shift logic is extracted into
bigint_*_core(a, b, collecting).rgc.add_memory_pressureanalog):charge_memory_pressureadds a
pressure_since_minorcounter and forces a minor when nursery struct-fill + chargedexternal bytes reach the nursery size (reset at each minor). The bignum site charges
ceil(bits/64)*8external limb-Vecbytes before allocating — so minor cadence reflectstrue footprint, not just the 48-byte struct. Collecting alone did nothing (the nursery only
counts the struct); the pressure charge is the load-bearing fix.
Results (release, peak working set), both dynasm and cranelift:
fib(100000)fib(300000)Also faster: the small working set wins cache/TLB locality (
fib_loop / int_loopratio3.54 → 2.46), outweighing the extra memzero. The residual-call gcmap-under-forced-collection
is proven safe on both backends (1000+ forced minors, all benches + function-loops correct).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes