From 3ab06d275c9ce54daaebf8e2f5384865c0cc7bc5 Mon Sep 17 00:00:00 2001 From: Matthew Hrehirchuk Date: Thu, 9 Jul 2026 19:05:45 -0600 Subject: [PATCH 1/4] fix: reduce retained geometry memory in large reviews --- src/ui/components/panes/copySelection.test.ts | 7 ++- src/ui/components/panes/copySelection.ts | 4 +- src/ui/diff/diffSectionGeometry.ts | 55 ++++++++++++++++--- src/ui/diff/rowWindowing.test.ts | 3 +- 4 files changed, 56 insertions(+), 13 deletions(-) diff --git a/src/ui/components/panes/copySelection.test.ts b/src/ui/components/panes/copySelection.test.ts index 86223442..71781f55 100644 --- a/src/ui/components/panes/copySelection.test.ts +++ b/src/ui/components/panes/copySelection.test.ts @@ -309,9 +309,10 @@ describe("renderCopySelectionText", () => { const { context, fileSectionLayouts, sectionGeometry } = buildContext("stack"); const section = fileSectionLayouts[0]!; const geometry = sectionGeometry[0]!; - const rowIndex = geometry.plannedRows.findIndex( - (row) => row.kind === "diff-row" && row.row.type === "stack-line", - ); + const rowIndex = geometry.rowBounds.findIndex((_, index) => { + const row = geometry.getPlannedRow(index); + return row?.kind === "diff-row" && row.row.type === "stack-line"; + }); const visualRow = section.bodyTop + geometry.rowBounds[rowIndex]!.top; const { gutterWidth } = resolveStackCellGeometry( context.width, diff --git a/src/ui/components/panes/copySelection.ts b/src/ui/components/panes/copySelection.ts index 71b0bc29..e0ff8e32 100644 --- a/src/ui/components/panes/copySelection.ts +++ b/src/ui/components/panes/copySelection.ts @@ -320,7 +320,7 @@ export function renderCopySelectionText({ for (let rowIndex = 0; rowIndex < geometry.rowBounds.length; rowIndex += 1) { const rowBounds = geometry.rowBounds[rowIndex]!; - const row = geometry.plannedRows[rowIndex]; + const row = geometry.getPlannedRow(rowIndex); if (!row || rowBounds.height <= 0) { continue; } @@ -466,7 +466,7 @@ export function expandSelectionPoint( return null; } - const row = geometry.plannedRows[rowIndex]; + const row = geometry.getPlannedRow(rowIndex); if (!row) { return null; } diff --git a/src/ui/diff/diffSectionGeometry.ts b/src/ui/diff/diffSectionGeometry.ts index 573ac280..de5281e6 100644 --- a/src/ui/diff/diffSectionGeometry.ts +++ b/src/ui/diff/diffSectionGeometry.ts @@ -25,14 +25,12 @@ export interface DiffSectionRowBounds extends VerticalBounds { /** * Cached placeholder sizing and hunk navigation geometry for one file section. * - * `plannedRows` is retained alongside the row-bounds map so downstream features (notably - * clipboard rendering of mouse selections) can re-render the exact same rows the layout was - * measured against, without rebuilding the plan. The cache is owned by the immutable DiffFile - * object and includes note content in its key, so memory grows with visible diff lifetimes. + * Copy selection occasionally needs the planned row behind a measured row. Keep that row stream + * lazy so ordinary large-review scrolling/navigation retain bounds instead of every render row. */ export interface DiffSectionGeometry extends SectionGeometry { + getPlannedRow: (rowIndex: number) => PlannedReviewRow | undefined; lineNumberDigits: number; - plannedRows: PlannedReviewRow[]; rowBounds: DiffSectionRowBounds[]; rowBoundsByKey: Map; rowBoundsByStableKey: Map; @@ -135,6 +133,41 @@ function setCachedSectionGeometry( SECTION_GEOMETRY_CACHE.set(file, cachedByFile); } +/** Build planned rows only for uncommon consumers that need row content after geometry exists. */ +function createLazyPlannedRowAccessor({ + expandedKeys, + file, + layout, + showHunkHeaders, + sourceStatus, + theme, + visibleAgentNotes, +}: { + expandedKeys: ReadonlySet; + file: DiffFile; + layout: Exclude; + showHunkHeaders: boolean; + sourceStatus: FileSourceStatus | undefined; + theme: AppTheme; + visibleAgentNotes: VisibleAgentNote[]; +}) { + let plannedRows: PlannedReviewRow[] | null = null; + + return (rowIndex: number) => { + plannedRows ??= buildDiffSectionRowPlan({ + expandedKeys, + file, + layout, + showHunkHeaders, + sourceStatus, + theme, + visibleAgentNotes, + }).plannedRows; + + return plannedRows[rowIndex]; + }; +} + interface DiffSectionRowHeightOptions { layout: Exclude; lineNumberDigits: number; @@ -223,10 +256,10 @@ export function measureDiffSectionGeometry( if (file.metadata.hunks.length === 0) { return { bodyHeight: 1, + getPlannedRow: () => undefined, hunkAnchorRows: new Map(), hunkBounds: new Map(), lineNumberDigits: String(findMaxLineNumber(file)).length, - plannedRows: [], rowBounds: [], rowBoundsByKey: new Map(), rowBoundsByStableKey: new Map(), @@ -330,10 +363,18 @@ export function measureDiffSectionGeometry( const geometry: DiffSectionGeometry = { bodyHeight, + getPlannedRow: createLazyPlannedRowAccessor({ + expandedKeys, + file, + layout, + showHunkHeaders, + sourceStatus, + theme, + visibleAgentNotes, + }), hunkAnchorRows, hunkBounds, lineNumberDigits, - plannedRows, rowBounds, rowBoundsByKey, rowBoundsByStableKey, diff --git a/src/ui/diff/rowWindowing.test.ts b/src/ui/diff/rowWindowing.test.ts index a39d3661..3c858da9 100644 --- a/src/ui/diff/rowWindowing.test.ts +++ b/src/ui/diff/rowWindowing.test.ts @@ -26,6 +26,7 @@ function createTestSectionGeometry( rowBounds: Array<{ key: string; top: number; height: number }>, bodyHeight: number, ): DiffSectionGeometry { + const plannedRows = rowBounds.map((row) => createTestPlannedRow(row.key)); const normalizedRowBounds = rowBounds.map((row) => ({ ...row, stableKey: row.key, @@ -34,10 +35,10 @@ function createTestSectionGeometry( return { bodyHeight, + getPlannedRow: (rowIndex) => plannedRows[rowIndex], hunkAnchorRows: new Map(), hunkBounds: new Map(), lineNumberDigits: 1, - plannedRows: rowBounds.map((row) => createTestPlannedRow(row.key)), rowBounds: normalizedRowBounds, rowBoundsByKey: new Map(normalizedRowBounds.map((row) => [row.key, row])), rowBoundsByStableKey: new Map(normalizedRowBounds.map((row) => [row.stableKey, row])), From ae8dd4900855e21aaa5be96aabe336faf71beaa8 Mon Sep 17 00:00:00 2001 From: Matthew Hrehirchuk Date: Thu, 9 Jul 2026 19:31:52 -0600 Subject: [PATCH 2/4] bench: add geometry memory benchmark --- .changeset/lazy-geometry-memory.md | 5 + benchmarks/README.md | 2 + benchmarks/geometry-memory.ts | 176 +++++++++++++++++++++++++++++ package.json | 1 + 4 files changed, 184 insertions(+) create mode 100644 .changeset/lazy-geometry-memory.md create mode 100644 benchmarks/geometry-memory.ts diff --git a/.changeset/lazy-geometry-memory.md b/.changeset/lazy-geometry-memory.md new file mode 100644 index 00000000..65cbc27e --- /dev/null +++ b/.changeset/lazy-geometry-memory.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Reduced retained memory for large reviews by lazily materializing cached geometry row plans only when copy selection needs them. diff --git a/benchmarks/README.md b/benchmarks/README.md index e43ce968..7d8e65b8 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -43,6 +43,7 @@ bun run bench:non-ascii-stream bun run bench:huge-stream bun run bench:large-stream-profile bun run bench:memory +bun run bench:geometry-memory bun run bench:navigation-memory bun run bench:resize-memory bun run bench:competitors @@ -61,6 +62,7 @@ bun run bench:competitors - `huge-stream.ts` — opt-in huge tier (`--include-huge` or `HUNK_BENCH_INCLUDE_HUGE=1`): cold first frame, scroll-tick and hunk-navigation latency, and memory ceilings on ~1k files / 300k+ diff lines plus one giant ~50k-line file. - `large-stream-profile.ts` — optional local profiler for the main pure planning stages behind the large split-stream benchmark. - `memory.ts` — optional local RSS/heap profiler after fixture loading, planning, first frame, and next-hunk navigation. +- `geometry-memory.ts` — optional local retained-memory profiler for all-files section geometry, including the lazy planned-row materialization path used by copy selection. - `navigation-memory.ts` — optional local retained-memory profiler for repeated hunk navigation through a mounted review stream. - `resize-memory.ts` — optional local retained-memory profiler for repeated terminal-width changes through a mounted review stream; this targets geometry-cache retention across resize variants. - `competitors.ts` — optional local informational comparisons against `git diff --no-ext-diff`, `delta`, `difftastic`, and `diff-so-fancy` when installed. diff --git a/benchmarks/geometry-memory.ts b/benchmarks/geometry-memory.ts new file mode 100644 index 00000000..a138548b --- /dev/null +++ b/benchmarks/geometry-memory.ts @@ -0,0 +1,176 @@ +// Track retained memory for the all-files geometry cache used by review scrolling/navigation. +import { performance } from "node:perf_hooks"; +import { measureDiffSectionGeometry } from "../src/ui/diff/diffSectionGeometry"; +import { resolveTheme } from "../src/ui/themes"; +import { + createLargeSplitStreamBootstrap, + DEFAULT_FILE_COUNT, + DEFAULT_LINES_PER_FILE, +} from "./large-stream-fixture"; + +type MemorySample = { + rssBytes: number; + heapUsedBytes: number; + heapTotalBytes: number; + externalBytes: number; + arrayBuffersBytes: number; +}; + +type CliOptions = { + fileCount: number; + linesPerFile: number; + width: number; + gc: boolean; +}; + +const defaultOptions: CliOptions = { + fileCount: DEFAULT_FILE_COUNT, + linesPerFile: DEFAULT_LINES_PER_FILE, + width: 240, + gc: true, +}; + +function parseNumberOption(name: string, value: string | undefined) { + if (value === undefined) { + throw new Error(`Missing value for ${name}.`); + } + + const parsed = Number(value); + if (!Number.isFinite(parsed) || parsed < 0) { + throw new Error(`Expected ${name} to be a non-negative number.`); + } + + return parsed; +} + +/** Parse benchmark-only flags without pulling a CLI dependency into local diagnostics. */ +function parseArgs(argv: string[]): CliOptions { + const options = { ...defaultOptions }; + + for (let index = 0; index < argv.length; index += 1) { + const arg = argv[index]!; + if (arg === "--help" || arg === "-h") { + console.log( + `Usage: bun run benchmarks/geometry-memory.ts [options]\n\nOptions:\n --file-count Synthetic review files (default ${defaultOptions.fileCount})\n --lines-per-file Source lines per synthetic file (default ${defaultOptions.linesPerFile})\n --width Geometry measurement width (default ${defaultOptions.width})\n --no-gc Do not force Bun.gc before memory samples\n`, + ); + process.exit(0); + } + + const next = () => argv[++index]; + switch (arg) { + case "--file-count": + options.fileCount = parseNumberOption(arg, next()); + break; + case "--lines-per-file": + options.linesPerFile = parseNumberOption(arg, next()); + break; + case "--width": + options.width = parseNumberOption(arg, next()); + break; + case "--no-gc": + options.gc = false; + break; + default: + throw new Error(`Unknown option: ${arg}`); + } + } + + options.fileCount = Math.max(1, Math.trunc(options.fileCount)); + options.linesPerFile = Math.max(1, Math.trunc(options.linesPerFile)); + options.width = Math.max(40, Math.trunc(options.width)); + return options; +} + +function maybeGc(enabled: boolean) { + if (enabled) { + Bun.gc(true); + } +} + +function sampleMemory(options: CliOptions): MemorySample { + maybeGc(options.gc); + const usage = process.memoryUsage(); + return { + rssBytes: usage.rss, + heapUsedBytes: usage.heapUsed, + heapTotalBytes: usage.heapTotal, + externalBytes: usage.external, + arrayBuffersBytes: usage.arrayBuffers, + }; +} + +function printMemory(prefix: string, sample: MemorySample) { + console.log(`METRIC ${prefix}_rss_bytes=${sample.rssBytes}`); + console.log(`METRIC ${prefix}_heap_used_bytes=${sample.heapUsedBytes}`); + console.log(`METRIC ${prefix}_heap_total_bytes=${sample.heapTotalBytes}`); + console.log(`METRIC ${prefix}_external_bytes=${sample.externalBytes}`); + console.log(`METRIC ${prefix}_array_buffers_bytes=${sample.arrayBuffersBytes}`); +} + +const options = parseArgs(process.argv.slice(2)); +const theme = resolveTheme("midnight", null); +const bootstrapStart = performance.now(); +const bootstrap = createLargeSplitStreamBootstrap({ + fileCount: options.fileCount, + linesPerFile: options.linesPerFile, +}); + +console.log( + `geometry memory fixture files=${options.fileCount} lines=${options.linesPerFile} ` + + `width=${options.width} gc=${options.gc ? "on" : "off"}`, +); +console.log(`METRIC bootstrap_fixture_ms=${(performance.now() - bootstrapStart).toFixed(2)}`); + +const afterBootstrap = sampleMemory(options); +printMemory("after_bootstrap", afterBootstrap); + +let bodyRows = 0; +let rowBounds = 0; +let materializedPlannedRows = 0; +const geometryStart = performance.now(); +const geometries = bootstrap.changeset.files.map((file) => { + const geometry = measureDiffSectionGeometry(file, "split", true, theme, [], options.width); + bodyRows += geometry.bodyHeight; + rowBounds += geometry.rowBounds.length; + return geometry; +}); + +const geometryMs = performance.now() - geometryStart; +const afterGeometry = sampleMemory(options); +printMemory("after_geometry", afterGeometry); + +// Materialize the lazy planned-row streams as an upper-bound diagnostic for copy-selection style +// consumers. Normal review scrolling/navigation should not pay this retained-memory cost. +const materializeStart = performance.now(); +for (const geometry of geometries) { + for (let index = 0; index < geometry.rowBounds.length; index += 1) { + if (geometry.getPlannedRow(index)) { + materializedPlannedRows += 1; + } + } +} +const materializeMs = performance.now() - materializeStart; +const afterMaterializedRows = sampleMemory(options); +printMemory("after_materialized_planned_rows", afterMaterializedRows); + +console.log(`METRIC geometry_ms=${geometryMs.toFixed(2)}`); +console.log(`METRIC geometry_body_rows=${bodyRows}`); +console.log(`METRIC geometry_row_bounds=${rowBounds}`); +console.log( + `METRIC geometry_heap_growth_bytes=${afterGeometry.heapUsedBytes - afterBootstrap.heapUsedBytes}`, +); +console.log(`METRIC geometry_rss_growth_bytes=${afterGeometry.rssBytes - afterBootstrap.rssBytes}`); +console.log(`METRIC materialize_planned_rows_ms=${materializeMs.toFixed(2)}`); +console.log(`METRIC materialized_planned_rows=${materializedPlannedRows}`); +console.log( + `METRIC materialized_planned_rows_heap_growth_bytes=${ + afterMaterializedRows.heapUsedBytes - afterGeometry.heapUsedBytes + }`, +); +console.log( + `METRIC materialized_planned_rows_rss_growth_bytes=${ + afterMaterializedRows.rssBytes - afterGeometry.rssBytes + }`, +); +console.log(`METRIC files=${options.fileCount}`); +console.log(`METRIC lines_per_file=${options.linesPerFile}`); diff --git a/package.json b/package.json index 126d9a92..133decb3 100644 --- a/package.json +++ b/package.json @@ -86,6 +86,7 @@ "bench:huge-stream": "bun run benchmarks/huge-stream.ts", "bench:large-stream-profile": "bun run benchmarks/large-stream-profile.ts", "bench:memory": "bun run benchmarks/memory.ts", + "bench:geometry-memory": "bun run benchmarks/geometry-memory.ts", "bench:navigation-memory": "bun run benchmarks/navigation-memory.ts", "bench:resize-memory": "bun run benchmarks/resize-memory.ts", "bench:daemon-memory": "bun run scripts/daemon-memory-check.ts", From 69e570732e0b4bbe2a91c6171173e765a114589f Mon Sep 17 00:00:00 2001 From: Matthew Hrehirchuk Date: Sat, 11 Jul 2026 11:51:35 -0600 Subject: [PATCH 3/4] refactor: make existing plannedRows lazy --- benchmarks/geometry-memory.ts | 6 +-- src/ui/components/panes/copySelection.test.ts | 7 ++-- src/ui/components/panes/copySelection.ts | 5 ++- src/ui/diff/diffSectionGeometry.test.ts | 18 +++++++++ src/ui/diff/diffSectionGeometry.ts | 37 ++++++++++--------- src/ui/diff/rowWindowing.test.ts | 2 +- 6 files changed, 46 insertions(+), 29 deletions(-) diff --git a/benchmarks/geometry-memory.ts b/benchmarks/geometry-memory.ts index a138548b..c00b502e 100644 --- a/benchmarks/geometry-memory.ts +++ b/benchmarks/geometry-memory.ts @@ -143,11 +143,7 @@ printMemory("after_geometry", afterGeometry); // consumers. Normal review scrolling/navigation should not pay this retained-memory cost. const materializeStart = performance.now(); for (const geometry of geometries) { - for (let index = 0; index < geometry.rowBounds.length; index += 1) { - if (geometry.getPlannedRow(index)) { - materializedPlannedRows += 1; - } - } + materializedPlannedRows += geometry.plannedRows.length; } const materializeMs = performance.now() - materializeStart; const afterMaterializedRows = sampleMemory(options); diff --git a/src/ui/components/panes/copySelection.test.ts b/src/ui/components/panes/copySelection.test.ts index 71781f55..86223442 100644 --- a/src/ui/components/panes/copySelection.test.ts +++ b/src/ui/components/panes/copySelection.test.ts @@ -309,10 +309,9 @@ describe("renderCopySelectionText", () => { const { context, fileSectionLayouts, sectionGeometry } = buildContext("stack"); const section = fileSectionLayouts[0]!; const geometry = sectionGeometry[0]!; - const rowIndex = geometry.rowBounds.findIndex((_, index) => { - const row = geometry.getPlannedRow(index); - return row?.kind === "diff-row" && row.row.type === "stack-line"; - }); + const rowIndex = geometry.plannedRows.findIndex( + (row) => row.kind === "diff-row" && row.row.type === "stack-line", + ); const visualRow = section.bodyTop + geometry.rowBounds[rowIndex]!.top; const { gutterWidth } = resolveStackCellGeometry( context.width, diff --git a/src/ui/components/panes/copySelection.ts b/src/ui/components/panes/copySelection.ts index e0ff8e32..5f213035 100644 --- a/src/ui/components/panes/copySelection.ts +++ b/src/ui/components/panes/copySelection.ts @@ -317,10 +317,11 @@ export function renderCopySelectionText({ (context.layout === "split" && start.kind === "review-row" ? resolveCopySelectionSide(start.column, context.layout, context.width) : undefined); + const plannedRows = geometry.plannedRows; for (let rowIndex = 0; rowIndex < geometry.rowBounds.length; rowIndex += 1) { const rowBounds = geometry.rowBounds[rowIndex]!; - const row = geometry.getPlannedRow(rowIndex); + const row = plannedRows[rowIndex]; if (!row || rowBounds.height <= 0) { continue; } @@ -466,7 +467,7 @@ export function expandSelectionPoint( return null; } - const row = geometry.getPlannedRow(rowIndex); + const row = geometry.plannedRows[rowIndex]; if (!row) { return null; } diff --git a/src/ui/diff/diffSectionGeometry.test.ts b/src/ui/diff/diffSectionGeometry.test.ts index 19d5e0d2..c8951984 100644 --- a/src/ui/diff/diffSectionGeometry.test.ts +++ b/src/ui/diff/diffSectionGeometry.test.ts @@ -57,6 +57,24 @@ describe("measureDiffSectionGeometry", () => { expect(differentAddNotePolicy).not.toBe(first); }); + test("caches planned rows after the lazy geometry property is first read", () => { + const geometry = measureDiffSectionGeometry( + createTestDiffFile(), + "split", + true, + theme, + [], + 120, + true, + false, + ); + + const plannedRows = geometry.plannedRows; + + expect(plannedRows).toHaveLength(geometry.rowBounds.length); + expect(geometry.plannedRows).toBe(plannedRows); + }); + test("replaces stale width variants while retaining separate base and note slots", () => { const file = createTestDiffFile(); const visibleAgentNotes: VisibleAgentNote[] = [ diff --git a/src/ui/diff/diffSectionGeometry.ts b/src/ui/diff/diffSectionGeometry.ts index de5281e6..6478e265 100644 --- a/src/ui/diff/diffSectionGeometry.ts +++ b/src/ui/diff/diffSectionGeometry.ts @@ -25,12 +25,12 @@ export interface DiffSectionRowBounds extends VerticalBounds { /** * Cached placeholder sizing and hunk navigation geometry for one file section. * - * Copy selection occasionally needs the planned row behind a measured row. Keep that row stream - * lazy so ordinary large-review scrolling/navigation retain bounds instead of every render row. + * Copy selection occasionally needs the planned rows behind the measured bounds. The geometry + * implementations keep that row stream lazy so ordinary scrolling/navigation retain only bounds. */ export interface DiffSectionGeometry extends SectionGeometry { - getPlannedRow: (rowIndex: number) => PlannedReviewRow | undefined; lineNumberDigits: number; + plannedRows: PlannedReviewRow[]; rowBounds: DiffSectionRowBounds[]; rowBoundsByKey: Map; rowBoundsByStableKey: Map; @@ -133,8 +133,8 @@ function setCachedSectionGeometry( SECTION_GEOMETRY_CACHE.set(file, cachedByFile); } -/** Build planned rows only for uncommon consumers that need row content after geometry exists. */ -function createLazyPlannedRowAccessor({ +/** Resolve planned rows only for uncommon consumers that need row content after geometry exists. */ +function createLazyPlannedRowsResolver({ expandedKeys, file, layout, @@ -153,7 +153,7 @@ function createLazyPlannedRowAccessor({ }) { let plannedRows: PlannedReviewRow[] | null = null; - return (rowIndex: number) => { + return () => { plannedRows ??= buildDiffSectionRowPlan({ expandedKeys, file, @@ -164,7 +164,7 @@ function createLazyPlannedRowAccessor({ visibleAgentNotes, }).plannedRows; - return plannedRows[rowIndex]; + return plannedRows; }; } @@ -256,10 +256,10 @@ export function measureDiffSectionGeometry( if (file.metadata.hunks.length === 0) { return { bodyHeight: 1, - getPlannedRow: () => undefined, hunkAnchorRows: new Map(), hunkBounds: new Map(), lineNumberDigits: String(findMaxLineNumber(file)).length, + plannedRows: [], rowBounds: [], rowBoundsByKey: new Map(), rowBoundsByStableKey: new Map(), @@ -361,20 +361,23 @@ export function measureDiffSectionGeometry( bodyHeight += height; } + const resolvePlannedRows = createLazyPlannedRowsResolver({ + expandedKeys, + file, + layout, + showHunkHeaders, + sourceStatus, + theme, + visibleAgentNotes, + }); const geometry: DiffSectionGeometry = { bodyHeight, - getPlannedRow: createLazyPlannedRowAccessor({ - expandedKeys, - file, - layout, - showHunkHeaders, - sourceStatus, - theme, - visibleAgentNotes, - }), hunkAnchorRows, hunkBounds, lineNumberDigits, + get plannedRows() { + return resolvePlannedRows(); + }, rowBounds, rowBoundsByKey, rowBoundsByStableKey, diff --git a/src/ui/diff/rowWindowing.test.ts b/src/ui/diff/rowWindowing.test.ts index 3c858da9..af39a2e1 100644 --- a/src/ui/diff/rowWindowing.test.ts +++ b/src/ui/diff/rowWindowing.test.ts @@ -35,10 +35,10 @@ function createTestSectionGeometry( return { bodyHeight, - getPlannedRow: (rowIndex) => plannedRows[rowIndex], hunkAnchorRows: new Map(), hunkBounds: new Map(), lineNumberDigits: 1, + plannedRows, rowBounds: normalizedRowBounds, rowBoundsByKey: new Map(normalizedRowBounds.map((row) => [row.key, row])), rowBoundsByStableKey: new Map(normalizedRowBounds.map((row) => [row.stableKey, row])), From 752aba9e9883b46c87a89644d68d103b00ddd189 Mon Sep 17 00:00:00 2001 From: Matthew Hrehirchuk Date: Mon, 13 Jul 2026 10:04:12 -0600 Subject: [PATCH 4/4] fix: review comment tweaks --- benchmarks/README.md | 2 +- benchmarks/geometry-memory.ts | 56 +++++++++++++++++------- src/ui/components/ui-components.test.tsx | 20 ++++++++- src/ui/diff/diffSectionGeometry.test.ts | 41 +++++++++++++++++ src/ui/diff/diffSectionGeometry.ts | 29 ++++++++---- 5 files changed, 119 insertions(+), 29 deletions(-) diff --git a/benchmarks/README.md b/benchmarks/README.md index 7d8e65b8..f19bce33 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -62,7 +62,7 @@ bun run bench:competitors - `huge-stream.ts` — opt-in huge tier (`--include-huge` or `HUNK_BENCH_INCLUDE_HUGE=1`): cold first frame, scroll-tick and hunk-navigation latency, and memory ceilings on ~1k files / 300k+ diff lines plus one giant ~50k-line file. - `large-stream-profile.ts` — optional local profiler for the main pure planning stages behind the large split-stream benchmark. - `memory.ts` — optional local RSS/heap profiler after fixture loading, planning, first frame, and next-hunk navigation. -- `geometry-memory.ts` — optional local retained-memory profiler for all-files section geometry, including the lazy planned-row materialization path used by copy selection. +- `geometry-memory.ts` — optional local retained-memory profiler for all-files section geometry, including JSC-native heap metrics and giant-file lazy planned-row materialization latency used by first copy selection. - `navigation-memory.ts` — optional local retained-memory profiler for repeated hunk navigation through a mounted review stream. - `resize-memory.ts` — optional local retained-memory profiler for repeated terminal-width changes through a mounted review stream; this targets geometry-cache retention across resize variants. - `competitors.ts` — optional local informational comparisons against `git diff --no-ext-diff`, `delta`, `difftastic`, and `diff-so-fancy` when installed. diff --git a/benchmarks/geometry-memory.ts b/benchmarks/geometry-memory.ts index c00b502e..04061d66 100644 --- a/benchmarks/geometry-memory.ts +++ b/benchmarks/geometry-memory.ts @@ -1,19 +1,21 @@ // Track retained memory for the all-files geometry cache used by review scrolling/navigation. +import { heapStats } from "bun:jsc"; import { performance } from "node:perf_hooks"; import { measureDiffSectionGeometry } from "../src/ui/diff/diffSectionGeometry"; import { resolveTheme } from "../src/ui/themes"; import { + createGiantSingleDiffFile, createLargeSplitStreamBootstrap, DEFAULT_FILE_COUNT, DEFAULT_LINES_PER_FILE, + GIANT_SINGLE_FILE_LINES, } from "./large-stream-fixture"; type MemorySample = { rssBytes: number; - heapUsedBytes: number; - heapTotalBytes: number; - externalBytes: number; - arrayBuffersBytes: number; + jscHeapSizeBytes: number; + jscExtraMemorySizeBytes: number; + jscObjectCount: number; }; type CliOptions = { @@ -89,22 +91,20 @@ function maybeGc(enabled: boolean) { function sampleMemory(options: CliOptions): MemorySample { maybeGc(options.gc); - const usage = process.memoryUsage(); + const jscHeap = heapStats(); return { - rssBytes: usage.rss, - heapUsedBytes: usage.heapUsed, - heapTotalBytes: usage.heapTotal, - externalBytes: usage.external, - arrayBuffersBytes: usage.arrayBuffers, + rssBytes: process.memoryUsage.rss(), + jscHeapSizeBytes: jscHeap.heapSize, + jscExtraMemorySizeBytes: jscHeap.extraMemorySize, + jscObjectCount: jscHeap.objectCount, }; } function printMemory(prefix: string, sample: MemorySample) { console.log(`METRIC ${prefix}_rss_bytes=${sample.rssBytes}`); - console.log(`METRIC ${prefix}_heap_used_bytes=${sample.heapUsedBytes}`); - console.log(`METRIC ${prefix}_heap_total_bytes=${sample.heapTotalBytes}`); - console.log(`METRIC ${prefix}_external_bytes=${sample.externalBytes}`); - console.log(`METRIC ${prefix}_array_buffers_bytes=${sample.arrayBuffersBytes}`); + console.log(`METRIC ${prefix}_jsc_heap_size_bytes=${sample.jscHeapSizeBytes}`); + console.log(`METRIC ${prefix}_jsc_extra_memory_size_bytes=${sample.jscExtraMemorySizeBytes}`); + console.log(`METRIC ${prefix}_jsc_object_count=${sample.jscObjectCount}`); } const options = parseArgs(process.argv.slice(2)); @@ -153,14 +153,16 @@ console.log(`METRIC geometry_ms=${geometryMs.toFixed(2)}`); console.log(`METRIC geometry_body_rows=${bodyRows}`); console.log(`METRIC geometry_row_bounds=${rowBounds}`); console.log( - `METRIC geometry_heap_growth_bytes=${afterGeometry.heapUsedBytes - afterBootstrap.heapUsedBytes}`, + `METRIC geometry_jsc_heap_size_growth_bytes=${ + afterGeometry.jscHeapSizeBytes - afterBootstrap.jscHeapSizeBytes + }`, ); console.log(`METRIC geometry_rss_growth_bytes=${afterGeometry.rssBytes - afterBootstrap.rssBytes}`); console.log(`METRIC materialize_planned_rows_ms=${materializeMs.toFixed(2)}`); console.log(`METRIC materialized_planned_rows=${materializedPlannedRows}`); console.log( - `METRIC materialized_planned_rows_heap_growth_bytes=${ - afterMaterializedRows.heapUsedBytes - afterGeometry.heapUsedBytes + `METRIC materialized_planned_rows_jsc_heap_size_growth_bytes=${ + afterMaterializedRows.jscHeapSizeBytes - afterGeometry.jscHeapSizeBytes }`, ); console.log( @@ -170,3 +172,23 @@ console.log( ); console.log(`METRIC files=${options.fileCount}`); console.log(`METRIC lines_per_file=${options.linesPerFile}`); + +// Measure the deferred first-copy path at the giant-file scale where rebuilding the complete plan +// can become interaction-visible. Keep this after retained-memory samples so fixture construction +// does not contaminate the moderate all-files geometry deltas above. +const giantFile = createGiantSingleDiffFile(options.fileCount + 1); +const giantGeometry = measureDiffSectionGeometry( + giantFile, + "split", + true, + theme, + [], + options.width, +); +const giantMaterializeStart = performance.now(); +const giantMaterializedRows = giantGeometry.plannedRows.length; +console.log( + `METRIC giant_first_copy_plan_ms=${(performance.now() - giantMaterializeStart).toFixed(2)}`, +); +console.log(`METRIC giant_materialized_planned_rows=${giantMaterializedRows}`); +console.log(`METRIC giant_file_lines=${GIANT_SINGLE_FILE_LINES}`); diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index b163124b..59301cd5 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -905,9 +905,16 @@ describe("UI components", () => { await setup.mockMouse.moveTo(32, secondHunkY); await setup.renderOnce(); }); - const affordanceFrame = await waitForFrame(setup, (frame) => frame.includes("[+]"), 12); + const affordanceFrame = await waitForFrame( + setup, + (frame) => + frame.split("\n").some((line) => line.includes("line60") && line.includes("[+]")), + 12, + ); const affordanceLines = affordanceFrame.split("\n"); - const addNoteY = affordanceLines.findIndex((line) => line.includes("[+]")); + const addNoteY = affordanceLines.findIndex( + (line) => line.includes("line60") && line.includes("[+]"), + ); const addNoteX = affordanceLines[addNoteY]?.indexOf("[+]") ?? -1; expect(addNoteY).toBeGreaterThanOrEqual(0); expect(addNoteX).toBeGreaterThanOrEqual(0); @@ -916,6 +923,15 @@ describe("UI components", () => { await setup.mockMouse.moveTo(addNoteX + 1, addNoteY); await setup.renderOnce(); }); + const stableAffordanceFrame = await waitForFrame( + setup, + (frame) => { + const targetLine = frame.split("\n")[addNoteY]; + return Boolean(targetLine?.includes("line60") && targetLine.includes("[+]")); + }, + 12, + ); + expect(stableAffordanceFrame.split("\n")[addNoteY]).toContain("[+]"); await act(async () => { await setup.mockMouse.click(addNoteX + 1, addNoteY); await setup.renderOnce(); diff --git a/src/ui/diff/diffSectionGeometry.test.ts b/src/ui/diff/diffSectionGeometry.test.ts index c8951984..27afd347 100644 --- a/src/ui/diff/diffSectionGeometry.test.ts +++ b/src/ui/diff/diffSectionGeometry.test.ts @@ -75,6 +75,47 @@ describe("measureDiffSectionGeometry", () => { expect(geometry.plannedRows).toBe(plannedRows); }); + test("keeps lazy planned rows aligned with bounds after caller-owned inputs mutate", () => { + const before = Array.from({ length: 30 }, (_, index) => `line ${index + 1}\n`).join(""); + const after = before.replace("line 5\n", "line 5 modified\n"); + const file = createTestDiffFile({ after, before, id: "snapshot", path: "snapshot.txt" }); + const expandedKeys = new Set(["trailing:0"]); + const visibleAgentNotes: VisibleAgentNote[] = [ + { + id: "original-note", + annotation: { newRange: [5, 5], summary: "Original note." }, + }, + ]; + const geometry = measureDiffSectionGeometry( + file, + "split", + true, + theme, + visibleAgentNotes, + 120, + true, + false, + expandedKeys, + { kind: "loaded", text: after }, + ); + + expandedKeys.clear(); + visibleAgentNotes[0]!.annotation.newRange = [1, 1]; + visibleAgentNotes[0]!.annotation.summary = "Mutated after geometry measurement."; + visibleAgentNotes.push({ + id: "late-note", + annotation: { newRange: [1, 1], summary: "Added after geometry measurement." }, + }); + + const plannedRows = geometry.plannedRows; + expect(plannedRows.map((row) => row.key)).toEqual( + geometry.rowBounds.map((bounds) => bounds.key), + ); + expect(plannedRows.find((row) => row.kind === "inline-note")?.annotation.summary).toBe( + "Original note.", + ); + }); + test("replaces stale width variants while retaining separate base and note slots", () => { const file = createTestDiffFile(); const visibleAgentNotes: VisibleAgentNote[] = [ diff --git a/src/ui/diff/diffSectionGeometry.ts b/src/ui/diff/diffSectionGeometry.ts index 6478e265..2d1f7c8b 100644 --- a/src/ui/diff/diffSectionGeometry.ts +++ b/src/ui/diff/diffSectionGeometry.ts @@ -15,6 +15,7 @@ import type { PlannedReviewRow } from "./reviewRenderPlan"; import { measureRenderedRowHeight } from "./renderRows"; const EMPTY_EXPANDED_GAP_KEYS: ReadonlySet = new Set(); +const EMPTY_VISIBLE_AGENT_NOTES: VisibleAgentNote[] = []; export interface DiffSectionRowBounds extends VerticalBounds { key: string; @@ -152,17 +153,27 @@ function createLazyPlannedRowsResolver({ visibleAgentNotes: VisibleAgentNote[]; }) { let plannedRows: PlannedReviewRow[] | null = null; + // Geometry bounds and deferred rows must describe the same immutable input snapshot. Callers + // normally replace these collections; clone only the serializable planning data while preserving + // note callbacks so later model additions cannot silently fall outside the snapshot boundary. + const rowPlanInputs = { + expandedKeys: expandedKeys.size === 0 ? EMPTY_EXPANDED_GAP_KEYS : new Set(expandedKeys), + file, + layout, + showHunkHeaders, + sourceStatus: sourceStatus ? structuredClone(sourceStatus) : undefined, + theme, + visibleAgentNotes: + visibleAgentNotes.length === 0 + ? EMPTY_VISIBLE_AGENT_NOTES + : visibleAgentNotes.map((note) => ({ + ...note, + annotation: structuredClone(note.annotation), + })), + }; return () => { - plannedRows ??= buildDiffSectionRowPlan({ - expandedKeys, - file, - layout, - showHunkHeaders, - sourceStatus, - theme, - visibleAgentNotes, - }).plannedRows; + plannedRows ??= buildDiffSectionRowPlan(rowPlanInputs).plannedRows; return plannedRows; };