Heap: size the mutator's headroom during a critical collection from the critical eden - #450
Conversation
…he critical eden While the process is over criticalGCMemoryThreshold, collectIfNecessaryOrDefer() requests a collection once m_maxEdenSizeWhenCritical bytes have been allocated, but both mutator schedulers still computed the allocation the mutator may do while that collection runs as concurrentGCMaxHeadroom * max(bytes allocated so far, m_maxEdenSize). m_maxEdenSize is the uncapped eden, so every critical collection let the mutator allocate the normal eden again on top of the critical one: with a 128 MiB ramSize and a ~70 MB heap the cycle is requested at 6.4 MiB and the headroom was 18 to 21 MiB, about three times the budget the collection was requested for. Add Heap::effectiveMaxEdenSize(), m_maxEdenSize capped to m_maxEdenSizeWhenCritical while over the threshold, and use it in SpaceTimeMutatorScheduler and StochasticSpaceTimeMutatorScheduler. In the case above the headroom becomes 3.2 MiB and the most the mutator allocated during one collection went from 11.7 MB to 3.4 MB. Outside the critical state the value is m_maxEdenSize, as before.
|
Warning Review limit reached
Next review available in: 37 seconds Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (4)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes concurrent GC mutator-scheduling heuristics — a performance-critical path where the throughput/memory tradeoff is subtle — a human look would still be worthwhile.
Checked: effectiveMaxEdenSize() mirrors the min(m_maxEdenSizeWhenCritical, ...) cap that collectIfNecessaryOrDefer() already applies. Verified beginCollection() runs in runBeginPhase after willStartCollection() → shouldDoFullCollection() has already made a cached overCriticalMemoryThreshold() read on the same thread, so this adds no new cross-thread access to m_overCriticalMemoryThreshold/m_percentAvailableMemoryCachedCallCount. Non-critical path returns m_maxEdenSize unchanged, so headroom is identical when not over threshold. On !USE(MEMORY_FOOTPRINT_API) builds overCriticalMemoryThreshold() returns false, so behavior there is unchanged.
Extended reasoning...
Overview
Adds Heap::effectiveMaxEdenSize() to Heap.{h,cpp} and swaps two direct m_heap.m_maxEdenSize reads in SpaceTimeMutatorScheduler::beginCollection() and StochasticSpaceTimeMutatorScheduler::beginCollection() for the new accessor. The accessor caps m_maxEdenSize to m_maxEdenSizeWhenCritical while overCriticalMemoryThreshold() is true — the same rule collectIfNecessaryOrDefer() already applies at Heap.cpp:2958-2960 when computing bytesAllowedThisCycle. Net effect: during a critical-memory collection the mutator's concurrent-allocation headroom is bounded by the critical eden rather than the full eden.
Security risks
None. This is a GC pacing heuristic; it reads existing size_t members and a cached bool, and only tightens an allocation-budget upper bound. No untrusted input, no auth/crypto/permissions surface.
Level of scrutiny
High. The diff is tiny (7 additions across four files, two of them one-token substitutions), the logic is straightforward, and it follows an existing pattern. But it lives in the concurrent GC scheduler, where headroom directly controls how aggressively the mutator is throttled during collection. Tightening it under memory pressure is intentional and well-argued, but GC heuristic changes can shift throughput/latency in workloads the author's benchmark didn't cover, so a maintainer familiar with JSC GC pacing should confirm the tradeoff is desired.
Other factors
- Verified call ordering:
runBeginPhase()→willStartCollection()(Heap.cpp:1544, callsshouldDoFullCollection()→ cachedoverCriticalMemoryThreshold()) → … →m_scheduler->beginCollection()(Heap.cpp:1614). Both cached reads happen on the thread with the conn while the world is stopped, so the new call sees the samem_overCriticalMemoryThresholdand adds no new race on the counter. - On platforms without
USE(MEMORY_FOOTPRINT_API),overCriticalMemoryThreshold()unconditionally returnsfalse, soeffectiveMaxEdenSize()==m_maxEdenSizeand behavior is byte-identical. std::max<double>with the beginning value is preserved, so if the mutator already allocated past the critical eden before the collection was requested, headroom still scales from the actual starting point (no zero/negative headroom).- The author reports empirical validation (logGC
h=drops from ~18-21 MiB to ~3.2 MiB, peak anon drops ~10-15 MB) plus a check that the default/non-critical config is unchanged. No automated test is added, which is typical for GC heuristic tuning. - Given this is core-engine GC scheduling, deferring rather than auto-approving.
Preview Builds
|
Problem
criticalGCMemoryThreshold,Heap::collectIfNecessaryOrDefer()requests a collection afterm_maxEdenSizeWhenCriticalbytes (ramSize * (1 - threshold) / 4, 6.4 MiB for a 128 MiBramSize). The amount the mutator may then allocate while that collection runs is computed inSpaceTimeMutatorScheduler::beginCollection()andStochasticSpaceTimeMutatorScheduler::beginCollection()asconcurrentGCMaxHeadroom * max(bytes allocated so far, m_heap.m_maxEdenSize), andm_maxEdenSizeis the uncapped eden fromupdateAllocationLimits().ramSizethe collection is requested at 6.4 MiB and the logged headroom (h=inlogGC) is 18 to 21 MiB; the mutator allocated up to 11.7 MB during single collections (median 2.4 MB), and everything allocated during a cycle also survives it, so the heap's capacity between collections is dominated by this rather than by the critical budget.overCriticalMemoryThreshold()can be true: macOS today, Linux once WTF: Linux memoryFootprint always parsed as 0, so the critical-memory GC mode never engaged #449 lands (the Linux footprint currently reads as 0).Fix
Heap::effectiveMaxEdenSize()returnsm_maxEdenSize, capped tom_maxEdenSizeWhenCriticalwhileoverCriticalMemoryThreshold()is true, mirroring the capcollectIfNecessaryOrDefer()applies to the trigger. Both schedulers use it in place ofm_maxEdenSize. Outside the critical state the value is unchanged, so the non-critical headroom is exactly what it was.beginCollection()runs in the begin phase with the world stopped, on the thread that has the conn, right afterwillStartCollection()has already consultedoverCriticalMemoryThreshold()for the collection scope, so the cached read here sees the same state and adds no new cross-thread access.BUN_JSC_logGC=1under an emulated 128 MiB limit past its 80% mark (forceRAMSizechosen so the critical eden is 6.4 MiB, threshold so the flag stays on): headroom per collectionh=3.2 MiB instead of 18 to 21 MiB, bytes allocated during one collection max 3.4 MB / median 0.6 MB instead of 11.7 MB / 2.4 MB, and peak anonymous memory 100 to 102 MB versus 112 to 117 MB with the footprint fix alone and 153 MB stock (a stop-the-world run of the same build gives 90 to 92 MB, so this recovers most of the remaining gap). Same check withuseStochasticMutatorScheduler=falseexercises the other scheduler. The default configuration on an unlimited host measured the same as before (152 to 154 MB peak anonymous on this workload either way), and a handful of GC-related Bun test files pass against the build both normally and withcriticalGCMemoryThreshold=0forcing the critical path for every collection.Background
beginCollection(), the mutator's share drops to zero and it is parked until the collection finishes (headroomFullness()/mutatorUtilization()). The headroom is therefore the bound on how far memory can grow past the trigger point during one collection.m_maxEdenSizeis set byupdateAllocationLimits()at the end of each collection toproportionalHeapSize(heap) - heap(24% of the heap in the large tier, 50% or 100% in the smaller tiers), independent of the critical state.m_maxEdenSizeWhenCriticalis fixed at construction fromramSize;collectIfNecessaryOrDefer()takes the minimum of the two when critical, which is what this change makes the schedulers do as well.