gc: throttle the interpreter safepoint on allocated bytes, not allocation count - #847
Conversation
…tion count The safepoint ran a whole-heap old-gen major every `COLLECT_THRESHOLD` (65536) interpreter-routed allocations. Nothing upstream counts allocations: the allocator accumulates a byte total (`nursery_free = result + totalsize`, minimark.py:556-557), and a major is scheduled against `get_total_memory_used()` (incminimark.py:1264-1268) reaching a threshold that `set_major_threshold_from` (incminimark.py:575-594) re-derives from what survived the last one. A count cannot bound a heap, because one allocation is not one size -- 65536 boxed characters and 65536 frames are three orders of magnitude apart. `note_alloc` now takes the payload size that was allocated, and the safepoint compares accumulated bytes against a threshold set to the surviving old-gen total times `major_collection_threshold - 1` (incminimark.py:198's 1.82 default), floored at a `min_heap_size` (incminimark.py:307) of 8 MiB. The comparison is kept in that delta form on purpose: having allocated `b` bytes since the last major, the total is `live + b`, so incminimark's `live + b >= live * threshold` is exactly `b >= live * (threshold - 1)`. The per-dispatch test therefore stays a single atomic compare, as the counter was, and the heap-stats read happens only after a collection. Adds `try_gc_heap_stats` alongside `try_gc_jitframe_empty`, routed to `majit_gc::active_heap_stats`, whose doc already names the interpreter safepoint as its consumer. Measurements, all min-of-three: no workload regressed. `s.lower()` in a loop is unchanged (0.93s -> 1.00s under `PYRE_GC_INTERP=1`, 0.92s -> 0.79s without it) and its RSS stays bounded and flat. With the JIT off, so the interpreter allocation path this module targets is actually taken, a string-boxing loop routed through the managed allocator ran 2.55s against 2.72s for the immortal allocator. No claim is made here that this unblocks routing more sites through the managed constructors: measuring that needs a managed-allocator build on this same base for the count policy, which was not run. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 51 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 Plus Run ID: 📒 Files selected for processing (11)
✨ 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 15f4215). 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
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15f4215956
ℹ️ 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".
| // proportionally more between majors and a steady-state one stops | ||
| // collecting altogether. | ||
| let (oldgen_live, _nursery_used) = crate::gc_hook::try_gc_heap_stats(); | ||
| let next = (oldgen_live / 100).saturating_mul(MAJOR_GROWTH_DELTA_PCT); |
There was a problem hiding this comment.
Apply the collector's capped next-major threshold
When PYRE_GC_INTERP runs with a large surviving heap—particularly with PYPY_GC_MAX_DELTA, PYPY_GC_MAX, or PYPY_GC_MAJOR_COLLECT configured—this hard-coded 0.82 * oldgen_live delta is not the threshold established by the collector. finish_incremental_cycle in majit/majit-gc/src/collector.rs:2658-2663 caps growth by max_delta, and set_major_threshold_from applies the configured growth, minimum, and maximum limits; bypassing those values can let cold-interpreter stable allocations grow far beyond the requested cap before another old-gen collection occurs. Expose the collector's actual remaining threshold instead of reconstructing it here.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| pub fn note_alloc(size: usize) { | ||
| ALLOC_BYTES_SINCE_GC.fetch_add(size, Ordering::Relaxed); |
There was a problem hiding this comment.
Charge the allocator's total object size
For PYRE_GC_INTERP workloads dominated by small stable allocations, the call sites pass only the payload size, while alloc_oldgen_typed in majit/majit-gc/src/collector.rs:3742-3744 adds GcHeader::SIZE and OldGen rounds/min-sizes the allocation before including it in oldgen_live. The delta counter and its threshold therefore measure different byte quantities, systematically delaying collection and allowing the actual high-water mark to exceed the intended budget; Unicode similarly omits the fixed Wtf8Buf storage-box allocation while charging its external byte length. Account the same total, rounded allocation size used by old-gen statistics.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
Why
The interpreter GC safepoint (
PYRE_GC_INTERP) ran a whole-heap old-gen major everyCOLLECT_THRESHOLD— 65536 — interpreter-routed allocations.Nothing upstream counts allocations. The allocator accumulates a byte total (
nursery_free = result + totalsize, minimark.py:556-557), and a major is scheduled againstget_total_memory_used()(incminimark.py:1264-1268) reaching a threshold thatset_major_threshold_from(incminimark.py:575-594) re-derives from what survived the last one, capped by a growth rate and floored atmin_heap_size(incminimark.py:307).A count cannot bound a heap, because one allocation is not one size — 65536 boxed characters and 65536 frames are three orders of magnitude apart. The count-based threshold is a pyre-only mechanism with no upstream counterpart.
What
note_alloctakes the payload size that was allocated. The safepoint compares accumulated bytes against a threshold set to the surviving old-gen total timesmajor_collection_threshold - 1(incminimark.py:198's 1.82 default), floored at 8 MiB.The comparison is kept in that delta form deliberately: having allocated
bbytes since the last major, the total islive + b, so incminimark'slive + b >= live * thresholdis exactlyb >= live * (threshold - 1). The per-dispatch test therefore stays a single atomic compare, exactly as the counter was, and the heap-stats read happens only after a collection rather than on every bytecode dispatch.Adds
try_gc_heap_statsalongsidetry_gc_jitframe_empty, routed tomajit_gc::active_heap_stats— whose existing doc already names the interpreter safepoint as its intended consumer.The eight call sites pass their real allocation size.
w_str_from_wtf8_managedcharges both the header and the value box holding the WTF-8 bytes, since strings are exactly where a count diverges most from bytes.Measurements
All min-of-three. No workload regressed.
s.lower()loop, defaults.lower()loop,PYRE_GC_INTERP=1PYRE_GC_INTERP=1RSS stays bounded — the point is amortising the major, not collecting less.
With the JIT off, so the interpreter allocation path this module targets is actually taken, a string-boxing loop routed through the managed allocator ran 2.55s against 2.72s for the immortal allocator. On the same workload with the JIT on, all four combinations are indistinguishable (0.53–0.54s), because the trace does not reach the interpreter allocation path at all.
No claim is made here that this unblocks routing more sites through the managed constructors. Measuring that needs a managed-allocator build on this same base under the count policy, which was not run. An earlier experiment on an older base did show the count policy imposing a 2.8x penalty on such routing (2.71s → 7.48s), and that penalty is absent under the byte policy — but the control arm for the current base is missing, so this PR is offered on the orthodoxy argument, not on that one.
Verification
check.py326/326 on dynasm and cranelift, each at default andPYRE_FBW_MULTIFRAME=1, run seriallycargo test -p pyre-jit-trace -p pyre-jit --features dynasm: 641 passed, 0 failedcargo fmt --all --checkcleanVerification ran against the tree before the last rebase onto #839; the diff is unchanged by it, but the suite has not been re-run on this exact base.
— authored by Claude