WTF: Linux memoryFootprint always parsed as 0, so the critical-memory GC mode never engaged - #449
Conversation
…s present LinuxMemory::footprint() skips statm's first field and then hands the rest of the line to parseInteger(), which rejects input that does not end after the number. The five fields following the resident count are still in the buffer, so the parse always failed, value_or(0) made the footprint 0, and memoryStatus().percentAvailableMemoryInUse was 0 for the life of the process. JSC::Heap::overCriticalMemoryThreshold() compares that value with criticalGCMemoryThreshold, so the critical memory GC mode (eden capped at ramSize * (1 - threshold) / 4, full collections only, synchronous sweeps) never engaged on Linux, including inside cgroup memory limits, where ramSize() is the limit. Use parseIntegerAllowingTrailingJunk(), which stops at the first character after the number. The Darwin reader (task_info phys_footprint) and WTF::memoryFootprint()'s strtoull based reader of the same file were already correct; this brings the Linux footprint used for the threshold in line with them.
|
Warning Review limit reached
Next review available in: 44 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 (1)
Comment |
There was a problem hiding this comment.
LGTM — one-line fix swapping parseInteger for parseIntegerAllowingTrailingJunk so the resident field of /proc/self/statm parses correctly.
What was reviewed:
- Confirmed in
StringToIntegerConversion.hthatparseIntegerdefaults toTrailingJunkPolicy::Disallow(fails on the trailing 5 fields →nullopt→ 0), and thatparseIntegerAllowingTrailingJunkkeepsParseIntegerWhitespacePolicy::Allowso the leading space before field 2 is still skipped. - Checked the sole consumer (
memoryStatus()→percentAvailableMemoryInUse) — a non-zero footprint only engages the existing critical-GC path at ≥80% oframSize(), matching macOS behavior; below that nothing changes. - No CODEOWNERS entry covers
Source/WTF/wtf/AvailableMemory.cpp.
Extended reasoning...
Overview
Single-line functional change in Source/WTF/wtf/AvailableMemory.cpp: LinuxMemory::footprint() now calls parseIntegerAllowingTrailingJunk<size_t>() instead of parseInteger<size_t>() when reading the second field of /proc/self/statm, plus a two-line explanatory comment. The buffer at parse time contains <resident> <shared> <text> <lib> <data> <dt>\n; parseInteger with the default TrailingJunkPolicy::Disallow skips leading whitespace, parses the resident count, skips one trailing space, then finds the shared-pages digits still present and returns nullopt, which .value_or(0) collapses to 0. The replacement stops at the first non-digit and returns the parsed value.
Security risks
None. This reads a procfs pseudo-file the process already had open, parses an unsigned page count, and multiplies by pageSize under checkedProduct<size_t> with an overflow guard. No external input, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. The code change itself is a trivially-verifiable parser fix — I confirmed the semantics of both functions in StringToIntegerConversion.h (the trailing-junk variant retains ParseIntegerWhitespacePolicy::Allow, so it still consumes the leading space before field 2). The interesting part is not the diff but the consequence: a previously-dead code path (Heap::overCriticalMemoryThreshold() on Linux) now becomes live. That path already runs on macOS, engages only above 80% of ramSize() (physical RAM or cgroup limit), and the PR description includes before/after logGC counts and peak-RSS measurements demonstrating the intended effect and no regression at the default threshold on an unconstrained host.
Other factors
- The bug hunting system found no issues.
- No CODEOWNERS rule matches
Source/WTF/wtf/in this fork's.github/CODEOWNERS. - The PR description is unusually thorough (root cause traced to
TrailingJunkPolicy, cross-referenced against thestrtoull-based reader inlinux/CurrentProcessMemoryStatus.cppthat was already correct, verified experimentally that the pre-fix value was exactly 0 viacriticalGCMemoryThreshold=-1vs0). - No prior reviews from me; only a coderabbit rate-limit notice on the timeline.
Preview Builds
|
Problem
WTF::memoryStatus().memoryFootprintis always 0, sopercentAvailableMemoryInUse()is always 0 andJSC::Heap::overCriticalMemoryThreshold()is never true. JSC's critical-memory GC mode (eden capped atramSize * (1 - criticalGCMemoryThreshold) / 4, full collections only, synchronous sweeps, meant to engage at 80% oframSize()) is dead code on Linux. It works on macOS, where the footprint comes fromtask_info.LinuxMemory::footprint()inSource/WTF/wtf/AvailableMemory.cppskips the first field of/proc/self/statmand parses the rest of the line withparseInteger<size_t>().parseIntegerusesTrailingJunkPolicy::Disallow(StringToIntegerConversion.h), and the five fields after the resident count are still in the buffer, so it returnsnulloptandvalue_or(0)turns the footprint into 0. Upstream WebKit has the same code.ramSize()is the limit (availableMemory()takesuv_get_constrained_memory()), so this is the only mechanism that would slow heap growth before the cgroup OOM killer fires. A 60 MB live set plus object churn in a 128 MiB limit currently peaks at 153 to 160 MB of anonymous memory and is killed; Node runs the same program there.Fix
parseIntegerAllowingTrailingJunk<size_t>(), which stops at the first character after the number.WTF::memoryFootprint()(thestrtoullreader of the same file inlinux/CurrentProcessMemoryStatus.cpp, used byproportionalHeapSize()) was already correct, which is why the RAM-fraction growth tiers worked while the critical threshold did not.BUN_JSC_logGC=1 BUN_JSC_forceRAMSize=128M:criticalGCMemoryThreshold=0(any non-zero footprint is critical): 36 full / 37 eden collections, 0 requests marked(critical), 0 synchronous sweeps. OnlycriticalGCMemoryThreshold=-1engaged the mode, i.e. the value being compared was exactly 0.criticalGCMemoryThreshold=0: 65 full / 0 eden, 63(critical)requests, 65 synchronous sweeps. The default threshold on a host with no limit behaves as before (peak memory unchanged within noise).forceRAMSizeso that the critical eden is 6.4 MiB, threshold so the flag stays on): peak anonymous memory of the churn script goes from 153 MB to 112 to 117 MB with this change alone, and to 100 to 102 MB together with Heap: size the mutator's headroom during a critical collection from the critical eden #450 (numbers in the details block).ramSize()(physical RAM, or the cgroup limit) now gets the same aggressive collection the macOS build already gets there. Below that nothing changes, and the/proc/self/statmread that feeds it was already being performed.Background
Heap::overCriticalMemoryThreshold()(Heap.cpp) re-readspercentAvailableMemoryInUse()directly at the end of every collection and every 100th call from the allocation slow path, and caches the result. When true,collectIfNecessaryOrDefer()caps the bytes allowed per cycle atm_maxEdenSizeWhenCritical,shouldDoFullCollection()returns true, andshouldSweepSynchronously()makesfinalize()sweep and release empty blocks before the mutator resumes.percentAvailableMemoryInUse()ismemoryFootprint / availableMemory(). On Linux the footprint is the resident set size from/proc/self/statm(second field, in pages);availableMemory()ismin(cgroup limit, sysinfo totalram)and is also whatramSize()returns, so the threshold is a fraction of the same number the heap growth heuristics are scaled to.Measurements (peak anonymous RSS / peak RSS in MB, 3 runs each, 4 s of a 60 MB live set plus 80k-object churn per tick, 253 GB host with a 32 GiB cgroup)
Limit emulation:
BUN_JSC_forceRAMSize=26843546 BUN_JSC_criticalGCMemoryThreshold=0.001, which gives the same critical eden (ramSize * (1 - threshold) / 4= 6.4 MiB) and the same 1.24x growth tier as a 128 MiB limit whose process is past 80%, and keeps the flag on, as it is for the whole run in such a limit (the live set plus the binary's pages already exceed 102 MiB). The host has no 128 MiB cgroup available, so the kill itself was not reproduced; the stock binary's 185 to 193 MB peak matches what was measured in the real container.