Fix non-monotonic GC.GetTotalAllocatedBytes under DATAS (Server GC)

[kkokosa]

Summary

GCHeap::GetTotalAllocatedBytes() summed each heap's total_alloc_bytes only over the
currently-active heaps [0, n_heaps). Under DATAS (Dynamic Adaptation To Application Sizes,
default-on for Server GC in .NET 9+), the active heap count shrinks and grows at run time, so the
returned value could drop when heaps were decommissioned (and jump back when re-added), making
GC.GetTotalAllocatedBytes() non-monotonic.

Fix: sum over all heaps that have ever existed [0, n_max_heaps).

-    for (int i = 0; i < gc_heap::n_heaps; i++)
+    for (int i = 0; i < gc_heap::n_max_heaps; i++)

Root cause

• Per-heap total_alloc_bytes_soh/uoh is a durable accumulator: it only ever increases while a
  heap is active, is frozen (not reset) while decommissioned, and is never absorbed onto another
  heap. decommission_heap(), recommission_heap(), and change_heap_count() deliberately leave it
  untouched (the only reset is per-heap init at startup).
• Allocation only happens on active heaps, and all n_max_heaps heaps are created once at startup
  (g_heaps is sized to n_max_heaps; n_max_heaps is set once and never reduced; the runtime
  already walks [n_heaps, n_max_heaps), e.g. check_decommissioned_heap).
• Therefore Σ[0, n_max_heaps) is the true, monotonic cumulative total. The old Σ[0, n_heaps)
  excluded decommissioned heaps' frozen bytes, so it under-reported by that amount whenever the
  heap count was below its maximum — i.e. it dropped on every DATAS shrink. When DATAS is off,
  n_heaps == n_max_heaps, so behavior is unchanged.

Impact on the two public APIs

Both GC.GetTotalAllocatedBytes(precise: false) and (precise: true) call this function.

• Precise path has no smoothing → exposed the drops directly (BenchmarkDotNet MemoryDiagnoser
  reported random 0 B and hallucinated-high per-op numbers — issue Measured allocations randomly zero #118826).
• Non-precise (default) path has a high_watermark clamp that never returns less than before →
  the drops were masked into a stuck plateau at the last (higher, more-correct) peak until real
  allocation climbed back past it (issue Allocation Rate / GC.GetTotalAllocatedBytes() stops updating #122299).

This fix makes the raw value monotonic at the source, resolving both surfaces.

Validation (A/B, private Release CoreCLR)

Server GC + regions + DATAS (defaults). Identical workload; only coreclr.dll swapped
(hash-verified live before each run). Harness drives oscillating load + per-iteration forced
blocking gen2 GC and counts contamination-proof final < initial decreases.

Variant	Runs	Result
unfixed	5	ALL reproduce: 5–12 drops/run, max drop ~64–94 GB
fixed	4	ALL clean: 0 drops (also 1.13M-probe monotonic loop)

Regression test

Adds src/tests/GC/API/GC/GetTotalAllocatedBytesServerGC.{cs,csproj} (priority 1, Server GC via
DOTNET_gcServer=1 + DOTNET_GCDynamicAdaptationMode=1, GCStressIncompatible). It drives an
oscillating allocation load with frequent forced blocking gen2 GCs and asserts the precise
cumulative total never decreases (beyond a 64 MB tolerance for the by-design per-thread
allocation-context jitter). Proven fail→pass on Release: unfixed exits 101 (observed a 22.5 GB
drop), fixed exits 100 (pass, stable across repeated runs).

Related

• Fixes Measured allocations randomly zero #118826
• Should also address Allocation Rate / GC.GetTotalAllocatedBytes() stops updating #122299 (same root cause; the non-precise high_watermark clamp surfaces it
  as a stuck/constant value instead of a drop).

---

Note

This pull request was generated with the assistance of AI (GitHub Copilot).

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @anicka-net, @dotnet/gc
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR fixes GCHeap::GetTotalAllocatedBytes() on Server GC when DATAS can dynamically change the active heap count, by ensuring the total includes allocation counters from all heaps that have ever existed (not just currently-active heaps). It also adds a targeted GC API regression test to validate monotonic behavior for the precise overload under Server GC + DATAS.

Changes:

• Update CoreCLR GC accounting to sum per-heap total_alloc_bytes_* across n_max_heaps instead of n_heaps.
• Add a new priority-1 CoreCLR-only regression test project that exercises Server GC + DATAS heap-count oscillation and asserts the precise total allocated bytes counter does not decrease.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/gc/interface.cpp	Adjusts GCHeap::GetTotalAllocatedBytes() to aggregate across n_max_heaps for monotonic totals under dynamic heap-count changes.
src/tests/GC/API/GC/GetTotalAllocatedBytesServerGC.csproj	Adds a Server-GC + DATAS configured test project (process-isolated, priority 1, CoreCLR-only).
src/tests/GC/API/GC/GetTotalAllocatedBytesServerGC.cs	Adds a regression test that drives allocation/load oscillation and checks GC.GetTotalAllocatedBytes(precise: true) doesn’t drop beyond a tolerance.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "ca2cf687e7d6fe50f04b231422e79aa3463b894a",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "d349aa7988e9adf0ee0f5f0473886c0582975955",
  "last_reviewed_commit": "ca2cf687e7d6fe50f04b231422e79aa3463b894a",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "d349aa7988e9adf0ee0f5f0473886c0582975955",
  "last_recorded_worker_run_id": "29744579983",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "9d7d71acdc5825d1bbb9ec2668a4058c5c8b3a5f",
      "review_id": 4734459012
    },
    {
      "commit": "ca2cf687e7d6fe50f04b231422e79aa3463b894a",
      "review_id": 4735253486
    }
  ]
}

