From d6e29dc9dee943b34d6b0d11441fa944c7bff7c9 Mon Sep 17 00:00:00 2001 From: Theo Browne Date: Fri, 4 Sep 2026 11:38:54 -0700 Subject: [PATCH] perf(mobile): bound the parsed review cache (#9749) Bound cached parsed review sections by eight entries and 4,194,304 full source characters. Evicted inactive parsed and native results can be collected. Raw patches, comments, and view state stay unchanged. Prewarm only nearby sections that fit beside the selected section. Use actual retained source weights for normalized cache hits, and skip oversized prewarms. Created with GPT-6 Astra (preview) in Codex. --- .../src/features/review/reviewState.test.ts | 106 ++++++++++- .../mobile/src/features/review/reviewState.ts | 64 ++++++- .../review/useReviewDiffPrewarming.test.ts | 177 ++++++++++++++++++ .../review/useReviewDiffPrewarming.ts | 66 ++++++- 4 files changed, 397 insertions(+), 16 deletions(-) create mode 100644 apps/mobile/src/features/review/useReviewDiffPrewarming.test.ts diff --git a/apps/mobile/src/features/review/reviewState.test.ts b/apps/mobile/src/features/review/reviewState.test.ts index 8e99636d417ffc924780726bec75ee1121e63ef7..28fb734f80dddedb33ce4c7d1521e9ef643430bc 100644 --- a/apps/mobile/src/features/review/reviewState.test.ts +++ b/apps/mobile/src/features/review/reviewState.test.ts @@ -1,11 +1,31 @@ -import { assert, it } from "vite-plus/test"; +import { afterEach, assert, it } from "vite-plus/test"; +import { appAtomRegistry } from "../../state/atom-registry"; +import { getCachedNativeReviewDiffData } from "./nativeReviewDiffAdapter"; import { + getCachedReviewParsedDiff, getReviewAsyncStateSnapshot, setReviewAsyncError, setReviewTurnDiffLoading, } from "./reviewState"; +const reviewInput = { + threadKey: "env-local:thread-review", + sectionId: "turn:1", + diff: [ + "diff --git a/src/a.ts b/src/a.ts", + "new file mode 100644", + "--- /dev/null", + "+++ b/src/a.ts", + "@@ -0,0 +1 @@", + "+export const value = 1;", + ].join("\n"), +}; + +afterEach(() => { + appAtomRegistry.reset(); +}); + it("stores review async loading and error state in atoms", () => { const threadKey = `env-local:thread-review-state-${Date.now()}`; @@ -25,3 +45,87 @@ it("stores review async loading and error state in atoms", () => { error: null, }); }); + +it("reuses unchanged parsed diffs and replaces changed sections", () => { + const parsed = getCachedReviewParsedDiff(reviewInput); + + assert.strictEqual( + getCachedReviewParsedDiff({ ...reviewInput, diff: `\n${reviewInput.diff}\n` }), + parsed, + ); + + const changed = { ...reviewInput, diff: reviewInput.diff.replace("value = 1", "value = 2") }; + const updated = getCachedReviewParsedDiff(changed); + assert.notStrictEqual(updated, parsed); + assert.strictEqual(getCachedReviewParsedDiff(changed), updated); +}); + +it("evicts the least recently used section across threads without changing live results or IDs", () => { + const recentInput = { ...reviewInput, threadKey: "env-local:thread-recent" }; + const oldestInput = { ...reviewInput, threadKey: "env-remote:thread-oldest" }; + const recent = getCachedReviewParsedDiff(recentInput); + const oldest = getCachedReviewParsedDiff(oldestInput); + const native = getCachedNativeReviewDiffData({ parsedDiff: oldest }); + for (let index = 0; index < 6; index += 1) { + getCachedReviewParsedDiff({ ...reviewInput, threadKey: `env-local:thread-${index}` }); + } + + assert.strictEqual(getCachedReviewParsedDiff(recentInput), recent); + getCachedReviewParsedDiff({ ...reviewInput, threadKey: "env-local:thread-new" }); + + assert.strictEqual(getCachedReviewParsedDiff(recentInput), recent); + const rebuilt = getCachedReviewParsedDiff(oldestInput); + assert.notStrictEqual(rebuilt, oldest); + assert.deepStrictEqual(rebuilt, oldest); + assert.strictEqual(getCachedNativeReviewDiffData({ parsedDiff: oldest }), native); + assert.deepStrictEqual(getCachedNativeReviewDiffData({ parsedDiff: rebuilt }), native); +}); + +it("limits cached diffs to 4 Mi source characters before the entry limit", () => { + const firstInput = { ...reviewInput, diff: "x".repeat(2 * 1024 * 1024) }; + const secondInput = { ...firstInput, sectionId: "turn:2" }; + const first = getCachedReviewParsedDiff(firstInput); + const second = getCachedReviewParsedDiff(secondInput); + + getCachedReviewParsedDiff({ ...firstInput, sectionId: "turn:3" }); + + assert.strictEqual(getCachedReviewParsedDiff(secondInput), second); + assert.notStrictEqual(getCachedReviewParsedDiff(firstInput), first); +}); + +it("reclaims the source budget when a cached section changes", () => { + const firstInput = { ...reviewInput, diff: "x".repeat(2 * 1024 * 1024) }; + const secondInput = { ...firstInput, sectionId: "turn:2" }; + getCachedReviewParsedDiff(firstInput); + const second = getCachedReviewParsedDiff(secondInput); + + getCachedReviewParsedDiff({ ...firstInput, diff: null }); + getCachedReviewParsedDiff({ ...firstInput, sectionId: "turn:3" }); + + assert.strictEqual(getCachedReviewParsedDiff(secondInput), second); +}); + +it("counts the full source and skips oversized diffs without evicting cached sections", () => { + const cached = getCachedReviewParsedDiff(reviewInput); + const oversizedInput = { + ...reviewInput, + sectionId: "turn:oversized", + diff: `${reviewInput.diff}${" ".repeat(4 * 1024 * 1024)}`, + }; + const first = getCachedReviewParsedDiff(oversizedInput); + const second = getCachedReviewParsedDiff(oversizedInput); + + assert.notStrictEqual(second, first); + assert.deepStrictEqual(second, first); + assert.strictEqual(getCachedReviewParsedDiff(reviewInput), cached); +}); + +it("drops parsed diffs when the app registry resets", () => { + const parsed = getCachedReviewParsedDiff(reviewInput); + + appAtomRegistry.reset(); + + const rebuilt = getCachedReviewParsedDiff(reviewInput); + assert.notStrictEqual(rebuilt, parsed); + assert.deepStrictEqual(rebuilt, parsed); +}); diff --git a/apps/mobile/src/features/review/reviewState.ts b/apps/mobile/src/features/review/reviewState.ts index a86d53c35859f7548ad393787c39d165e416e240..f211b4bc06d9b7b9c688279e8834fd306cd24af7 100644 --- a/apps/mobile/src/features/review/reviewState.ts +++ b/apps/mobile/src/features/review/reviewState.ts @@ -87,12 +87,21 @@ const reviewViewedFileIdsByThreadKeyAtom = Atom.family((threadKey: string) => ), ); -const reviewParsedDiffBySectionCacheKeyAtom = Atom.family((cacheKey: string) => - Atom.make<{ readonly diff: string | null; readonly parsed: ReviewParsedDiff } | null>(null).pipe( - Atom.keepAlive, - Atom.withLabel(`mobile:review:parsed-diffs:${cacheKey}`), - ), -); +export const MAX_CACHED_REVIEW_DIFFS = 8; +// This bounds source string length, not the parsed or native heap size. +export const MAX_CACHED_REVIEW_SOURCE_CHARACTERS = 4 * 1024 * 1024; + +interface CachedReviewParsedDiff { + readonly diff: string | null; + readonly parsed: ReviewParsedDiff; + readonly sourceCharacterCount: number; +} + +// The factory keeps this mutable cache local to the registry and releases it on reset or disposal. +const reviewParsedDiffCacheAtom = Atom.make(() => ({ + entries: new Map(), + sourceCharacterCount: 0, +})).pipe(Atom.keepAlive, Atom.withLabel("mobile:review:parsed-diffs")); export interface ReviewCacheForThread { readonly threadKey: string | null; @@ -256,6 +265,20 @@ export function updateReviewViewedFileIds( }); } +/** Returns the larger of current input and matching cached source, without changing recency. */ +export function getReviewParsedDiffSourceCharacterCount(input: { + readonly threadKey: string; + readonly sectionId: string; + readonly diff: string | null; +}): number { + const sourceCharacterCount = input.diff?.length ?? 0; + const cache = appAtomRegistry.get(reviewParsedDiffCacheAtom); + const cached = cache.entries.get(buildSectionCacheKey(input.threadKey, input.sectionId)); + return cached && cached.diff === (input.diff?.trim() ?? null) + ? Math.max(sourceCharacterCount, cached.sourceCharacterCount) + : sourceCharacterCount; +} + export function getCachedReviewParsedDiff(input: { readonly threadKey: string | null; readonly sectionId: string | null; @@ -267,16 +290,39 @@ export function getCachedReviewParsedDiff(input: { const cacheKey = buildSectionCacheKey(input.threadKey, input.sectionId); const normalizedDiff = input.diff?.trim() ?? null; - const atom = reviewParsedDiffBySectionCacheKeyAtom(cacheKey); - const cached = appAtomRegistry.get(atom); + const cache = appAtomRegistry.get(reviewParsedDiffCacheAtom); + const cached = cache.entries.get(cacheKey); if (cached && cached.diff === normalizedDiff) { + cache.entries.delete(cacheKey); + cache.entries.set(cacheKey, cached); return cached.parsed; } const parsed = buildReviewParsedDiff(input.diff, input.sectionId); - appAtomRegistry.set(atom, { + if (cached) { + cache.entries.delete(cacheKey); + cache.sourceCharacterCount -= cached.sourceCharacterCount; + } + const sourceCharacterCount = input.diff?.length ?? 0; + if (sourceCharacterCount > MAX_CACHED_REVIEW_SOURCE_CHARACTERS) { + return parsed; + } + + for (const [oldestKey, oldest] of cache.entries) { + if ( + cache.entries.size < MAX_CACHED_REVIEW_DIFFS && + cache.sourceCharacterCount + sourceCharacterCount <= MAX_CACHED_REVIEW_SOURCE_CHARACTERS + ) { + break; + } + cache.entries.delete(oldestKey); + cache.sourceCharacterCount -= oldest.sourceCharacterCount; + } + cache.entries.set(cacheKey, { diff: normalizedDiff, parsed, + sourceCharacterCount, }); + cache.sourceCharacterCount += sourceCharacterCount; return parsed; } diff --git a/apps/mobile/src/features/review/useReviewDiffPrewarming.test.ts b/apps/mobile/src/features/review/useReviewDiffPrewarming.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..b58de1ffe467a91b2b8582608a60607d05fdc1c4 --- /dev/null +++ b/apps/mobile/src/features/review/useReviewDiffPrewarming.test.ts @@ -0,0 +1,177 @@ +import { afterEach, assert, it, vi } from "vite-plus/test"; + +import { appAtomRegistry } from "../../state/atom-registry"; +import { getCachedNativeReviewDiffData } from "./nativeReviewDiffAdapter"; +import * as ReviewModel from "./reviewModel"; +import { getCachedReviewParsedDiff, MAX_CACHED_REVIEW_SOURCE_CHARACTERS } from "./reviewState"; +import { getReviewDiffPrewarmSections, prewarmReviewDiffSection } from "./useReviewDiffPrewarming"; + +const threadKey = "env:thread-prewarm"; +const reviewDiff = [ + "diff --git a/src/a.ts b/src/a.ts", + "new file mode 100644", + "--- /dev/null", + "+++ b/src/a.ts", + "@@ -0,0 +1 @@", + "+export const value = 1;", +].join("\n"); + +function makeSection(index: number, diff: string | null = reviewDiff) { + return { + id: `turn:${index}`, + kind: "turn", + title: `Turn ${index}`, + subtitle: null, + isLoading: false, + diff, + } satisfies ReviewModel.ReviewSectionItem; +} + +function prepareSelection( + sections: ReadonlyArray, + selectedSectionId: string, +) { + const section = sections.find((candidate) => candidate.id === selectedSectionId); + assert.ok(section); + const input = { threadKey, sectionId: section.id, diff: section.diff }; + const parsed = getCachedReviewParsedDiff(input); + const native = getCachedNativeReviewDiffData({ parsedDiff: parsed }); + for (const pending of getReviewDiffPrewarmSections({ threadKey, sections, selectedSectionId })) { + prewarmReviewDiffSection({ threadKey: input.threadKey, section: pending }); + } + assert.strictEqual(getCachedReviewParsedDiff(input), parsed); + assert.strictEqual(getCachedNativeReviewDiffData({ parsedDiff: parsed }), native); + return { parsed, native }; +} + +afterEach(() => { + appAtomRegistry.reset(); + vi.restoreAllMocks(); +}); + +it("warms the nearest sections within the remaining entry budget", () => { + const sections = Array.from({ length: 12 }, (_, index) => makeSection(index)); + + const pending = getReviewDiffPrewarmSections({ + threadKey, + sections, + selectedSectionId: "turn:5", + }); + + assert.deepStrictEqual( + pending.map((section) => section.id), + ["turn:4", "turn:6", "turn:3", "turn:7", "turn:2", "turn:8", "turn:1"], + ); +}); + +it("reserves the selected source budget and skips unloaded or oversized sections", () => { + const halfBudget = "x".repeat(2 * 1024 * 1024); + const sections = [ + makeSection(0), + makeSection(1, halfBudget), + makeSection(2, "x".repeat(MAX_CACHED_REVIEW_SOURCE_CHARACTERS + 1)), + makeSection(3, halfBudget), + makeSection(4, null), + makeSection(5), + ]; + + const pending = getReviewDiffPrewarmSections({ + threadKey, + sections, + selectedSectionId: "turn:3", + }); + + assert.deepStrictEqual( + pending.map((section) => section.id), + ["turn:1"], + ); + prepareSelection(sections, "turn:3"); +}); + +it("reserves a larger retained source when the selected input loses whitespace", () => { + const parsed = getCachedReviewParsedDiff({ + threadKey, + sectionId: "turn:0", + diff: reviewDiff.padEnd(MAX_CACHED_REVIEW_SOURCE_CHARACTERS, " "), + }); + const native = getCachedNativeReviewDiffData({ parsedDiff: parsed }); + + const selected = prepareSelection([makeSection(0), makeSection(1)], "turn:0"); + + assert.strictEqual(selected.parsed, parsed); + assert.strictEqual(selected.native, native); +}); + +it("reserves a larger retained source when a neighboring input loses whitespace", () => { + const parsed = getCachedReviewParsedDiff({ + threadKey, + sectionId: "turn:1", + diff: reviewDiff.padEnd(MAX_CACHED_REVIEW_SOURCE_CHARACTERS - reviewDiff.length, " "), + }); + const native = getCachedNativeReviewDiffData({ parsedDiff: parsed }); + + prepareSelection([makeSection(0), makeSection(1), makeSection(2)], "turn:0"); + + assert.strictEqual( + getCachedReviewParsedDiff({ + threadKey, + sectionId: "turn:1", + diff: reviewDiff, + }), + parsed, + ); + assert.strictEqual(getCachedNativeReviewDiffData({ parsedDiff: parsed }), native); +}); + +it.each([null, "turn:missing"])( + "does not prewarm without a selected section: %s", + (selectedSectionId) => { + assert.deepStrictEqual( + getReviewDiffPrewarmSections({ threadKey, sections: [makeSection(0)], selectedSectionId }), + [], + ); + }, +); + +it("does not prewarm when the selected section exceeds the source budget", () => { + const sections = [ + makeSection(0, "x".repeat(MAX_CACHED_REVIEW_SOURCE_CHARACTERS + 1)), + makeSection(1), + ]; + + assert.deepStrictEqual( + getReviewDiffPrewarmSections({ threadKey, sections, selectedSectionId: "turn:0" }), + [], + ); +}); + +it("does not parse an oversized direct prewarm that the cache cannot retain", () => { + const parse = vi.spyOn(ReviewModel, "buildReviewParsedDiff"); + + prewarmReviewDiffSection({ + threadKey, + section: makeSection(0, "x".repeat(MAX_CACHED_REVIEW_SOURCE_CHARACTERS + 1)), + }); + + assert.strictEqual(parse.mock.calls.length, 0); +}); + +it("reuses selected and native results across repeated nearby selections", () => { + const parse = vi.spyOn(ReviewModel, "buildReviewParsedDiff"); + const sections = Array.from({ length: 10 }, (_, index) => makeSection(index)); + const first = prepareSelection(sections, "turn:0"); + assert.strictEqual(parse.mock.calls.length, 8); + parse.mockClear(); + const second = prepareSelection(sections, "turn:1"); + + for (let pass = 0; pass < 3; pass += 1) { + const firstAgain = prepareSelection(sections, "turn:0"); + assert.strictEqual(firstAgain.parsed, first.parsed); + assert.strictEqual(firstAgain.native, first.native); + const secondAgain = prepareSelection(sections, "turn:1"); + assert.strictEqual(secondAgain.parsed, second.parsed); + assert.strictEqual(secondAgain.native, second.native); + } + + assert.strictEqual(parse.mock.calls.length, 0); +}); diff --git a/apps/mobile/src/features/review/useReviewDiffPrewarming.ts b/apps/mobile/src/features/review/useReviewDiffPrewarming.ts index 80a2a64dbf6e9e5a8e82fe735e6b9a0db58f2240..b110a7e2d4c31d4ae0377013194aa7a4923ac8f0 100644 --- a/apps/mobile/src/features/review/useReviewDiffPrewarming.ts +++ b/apps/mobile/src/features/review/useReviewDiffPrewarming.ts @@ -2,7 +2,12 @@ import { useEffect } from "react"; import { getCachedNativeReviewDiffData } from "./nativeReviewDiffAdapter"; import type { ReviewSectionItem } from "./reviewModel"; -import { getCachedReviewParsedDiff } from "./reviewState"; +import { + getCachedReviewParsedDiff, + getReviewParsedDiffSourceCharacterCount, + MAX_CACHED_REVIEW_DIFFS, + MAX_CACHED_REVIEW_SOURCE_CHARACTERS, +} from "./reviewState"; interface IdleDeadlineLike { readonly didTimeout: boolean; @@ -35,7 +40,7 @@ export function prewarmReviewDiffSection(input: { readonly section: ReviewSectionItem; }): void { const { section, threadKey } = input; - if (section.diff === null) { + if (section.diff === null || section.diff.length > MAX_CACHED_REVIEW_SOURCE_CHARACTERS) { return; } @@ -47,7 +52,54 @@ export function prewarmReviewDiffSection(input: { getCachedNativeReviewDiffData({ parsedDiff, comments: [] }); } -/** Warms one cached section per idle period, after navigation animations finish. */ +/** Selects nearby loaded sections that fit in the cache with the selected section. */ +export function getReviewDiffPrewarmSections(input: { + readonly threadKey: string; + readonly sections: ReadonlyArray; + readonly selectedSectionId: string | null; +}): ReadonlyArray { + const { threadKey, sections, selectedSectionId } = input; + const selectedIndex = sections.findIndex((section) => section.id === selectedSectionId); + const selectedSection = sections[selectedIndex]; + if (!selectedSection) { + return []; + } + + let sourceCharacterCount = getReviewParsedDiffSourceCharacterCount({ + threadKey, + sectionId: selectedSection.id, + diff: selectedSection.diff, + }); + if (sourceCharacterCount > MAX_CACHED_REVIEW_SOURCE_CHARACTERS) { + return []; + } + + const pendingSections: ReviewSectionItem[] = []; + for (let distance = 1; distance < sections.length; distance += 1) { + for (const index of [selectedIndex - distance, selectedIndex + distance]) { + if (pendingSections.length >= MAX_CACHED_REVIEW_DIFFS - 1) { + return pendingSections; + } + const section = sections[index]; + if (!section || section.diff === null) { + continue; + } + const sectionCharacterCount = getReviewParsedDiffSourceCharacterCount({ + threadKey, + sectionId: section.id, + diff: section.diff, + }); + if (sourceCharacterCount + sectionCharacterCount > MAX_CACHED_REVIEW_SOURCE_CHARACTERS) { + continue; + } + pendingSections.push(section); + sourceCharacterCount += sectionCharacterCount; + } + } + return pendingSections; +} + +/** Warms one nearby section per idle period, after navigation animations finish. */ export function useReviewDiffPrewarming(input: { readonly threadKey: string | null; readonly sections: ReadonlyArray; @@ -60,9 +112,11 @@ export function useReviewDiffPrewarming(input: { return; } - const pendingSections = sections.filter( - (section) => section.id !== selectedSectionId && section.diff !== null, - ); + const pendingSections = getReviewDiffPrewarmSections({ + threadKey, + sections, + selectedSectionId, + }); if (pendingSections.length === 0) { return; } -- 2.54.0