Fix GC.GetTotalMemory returning negative under regions GC#130888
Fix GC.GetTotalMemory returning negative under regions GC#130888kkokosa wants to merge 2 commits into
Conversation
GCHeap::ApproxTotalBytesInUse computes gen0's live size as gen0_size - gen0_frag using unsigned arithmetic. gen0_frag (free_list_space + free_obj_space) is a per-generation total spanning every gen0 region, but gen0_size only summed region spans up to the ephemeral region, dropping any gen0 region linked after it (for example a pinned region swept in place). When the dropped regions' fragmentation exceeded their omitted span, gen0_frag exceeded gen0_size and the unsigned subtraction underflowed, surfacing as a negative value from GC.GetTotalMemory. Walk every gen0 region so the counted span matches the fragmentation total. Also reorder the free-list discard bookkeeping in a_fit_free_list_p so a lock-free reader observes a harmless under-count instead of a transient over-count. Segments are unaffected: the single contiguous ephemeral segment already bounds all gen0 fragmentation within the counted span. Adds a regression test that drives concurrent allocation with a large, continuously-refreshed pinned-object ring while probing GC.GetTotalMemory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 975e6875-e7d4-4172-8d1f-5ed514017a4d
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @anicka-net, @dotnet/gc |
|
@dotnet-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness bug in CoreCLR GC accounting where GC.GetTotalMemory(false) could intermittently surface a negative long under regions GC, by ensuring gen0 span accounting covers all gen0 regions and by adjusting related free-space bookkeeping. It also adds a regression test intended to reproduce the negative-value symptom under concurrent allocation/pinning.
Changes:
- CoreCLR GC: In regions builds, compute gen0 size by walking all gen0 regions instead of stopping at the ephemeral region (
GCHeap::ApproxTotalBytesInUse). - CoreCLR GC allocation: Reorder discard-path counter updates to reduce transient fragmentation over-count exposure to concurrent readers.
- Tests: Add a new priority-1, process-isolated GC API regression test exercising concurrent pin churn +
GC.GetTotalMemory(false)probing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/gc/interface.cpp | Fix gen0 size accounting under regions by walking the full gen0 region chain. |
| src/coreclr/gc/allocation.cpp | Reorder discard-path free-space counter updates to reduce transient over-count windows. |
| src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs | New regression test that probes for negative GC.GetTotalMemory(false) while inducing retained gen0 regions via pin churn. |
| src/tests/GC/API/GC/GetTotalMemoryConcurrent.csproj | New test project configuration (process isolation, priority 1, GCStress incompatible). |
| // A few seconds is far more than enough: the unfixed runtime fails almost immediately. | ||
| // Even a single allocating thread is sufficient to build the region layout that triggers | ||
| // the miscount, so this does not depend on the number of processors. | ||
| const int DurationSeconds = 8; | ||
| int workers = Math.Max(2, Math.Min(8, Environment.ProcessorCount)); |
There was a problem hiding this comment.
Improved by 7ce70ab2ec7747a620650c64afd650a6cbfdd348
| // Update ordering for lock-free readers (e.g. GCHeap::ApproxTotalBytesInUse): | ||
| // remove the bytes from free_list_space before adding them to free_obj_space so | ||
| // the transient a concurrent reader can observe is a harmless under-count rather | ||
| // than an over-count that would make gen0 fragmentation (free_list_space + | ||
| // free_obj_space) appear larger than the gen0 span and underflow (issue #106712). |
There was a problem hiding this comment.
The only other place not under gc lock is a sibling allocator path for UOH (a_fit_free_list_uoh_p) but I would not touch it. This change was cosmetic change in the first place, just because it was easy. It is not for UOH path. @janvorli wdyt?
There was a problem hiding this comment.
I would leave out the reordering of the updates. It is not guaranteed to be seen in the code order e.g. on arm64 anyways and the compiler can reorder the instructions. Unless we want to add the barriers as @jkotas suggested.
There was a problem hiding this comment.
At minimum, the comment is misleading. The comment should make it clear that this is best effort and that GetTotalMemory may return inaccurate results.
| dprintf (3, ("couldn't use this free area, discarding")); | ||
| generation_free_obj_space (gen) += free_list_size; | ||
|
|
||
| // Update ordering for lock-free readers (e.g. GCHeap::ApproxTotalBytesInUse): |
There was a problem hiding this comment.
Hardware or C/C++ compiler can reorder the memory reads/writes. Does this need to be read/written with the right memory barriers (VolatileLoad/VolatileStore)?
There was a problem hiding this comment.
this is mostly by design where this accounting is best effort. Its a simple increment/decrement in most places like:
There was a problem hiding this comment.
It means that this change is just making the problem happen less often, not actually fixing it.
Also, if we want to keep this behavior, it should be documented. The current documentation for forceFullCollection does not say anything about a potential for returning grossly inaccurate result with forceFullCollection: false.
There was a problem hiding this comment.
As explicitly said in the PR, this reordering is to occasionally under-count rather than over-count, but it does not solve inconsitency. Over-count is much more confusing. And imho it is not worth it to fight for it, docs mentions should work.
There was a problem hiding this comment.
It means that this change is just making the problem happen less often, not actually fixing it.
The main fix in this change is for the potentially massive miscounting we are getting with regions due to the not taking into account some regions for the space while using all them all for the holes size calculation.
| // that survived in place) is counted up to its heap_segment_allocated. Stopping at the | ||
| // ephemeral region here used to drop those regions' span while still subtracting their | ||
| // fragmentation, making gen0_frag exceed gen0_size and underflowing the subtraction below | ||
| // (issue #106712). |
There was a problem hiding this comment.
| // (issue #106712). |
Is the link to the issue required to understand the code?
There was a problem hiding this comment.
Yeah, but tbh maybe the comment is too extensive in the first place?
There was a problem hiding this comment.
Right, AI likes to include a lot of fluff in the comments.
Fixes #106712.
Summary
GC.GetTotalMemory(false)intermittently returns a negative value under the regions GC (the default since .NET 7). The value originates inGCHeap::ApproxTotalBytesInUse, which estimates gen0 live bytes asgen0_size - gen0_fragusing unsignedsize_tarithmetic.gen0_frag(free-list + free-object space) is a per-generation total that spans every gen0 region, butgen0_sizeonly summed region spans up to the ephemeral region. When gen0 retains another region (for example one pinned in place and swept rather than compacted), that region's fragmentation stays ingen0_fragwhile its span is dropped fromgen0_size, sogen0_frag > gen0_size, the subtraction underflows, and the public API casts the near-2^64size_tto a negativelong.In plain words: gen0 live bytes are "space minus holes." With segments, gen0 is a single contiguous block, so the holes are always a subset of the space and the subtraction can't go negative. With regions, gen0 is a linked list of blocks, and the code summed the holes over the whole list but measured the space over only part of it — so the holes could exceed the space and the unsigned subtraction wrapped.
The negative is only the visible symptom. The same miscount also makes
GetTotalMemorysilently under-report gen0 whenever the dropped span is smaller thangen0_frag(positive but too low) — observed returning ~0.4 MB where the correct value was ~16.0 MB.Root cause
Two mechanisms, neither a GC race:
gen0_sizewhile its fragmentation stayed ingen0_frag.a_fit_free_list_p's free-list discard path,free_obj_spacewas incremented beforefree_list_spacewas decremented, so a lock-free reader could briefly observe both — a transient over-count.Why a forced
GC.GetTotalMemory(true)(and continued allocation) self-healsThe reported observation that a forced
GetTotalMemory(true)returns a sane value and stops the negatives — and that continued allocation walks the value back positive — follows directly from Mechanism #1. The bytes are never wrong; only the lock-free measurement is.GetTotalMemory(true)forces blocking, compacting GC(s). Without pinning, gen0 is fully compacted: survivors are relocated, the swept-in-place regions linked after the ephemeral one are reclaimed, and gen0's free-space counters reset. Gen0 collapses to a clean layout where the counted span again covers all its fragmentation (gen0_frag ≤ gen0_size), so the next computation is correct — the bad layout is repaired.alloc_allocatedadvances in the ephemeral region, so the countedgen0_sizegrows; once it climbs back abovegen0_fragthe subtraction stops underflowing. The value flip-flops as allocation and GCs change which snapshot a read catches.The fix
src/coreclr/gc/interface.cpp— walk every gen0 region when computinggen0_size(removing the earlybreakat the ephemeral region), so the counted span matches the per-generation fragmentation total. This mirrors howgeneration_size()inplan_phase.cppwalks the whole region chain.src/coreclr/gc/allocation.cpp— reorder the discard-branch bookkeeping (unlink→free_list_space -=→free_obj_space +=) so the only transient a concurrent reader can observe is a harmless under-count instead of an over-count that could underflow. The final state is identical; this flips the transient's direction rather than removing it.No clamping: the negative is fixed at its source (count all gen0 regions), not masked with a
(gen0_size > gen0_frag) ? ... : 0guard.Why this is regions-only
With segments, gen0 lives inside a single contiguous ephemeral segment; its fragmentation is by construction a subset of the counted span, so the subtraction can never underflow. The bug requires gen0 to span multiple regions with one retained past the ephemeral one — only possible under regions. Empirically, with the same source and workload: built-in
coreclr.dll(regions) → NEG −12,194,016 @ 97 ms; standaloneclrgcexp.dll(regions) → NEG −12,158,984 @ 96 ms; standaloneclrgc.dll(segments) → 0 negatives over 13.5 M probes. Matches the field report thatDOTNET_GCName=clrgc.dlldoesn't repro.Reproduction
Two repros, both run by swapping only
coreclr.dllbetween fixed and unfixed builds:Amplifier (reliable, used by the regression test). A console app keeps a large, continuously-refreshed ring of pinned tiny objects (forcing retained gen0 regions) while flooding gen0 with garbage and probing
GC.GetTotalMemoryon another thread. Fails in well under a second (~60–70 K probes, e.g. NEG −12,183,896), and fires even single-threaded (threads=1) — confirming a structural miscount, not a race.The reporter's exact repros from the issue (only change: a wall-clock cap), which use no pinning:
Takeaway: pinning is a reliable amplifier, not a prerequisite — heavy multithreaded churn triggers frequent GCs that transiently reshuffle the gen0 region list into the same buggy layout.
Testing
New regression test
src/tests/GC/API/GC/GetTotalMemoryConcurrent.cs(+.csproj), priority 1, process-isolated,GCStressIncompatible,[Fact] TestEntryPointidiom. It runs the pinned + concurrent-probe workload and fails if any probe is negative. Pinning converts the rare transient into a deterministic, structural repro that fires in milliseconds.Proven fail → pass (swapping only
coreclr.dllfixed↔unfixed), on both Debug and full Release:-11,997,832after 100,285 probes-11,970,520after only 83,187 probesThe Release run proves the fix holds on an optimized runtime; the unfixed Release binary reproduces the negative even faster than Debug.
Existing suite (no regressions)
GC/API/GC/GetTotalMemory,TotalMemory,TotalMemory2,GetGCMemoryInfo,GetTotalAllocatedBytes,GetAllocatedBytesForCurrentThread— all exit 100 against the fixed runtime (default regions GC).The ".NET 10 fixed the single-threaded repro" claim — disproved
The issue notes the single-threaded (no-pin) repro was "fixed on .NET 10." Testing shows this is a measurement artifact, not a code fix: the accounting site is byte-identical since regions shipped in .NET 7 (
git log -Son the guard block returns only PR #59283 plus mechanicalgc.cpp → interface.cppsplit commits — no .NET 8/9/10 change to the gen0 loop or subtraction). Against installed retail runtimes:The structural underflow is single-thread-reproducible on every shipping version (15–49 ms with pinning). The no-pin single-threaded case is simply a rare transient on all versions (identical .NET 9 vs 10 behavior) — the perceived ".NET 10 fix" is timing noise, not a behavioral change. This PR is the first actual correction of the root cause.
Impact & blast radius
ApproxTotalBytesInUseis reached only viaGCHeap::GetTotalBytesInUse, whose only callers are theSystem.GC.GetTotalMemory(bool)QCall (CoreCLR) andRhpGetTotalBytesInUse(NativeAOT). The one in-box managed consumer isRuntimeEventSource'sgc-heap-sizePollingCounter(GC.GetTotalMemory(false) / 1e6), which surfaces the negative through EventCounters /dotnet-counters/ EventPipe / APM telemetry.forceFullCollection: truestabilization loop inGetTotalMemory(bool), whosediff = (newSize − size) / sizeconvergence test was meaningless whilesizewas underflowed.interface.cppadds a bounded region walk (gen0 region count, single digits) under the already-heldgc_lock;allocation.cpponly reorders two counter stores.Performance impact
GetTotalMemoryis a diagnostic API (thegc-heap-sizecounter polls it ~1×/sec), not a hot path. Local A/B (same build, onlycoreclr.dllswapped; no allocation in the timed section; batched quantum-free mean; workstation GC, x64):Release (shipping configuration — authoritative):
pins=0, single gen0 region)pins=40000, several regions)Debug (checked; absolute numbers and delta both inflated by contract checks):
pins=0)pins=40000)The common case is free on either config (a single gen0 region means the fix walks the same one region). The Debug fragmented +53 ns is Debug-accessor overhead (contract checks on
heap_segment_next/_mem/_allocated,in_range_for_segment), not a real cost — Release shows ≈0 in both scenarios. Cost is bounded by the (inherently small) gen0 region count.Note
This pull request was prepared with the assistance of AI (GitHub Copilot). The root-cause analysis, fix, and regression test were reviewed by me before submitting.