diff --git a/.changeset/highlight-cache-line-budget.md b/.changeset/highlight-cache-line-budget.md new file mode 100644 index 000000000..5256b2b07 --- /dev/null +++ b/.changeset/highlight-cache-line-budget.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Budget the syntax highlighting cache by lines instead of file count, so reviews of many small files stop re-highlighting as you scroll and reviews of very large files stay within a bounded memory footprint. diff --git a/.changeset/lru-highlight-cache.md b/.changeset/lru-highlight-cache.md new file mode 100644 index 000000000..b097eb8cb --- /dev/null +++ b/.changeset/lru-highlight-cache.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Keep syntax highlighting cached for the files you are actually reviewing, so scrolling back to a recent file no longer re-highlights it. diff --git a/.changeset/skip-generated-file-highlighting.md b/.changeset/skip-generated-file-highlighting.md new file mode 100644 index 000000000..5b8edc6c7 --- /dev/null +++ b/.changeset/skip-generated-file-highlighting.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Render diffs larger than 10,000 lines as plain rows instead of syntax highlighting them, so a regenerated lockfile appears immediately and stops delaying color on the files around it. diff --git a/src/ui/diff/diffRows.test.ts b/src/ui/diff/diffRows.test.ts index 78b25eddf..b1d3f7742 100644 --- a/src/ui/diff/diffRows.test.ts +++ b/src/ui/diff/diffRows.test.ts @@ -5,8 +5,10 @@ import type { DiffFile } from "../../core/types"; import { buildSplitRows, buildStackRows, + highlightedDiffLineCount, loadHighlightedDiff, loadHighlightedSourceLines, + shouldHighlightDiff, spansForHighlightedSourceLine, type DiffRow, } from "./diffRows"; @@ -50,6 +52,26 @@ function createDiffFile(): DiffFile { }; } +/** Build an added-file diff whose every line lands on the addition side, as generated output does. */ +function createGeneratedFileDiff(contents: string): DiffFile { + const metadata = parseDiffFromFile( + { name: "bun.lock", contents: "", cacheKey: "generated:before" }, + { name: "bun.lock", contents, cacheKey: "generated:after" }, + { context: 3 }, + true, + ); + + return { + id: "generated", + path: "bun.lock", + patch: "", + language: "json", + stats: { additions: metadata.additionLines.length, deletions: 0 }, + metadata, + agent: null, + }; +} + const ELIXIR_HEREDOC_BEFORE = `defmodule Repro do @doc """ Line one. @@ -169,6 +191,30 @@ describe("Pierre diff rows", () => { ).toBe(true); }); + test("renders a generated-scale diff as plain rows instead of highlighting it", async () => { + const generatedLines = Array.from( + { length: 12_000 }, + (_, index) => ` "package-${index}": "npm:package-${index}@1.0.${index}",`, + ).join("\n"); + const file = createGeneratedFileDiff(`{\n${generatedLines}\n}\n`); + const theme = resolveTheme("github-dark-default", null); + + expect(shouldHighlightDiff(file)).toBe(false); + expect(highlightedDiffLineCount(file.metadata)).toBeGreaterThan(10_000); + + const highlighted = await loadHighlightedDiff(file, theme); + + expect(highlighted.deletionLines).toHaveLength(0); + expect(highlighted.additionLines).toHaveLength(0); + + // Plain rows still carry the diff itself, so the file stays reviewable without color. + const rows = buildStackRows(file, highlighted, theme); + expect(rows.some((row) => row.type === "stack-line")).toBe(true); + + // A source file of ordinary size is unaffected. + expect(shouldHighlightDiff(createDiffFile())).toBe(true); + }); + test("uses full source to keep partial Elixir hunks inside the correct heredoc state", async () => { const sourceFetcher = createTestSourceFetcher((side) => side === "old" ? ELIXIR_HEREDOC_BEFORE : ELIXIR_HEREDOC_AFTER, diff --git a/src/ui/diff/diffRows.ts b/src/ui/diff/diffRows.ts index 19077eefd..d670ad607 100644 --- a/src/ui/diff/diffRows.ts +++ b/src/ui/diff/diffRows.ts @@ -643,11 +643,46 @@ function renderHighlightedDiff( }); } +/** + * Largest diff, in lines across both sides, that is worth syntax highlighting. + * + * Past this size the work stops paying for itself twice over: the job occupies the serialized + * highlight queue for seconds while nothing else can be colorized, and the result is too large for + * the shared cache to hold beside the files around it, so it evicts its neighbors and is evicted + * back on the next scroll. Diffs this big are generated output — lockfiles, snapshots, vendored + * bundles — where color earns little. They render as plain rows instead, immediately. + * + * Keep this at or below the cache budget in `highlightedDiffCache.ts`; that is what guarantees no + * single entry can push the rest of the working set out. + */ +const MAX_HIGHLIGHTED_DIFF_LINES = 10_000; + +/** Shared plain-rows result. Read-only, so one instance can back every skipped file. */ +const UNHIGHLIGHTED_DIFF: HighlightedDiffCode = Object.freeze({ + deletionLines: [], + additionLines: [], +}); + +/** Count the diff lines one file retains when highlighted, across both sides. */ +export function highlightedDiffLineCount(metadata: FileDiffMetadata) { + return (metadata.deletionLines?.length ?? 0) + (metadata.additionLines?.length ?? 0); +} + +/** Return whether one file's diff is small enough that highlighting it is worth the work. */ +export function shouldHighlightDiff(file: DiffFile) { + return highlightedDiffLineCount(file.metadata) <= MAX_HIGHLIGHTED_DIFF_LINES; +} + /** Highlight a diff file and return just the rendered line trees the UI needs. */ export async function loadHighlightedDiff( file: DiffFile, theme: HighlightThemeInput = "dark", ): Promise { + // Checked before the source read so an oversized file costs neither I/O nor queue time. + if (!shouldHighlightDiff(file)) { + return UNHIGHLIGHTED_DIFF; + } + const sourcePlan = await loadSourceBackedHighlightPlan(file); try { diff --git a/src/ui/diff/highlightedDiffCache.test.ts b/src/ui/diff/highlightedDiffCache.test.ts new file mode 100644 index 000000000..f2d360dfc --- /dev/null +++ b/src/ui/diff/highlightedDiffCache.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, test } from "bun:test"; +import type { HighlightedDiffCode } from "./diffRows"; +import { createHighlightedDiffCache } from "./highlightedDiffCache"; + +/** Build a result retaining a known line count; identity is what these tests compare. */ +function createTestHighlightedDiffCode(lines: number): HighlightedDiffCode { + return { + deletionLines: Array.from({ length: lines }, () => undefined), + additionLines: [], + }; +} + +describe("highlighted diff cache", () => { + test("evicts the least recently used entry rather than the oldest highlight", () => { + // Three 10-line results cost 18 each with per-entry overhead, so two fit and a third evicts. + const cache = createHighlightedDiffCache(40); + const onScreen = createTestHighlightedDiffCode(10); + const scrolledPast = createTestHighlightedDiffCode(10); + const prefetched = createTestHighlightedDiffCode(10); + + cache.set("on-screen", onScreen); + cache.set("scrolled-past", scrolledPast); + + // The viewport prefetch re-reads the file the user is looking at before warming the next one. + expect(cache.get("on-screen")).toBe(onScreen); + cache.set("prefetched", prefetched); + + expect(cache.peek("on-screen")).toBe(onScreen); + expect(cache.peek("prefetched")).toBe(prefetched); + expect(cache.peek("scrolled-past")).toBeUndefined(); + }); + + test("peeking does not protect an entry from eviction", () => { + const cache = createHighlightedDiffCache(20); + const first = createTestHighlightedDiffCode(10); + const second = createTestHighlightedDiffCode(10); + + cache.set("first", first); + expect(cache.peek("first")).toBe(first); + + cache.set("second", second); + expect(cache.peek("first")).toBeUndefined(); + expect(cache.peek("second")).toBe(second); + }); + + test("holds far more small files than large ones under the same budget", () => { + const cache = createHighlightedDiffCache(600); + + // A window of one-line fixes is what a lint or import sweep looks like. + for (let index = 0; index < 50; index += 1) { + cache.set(`small-${index}`, createTestHighlightedDiffCode(2)); + } + expect(cache.peek("small-0")).toBeDefined(); + expect(cache.peek("small-49")).toBeDefined(); + + // The same count of generated files cannot fit, and the recent ones win. + for (let index = 0; index < 50; index += 1) { + cache.set(`large-${index}`, createTestHighlightedDiffCode(60)); + } + expect(cache.peek("large-49")).toBeDefined(); + expect(cache.peek("large-0")).toBeUndefined(); + expect(cache.peek("small-0")).toBeUndefined(); + }); + + test("keeps a result larger than the whole budget rather than dropping it", () => { + const cache = createHighlightedDiffCache(100); + const neighbor = createTestHighlightedDiffCode(50); + const generated = createTestHighlightedDiffCode(5000); + + cache.set("neighbor", neighbor); + cache.set("generated", generated); + + expect(cache.peek("generated")).toBe(generated); + expect(cache.peek("neighbor")).toBeUndefined(); + }); + + test("releases the budget a replaced result was holding", () => { + const cache = createHighlightedDiffCache(100); + const reloaded = createTestHighlightedDiffCode(4); + const kept = createTestHighlightedDiffCode(40); + + // A file whose diff shrinks between reloads must not keep charging its old size. + cache.set("reloaded", createTestHighlightedDiffCode(90)); + cache.set("reloaded", reloaded); + cache.set("kept", kept); + + expect(cache.peek("reloaded")).toBe(reloaded); + expect(cache.peek("kept")).toBe(kept); + }); + + test("reclaims entries that retain no lines at all", () => { + const cache = createHighlightedDiffCache(100); + + // Diffs past the highlight ceiling cache an empty result. A watch session reloading one + // produces a fresh key per reload, so these have to age out like any other entry. + for (let index = 0; index < 200; index += 1) { + cache.set(`skipped-${index}`, createTestHighlightedDiffCode(0)); + } + + expect(cache.peek("skipped-199")).toBeDefined(); + expect(cache.peek("skipped-0")).toBeUndefined(); + }); + + test("keeps one entry when given a degenerate budget", () => { + const cache = createHighlightedDiffCache(0); + const only = createTestHighlightedDiffCode(5); + + cache.set("only", only); + + expect(cache.peek("only")).toBe(only); + }); +}); diff --git a/src/ui/diff/highlightedDiffCache.ts b/src/ui/diff/highlightedDiffCache.ts new file mode 100644 index 000000000..00b7176a9 --- /dev/null +++ b/src/ui/diff/highlightedDiffCache.ts @@ -0,0 +1,112 @@ +import type { HighlightedDiffCode } from "./diffRows"; + +/** + * Cache budget, counted in highlighted diff lines. + * + * Highlighted HAST nodes and their flattened render spans cost roughly 1.4KB per line, so a budget + * in lines bounds memory directly where a file count could not: 40 files of a 2400-line diff is + * ~130MB, while 40 single-line fixes is a rounding error. Lines are also the unit the prefetch + * window is measured in, so the files it warms always fit — a window spans about seven viewport + * heights of rows, a rounding error against this budget. + * + * The budget holds several of the largest highlightable files at once. `MAX_HIGHLIGHTED_DIFF_LINES` + * caps any single entry well below it, which is what keeps one file from evicting its own neighbors + * and being evicted back on the next scroll. Worst case is ~84MB at the measured rate. + */ +const MAX_HIGHLIGHTED_DIFF_CACHE_LINES = 60_000; + +export interface HighlightedDiffCache { + /** Read one result and mark it most recently used. */ + get: (key: string) => HighlightedDiffCode | undefined; + /** Read one result without changing recency, for render paths that must stay side-effect free. */ + peek: (key: string) => HighlightedDiffCode | undefined; + /** Store one result as most recently used, evicting the least recently used entries over budget. */ + set: (key: string, value: HighlightedDiffCode) => void; +} + +/** + * Bookkeeping charged to every entry, in line-equivalents. + * + * A skipped or failed highlight retains no lines but still holds a cache key and an entry object. + * Charged nothing, those never reach the eviction loop, so a long watch session reloading an + * oversized file would accumulate keys the budget could never reclaim. + */ +const ENTRY_OVERHEAD_LINES = 8; + +interface HighlightedDiffCacheEntry { + cost: number; + value: HighlightedDiffCode; +} + +/** Count the highlighted lines one result retains, across both diff sides. */ +function highlightedLineCount(value: HighlightedDiffCode) { + return value.deletionLines.length + value.additionLines.length; +} + +/** Charge one result its retained lines plus per-entry bookkeeping. */ +function entryCost(value: HighlightedDiffCode) { + return highlightedLineCount(value) + ENTRY_OVERHEAD_LINES; +} + +/** + * Holds highlight results under a bounded least-recently-used line budget. + * + * Reads refresh recency, so eviction drops the files the review has stopped touching rather than + * the ones highlighted longest ago. Viewport prefetch re-reads its whole window on every scroll, + * which keeps the files on screen resident while files ahead of them are warmed. + * + * A result larger than the whole budget is still cached, alone: the file being read must stay + * highlighted, so a single generated file can push its neighbors out. That trade is the point of + * budgeting memory rather than file count. + */ +export function createHighlightedDiffCache( + maxLines = MAX_HIGHLIGHTED_DIFF_CACHE_LINES, +): HighlightedDiffCache { + const entries = new Map(); + const budget = Math.max(1, Math.floor(maxLines)); + let cachedCost = 0; + + /** Move one key to the most-recently-used end of Map iteration order. */ + const touch = (key: string, entry: HighlightedDiffCacheEntry) => { + entries.delete(key); + entries.set(key, entry); + }; + + return { + get(key) { + const entry = entries.get(key); + if (entry === undefined) { + return undefined; + } + + touch(key, entry); + return entry.value; + }, + + peek(key) { + return entries.get(key)?.value; + }, + + set(key, value) { + cachedCost -= entries.get(key)?.cost ?? 0; + + const cost = entryCost(value); + touch(key, { cost, value }); + cachedCost += cost; + + // Map iteration order is insertion order and every read re-inserts, so the first keys are the + // least recently used. Stop at one entry so the result just stored survives its own eviction + // pass even when it alone exceeds the budget. + while (cachedCost > budget && entries.size > 1) { + const leastRecentlyUsed = entries.entries().next().value; + if (leastRecentlyUsed === undefined) { + return; + } + + const [evictedKey, evictedEntry] = leastRecentlyUsed; + entries.delete(evictedKey); + cachedCost -= evictedEntry.cost; + } + }, + }; +} diff --git a/src/ui/diff/useHighlightedDiff.ts b/src/ui/diff/useHighlightedDiff.ts index 637c025c4..1a9f66dc0 100644 --- a/src/ui/diff/useHighlightedDiff.ts +++ b/src/ui/diff/useHighlightedDiff.ts @@ -2,33 +2,14 @@ import { useLayoutEffect, useState } from "react"; import type { DiffFile } from "../../core/types"; import type { AppTheme } from "../themes"; import { loadHighlightedDiff, type HighlightedDiffCode } from "./diffRows"; +import { createHighlightedDiffCache } from "./highlightedDiffCache"; import { syntaxHighlightThemeName } from "./syntaxHighlightTheme"; -/** - * Maximum cached highlight results. - * - * Highlighted HAST nodes and their flattened render spans are expensive enough that a whole-review - * cache can dominate memory while navigating large changesets. Keep a viewport-local working set - * instead of retaining every file the user has visited in the current review. - */ -const MAX_CACHE_ENTRIES = 40; - -const SHARED_HIGHLIGHTED_DIFF_CACHE = new Map(); +const SHARED_HIGHLIGHTED_DIFF_CACHE = createHighlightedDiffCache(); const SHARED_HIGHLIGHT_PROMISES = new Map>(); const sourceFetcherIds = new WeakMap, number>(); let nextSourceFetcherId = 1; -/** Evict the oldest entries when the cache exceeds MAX_CACHE_ENTRIES. - * Map iteration order is insertion order, so the first keys are the oldest. */ -function enforceCacheLimit() { - while (SHARED_HIGHLIGHTED_DIFF_CACHE.size > MAX_CACHE_ENTRIES) { - const oldest = SHARED_HIGHLIGHTED_DIFF_CACHE.keys().next().value; - if (oldest !== undefined) { - SHARED_HIGHLIGHTED_DIFF_CACHE.delete(oldest); - } - } -} - /** Summarize rendered diff lines without serializing whole arrays into the cache key. */ function lineSetFingerprint(lines: string[] | undefined) { let totalChars = 0; @@ -118,7 +99,6 @@ function commitHighlightResult( SHARED_HIGHLIGHT_PROMISES.delete(cacheKey); SHARED_HIGHLIGHTED_DIFF_CACHE.set(cacheKey, result); - enforceCacheLimit(); return true; } @@ -128,6 +108,9 @@ function ensureHighlightedDiffLoaded( theme: AppTheme, cacheKey = highlightedDiffCacheKey(theme, file), ) { + // Viewport prefetch calls this for every file in its halo on each scroll, so this read is also + // what keeps the files around the viewport at the recent end of the cache while files entering + // the halo evict older ones. const cached = SHARED_HIGHLIGHTED_DIFF_CACHE.get(cacheKey); if (cached) { return Promise.resolve(cached); @@ -180,7 +163,9 @@ function resolveHighlightedSnapshot({ return highlighted; } - return SHARED_HIGHLIGHTED_DIFF_CACHE.get(appearanceCacheKey) ?? null; + // Peek rather than read: render stays side-effect free, and the layout effect below refreshes + // recency for this same key during commit. + return SHARED_HIGHLIGHTED_DIFF_CACHE.peek(appearanceCacheKey) ?? null; } /** Resolve highlighted diff content with shared caching and background prefetch support. */