From 5c0da254dcadd80b733369c91ddde44ba631ddb9 Mon Sep 17 00:00:00 2001 From: ketan0 Date: Thu, 24 Sep 2026 17:01:50 -0700 Subject: [PATCH] Release document search models per file Search document peeks one file at a time and dispose that file's model references, diff provider, and lens child service before loading the next file. This prevents listener and working-copy retention from growing with the number of files in a large search. Add focused coverage for sequential 150-file search and cleanup after acquisition, diff, and cancellation failures. Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2 --- .../review/services/reviewDiffViewService.ts | 47 ++++++----- .../services/reviewDocumentFind.test.ts | 83 +++++++++++++++++++ 2 files changed, 109 insertions(+), 21 deletions(-) create mode 100644 apps/review-desktop/code-oss/src/vs/review/services/reviewDocumentFind.test.ts diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts index e8b458b7d..32b30ac9a 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewDiffViewService.ts @@ -163,30 +163,35 @@ export class ReviewDiffViewService extends Disposable { const entries = data.entries.filter(entry => lensRanges(lens, entry).length > 0); const structural = data.session ? createStructuralDiffEditors(this.instantiationService, entries, lifetime, data.session) : { instantiation: this.instantiationService, entries }; - const instantiation = withLens(structural.instantiation, structural.entries, lens, lifetime, () => undefined, () => ({ dispose() {} })); - const resolver = instantiation.invokeFunction(a => a.get(ITextModelService)); - const factory = instantiation.invokeFunction(a => a.get(IDiffProviderFactoryService)); + const resolver = structural.instantiation.invokeFunction(a => a.get(ITextModelService)); const matches: DocumentMatch[] = []; for (const entry of structural.entries) { - const original = entry.original ? lifetime.add(await resolver.createModelReference(entry.original)).object.textEditorModel : undefined; - const modified = entry.modified ? lifetime.add(await resolver.createModelReference(entry.modified)).object.textEditorModel : undefined; - const provider = factory.createDiffProvider({ diffAlgorithm: "advanced" }); - if (isDisposable(provider)) lifetime.add(provider); - const diff = original && modified ? await provider.computeDiff(original, modified, { ignoreTrimWhitespace: false, maxComputationTimeMs: 0, computeMoves: false }, CancellationToken.None) : undefined; - const pairs = diff && original && modified ? new Map(alignmentRows(diff, original.getLineCount(), modified.getLineCount()).filter((row): row is [number, number] => row[0] !== null && row[1] !== null)) : new Map(); - for (const [side, model] of [["base", original], ["head", modified]] as const) { - if (!model) continue; - const ranges = lensRanges(lens, entry).filter(range => range.side === side); - const found = model.findMatches(query.text, false, query.isRegex, query.matchCase, query.wholeWord ? USUAL_WORD_SEPARATORS : null, false); - for (const match of found) { - const line = match.range.startLineNumber; - const outside = diff?.contextGaps?.some(gap => gap.label === "Outside lens" && line >= (side === "base" ? gap.originalStart : gap.modifiedStart) && line < (side === "base" ? gap.originalStart + gap.originalCount : gap.modifiedStart + gap.modifiedCount)); - if (outside || (!diff && !ranges.some(range => line >= range.fromLine && line <= range.toLine))) continue; - const headLine = pairs.get(line - 1); - if (side === "base" && headLine !== undefined && modified && match.range.startLineNumber === match.range.endLineNumber && model.getLineContent(line) === modified.getLineContent(headLine + 1)) continue; - matches.push({ file: side === "base" ? entry.file.previousPath ?? entry.file.path : entry.file.path, side, range: match.range }); + // Search is sequential: only the current file needs live models and + // language listeners. Matches retain ranges, never model references. + const fileLifetime = new DisposableStore(); + try { + const instantiation = withLens(structural.instantiation, [entry], lens, fileLifetime, () => undefined, () => ({ dispose() {} })); + const factory = instantiation.invokeFunction(a => a.get(IDiffProviderFactoryService)); + const original = entry.original ? fileLifetime.add(await resolver.createModelReference(entry.original)).object.textEditorModel : undefined; + const modified = entry.modified ? fileLifetime.add(await resolver.createModelReference(entry.modified)).object.textEditorModel : undefined; + const provider = factory.createDiffProvider({ diffAlgorithm: "advanced" }); + if (isDisposable(provider)) fileLifetime.add(provider); + const diff = original && modified ? await provider.computeDiff(original, modified, { ignoreTrimWhitespace: false, maxComputationTimeMs: 0, computeMoves: false }, CancellationToken.None) : undefined; + const pairs = diff && original && modified ? new Map(alignmentRows(diff, original.getLineCount(), modified.getLineCount()).filter((row): row is [number, number] => row[0] !== null && row[1] !== null)) : new Map(); + for (const [side, model] of [["base", original], ["head", modified]] as const) { + if (!model) continue; + const ranges = lensRanges(lens, entry).filter(range => range.side === side); + const found = model.findMatches(query.text, false, query.isRegex, query.matchCase, query.wholeWord ? USUAL_WORD_SEPARATORS : null, false); + for (const match of found) { + const line = match.range.startLineNumber; + const outside = diff?.contextGaps?.some(gap => gap.label === "Outside lens" && line >= (side === "base" ? gap.originalStart : gap.modifiedStart) && line < (side === "base" ? gap.originalStart + gap.originalCount : gap.modifiedStart + gap.modifiedCount)); + if (outside || (!diff && !ranges.some(range => line >= range.fromLine && line <= range.toLine))) continue; + const headLine = pairs.get(line - 1); + if (side === "base" && headLine !== undefined && modified && match.range.startLineNumber === match.range.endLineNumber && model.getLineContent(line) === modified.getLineContent(headLine + 1)) continue; + matches.push({ file: side === "base" ? entry.file.previousPath ?? entry.file.path : entry.file.path, side, range: match.range }); + } } - } + } finally { fileLifetime.dispose(); } } return matches; } finally { lifetime.dispose(); } diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewDocumentFind.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewDocumentFind.test.ts new file mode 100644 index 000000000..5af9e5a0b --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewDocumentFind.test.ts @@ -0,0 +1,83 @@ +import assert from 'node:assert/strict'; +import { createRequire, registerHooks } from 'node:module'; +import test from 'node:test'; +import { Event } from '../../base/common/event.js'; +import { URI } from '../../base/common/uri.js'; +import { CancellationError } from '../../base/common/errors.js'; +import { Range } from '../../editor/common/core/range.js'; +import { ITextModelService } from '../../editor/common/services/resolverService.js'; +import { IDiffProviderFactoryService } from '../../editor/browser/widget/diffEditor/diffProviderFactoryService.js'; +import type { IInstantiationService } from '../../platform/instantiation/common/instantiation.js'; +import type { ReviewDiffLens } from '../common/reviewProtocol.js'; +import type { ReviewDiffViewSource } from './reviewDiffViewService.js'; + +const { JSDOM } = createRequire(import.meta.url)('jsdom'); +const dom = new JSDOM(''); +for (const key of ['window', 'document', 'HTMLElement', 'HTMLCanvasElement', 'Node', 'MutationObserver', 'Element', 'navigator', 'customElements', 'UIEvent', 'MouseEvent', 'KeyboardEvent'] as const) { + Object.defineProperty(globalThis, key, { configurable: true, value: dom.window[key] }); +} +dom.window.matchMedia = () => ({ matches: false, addEventListener() { }, removeEventListener() { } }) as never; +registerHooks({ load(url, context, next) { + return url.endsWith('.css') ? { format: 'module', source: '', shortCircuit: true } : next(url, context); +} }); +const { ReviewDiffViewService } = await import('./reviewDiffViewService.js'); + +function setup(fail?: 'acquire' | 'diff' | 'cancel') { + let active = 0, peak = 0, providers = 0, peakProviders = 0; + const source: ReviewDiffViewSource = { + files: async () => [], + load: async () => ({ sourceUri: URI.parse('review:test'), entries: Array.from({ length: 150 }, (_, i) => ({ + file: { path: `${i}.ts`, status: 'modified', additions: 1, deletions: 1 }, + original: URI.parse(`review:/base/${i}.ts`), modified: URI.parse(`review:/head/${i}.ts`), goToFileResource: URI.parse(`review:/head/${i}.ts`), + })) }), + }; + const lens: ReviewDiffLens = { id: 'find', title: 'find', reviewId: 'review', version: 1, ranges: Array.from({ length: 150 }, (_, i) => ({ file: `${i}.ts`, side: 'head', fromLine: 1, toLine: 1 })) }; + const resolver = { async createModelReference(uri: URI) { + if (fail === 'acquire' && uri.path === '/head/1.ts') throw new Error('read failed'); + active++; peak = Math.max(peak, active); + let disposed = false; + const alive = () => assert.equal(disposed, false, 'search used a disposed model'); + return { object: { textEditorModel: { + uri, + getLineCount() { alive(); return 1; }, + getLineContent() { alive(); return uri.path.includes('/head/') ? 'match' : 'old'; }, + findMatches() { alive(); return uri.path.includes('/head/') ? [{ range: new Range(1, 1, 1, 6) }] : []; }, + } }, dispose() { assert.equal(disposed, false); disposed = true; active--; } }; + } }; + const factory = { createDiffProvider() { + providers++; peakProviders = Math.max(peakProviders, providers); + return { onDidChange: Event.None, async computeDiff() { + if (fail === 'diff') throw new Error('diff failed'); + if (fail === 'cancel') throw new CancellationError(); + return { changes: [], moves: [], identical: false, quitEarly: false }; + }, dispose() { providers--; } }; + } }; + function instantiation(overrides?: { get(key: unknown): unknown }): IInstantiationService { + return { + invokeFunction: (fn: (accessor: { get(key: unknown): unknown }) => unknown) => fn({ get: key => overrides?.get(key) ?? (key === ITextModelService ? resolver : key === IDiffProviderFactoryService ? factory : undefined) }), + createChild: (services: { get(key: unknown): unknown }) => instantiation(services), + dispose() {}, + } as unknown as IInstantiationService; + } + const run = () => ReviewDiffViewService.prototype.findDocument.call({ instantiationService: instantiation() } as never, lens, source, { text: 'match', isRegex: false, matchCase: false, wholeWord: false }); + return { run, counts: () => ({ active, peak, providers, peakProviders }) }; +} + +test('searching 150 files releases each model pair and provider before reading the next', async () => { + const { run, counts } = setup(); + for (let i = 0; i < 3; i++) { + const matches = await run(); + assert.equal(matches.length, 150); + assert.equal(matches[149].file, '149.ts'); + assert.equal(matches[149].range.startLineNumber, 1); + assert.deepEqual(counts(), { active: 0, peak: 2, providers: 0, peakProviders: 1 }); + } +}); +for (const failure of ['acquire', 'diff', 'cancel'] as const) { + test(`search releases acquired models and providers after ${failure} failure`, async () => { + const { run, counts } = setup(failure); + await assert.rejects(run(), failure === 'cancel' ? /Canceled/ : /failed/); + assert.equal(counts().active, 0); + assert.equal(counts().providers, 0); + }); +}