Repository navigation
perf(diff): stop re-highlighting files while scrolling a large changeset #754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ffc4d69
perf(diff): evict highlight cache by recency, not highlight age
claude a696ee4
perf(diff): budget the highlight cache by lines, not file count
claude ec69971
fix(diff): size the highlight cache to hold a generated file and its …
claude c282a21
perf(diff): stop highlighting diffs past 10000 lines
claude 7f74a05
fix(diff): charge cache bookkeeping so empty results age out
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, HighlightedDiffCacheEntry>(); | ||
| 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; | ||
| } | ||
| }, | ||
| }; | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.