diff --git a/src/shared/bounded-string-key-memo.ts b/src/shared/bounded-string-key-memo.ts deleted file mode 100644 index bc5e461e6b3..00000000000 --- a/src/shared/bounded-string-key-memo.ts +++ /dev/null @@ -1,29 +0,0 @@ -/** - * FIFO-bounded memo for a pure `(key: string) => T`. - * - * Insertion-ordered eviction: the oldest key is the one whose input has been superseded longest, so - * it is the least likely to be asked for again. Entries hold a reference to a string the caller - * already retains, so a live entry costs the map slot and nothing else. - */ -export function memoizeByStringKey( - compute: (key: string) => T, - maxEntries: number -): (key: string) => T { - // Boxed values so `undefined`/`null` results are still cache hits. - const cache = new Map() - return (key: string): T => { - const cached = cache.get(key) - if (cached) { - return cached.value - } - const value = compute(key) - if (cache.size >= maxEntries) { - const oldest = cache.keys().next() - if (!oldest.done) { - cache.delete(oldest.value) - } - } - cache.set(key, { value }) - return value - } -} diff --git a/src/shared/repo-icon.test.ts b/src/shared/repo-icon.test.ts index 5c85d89d9bd..fde215bf943 100644 --- a/src/shared/repo-icon.test.ts +++ b/src/shared/repo-icon.test.ts @@ -207,7 +207,6 @@ describe('githubAvatarSlug', () => { describe('repo icon source validation memo', () => { const HYDRATIONS = 25 - const MAX_MEMOIZED_ICON_SOURCES = 64 function uploadIcon(width: number): { type: 'image'; src: string; source: 'upload' } { return { type: 'image', src: `data:image/png;base64,${pngBase64(width, 1)}`, source: 'upload' } @@ -250,15 +249,39 @@ describe('repo icon source validation memo', () => { expect(sanitizeRepoIcon({ type: 'image', src, source: 'upload' })).toBeUndefined() }) - it('evicts oldest entries instead of growing without bound', () => { - const overflow = MAX_MEMOIZED_ICON_SOURCES + 6 - for (let index = 1; index <= overflow; index += 1) { - sanitizeRepoIcon(uploadIcon(1000 + index)) + // Guard for the removed cap: the memo hangs off the persisted icon object, so it holds a verdict + // for every live icon no matter how many there are. A fixed-size map would evict the earliest + // entries here and re-decode them on the next hydration. + it('keeps a verdict for every live icon, however many repos have one', () => { + const LIVE_ICONS = 200 + const icons = Array.from({ length: LIVE_ICONS }, (_, index) => uploadIcon(1000 + index)) + for (const icon of icons) { + sanitizeRepoIcon(icon) } dataUriValidations.count = 0 - sanitizeRepoIcon(uploadIcon(1000 + overflow)) + + for (const icon of icons) { + expect(sanitizeRepoIcon(icon)).toEqual(icon) + } + expect(dataUriValidations.count).toBe(0) - sanitizeRepoIcon(uploadIcon(1001)) + }) + + // Guard for the hazard object keying introduces: the stored src/source are re-checked on a hit, + // so a persisted icon edited in place can never be served its previous verdict. + it('re-validates an icon object whose src or source is mutated in place', () => { + const icon = { type: 'image', src: `data:image/png;base64,${pngBase64(7, 1)}`, source: 'file' } + expect(sanitizeRepoIcon(icon)).toEqual(icon) + + icon.src = `data:image/png;base64,${pngBase64(8, 1)}` + dataUriValidations.count = 0 + expect(sanitizeRepoIcon(icon)).toEqual(icon) expect(dataUriValidations.count).toBe(1) + + // WebP is a `file` icon but not an `upload` icon, so the same object must flip verdicts. + icon.src = `data:image/webp;base64,${WEBP_1X1_BASE64}` + expect(sanitizeRepoIcon(icon)).toEqual(icon) + icon.source = 'upload' + expect(sanitizeRepoIcon(icon)).toBeUndefined() }) }) diff --git a/src/shared/repo-icon.ts b/src/shared/repo-icon.ts index e95fd05e114..35f57a8f191 100644 --- a/src/shared/repo-icon.ts +++ b/src/shared/repo-icon.ts @@ -1,4 +1,3 @@ -import { memoizeByStringKey } from './bounded-string-key-memo' import { validateRasterImageDataUri } from './image-data-uri' export type RepoIconImageSource = 'upload' | 'file' | 'favicon' | 'github' @@ -80,13 +79,6 @@ function normalizeGitHubAvatarHost(rawHost?: string): string { } } -/** - * Cap: comfortably above the icon working set of a heavy multi-repo user, small enough that a - * session of icon edits cannot grow the map without limit. Keys are the same `src` strings the - * persisted repo list already holds, so a live entry costs the map slot and nothing else. - */ -const MAX_MEMOIZED_ICON_SOURCES = 64 - function computeIsSupportedImageSrc(src: string, source: RepoIconImageSource): boolean { if (source === 'upload') { return ( @@ -120,33 +112,32 @@ function computeIsSupportedImageSrc(src: string, source: RepoIconImageSource): b return url.hostname === 'www.google.com' && url.pathname === '/s2/favicons' } +type ImageSrcVerdict = { src: unknown; source: unknown; supported: boolean } + /** - * Why: repo icons are immutable persisted strings, so a given `src` always validates the same way - * and there is no invalidation window — a changed icon is simply a new key. `getRepos()` re-hydrates - * every repo on every call, and validating one inline data URI means scanning it twice with a regex - * and base64-decoding its header. + * Why: `getRepos()` re-hydrates every repo on every call, and validating one inline data URI means + * scanning a 400 KB string twice with a regex and base64-decoding its header. `hydrateRepo` is + * handed the *same* persisted `repoIcon` object every time, so the verdict is cached on that object + * and dies with it — no cap, no eviction, and nothing retained once a repo or an icon is replaced. * - * One memo per source rather than a composite key, because V8 caches a string's hash in the string - * itself: keying on `src` makes a 400 KB data URI an O(1) lookup, while `${source}\0${src}` would - * rebuild and rehash the whole thing on every call. + * `src`/`source` are re-checked on a hit, so mutating the persisted icon in place cannot serve a + * stale verdict. Both are the identical string references in the steady state, so the compare is a + * pointer check, not a 400 KB scan. */ -const memoizedIsSupportedImageSrc: Record boolean> = { - upload: memoizeByStringKey( - (src) => computeIsSupportedImageSrc(src, 'upload'), - MAX_MEMOIZED_ICON_SOURCES - ), - file: memoizeByStringKey( - (src) => computeIsSupportedImageSrc(src, 'file'), - MAX_MEMOIZED_ICON_SOURCES - ), - favicon: memoizeByStringKey( - (src) => computeIsSupportedImageSrc(src, 'favicon'), - MAX_MEMOIZED_ICON_SOURCES - ), - github: memoizeByStringKey( - (src) => computeIsSupportedImageSrc(src, 'github'), - MAX_MEMOIZED_ICON_SOURCES - ) +const imageSrcVerdicts = new WeakMap() + +function isSupportedImageSrc( + candidate: Record, + src: string, + source: RepoIconImageSource +): boolean { + const cached = imageSrcVerdicts.get(candidate) + if (cached && cached.src === candidate.src && cached.source === candidate.source) { + return cached.supported + } + const supported = computeIsSupportedImageSrc(src, source) + imageSrcVerdicts.set(candidate, { src: candidate.src, source: candidate.source, supported }) + return supported } export function sanitizeRepoIcon(value: unknown): RepoIcon | null | undefined { @@ -183,7 +174,7 @@ export function sanitizeRepoIcon(value: unknown): RepoIcon | null | undefined { if (!isRepoIconImageSource(source) || src.length > MAX_REPO_ICON_DATA_URL_LENGTH) { return undefined } - if (!memoizedIsSupportedImageSrc[source](src)) { + if (!isSupportedImageSrc(candidate, src, source)) { return undefined } const label = typeof candidate.label === 'string' ? candidate.label.trim().slice(0, 80) : '' diff --git a/src/shared/terminal-title-classification-memo.ts b/src/shared/terminal-title-classification-memo.ts index a81cf44cdbc..c712ca65c96 100644 --- a/src/shared/terminal-title-classification-memo.ts +++ b/src/shared/terminal-title-classification-memo.ts @@ -1,5 +1,3 @@ -import { memoizeByStringKey } from './bounded-string-key-memo' - /** * Bounded memo for pure `(title: string) => T` terminal-title classifiers. * @@ -23,5 +21,23 @@ const MAX_MEMOIZED_TITLES = 1024 export function memoizeTitleClassification( classify: (title: string) => T ): (title: string) => T { - return memoizeByStringKey(classify, MAX_MEMOIZED_TITLES) + // Boxed values so `undefined`/`null` verdicts are still cache hits. + const cache = new Map() + return (title: string): T => { + const cached = cache.get(title) + if (cached) { + return cached.value + } + const value = classify(title) + // Insertion-ordered FIFO eviction: a pane's superseded title frames are the + // oldest keys and the least likely to be asked for again. + if (cache.size >= MAX_MEMOIZED_TITLES) { + const oldest = cache.keys().next() + if (!oldest.done) { + cache.delete(oldest.value) + } + } + cache.set(title, { value }) + return value + } }