[github-actions]

Holistic Review

Motivation: GCHeap::GetTotalAllocatedBytes() summed each heap's cumulative total_alloc_bytes_soh/uoh only over the currently-active heaps [0, n_heaps). Under DATAS (default-on for Server GC), the active heap count shrinks and grows at run time, so decommissioned heaps' frozen (but real) accumulated bytes were dropped from the sum, making the public GC.GetTotalAllocatedBytes() non-monotonic (issues #118826 and #122299). This is a genuine correctness bug in a documented monotonic counter.

Approach: The one-line change iterates over [0, n_max_heaps) instead of [0, n_heaps), so the sum includes all heaps that have ever existed. This is the minimal, targeted fix. A priority-1 regression test (src/tests/GC/API/GC/GetTotalAllocatedBytesServerGC.{cs,csproj}) drives an oscillating allocation load with frequent forced blocking gen2 GCs under Server GC + DATAS and asserts the precise cumulative total never decreases beyond a 64 MB tolerance.

Summary: The fix is correct and well-scoped. I verified the premise against the current tree: initialize_gc is called with nhp = n_max_heaps (interface.cpp:518-519), g_heaps is allocated with number_of_heaps == n_max_heaps and every heap is constructed once at startup (init.cpp:999), the only reset of total_alloc_bytes_soh/uoh is per-heap init (gc.cpp:3580-3581) — decommission/recommission/change_heap_count leave it untouched — and existing code already safely walks [n_heaps, n_max_heaps) (e.g. diagnostics.cpp:1559), confirming those g_heaps[i] slots are non-null and valid. When DATAS is off, n_heaps == n_max_heaps, so behavior is unchanged. The change is low-risk and confined to the GC portability surface (no interface change).

Detailed Findings

No blocking issues.

Minor, non-blocking observations on the regression test (body-only, no precise changed-line action required):

• The test runs a fixed 20-second budget with a per-iteration forced blocking gen2 GC and multiple burst allocator threads. This adds meaningful wall-clock cost to the GC test suite. It is gated to CoreCLR and marked GCStressIncompatible, which is appropriate, but consider whether the 20s window could be shortened while still reliably exercising a DATAS shrink.
• The test is inherently dependent on DATAS actually oscillating the heap count to exercise the bug; if the heap count never shrinks on a given machine it degrades to a trivial pass rather than a false failure, which is the safe direction. Worth being aware of for coverage expectations.
• These are observations only; the correctness fix and the fail→pass validation described in the PR make the test a reasonable guard.

Verdict: LGTM.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 67.7 AIC · ⌖ 10.6 AIC · ⊞ 10K

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[github-actions]

Holistic Review

Motivation: GCHeap::GetTotalAllocatedBytes() summed each heap's cumulative total_alloc_bytes_soh/uoh only over the currently-active heaps [0, n_heaps). Under DATAS (default-on for Server GC), the active heap count shrinks and grows at run time, so decommissioned heaps' frozen (but real) accumulated bytes were dropped from the sum, making the public GC.GetTotalAllocatedBytes() non-monotonic. This is a genuine correctness bug in a documented monotonic counter.

Approach: The one-line change iterates over [0, n_max_heaps) instead of [0, n_heaps), so the sum includes all heaps that have ever existed. A priority-1 regression test drives an oscillating allocation load with frequent forced blocking gen2 GCs under Server GC + DATAS and asserts the precise cumulative total never decreases beyond a 64 MB tolerance. The new commit in this range only removes an unused s_sink field from that test.

Summary: The incremental change since the prior review (commit ca2cf68) is a pure cleanup: it deletes the unused private static volatile object s_sink; field from GetTotalAllocatedBytesServerGC.cs. The field was never read or written, so its removal is safe and does not affect the test's behavior or the DATAS oscillation it exercises. The cumulative fix and test remain correct and well-scoped, and the change is low-risk and confined to the GC portability surface.

Assessment History

• review 4734459012 reviewed commit 9d7d71a with verdict LGTM. Current verdict is LGTM. Assessment unchanged: motivation, approach, and risk are identical; the only new patch (removing an unused test field) is cosmetic and does not alter correctness, coverage, or risk.

Detailed Findings

No blocking issues. The incremental change (removing the unused s_sink field) is a correct, harmless cleanup and improves the test slightly.

Verdict: LGTM.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 47.7 AIC · ⌖ 20.1 AIC · ⊞ 10K

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/tests/GC/API/GC/GetTotalAllocatedBytesServerGC.cs:86

• Joining burst threads with a timeout means a thread can still be allocating into the subsequent QUIET period, which risks a false-pass (DATAS never shrinks) and also makes it possible for the using var burstCts to dispose while still being observed by workers. If a burst thread doesn't stop promptly after cancellation, fail the test rather than continuing.

                // Always cancel and join the burst threads so no allocator outlives the
                // BURST -- this guarantees the following QUIET period is truly allocation-free.
                burstCts.Cancel();
                foreach (var t in burst)
                    t.Join(2000);

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/tests/GC/API/GC/GetTotalAllocatedBytesServerGC.cs:87

• The BURST teardown comment says joining the burst threads "guarantees" the following QUIET period is allocation-free, but Thread.Join(2000) is not checked. If any join times out, allocator threads may continue running into the QUIET phase, making the test non-deterministic and potentially missing the regression.

Consider checking the Join return value and failing fast if a thread didn’t stop within a reasonable timeout.

                // Always cancel and join the burst threads so no allocator outlives the
                // BURST -- this guarantees the following QUIET period is truly allocation-free.
                burstCts.Cancel();
                foreach (var t in burst)
                    t.Join(2000);

src/tests/GC/API/GC/GetTotalAllocatedBytesServerGC.cs:133

• controller.Join(3000) is a timed join whose return value is ignored. If the controller thread doesn’t stop promptly after cancellation, it can keep spawning BURST threads and allocating, which can interfere with test completion and cleanup.

Check the Join result and fail if cleanup didn’t complete.

        finally
        {
            cts.Cancel();
            controller.Join(3000);
        }

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/tests/GC/API/GC/GetTotalAllocatedBytesServerGC.cs:92

• Thread.Join(timeout) returns a bool indicating whether the thread actually terminated. Ignoring it can let allocator threads continue running into the intended QUIET window, making the test nondeterministic (and potentially masking DATAS shrink behavior). Consider failing fast if any burst thread fails to join within the timeout.

                // Always cancel and join the burst threads so no allocator outlives the
                // BURST -- this guarantees the following QUIET period is truly allocation-free.
                burstCts.Cancel();
                foreach (var t in burst)
                    t.Join(2000);

src/tests/GC/API/GC/GetTotalAllocatedBytesServerGC.cs:137

• controller.Join(3000) is a timed wait; if it times out, the test will proceed while the controller thread is still running. Failing the test on timeout makes the behavior deterministic and avoids background work continuing unexpectedly.

        {
            cts.Cancel();
            controller.Join(3000);
        }