From de0ff8683845e4b1bcce3e6845d8d1585e20bcf1 Mon Sep 17 00:00:00 2001 From: m4air Date: Thu, 10 Sep 2026 17:34:53 -0700 Subject: [PATCH] review: measure observed band residency and survive a stalled sampler --- .../src/lib/renderer-memory-sampling.test.ts | 32 ++++++++++++--- .../src/lib/renderer-memory-sampling.ts | 41 ++++++++++++++----- 2 files changed, 56 insertions(+), 17 deletions(-) diff --git a/src/renderer/src/lib/renderer-memory-sampling.test.ts b/src/renderer/src/lib/renderer-memory-sampling.test.ts index cc48730bb66..eac9b5204e2 100644 --- a/src/renderer/src/lib/renderer-memory-sampling.test.ts +++ b/src/renderer/src/lib/renderer-memory-sampling.test.ts @@ -174,11 +174,11 @@ describe('renderer memory highwater census re-arming', () => { // A refresh replaces the retained crumb's own createdAt, so the census must carry how long the // renderer has been over the mark — that is the axis that identified the 21h-stale census. - it('reports minutes above the mark across refreshes', async () => { + it('reports minutes near the mark across refreshes', async () => { stubFootprint(658) await tick() await tick() - expect(censuses().at(-1)).toMatchObject({ thresholdPrivateMB: 600, aboveMarkMinutes: 0 }) + expect(censuses().at(-1)).toMatchObject({ thresholdPrivateMB: 600, nearMarkMinutes: 0 }) stubFootprint(577) for (let minute = 0; minute < 120; minute += 1) { @@ -187,16 +187,16 @@ describe('renderer memory highwater census re-arming', () => { // Still over the band, so it refreshed — and it says how long it has been up there. expect(censuses().length).toBeGreaterThan(1) - expect(censuses().at(-1)).toMatchObject({ thresholdPrivateMB: 600, aboveMarkMinutes: 120 }) + expect(censuses().at(-1)).toMatchObject({ thresholdPrivateMB: 600, nearMarkMinutes: 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 () => { + it('re-anchors minutes-near-mark after a long observed spell below the band', async () => { stubFootprint(658) await tick() await tick() - expect(censuses().at(-1)).toMatchObject({ aboveMarkMinutes: 0 }) + expect(censuses().at(-1)).toMatchObject({ nearMarkMinutes: 0 }) // 10 hours far below the band, then back up. stubFootprint(100) @@ -209,7 +209,27 @@ describe('renderer memory highwater census re-arming', () => { // Not 602: the renderer spent those 600 minutes at 100MB. expect(censuses().at(-1)).toMatchObject({ thresholdPrivateMB: 600 }) - expect(censuses().at(-1)?.aboveMarkMinutes).toBeLessThanOrEqual(2) + expect(censuses().at(-1)?.nearMarkMinutes).toBeLessThanOrEqual(2) + }) + + // A wedged main thread or a suspend stops the 60s sampler. That is not the renderer leaving the + // band — and it is exactly when the renderer is sickest, so the clock must not reset. + it('does not reset the clock when sampling itself stalls', async () => { + stubFootprint(658) + await tick() + await tick() + for (let minute = 0; minute < 30; minute += 1) { + await tick() + } + const beforeGap = censuses().at(-1)?.nearMarkMinutes as number + expect(beforeGap).toBeGreaterThanOrEqual(30) + + // Sampler stalls for 8 hours, then resumes with the renderer still heavy. + nowMs += 8 * 60 * 60_000 + await tick() + await tick() + + expect(censuses().at(-1)?.nearMarkMinutes as number).toBeGreaterThan(beforeGap + 400) }) it('emits at most one census per mark while a renderer oscillates around it', async () => { diff --git a/src/renderer/src/lib/renderer-memory-sampling.ts b/src/renderer/src/lib/renderer-memory-sampling.ts index 30ef3664f26..863a9992359 100644 --- a/src/renderer/src/lib/renderer-memory-sampling.ts +++ b/src/renderer/src/lib/renderer-memory-sampling.ts @@ -57,6 +57,8 @@ type HighwaterMarkState = { firstCrossedAtMs: number lastEmittedAtMs: number lastWithinBandAtMs: number + /** When the renderer was first observed below the band; null while it is in band. */ + belowBandSinceMs: number | null } const emittedHighwaterRatios = new Map() const emittedPrivateHighwaterMarks = new Map() @@ -228,11 +230,11 @@ function recordRendererMemoryHighwater( if (!isHighwaterCensusDue(emittedHighwaterRatios, threshold, nowMs, ratio)) { continue } - const aboveMarkMinutes = stampHighwaterMark(emittedHighwaterRatios, threshold, nowMs) + const nearMarkMinutes = stampHighwaterMark(emittedHighwaterRatios, threshold, nowMs) recordRendererCrashBreadcrumb('renderer_memory_highwater', { ...profile, thresholdPct: Math.round(threshold * 100), - aboveMarkMinutes + nearMarkMinutes }) } } @@ -241,11 +243,11 @@ function recordRendererMemoryHighwater( if (!isHighwaterCensusDue(emittedPrivateHighwaterMarks, mark, nowMs, privateMB)) { continue } - const aboveMarkMinutes = stampHighwaterMark(emittedPrivateHighwaterMarks, mark, nowMs) + const nearMarkMinutes = stampHighwaterMark(emittedPrivateHighwaterMarks, mark, nowMs) recordRendererCrashBreadcrumb('renderer_memory_highwater', { ...profile, thresholdPrivateMB: mark, - aboveMarkMinutes + nearMarkMinutes }) } } @@ -272,21 +274,33 @@ function noteHighwaterBandResidency( value: number ): void { const state = emitted.get(mark) - if (state === undefined || value < mark * RENDERER_HIGHWATER_RECENSUS_BAND) { + if (state === undefined) { return } - const lapsed = nowMs - state.lastWithinBandAtMs > RENDERER_HIGHWATER_RECENSUS_MS + if (value < mark * RENDERER_HIGHWATER_RECENSUS_BAND) { + // Why stamp rather than re-anchor here: the spell has to be measured from observed samples. + emitted.set(mark, { ...state, belowBandSinceMs: state.belowBandSinceMs ?? nowMs }) + return + } + // Why an observed spell and not elapsed time: a renderer that is simply not being sampled — a + // main thread wedged past the sample interval, or a suspend — has not left the band, and must + // not have its clock reset. Only samples we actually saw below the band count. + const lapsed = + state.belowBandSinceMs !== null && + nowMs - state.belowBandSinceMs > RENDERER_HIGHWATER_RECENSUS_MS emitted.set(mark, { firstCrossedAtMs: lapsed ? nowMs : state.firstCrossedAtMs, lastEmittedAtMs: state.lastEmittedAtMs, - lastWithinBandAtMs: nowMs + lastWithinBandAtMs: nowMs, + belowBandSinceMs: null }) } /** - * 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 - * renderer been over the mark" — the axis that identified the stale census — becomes unrecoverable. + * Records this emission and returns minutes the renderer has been *within the refresh band* of the + * mark (>=90% of it), not strictly above it — a renderer sitting at 577MB under a 600MB mark is + * the sustained-pressure signal worth reporting. Why carry it in the payload: a refresh replaces + * the retained crumb's `createdAt`, so without this the duration axis becomes unrecoverable. */ function stampHighwaterMark( emitted: Map, @@ -294,7 +308,12 @@ function stampHighwaterMark( nowMs: number ): number { const firstCrossedAtMs = emitted.get(mark)?.firstCrossedAtMs ?? nowMs - emitted.set(mark, { firstCrossedAtMs, lastEmittedAtMs: nowMs, lastWithinBandAtMs: nowMs }) + emitted.set(mark, { + firstCrossedAtMs, + lastEmittedAtMs: nowMs, + lastWithinBandAtMs: nowMs, + belowBandSinceMs: null + }) return Math.round((nowMs - firstCrossedAtMs) / 60_000) }