gc: drive the interpreter safepoint from the collector, and root the __main__ frame across the bootstrap - #861
Conversation
The safepoint kept its own model of heap growth: bytes charged by `note_alloc` at eight allocation sites, compared against a threshold re-derived after each collection from `get_total_memory_used()` times a growth delta. Both halves described a different heap from the one being collected. The tally saw only the sites that remembered to report. Dicts, lists, tuples, instances and every JIT promotion grow the old generation without charging it, so the comparison was not the rearrangement of `total >= live * threshold` its comment claimed, and the heap could pass the threshold with the safepoint still counting. The charges were payload-only while the collector's total counts each `GcHeader`, and the string charge added `byte_len` for a WTF-8 buffer that lives on the Rust heap, not in the collector's. The re-derived threshold applied `major_collection_threshold` alone, dropping the `growth_rate_max` and `max_delta` caps and the `max_heap_size` bound that `set_major_threshold_from` (incminimark.py:575-594) applies. All of that is already ported in `majit-gc`. `threshold_reached` (incminimark.py:1288-1290) weighs `get_total_memory_used()` -- every byte the collector is responsible for, whatever allocated it -- against the threshold set from the last major's survivors, and `do_collect_oldgen_nonmoving` already runs `major_collection_step` through `finish_incremental_cycle`, which sets the next one. Exposed as a `GcAllocator::major_threshold_reached` query shaped like `heap_byte_stats` and routed to pyre through the same hook layer. Deleted: `note_alloc` and its eight call sites, `ALLOC_BYTES_SINCE_GC`, `NEXT_MAJOR_BYTES`, the growth-delta and min-heap constants, and the heap-stats hook added for the re-derivation. What remains on the pyre side is a poll interval. It carries no heap semantics and cannot cause a collection the collector did not ask for; it only bounds how often the query is made, because reaching the collector costs a thread-local borrow. Interleaved min-of-7 against the previous policy built from the same base: every workload within 1% with `PYRE_GC_INTERP` off and on, and the completed majors per run unchanged (14/13, 34/33, 12/12), so the collector's own threshold reproduces the cadence the hand-rolled one approximated. RSS over a `s.lower()` loop stays flat at 154 MB either way. Assisted-by: Claude
`pop` asserted `valuestackdepth > stack_base()`. Replace the bare assertion with a `#[cold]` reporter that panics with the depth, the base and its nlocals/ncells split, the pc, `last_instr`, the instruction count, the code object's name and file, the frame, pycode and f_backref pointers, and an eight-unit opcode window around the pc. Assisted-by: Claude
`run_source` built the `__main__` frame and then ran the `sys` importhook, the importlib bootstrap, `seed_main_loader` and `import_site` before handing it to `eval_with_jit`. The frame is a GC-managed `FrameBox`, and the only root that reaches one is the `CURRENT_FRAME` / `f_backref` chain `walk_pyframe_roots` follows — which it joins when `eval_with_jit` installs it. A collection driven by any of those bootstrap steps therefore reclaimed a live frame and handed its block to the next stable frame allocation; with min_heap_size near 1 MB the first major lands inside `import_site` and the block goes to importlib's `_get_module_lock` weakref callback, which leaves `last_instr` at its `ReturnValue` and a value stack at base. `eval_with_jit` then entered that block and `pop` underflowed. Pin the frame for the span, as `FrameBox::new`, `call_function_impl_result` and `finalize_weakrefs` already do for the same reason: an RPython local holding a GC object is a shadow-stack root, a Rust local is not. `pyre-wasm`'s `run_python_impl` has the same shape over a shorter window — the `__main__` module registration allocates — and gets the same pin. `pyre-wasm-test` enters the frame immediately and is left alone. Assisted-by: Claude
The safepoint only ran a major when no compiled trace was suspended on this thread. A trace suspends by making a residual call into the interpreter, and a call site is where the backend emits a gcmap, so `enumerate_root_walker_values` already reaches a suspended frame's live refs by expanding each jitframe on the JF shadow stack through `trace_libc_jitframe`; the gate has no upstream counterpart and suppressed most of the safepoint's collections. Remove it root and branch: `majit_gc::jitframe_shadow_stack_empty`, `pyre-object`'s `GcJitframeEmptyHookFn` / `GC_JITFRAME_EMPTY_HOOK` / `register_gc_jitframe_empty_hook` / `clear_gc_jitframe_empty_hook` / `try_gc_jitframe_empty`, the `pyre-jit` trampoline and its registration, and the two `jit_trace_fnaddrs` aliases with their test assertions. On p_calleeloop.py this takes the safepoint from 12 majors to 23 against a JIT-off baseline of 26. Assisted-by: Claude
WalkthroughThe PR adds collector-provided major-threshold queries across GC backends, replaces interpreter allocation accounting with periodic safepoint polling, updates JIT hook wiring, removes allocation notifications, improves stack-underflow diagnostics, and pins execution frames as GC roots. ChangesCollector-driven major GC thresholds
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)majit/majit-backend-cranelift/src/compiler.rsast-grep timed out on this file 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/134fee11694038253d3ceb00246fa7a9d8fae710/pyre-object/src/gc_interp.rs#L137
When PYRE_GC_INTERP is enabled, an opcode can allocate far more than the assumed few dozen bytes—for example, s.lower() can allocate an entire large managed string—yet the collector threshold is not queried until 1,024 eligible dispatches have elapsed. Such a loop can therefore add hundreds or thousands of large old-gen objects after the threshold is exceeded and exhaust memory before collection; moreover, the TLS counter resets independently per thread even though the standalone collector's heap threshold is shared, so workloads spread across short-lived threads may never reach 1,024 dispatches anywhere. Pace this from global allocation progress or arrange for crossing the collector threshold to force a query at the next safepoint.
AGENTS.md reference: AGENTS.md:L157-L162
ℹ️ 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 32ef2ee). 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)
4. Structural adaptations
|
…shold `external_malloc` (incminimark.py:987-994) tests `threshold_reached(raw_malloc_usage(totalsize))` before allocating and drives `minor_collection_with_major_progress` when it holds. `alloc_oldgen_typed` is the structural counterpart and has no such check, because the callers it exists for hold the returned raw pointer on the Rust stack, where it is not a root — the reason upstream can collect mid-allocation and this entry cannot. Note the divergence and where old-gen growth is answered instead. Assisted-by: Claude
Local verificationBuilt on the rebased base ( Defect-3 repro matrix — the eight configurations that previously died with a value-stack underflow in importlib's
Every control failure is the same frame: check.py — serially, both backends × default and
CI's ubuntu run does not hit it (331 passed there too). cargo test ( Perf A/B —
No systematic cost from removing the gate. On the CI
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32ef2eeb9c
ℹ️ 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".
| /// Thread-local rather than atomic because the query it paces is itself | ||
| /// per-thread, and a `fetch_add` on every dispatch would cost about what it | ||
| /// is here to avoid. | ||
| static POLL_TICK: Cell<u32> = const { Cell::new(0) }; |
There was a problem hiding this comment.
With PYRE_GC_INTERP=1, an application that repeatedly creates Python threads which each execute fewer than 1,024 eligible dispatches can grow the process-global old generation past its threshold without ever querying it: each thread's POLL_TICK is discarded before reaching the interval, while its stable int/float/frame allocations remain charged to the shared collector. This can leave interpreter-routed garbage uncollected indefinitely, so pace the shared collector with process/interpreter-owned state or force a threshold query when a mutator thread exits.
AGENTS.md reference: AGENTS.md:L148-L162
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/jit_fnaddr.rs (1)
3068-3117: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest doesn't cover the two new safepoint residual bindings it should pin.
The updated comment at lines 1014-1020 calls out "five toucher residuals," but
jit_trace_fnaddrs_covers_interp_gc_safepoint_readersonly asserts on three of them (collect_enabled,at_outermost_activation,try_gc_collect_oldgen) plus the odometer/strategy-gate entries — it's missing assertions for the newly-registeredpoll_dueandtry_gc_major_threshold_reachedbindings. This test exists specifically to catch a typo'd alias silently degrading to asymbolic_fnaddr_for_pathhash (a SEGV-at-trace-time risk per this file's own rationale), so the two new residuals should be pinned the same way the others are.✅ Suggested additions to the test
let collect_oldgen = pyre_object::gc_hook::try_gc_collect_oldgen as *const () as usize as i64; assert_eq!( bindings["pyre_object::gc_hook::try_gc_collect_oldgen"], collect_oldgen ); assert_eq!( bindings["pyre_object::try_gc_collect_oldgen"], collect_oldgen ); + let poll_due = pyre_object::gc_interp::poll_due as *const () as usize as i64; + assert_eq!(bindings["pyre_object::gc_interp::poll_due"], poll_due); + assert_eq!(bindings["pyre_object::poll_due"], poll_due); + + let threshold_reached = + pyre_object::gc_hook::try_gc_major_threshold_reached as *const () as usize as i64; + assert_eq!( + bindings["pyre_object::gc_hook::try_gc_major_threshold_reached"], + threshold_reached + ); + assert_eq!( + bindings["pyre_object::try_gc_major_threshold_reached"], + threshold_reached + ); + let itemsblock = pyre_object::object_array::itemsblock_gc_enabled as *const () as usize as i64;🤖 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-interpreter/src/jit_fnaddr.rs` around lines 3068 - 3117, Extend jit_trace_fnaddrs_covers_interp_gc_safepoint_readers to validate the newly registered poll_due and try_gc_major_threshold_reached bindings. Resolve each function pointer to its address and assert both the module-qualified and public alias entries match, following the existing collect_enabled, at_outermost_activation, and try_gc_collect_oldgen assertions.
🤖 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.
Outside diff comments:
In `@pyre/pyre-interpreter/src/jit_fnaddr.rs`:
- Around line 3068-3117: Extend
jit_trace_fnaddrs_covers_interp_gc_safepoint_readers to validate the newly
registered poll_due and try_gc_major_threshold_reached bindings. Resolve each
function pointer to its address and assert both the module-qualified and public
alias entries match, following the existing collect_enabled,
at_outermost_activation, and try_gc_collect_oldgen assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f58bd12c-70f6-4634-ae43-ed2bcf6d82f1
📒 Files selected for processing (20)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-gc/src/collector.rsmajit/majit-gc/src/lib.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-interpreter/src/pytraceback.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/floatobject.rspyre/pyre-object/src/gc_hook.rspyre/pyre-object/src/gc_interp.rspyre/pyre-object/src/generator.rspyre/pyre-object/src/interp_exceptions.rspyre/pyre-object/src/intobject.rspyre/pyre-object/src/longobject.rspyre/pyre-object/src/unicodeobject.rspyre/pyre-wasm/src/lib.rspyre/pyrex/src/lib.rs
💤 Files with no reviewable changes (7)
- pyre/pyre-object/src/generator.rs
- pyre/pyre-object/src/floatobject.rs
- pyre/pyre-object/src/unicodeobject.rs
- pyre/pyre-object/src/intobject.rs
- pyre/pyre-object/src/interp_exceptions.rs
- pyre/pyre-interpreter/src/pytraceback.rs
- pyre/pyre-object/src/longobject.rs
1 participant
Four commits on the interpreter GC safepoint. The first two replace the safepoint's hand-rolled heap model and its jitframe gate with the collector's own answers; in between sits the frame-lifetime bug that removing the gate exposes, and the diagnostic that found it.
gc: ask the collector when to run the interpreter safepoint's majorThe safepoint kept its own model of heap growth: bytes charged by
note_allocat eight allocation sites, compared against a threshold re-derived after each collection fromget_total_memory_used()times a growth delta. Both halves described a different heap from the one being collected — the tally saw only the sites that remembered to report (dicts, lists, tuples, instances and every JIT promotion grow the old generation without charging it), the charges were payload-only while the collector's total counts eachGcHeader, and the re-derived threshold dropped thegrowth_rate_max/max_deltacaps and themax_heap_sizebound thatset_major_threshold_from(incminimark.py:575-594) applies.All of that is already ported in
majit-gc.threshold_reached(incminimark.py:1288-1290) weighsget_total_memory_used()against the threshold set from the last major's survivors, anddo_collect_oldgen_nonmovingalready runsmajor_collection_stepthroughfinish_incremental_cycle, which sets the next one. Exposed asGcAllocator::major_threshold_reached. What remains on the pyre side is a poll interval carrying no heap semantics.Interleaved min-of-7 against the previous policy: every workload within 1% with
PYRE_GC_INTERPoff and on, completed majors per run unchanged (14/13, 34/33, 12/12), RSS over as.lower()loop flat at 154 MB either way.pyframe: name the code object and pc when the value stack underflowspop's bare depth assertion said an opcode popped from an empty stack, but not which opcode of which code object — the whole question when the underflow means the frame is no longer the one the loop started with. Replaced with a#[cold]reporter carrying depth/base/nlocals/ncells, pc,last_instr, the code object's name and file, the frame/pycode/f_backref pointers, and an opcode window.interp: root the __main__ frame across the bootstrap importsrun_sourcebuilt the__main__frame and then ran thesysimporthook, the importlib bootstrap,seed_main_loaderandimport_sitebefore handing it toeval_with_jit. The frame is a GC-managedFrameBox, and the only root that reaches one is theCURRENT_FRAME/f_backrefchainwalk_pyframe_rootsfollows — which it joins wheneval_with_jitinstalls it. A collection driven by any bootstrap step therefore reclaimed a live frame and handed its block to the next stable frame allocation.With min_heap_size near 1 MB the first major lands inside
import_siteand the block goes to importlib's_get_module_lockweakref callback, which leaveslast_instrat itsReturnValueand a value stack at base;eval_with_jitthen enters that block andpopunderflows. Confirmed under lldb: theFrameBoxpointer returned bynew_with_contextis the frame the panic names.Pin the frame for the span, as
FrameBox::new,call_function_impl_resultandfinalize_weakrefsalready do for the same reason — an RPython local holding a GC object is a shadow-stack root, a Rust local is not.pyre-wasm'srun_python_implhas the same shape over a shorter window and gets the same pin.gc: drop the jitframe-empty gate from the interpreter safepointThe safepoint only ran a major when no compiled trace was suspended on this thread. A trace suspends by making a residual call into the interpreter, and a call site is where the backend emits a gcmap, so
enumerate_root_walker_valuesalready reaches a suspended frame's live refs by expanding each jitframe on the JF shadow stack throughtrace_libc_jitframe. The gate has no upstream counterpart and suppressed most of the safepoint's collections.Removed root and branch, along with its hook layer, trampoline and fn-addr aliases. On
p_calleeloop.pythis takes the safepoint from 12 majors to 23, against a JIT-off baseline of 26.Verification is running locally (repro matrix,
check.pyon both backends × default and gated,cargo test, perf A/B); this PR will be updated if anything comes back red.— opened by Claude
Summary by CodeRabbit