From 4b44a1528677c46a8a75f04ee806bca1dab4fa69 Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 5 Jul 2026 19:26:28 +0200 Subject: [PATCH] fix(pipeline): MP0 - profiler toggle-path hygiene + Max() seed (review follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review finds, all in the runtime-toggle path or edge math (the steady-state path was confirmed correct): 1. GPU stale-slot across toggle-off/on: the disabled branch now Disposes the GpuFrameTimer (and nulls it) instead of Stop()-ing and keeping it — a kept instance would poll slots left pending from BEFORE the pause on re-enable and report temporally stale GPU samples. The re-enable branch's existing null guard rebuilds the ring fresh. Dispose is safe there: the branch runs at the top of OnRender with the GL context current. 2. Stale stage accumulation across mid-stage toggle-off: a StageScope disposed after the flag flips still calls EndStage, leaving a partial delta in _stageAccumTicks. The re-baseline branch now Array.Clear()s it alongside the GC re-baselining. 3. FrameStatsBuffer.Max() seeded with 0, clamping all-negative windows (the alloc channel can go negative if the boundary ever crosses threads) and disagreeing with Percentile(). Now seeds from the first live sample after the empty check (slots [0.._count) are always the live window regardless of ring wraparound); empty still returns 0 to match Percentile. TDD: Max_AllNegative_ReturnsTrueMax failed (returned 0) before the fix, passes after. Co-Authored-By: Claude Sonnet 5 --- src/AcDream.App/Diagnostics/FrameProfiler.cs | 20 +++++++++++++++++-- .../Diagnostics/FrameStatsBuffer.cs | 9 +++++++-- .../FrameStatsBufferTests.cs | 8 ++++++++ 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/AcDream.App/Diagnostics/FrameProfiler.cs b/src/AcDream.App/Diagnostics/FrameProfiler.cs index dd01f9a1..2ac85dcd 100644 --- a/src/AcDream.App/Diagnostics/FrameProfiler.cs +++ b/src/AcDream.App/Diagnostics/FrameProfiler.cs @@ -75,7 +75,18 @@ public sealed class FrameProfiler : IDisposable bool enabled = RenderingDiagnostics.FrameProfEnabled; if (!enabled) { - if (_wasEnabled) { _gpuTimer?.Stop(); _wasEnabled = false; _lastBoundaryTimestamp = 0; } + if (_wasEnabled) + { + // Dispose (not just Stop) so a later re-enable rebuilds the + // query ring fresh — a kept instance would poll slots left + // pending from BEFORE the pause and report temporally stale + // GPU samples. Safe here: this runs at the top of OnRender + // with the GL context current. + _gpuTimer?.Dispose(); + _gpuTimer = null; + _wasEnabled = false; + _lastBoundaryTimestamp = 0; + } return; } @@ -92,9 +103,14 @@ public sealed class FrameProfiler : IDisposable if (!_wasEnabled) { // First enabled frame (startup or runtime toggle-on): establish - // baselines, emit nothing. + // baselines, emit nothing. Clear any stage ticks a StageScope + // disposed after toggle-off may have accumulated mid-pause — + // EndStage still runs on scopes that were live when the flag + // flipped, and that partial delta must not leak into the first + // re-enabled frame. _wasEnabled = true; _lastReportTicks = DateTime.UtcNow.Ticks; + Array.Clear(_stageAccumTicks); _gc0Base = GC.CollectionCount(0); _gc1Base = GC.CollectionCount(1); _gc2Base = GC.CollectionCount(2); if (_gpuTimer is null && !_wbDiagActive) _gpuTimer = new GpuFrameTimer(gl); if (_wbDiagActive && !_wbDiagNoticePrinted) diff --git a/src/AcDream.App/Diagnostics/FrameStatsBuffer.cs b/src/AcDream.App/Diagnostics/FrameStatsBuffer.cs index 080e6ac8..60773235 100644 --- a/src/AcDream.App/Diagnostics/FrameStatsBuffer.cs +++ b/src/AcDream.App/Diagnostics/FrameStatsBuffer.cs @@ -56,8 +56,13 @@ public sealed class FrameStatsBuffer public long Max() { - long max = 0; - for (int i = 0; i < _count; i++) + if (_count == 0) return 0; // documented empty behavior, matches Percentile + // Seed from the first live sample so all-negative windows (the alloc + // channel can go negative if the boundary ever crosses threads) return + // the true max instead of clamping to 0. Slots [0.._count) are always + // the live window regardless of ring wraparound. + long max = _samples[0]; + for (int i = 1; i < _count; i++) if (_samples[i] > max) max = _samples[i]; return max; } diff --git a/tests/AcDream.App.Tests/FrameStatsBufferTests.cs b/tests/AcDream.App.Tests/FrameStatsBufferTests.cs index 0d4cd438..725f370c 100644 --- a/tests/AcDream.App.Tests/FrameStatsBufferTests.cs +++ b/tests/AcDream.App.Tests/FrameStatsBufferTests.cs @@ -48,4 +48,12 @@ public class FrameStatsBufferTests Assert.Equal(0, buf.Count); Assert.Equal(0, buf.Percentile(0.5)); } + + [Fact] + public void Max_AllNegative_ReturnsTrueMax() + { + var buf = new FrameStatsBuffer(capacity: 4); + buf.Push(-5); buf.Push(-2); buf.Push(-9); + Assert.Equal(-2, buf.Max()); + } }