Skip to content

Commit 6758803

Browse files
wenytang-mswenytang-ms
authored
perf: Avoid repeated test discovery for resolved test items (#1916)
* fix: avoid repeated test discovery * fix: keep test discovery cache consistent * fix: address test discovery race conditions * fix: coalesce forced test discovery * fix: keep resolution metadata current * fix: invalidate discovery after file deletion
1 parent 7d53505 commit 6758803

7 files changed

Lines changed: 630 additions & 45 deletions

File tree

‎src/commands/testExplorerCommands.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ export async function refreshExplorer(): Promise<void> {
4545
// Force re-resolution of all existing project roots
4646
const loadPromises: Promise<void>[] = [];
4747
testController?.items.forEach((root: TestItem) => {
48-
loadPromises.push(loadChildren(root));
48+
loadPromises.push(loadChildren(root, undefined, true));
4949
});
5050
await Promise.all(loadPromises);
5151

@@ -90,13 +90,13 @@ export async function refreshProject(classpathUri: Uri): Promise<void> {
9090

9191
if (matchedProject) {
9292
// Re-resolve only the matched project's children
93-
await loadChildren(matchedProject);
93+
await loadChildren(matchedProject, undefined, true);
9494
} else if (childProjectMatched) {
9595
// The classpath URI is an ancestor containing test projects – refresh all children
9696
const loadPromises: Promise<void>[] = [];
9797
testController?.items.forEach((root: TestItem) => {
9898
if (root.uri && ensureTrailingSeparator(root.uri.toString()).startsWith(uriString)) {
99-
loadPromises.push(loadChildren(root));
99+
loadPromises.push(loadChildren(root, undefined, true));
100100
}
101101
});
102102
await Promise.all(loadPromises);

‎src/controller/testController.ts‎

Lines changed: 79 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ import { JUnitLaunchProtocol } from '../constants';
1616
import { IJavaTestItem } from '../types';
1717
import { loadRunConfig } from '../utils/configUtils';
1818
import { resolveLaunchConfigurationForRunner } from '../utils/launchUtils';
19-
import { dataCache, ITestItemData } from './testItemDataCache';
20-
import { createTestItem, findDirectTestChildrenForClass, findTestPackagesAndTypes, findTestTypesAndMethods, loadJavaProjects, resolvePath, synchronizeItemsRecursively, updateItemForDocumentWithDebounce } from './utils';
19+
import { dataCache, getResolutionVersion, invalidateResolutionVersion, ITestItemData } from './testItemDataCache';
20+
import { createTestItem, findDirectTestChildrenForClass, findTestPackagesAndTypes, findTestTypesAndMethods, loadJavaProjects, removeOutdatedTestItemsForDocument, resolvePath, synchronizeItemsRecursively, updateItemForDocumentWithDebounce } from './utils';
2121
import { JavaTestCoverageProvider } from '../provider/JavaTestCoverageProvider';
2222
import { testRunnerService } from './testRunnerService';
2323
import { IRunTestContext, TestRunner, TestFinishEvent, TestItemStatusChangeEvent, TestKind, TestLevel, TestResultState, TestIdParts } from '../java-test-runner.api';
@@ -27,6 +27,7 @@ import { parsePartsFromTestId } from '../utils/testItemUtils';
2727
export let testController: TestController | undefined;
2828
export const watchers: Disposable[] = [];
2929
export const runnableTag: TestTag = new TestTag('runnable');
30+
const pendingTestItemResolutions: WeakMap<TestItem, Promise<void>> = new WeakMap();
3031

3132
export function createTestController(): void {
3233
testController?.dispose();
@@ -51,18 +52,68 @@ export function creatTestProfile(name: string, kind: TestRunProfileKind): void {
5152
testController?.createRunProfile(name, kind, runHandler, false, runnableTag);
5253
}
5354

54-
export const loadChildren: (item: TestItem, token?: CancellationToken) => any = instrumentOperation('java.test.explorer.loadChildren', async (_operationId: string, item: TestItem, token?: CancellationToken) => {
55+
export const loadChildren: (item: TestItem, token?: CancellationToken, force?: boolean) => Promise<void> = instrumentOperation('java.test.explorer.loadChildren', async (_operationId: string, item: TestItem, token?: CancellationToken, force: boolean = false) => {
5556
if (!item) {
5657
await loadJavaProjects();
5758
return;
5859
}
5960

60-
const data: ITestItemData | undefined = dataCache.get(item);
61-
if (!data) {
61+
if (!dataCache.get(item)) {
6262
return;
6363
}
64+
65+
if (token?.isCancellationRequested) {
66+
return;
67+
}
68+
69+
if (force) {
70+
invalidateTestItemResolution(item);
71+
} else if (!item.canResolveChildren) {
72+
return;
73+
}
74+
75+
while (item.canResolveChildren) {
76+
if (token?.isCancellationRequested) {
77+
return;
78+
}
79+
80+
const pendingResolution: Promise<void> | undefined = pendingTestItemResolutions.get(item);
81+
if (pendingResolution) {
82+
await pendingResolution;
83+
if (pendingTestItemResolutions.get(item) === pendingResolution) {
84+
pendingTestItemResolutions.delete(item);
85+
}
86+
continue;
87+
}
88+
89+
const data: ITestItemData | undefined = dataCache.get(item);
90+
if (!data) {
91+
return;
92+
}
93+
const resolutionVersion: number = getResolutionVersion(item);
94+
const resolution: Promise<void> = resolveTestItemChildren(item, data, resolutionVersion, token);
95+
pendingTestItemResolutions.set(item, resolution);
96+
try {
97+
await resolution;
98+
} finally {
99+
if (pendingTestItemResolutions.get(item) === resolution) {
100+
pendingTestItemResolutions.delete(item);
101+
}
102+
}
103+
104+
if (token?.isCancellationRequested || resolutionVersion === getResolutionVersion(item)) {
105+
return;
106+
}
107+
}
108+
});
109+
110+
async function resolveTestItemChildren(item: TestItem, data: ITestItemData, resolutionVersion: number,
111+
token?: CancellationToken): Promise<void> {
64112
if (data.testLevel === TestLevel.Project) {
65113
const packageAndTypes: IJavaTestItem[] = await findTestPackagesAndTypes(data.jdtHandler, token);
114+
if (token?.isCancellationRequested || resolutionVersion !== getResolutionVersion(item)) {
115+
return;
116+
}
66117
synchronizeItemsRecursively(item, packageAndTypes);
67118
} else if (data.testLevel === TestLevel.Package) {
68119
// unreachable code
@@ -72,9 +123,30 @@ export const loadChildren: (item: TestItem, token?: CancellationToken) => any =
72123
return;
73124
}
74125
const testMethods: IJavaTestItem[] = await findDirectTestChildrenForClass(data.jdtHandler, token);
126+
if (token?.isCancellationRequested || resolutionVersion !== getResolutionVersion(item)) {
127+
return;
128+
}
75129
synchronizeItemsRecursively(item, testMethods);
76130
}
77-
});
131+
132+
if (resolutionVersion !== getResolutionVersion(item)) {
133+
return;
134+
}
135+
item.canResolveChildren = false;
136+
}
137+
138+
function invalidateTestItemResolution(item: TestItem): void {
139+
const testLevel: TestLevel | undefined = dataCache.get(item)?.testLevel;
140+
if (testLevel === TestLevel.Project || testLevel === TestLevel.Class) {
141+
invalidateResolutionVersion(item);
142+
}
143+
if (testLevel !== undefined && testLevel <= TestLevel.Class) {
144+
item.canResolveChildren = true;
145+
}
146+
item.children.forEach((child: TestItem) => {
147+
invalidateTestItemResolution(child);
148+
});
149+
}
78150

79151
async function startWatchingWorkspace(): Promise<void> {
80152
if (!workspace.workspaceFolders) {
@@ -127,11 +199,7 @@ async function startWatchingWorkspace(): Promise<void> {
127199
return;
128200
}
129201

130-
belongingPackage.children.forEach((item: TestItem) => {
131-
if (item.uri?.toString() === uri.toString()) {
132-
belongingPackage.children.delete(item.id);
133-
}
134-
});
202+
removeOutdatedTestItemsForDocument(belongingPackage, uri, new Set<string>());
135203

136204
if (belongingPackage.children.size === 0) {
137205
belongingProject.children.delete(belongingPackage.id);

‎src/controller/testItemDataCache.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,16 @@ class TestItemDataCache {
2727

2828
export const dataCache: TestItemDataCache = new TestItemDataCache();
2929

30+
const resolutionVersions: WeakMap<TestItem, number> = new WeakMap();
31+
32+
export function getResolutionVersion(item: TestItem): number {
33+
return resolutionVersions.get(item) ?? 0;
34+
}
35+
36+
export function invalidateResolutionVersion(item: TestItem): void {
37+
resolutionVersions.set(item, getResolutionVersion(item) + 1);
38+
}
39+
3040
export interface ITestItemData {
3141
jdtHandler: string;
3242
fullName: string;

‎src/controller/utils.ts‎

Lines changed: 80 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { IJavaTestItem, ProjectType } from '../types';
1212
import { executeJavaLanguageServerCommand } from '../utils/commandUtils';
1313
import { getRequestDelay, lruCache, MovingAverage } from './debouncing';
1414
import { runnableTag, testController } from './testController';
15-
import { dataCache } from './testItemDataCache';
15+
import { dataCache, invalidateResolutionVersion } from './testItemDataCache';
1616
import { TestKind, TestLevel } from '../java-test-runner.api';
1717

1818
/**
@@ -90,30 +90,43 @@ export async function getProjectType(item: IJavaTestItem): Promise<ProjectType>
9090
* - If an existing child is not contained in the childrenData parameter, it will be deleted
9191
* - If a child does not exist, create it, otherwise, update it as well as its metadata.
9292
*/
93-
export function synchronizeItemsRecursively(parent: TestItem, childrenData: IJavaTestItem[] | undefined): void {
94-
if (childrenData) {
95-
// remove the out-of-date children
96-
parent.children.forEach((child: TestItem) => {
97-
if (dataCache.get(child)?.testLevel === TestLevel.Invocation) {
98-
// only remove the invocation items before a new test session starts
99-
return;
100-
}
101-
const existingItem: IJavaTestItem | undefined = childrenData.find((data: IJavaTestItem) => data.id === child.id);
102-
if (!existingItem) {
103-
parent.children.delete(child.id);
104-
}
105-
});
106-
// update/create children
107-
for (const child of childrenData) {
108-
const childItem: TestItem = updateOrCreateTestItem(parent, child);
109-
if (child.testLevel <= TestLevel.Class) {
110-
childItem.canResolveChildren = true;
111-
}
112-
synchronizeItemsRecursively(childItem, child.children);
93+
export function synchronizeItemsRecursively(parent: TestItem, childrenData: IJavaTestItem[] | undefined,
94+
childrenAreComplete: boolean = false): void {
95+
if (!childrenData && !childrenAreComplete) {
96+
return;
97+
}
98+
99+
const children: IJavaTestItem[] = childrenData ?? [];
100+
// remove the out-of-date children
101+
parent.children.forEach((child: TestItem) => {
102+
if (dataCache.get(child)?.testLevel === TestLevel.Invocation) {
103+
// only remove the invocation items before a new test session starts
104+
return;
105+
}
106+
const existingItem: IJavaTestItem | undefined = children.find((data: IJavaTestItem) => data.id === child.id);
107+
if (!existingItem) {
108+
parent.children.delete(child.id);
109+
}
110+
});
111+
// update/create children
112+
for (const child of children) {
113+
const childItem: TestItem = updateOrCreateTestItem(parent, child);
114+
if (child.testLevel <= TestLevel.Class) {
115+
childItem.canResolveChildren = true;
113116
}
117+
synchronizeItemsRecursively(childItem, child.children, childrenAreComplete);
114118
}
115119
}
116120

121+
export function markTestClassesResolvedRecursively(item: TestItem): void {
122+
if (dataCache.get(item)?.testLevel === TestLevel.Class) {
123+
item.canResolveChildren = false;
124+
}
125+
item.children.forEach((child: TestItem) => {
126+
markTestClassesResolvedRecursively(child);
127+
});
128+
}
129+
117130
export function updateOrCreateTestItem(parent: TestItem, childData: IJavaTestItem): TestItem {
118131
let childItem: TestItem | undefined = parent.children.get(childData.id);
119132
if (childItem) {
@@ -125,6 +138,7 @@ export function updateOrCreateTestItem(parent: TestItem, childData: IJavaTestIte
125138
}
126139

127140
function updateTestItem(testItem: TestItem, metaInfo: IJavaTestItem): void {
141+
const previousJdtHandler: string | undefined = dataCache.get(testItem)?.jdtHandler;
128142
testItem.range = asRange(metaInfo.range);
129143
testItem.label = metaInfo.label;
130144
dataCache.set(testItem, {
@@ -134,6 +148,10 @@ function updateTestItem(testItem: TestItem, metaInfo: IJavaTestItem): void {
134148
testLevel: metaInfo.testLevel,
135149
testKind: metaInfo.testKind,
136150
});
151+
if (previousJdtHandler !== undefined && previousJdtHandler !== metaInfo.jdtHandler &&
152+
(metaInfo.testLevel === TestLevel.Project || metaInfo.testLevel === TestLevel.Class)) {
153+
invalidateResolutionVersion(testItem);
154+
}
137155
}
138156

139157
/**
@@ -217,15 +235,11 @@ export async function updateItemForDocument(uri: Uri, testTypes?: IJavaTestItem[
217235
return [];
218236
}
219237

238+
const expectedTypeIds: Set<string> = new Set(testTypes.map((testType: IJavaTestItem) => testType.id));
239+
removeOutdatedTestItemsForDocument(belongingPackage, uri, expectedTypeIds);
240+
220241
const tests: TestItem[] = [];
221-
if (testTypes.length === 0) {
222-
// Remove the children with the same uri when no test items is found
223-
belongingPackage.children.forEach((typeItem: TestItem) => {
224-
if (path.relative(typeItem.uri?.fsPath || '', uri.fsPath) === '') {
225-
belongingPackage!.children.delete(typeItem.id);
226-
}
227-
});
228-
} else {
242+
if (testTypes.length > 0) {
229243
for (const testType of testTypes) {
230244
// here we do not directly call synchronizeItemsRecursively() because testTypes here are just part of the
231245
// children of the belonging package, we don't want to delete other children unexpectedly.
@@ -237,7 +251,8 @@ export async function updateItemForDocument(uri: Uri, testTypes?: IJavaTestItem[
237251
updateTestItem(testTypeItem, testType);
238252
}
239253
tests.push(testTypeItem);
240-
synchronizeItemsRecursively(testTypeItem, testType.children);
254+
synchronizeItemsRecursively(testTypeItem, testType.children, true);
255+
markTestClassesResolvedRecursively(testTypeItem);
241256
}
242257
}
243258

@@ -248,6 +263,41 @@ export async function updateItemForDocument(uri: Uri, testTypes?: IJavaTestItem[
248263
return tests;
249264
}
250265

266+
export function removeOutdatedTestItemsForDocument(belongingPackage: TestItem, uri: Uri,
267+
expectedTypeIds: Set<string>): void {
268+
const belongingProject: TestItem | undefined = belongingPackage.parent;
269+
if (!belongingProject) {
270+
return;
271+
}
272+
273+
invalidateResolutionVersion(belongingProject);
274+
belongingProject.children.forEach((packageItem: TestItem) => {
275+
packageItem.children.forEach((typeItem: TestItem) => {
276+
if (path.relative(typeItem.uri?.fsPath || '', uri.fsPath) !== '') {
277+
return;
278+
}
279+
280+
invalidateResolutionRecursively(typeItem);
281+
if (!expectedTypeIds.has(typeItem.id)) {
282+
packageItem.children.delete(typeItem.id);
283+
}
284+
});
285+
286+
if (packageItem !== belongingPackage && packageItem.children.size === 0) {
287+
belongingProject.children.delete(packageItem.id);
288+
}
289+
});
290+
}
291+
292+
function invalidateResolutionRecursively(item: TestItem): void {
293+
if (dataCache.get(item)?.testLevel === TestLevel.Class) {
294+
invalidateResolutionVersion(item);
295+
}
296+
item.children.forEach((child: TestItem) => {
297+
invalidateResolutionRecursively(child);
298+
});
299+
}
300+
251301
/**
252302
* Give a test item for a type, find its belonging package item according to its id.
253303
*/

‎src/types.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { Range } from 'vscode';
55
import { TestKind, TestLevel } from './java-test-runner.api';
66

77
export interface IJavaTestItem {
8-
children: IJavaTestItem[];
8+
children?: IJavaTestItem[];
99
uri: string | undefined;
1010
range: Range | undefined;
1111
jdtHandler: string;

0 commit comments

Comments
 (0)