From e25a5a4f0419263a7563fa7dfd2559ab8e2adeb3 Mon Sep 17 00:00:00 2001 From: m4air Date: Thu, 10 Sep 2026 17:07:04 -0700 Subject: [PATCH] review: band the re-census so a released-memory renderer keeps its peak --- .../src/lib/renderer-memory-sampling.test.ts | 51 +++++++++++++++++++ .../src/lib/renderer-memory-sampling.ts | 37 +++++++++++--- 2 files changed, 81 insertions(+), 7 deletions(-) diff --git a/src/renderer/src/lib/renderer-memory-sampling.test.ts b/src/renderer/src/lib/renderer-memory-sampling.test.ts index f5bfd03e191..d74fd4d034d 100644 --- a/src/renderer/src/lib/renderer-memory-sampling.test.ts +++ b/src/renderer/src/lib/renderer-memory-sampling.test.ts @@ -121,6 +121,57 @@ describe('renderer memory highwater census re-arming', () => { expect(censuses().length).toBeLessThanOrEqual(90) }) + // The literal fb476b1c ending: crossed 600MB, then spent 21h BELOW the mark and died at 577MB. + // A re-census gated on the current value never fires here, which was the whole defect. + it('re-censuses a renderer that crossed a mark and then settled back below it', async () => { + stubFootprint(658) + await tick() + await tick() + expect(censuses()).toHaveLength(1) + const firstCensus = censuses()[0] + + // New workload, permanently below the mark, for 21 hours. + stubFootprint(577) + heapStats.blinkAllocatedKB = 21 * KB + webviewProfile = { browserWebviewCount: 1, registeredBrowserGuestCount: 1 } + for (let minute = 0; minute < 1260; minute += 1) { + await tick() + } + + expect(censuses().length).toBeGreaterThan(1) + expect(censuses().at(-1)).not.toBe(firstCensus) + expect(censuses().at(-1)).toMatchObject({ + thresholdPrivateMB: 600, + privateMB: 577, + blinkAllocatedMB: 21, + browserWebviews: 1 + }) + expect(censuses().length).toBeLessThanOrEqual(90) + }) + + // The retained crumb is one slot per mark, so a refresh overwrites the peak census. A renderer + // that released its memory must keep the peak evidence rather than ship privateMB:50 under a + // thresholdPrivateMB:600 label. + it('keeps the peak census when a renderer releases its memory', async () => { + stubFootprint(1200) + heapStats.blinkAllocatedKB = 900 * KB + await tick() + await tick() + // 1200MB crosses both private marks, so both slots are censused at the peak. + expect(censuses()).toHaveLength(2) + + // Leak released: 6 hours far below the mark. + stubFootprint(50) + heapStats.blinkAllocatedKB = 1 * KB + for (let minute = 0; minute < 360; minute += 1) { + await tick() + } + + // No refresh fired, so the retained censuses still describe the peak. + expect(censuses()).toHaveLength(2) + expect(censuses().map((c) => c.privateMB)).toEqual([1200, 1200]) + }) + it('emits at most one census per mark while a renderer oscillates around it', async () => { stubFootprint(601) await tick() diff --git a/src/renderer/src/lib/renderer-memory-sampling.ts b/src/renderer/src/lib/renderer-memory-sampling.ts index 8e246943a85..529921f2505 100644 --- a/src/renderer/src/lib/renderer-memory-sampling.ts +++ b/src/renderer/src/lib/renderer-memory-sampling.ts @@ -31,6 +31,10 @@ const RENDERER_PRIVATE_HIGHWATER_MB = [600, 1000] as const // stale census. 15min = 1 census per 15 samples, and the store keys retained // highwater crumbs by mark, so a refresh replaces the stale one. const RENDERER_HIGHWATER_RECENSUS_MS = 15 * 60_000 +// Why 0.9 and not 0: the retained crumb is one slot per mark, so a refresh overwrites the census +// taken at the peak. Refreshing only while the renderer is still near the mark keeps the peak +// evidence for a renderer that released its memory, and still re-censuses one that stays large. +const RENDERER_HIGHWATER_RECENSUS_BAND = 0.9 export type RendererSurface = 'main' | 'dashboard-popout' @@ -170,7 +174,7 @@ function recordRendererMemoryHighwater( let crossedThreshold = false if (ratio !== null) { for (const threshold of RENDERER_MEMORY_HIGHWATER_RATIOS) { - if (ratio >= threshold && isHighwaterMarkArmed(emittedHighwaterRatios, threshold, nowMs)) { + if (isHighwaterCensusDue(emittedHighwaterRatios, threshold, nowMs, ratio)) { crossedThreshold = true break } @@ -178,7 +182,7 @@ function recordRendererMemoryHighwater( } if (privateMB !== null) { for (const mark of RENDERER_PRIVATE_HIGHWATER_MB) { - if (privateMB >= mark && isHighwaterMarkArmed(emittedPrivateHighwaterMarks, mark, nowMs)) { + if (isHighwaterCensusDue(emittedPrivateHighwaterMarks, mark, nowMs, privateMB)) { crossedThreshold = true break } @@ -205,7 +209,7 @@ function recordRendererMemoryHighwater( }) if (ratio !== null) { for (const threshold of RENDERER_MEMORY_HIGHWATER_RATIOS) { - if (ratio < threshold || !isHighwaterMarkArmed(emittedHighwaterRatios, threshold, nowMs)) { + if (!isHighwaterCensusDue(emittedHighwaterRatios, threshold, nowMs, ratio)) { continue } emittedHighwaterRatios.set(threshold, nowMs) @@ -217,7 +221,7 @@ function recordRendererMemoryHighwater( } if (privateMB !== null) { for (const mark of RENDERER_PRIVATE_HIGHWATER_MB) { - if (privateMB < mark || !isHighwaterMarkArmed(emittedPrivateHighwaterMarks, mark, nowMs)) { + if (!isHighwaterCensusDue(emittedPrivateHighwaterMarks, mark, nowMs, privateMB)) { continue } emittedPrivateHighwaterMarks.set(mark, nowMs) @@ -229,10 +233,29 @@ function recordRendererMemoryHighwater( } } -/** Why monotonic: a wall-clock correction must not stretch or collapse the window. */ -function isHighwaterMarkArmed(emitted: Map, mark: number, nowMs: number): boolean { +/** + * A mark is due on its first crossing, and thereafter every `RENDERER_HIGHWATER_RECENSUS_MS` + * while the renderer stays within `RENDERER_HIGHWATER_RECENSUS_BAND` of it. Why not a strict + * `value >= mark`: report fb476b1c crossed 600MB then died 21h later at 577MB, and a re-census + * gated on the mark never fires again — that is the stale-census bug. Why not value-independent + * either: the refresh overwrites the one retained slot, so a renderer that released its memory + * would lose the census taken at its peak. Why monotonic: a wall-clock correction must not + * stretch or collapse the window. + */ +function isHighwaterCensusDue( + emitted: Map, + mark: number, + nowMs: number, + value: number +): boolean { const lastEmittedAtMs = emitted.get(mark) - return lastEmittedAtMs === undefined || nowMs - lastEmittedAtMs >= RENDERER_HIGHWATER_RECENSUS_MS + if (lastEmittedAtMs === undefined) { + return value >= mark + } + return ( + nowMs - lastEmittedAtMs >= RENDERER_HIGHWATER_RECENSUS_MS && + value >= mark * RENDERER_HIGHWATER_RECENSUS_BAND + ) } function isFiniteHeapBytes(value: number | undefined): value is number {