Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/highlight-cache-line-budget.md
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 5 additions & 0 deletions .changeset/lru-highlight-cache.md
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 5 additions & 0 deletions .changeset/skip-generated-file-highlighting.md
Original file line number Diff line number Diff line change
@@ -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.
46 changes: 46 additions & 0 deletions src/ui/diff/diffRows.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ import type { DiffFile } from "../../core/types";
import {
buildSplitRows,
buildStackRows,
highlightedDiffLineCount,
loadHighlightedDiff,
loadHighlightedSourceLines,
shouldHighlightDiff,
spansForHighlightedSourceLine,
type DiffRow,
} from "./diffRows";
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
35 changes: 35 additions & 0 deletions src/ui/diff/diffRows.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<HighlightedDiffCode> {
// 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 {
Expand Down
98 changes: 98 additions & 0 deletions src/ui/diff/highlightedDiffCache.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
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", () => {
const cache = createHighlightedDiffCache(20);
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(10);
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(100);

// 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("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);
});
});
98 changes: 98 additions & 0 deletions src/ui/diff/highlightedDiffCache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
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;
}

interface HighlightedDiffCacheEntry {
lines: 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;
Comment thread
benvinegar marked this conversation as resolved.
}

/**
* 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<string, HighlightedDiffCacheEntry>();
const budget = Math.max(1, Math.floor(maxLines));
let cachedLines = 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) {
cachedLines -= entries.get(key)?.lines ?? 0;

const lines = highlightedLineCount(value);
touch(key, { lines, value });
cachedLines += lines;

// 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 (cachedLines > budget && entries.size > 1) {
const leastRecentlyUsed = entries.entries().next().value;
if (leastRecentlyUsed === undefined) {
return;
}

const [evictedKey, evictedEntry] = leastRecentlyUsed;
entries.delete(evictedKey);
cachedLines -= evictedEntry.lines;
}
},
};
}
31 changes: 8 additions & 23 deletions src/ui/diff/useHighlightedDiff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, HighlightedDiffCode>();
const SHARED_HIGHLIGHTED_DIFF_CACHE = createHighlightedDiffCache();
const SHARED_HIGHLIGHT_PROMISES = new Map<string, Promise<HighlightedDiffCode>>();
const sourceFetcherIds = new WeakMap<NonNullable<DiffFile["sourceFetcher"]>, 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;
Expand Down Expand Up @@ -118,7 +99,6 @@ function commitHighlightResult(

SHARED_HIGHLIGHT_PROMISES.delete(cacheKey);
SHARED_HIGHLIGHTED_DIFF_CACHE.set(cacheKey, result);
enforceCacheLimit();
return true;
}

Expand All @@ -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);
Expand Down Expand Up @@ -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. */
Expand Down
Loading