From 52aa064dac8caafa75c0d05ac9eefbc06b21c7bf Mon Sep 17 00:00:00 2001 From: m4air Date: Thu, 10 Sep 2026 17:23:59 -0700 Subject: [PATCH] review: re-anchor time-above-mark and stop refreshes evicting the peak census --- .../crash-reporting/crash-breadcrumb-store.ts | 4 +- .../src/lib/renderer-memory-sampling.test.ts | 22 ++++++++++ .../src/lib/renderer-memory-sampling.ts | 41 ++++++++++++++++++- 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/src/main/crash-reporting/crash-breadcrumb-store.ts b/src/main/crash-reporting/crash-breadcrumb-store.ts index 2c0b68b043e..1ff682e4dd4 100644 --- a/src/main/crash-reporting/crash-breadcrumb-store.ts +++ b/src/main/crash-reporting/crash-breadcrumb-store.ts @@ -72,7 +72,9 @@ export function recordCrashBreadcrumb( } const retainedKey = retainedBreadcrumbKey(breadcrumb) if (retainedKey) { - retainedBreadcrumbs.delete(retainedKey) + // Why no delete-then-set: re-inserting moves the key to the back, so a slot that refreshes + // outranks one that froze at its peak — and the frozen peak census is exactly the evidence + // worth keeping. Plain set preserves insertion order, keeping eviction FIFO by first crossing. retainedBreadcrumbs.set(retainedKey, breadcrumb) while (retainedBreadcrumbs.size > MAX_RETAINED_BREADCRUMBS) { const oldestKey = retainedBreadcrumbs.keys().next() diff --git a/src/renderer/src/lib/renderer-memory-sampling.test.ts b/src/renderer/src/lib/renderer-memory-sampling.test.ts index cad8fb3b8f5..cc48730bb66 100644 --- a/src/renderer/src/lib/renderer-memory-sampling.test.ts +++ b/src/renderer/src/lib/renderer-memory-sampling.test.ts @@ -190,6 +190,28 @@ describe('renderer memory highwater census re-arming', () => { expect(censuses().at(-1)).toMatchObject({ thresholdPrivateMB: 600, aboveMarkMinutes: 120 }) }) + // Sawtooth (build, GC, build) is the ordinary shape of renderer memory. Two spikes hours apart + // must not read as sustained pressure, or triage starts a leak hunt that has no leak. + it('re-anchors minutes-above-mark after a long spell below the band', async () => { + stubFootprint(658) + await tick() + await tick() + expect(censuses().at(-1)).toMatchObject({ aboveMarkMinutes: 0 }) + + // 10 hours far below the band, then back up. + stubFootprint(100) + for (let minute = 0; minute < 600; minute += 1) { + await tick() + } + stubFootprint(700) + await tick() + await tick() + + // Not 602: the renderer spent those 600 minutes at 100MB. + expect(censuses().at(-1)).toMatchObject({ thresholdPrivateMB: 600 }) + expect(censuses().at(-1)?.aboveMarkMinutes).toBeLessThanOrEqual(2) + }) + 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 39272a080f1..30ef3664f26 100644 --- a/src/renderer/src/lib/renderer-memory-sampling.ts +++ b/src/renderer/src/lib/renderer-memory-sampling.ts @@ -53,7 +53,11 @@ type HeapMetrics = BrowserPerformanceMemory & { /** Mark -> monotonic time it last emitted a census. */ /** First crossing is kept separately: a refresh replaces the crumb's own `createdAt`. */ -type HighwaterMarkState = { firstCrossedAtMs: number; lastEmittedAtMs: number } +type HighwaterMarkState = { + firstCrossedAtMs: number + lastEmittedAtMs: number + lastWithinBandAtMs: number +} const emittedHighwaterRatios = new Map() const emittedPrivateHighwaterMarks = new Map() let lastProcessFootprint: RendererProcessMemory | null = null @@ -173,6 +177,16 @@ function recordRendererMemoryHighwater( const privateMB = footprint === null ? null : (toMegabytes(footprint.privateKB * BYTES_PER_KILOBYTE) ?? null) const nowMs = performance.now() + if (ratio !== null) { + for (const threshold of RENDERER_MEMORY_HIGHWATER_RATIOS) { + noteHighwaterBandResidency(emittedHighwaterRatios, threshold, nowMs, ratio) + } + } + if (privateMB !== null) { + for (const mark of RENDERER_PRIVATE_HIGHWATER_MB) { + noteHighwaterBandResidency(emittedPrivateHighwaterMarks, mark, nowMs, privateMB) + } + } let crossedThreshold = false if (ratio !== null) { for (const threshold of RENDERER_MEMORY_HIGHWATER_RATIOS) { @@ -246,6 +260,29 @@ function recordRendererMemoryHighwater( * would lose the census taken at its peak. Why monotonic: a wall-clock correction must not * stretch or collapse the window. */ +/** + * Tracks whether the mark is still occupied. Why re-anchor after a gap: the census reports how + * long the renderer has been heavy, and two spikes hours apart are not sustained pressure — + * sawtooth (build, GC, build) is the ordinary shape of renderer memory. + */ +function noteHighwaterBandResidency( + emitted: Map, + mark: number, + nowMs: number, + value: number +): void { + const state = emitted.get(mark) + if (state === undefined || value < mark * RENDERER_HIGHWATER_RECENSUS_BAND) { + return + } + const lapsed = nowMs - state.lastWithinBandAtMs > RENDERER_HIGHWATER_RECENSUS_MS + emitted.set(mark, { + firstCrossedAtMs: lapsed ? nowMs : state.firstCrossedAtMs, + lastEmittedAtMs: state.lastEmittedAtMs, + lastWithinBandAtMs: nowMs + }) +} + /** * Records this emission and returns minutes since the mark was first crossed. Why carry it in the * payload: a refresh replaces the retained crumb's `createdAt`, so without this "how long has the @@ -257,7 +294,7 @@ function stampHighwaterMark( nowMs: number ): number { const firstCrossedAtMs = emitted.get(mark)?.firstCrossedAtMs ?? nowMs - emitted.set(mark, { firstCrossedAtMs, lastEmittedAtMs: nowMs }) + emitted.set(mark, { firstCrossedAtMs, lastEmittedAtMs: nowMs, lastWithinBandAtMs: nowMs }) return Math.round((nowMs - firstCrossedAtMs) / 60_000) }