From bbbef03cc4abb44eba42b36f0254c59d04ae2b21 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Fri, 30 Jun 2023 12:14:57 -0700 Subject: [PATCH 1/2] testing: improve performance when ending test with large number of results Takes `markTaskComplete` 11,200ms to 70ms when running a 10k test suite, by maintaining a count of computed states for children and avoiding refreshing nodes unnecessarily. For https://github.com/microsoft/vscode-python/issues/21507 --- .../browser/testingProgressUiService.ts | 4 +- .../testing/common/getComputedState.ts | 62 +++++++++++++++---- .../contrib/testing/common/testResult.ts | 18 +----- .../contrib/testing/common/testingStates.ts | 10 +++ .../test/common/testResultService.test.ts | 53 +++++++--------- 5 files changed, 87 insertions(+), 60 deletions(-) diff --git a/src/vs/workbench/contrib/testing/browser/testingProgressUiService.ts b/src/vs/workbench/contrib/testing/browser/testingProgressUiService.ts index 570572ded8f79..a3ffe5863c2af 100644 --- a/src/vs/workbench/contrib/testing/browser/testingProgressUiService.ts +++ b/src/vs/workbench/contrib/testing/browser/testingProgressUiService.ts @@ -13,8 +13,8 @@ import { ProgressLocation, UnmanagedProgress } from 'vs/platform/progress/common import { IViewsService } from 'vs/workbench/common/views'; import { AutoOpenTesting, getTestingConfiguration, TestingConfigKeys } from 'vs/workbench/contrib/testing/common/configuration'; import { Testing } from 'vs/workbench/contrib/testing/common/constants'; -import { isFailedState } from 'vs/workbench/contrib/testing/common/testingStates'; -import { LiveTestResult, TestResultItemChangeReason, TestStateCount } from 'vs/workbench/contrib/testing/common/testResult'; +import { isFailedState, TestStateCount } from 'vs/workbench/contrib/testing/common/testingStates'; +import { LiveTestResult, TestResultItemChangeReason } from 'vs/workbench/contrib/testing/common/testResult'; import { ITestResultService } from 'vs/workbench/contrib/testing/common/testResultService'; import { TestResultState } from 'vs/workbench/contrib/testing/common/testTypes'; diff --git a/src/vs/workbench/contrib/testing/common/getComputedState.ts b/src/vs/workbench/contrib/testing/common/getComputedState.ts index ad35eaf381e31..bcffb8fb522ea 100644 --- a/src/vs/workbench/contrib/testing/common/getComputedState.ts +++ b/src/vs/workbench/contrib/testing/common/getComputedState.ts @@ -5,7 +5,7 @@ import { Iterable } from 'vs/base/common/iterator'; import { TestResultState } from 'vs/workbench/contrib/testing/common/testTypes'; -import { maxPriority, statePriority } from 'vs/workbench/contrib/testing/common/testingStates'; +import { makeEmptyCounts, maxPriority, statePriority } from 'vs/workbench/contrib/testing/common/testingStates'; /** * Accessor for nodes in get and refresh computed state. @@ -32,18 +32,28 @@ const isDurationAccessor = (accessor: IComputedStateAccessor): accessor is * if it was previously set. */ -const getComputedState = (accessor: IComputedStateAccessor, node: T, force = false) => { +const getComputedState = (accessor: IComputedStateAccessor, node: T, force = false) => { let computed = accessor.getCurrentComputedState(node); if (computed === undefined || force) { computed = accessor.getOwnState(node) ?? TestResultState.Unset; + let childrenCount = 0; + const stateMap = makeEmptyCounts(); + for (const child of accessor.getChildren(node)) { const childComputed = getComputedState(accessor, child); + childrenCount++; + stateMap[childComputed]++; + // If all children are skipped, make the current state skipped too if unset (#131537) computed = childComputed === TestResultState.Skipped && computed === TestResultState.Unset ? TestResultState.Skipped : maxPriority(computed, childComputed); } + if (childrenCount > LARGE_NODE_THRESHOLD) { + largeNodeChildrenStates.set(node, stateMap); + } + accessor.setComputedState(node, computed); } @@ -72,11 +82,19 @@ const getComputedDuration = (accessor: IComputedStateAndDurationAccessor, return computed; }; +const LARGE_NODE_THRESHOLD = 64; + +/** + * Map of how many nodes have in each state. This is used to optimize state + * computation in large nodes with children above the `LARGE_NODE_THRESHOLD`. + */ +const largeNodeChildrenStates = new WeakMap(); + /** * Refreshes the computed state for the node and its parents. Any changes * elements cause `addUpdated` to be called. */ -export const refreshComputedState = ( +export const refreshComputedState = ( accessor: IComputedStateAccessor, node: T, explicitNewComputedState?: TestResultState, @@ -92,28 +110,46 @@ export const refreshComputedState = ( accessor.setComputedState(node, newState); toUpdate.add(node); - if (newPriority > oldPriority) { - // Update all parents to ensure they're at least this priority. - for (const parent of accessor.getParents(node)) { - const prev = accessor.getCurrentComputedState(parent); + let moveFromState = oldState; + let moveToState = newState; + + for (const parent of accessor.getParents(node)) { + const lnm = largeNodeChildrenStates.get(parent); + if (lnm) { + lnm[moveFromState]--; + lnm[moveToState]++; + } + + const prev = accessor.getCurrentComputedState(parent); + if (newPriority > oldPriority) { + // Update all parents to ensure they're at least this priority. if (prev !== undefined && statePriority[prev] >= newPriority) { break; } + if (lnm && lnm[moveToState] > 1) { + break; + } + + // moveToState remains the same, the new higher priority node state accessor.setComputedState(parent, newState); toUpdate.add(parent); - } - } else if (newPriority < oldPriority) { - // Re-render all parents of this node whose computed priority might have come from this node - for (const parent of accessor.getParents(node)) { - const prev = accessor.getCurrentComputedState(parent); + } else /* newProirity < oldPriority */ { + // Update all parts whose statese might have been based on this one if (prev === undefined || statePriority[prev] > oldPriority) { break; } - accessor.setComputedState(parent, getComputedState(accessor, parent, true)); + if (lnm && lnm[moveFromState] > 0) { + break; + } + + moveToState = getComputedState(accessor, parent, true); + accessor.setComputedState(parent, moveToState); toUpdate.add(parent); } + + moveFromState = prev; } } diff --git a/src/vs/workbench/contrib/testing/common/testResult.ts b/src/vs/workbench/contrib/testing/common/testResult.ts index c2130600cdbe7..fad1eefcadba4 100644 --- a/src/vs/workbench/contrib/testing/common/testResult.ts +++ b/src/vs/workbench/contrib/testing/common/testResult.ts @@ -14,7 +14,7 @@ import { IComputedStateAccessor, refreshComputedState } from 'vs/workbench/contr import { IObservableValue, MutableObservableValue, staticObservableValue } from 'vs/workbench/contrib/testing/common/observableValue'; import { TestCoverage } from 'vs/workbench/contrib/testing/common/testCoverage'; import { TestId } from 'vs/workbench/contrib/testing/common/testId'; -import { maxPriority, statesInOrder, terminalStatePriorities } from 'vs/workbench/contrib/testing/common/testingStates'; +import { makeEmptyCounts, maxPriority, statesInOrder, terminalStatePriorities, TestStateCount } from 'vs/workbench/contrib/testing/common/testingStates'; import { getMarkId, IRichLocation, ISerializedTestResults, ITestItem, ITestMessage, ITestOutputMessage, ITestRunTask, ITestTaskState, ResolvedTestRunRequest, TestItemExpandState, TestMessageType, TestResultItem, TestResultState } from 'vs/workbench/contrib/testing/common/testTypes'; export interface ITestRunTaskResults extends ITestRunTask { @@ -185,20 +185,6 @@ export const resultItemParents = function* (results: ITestResult, item: TestResu } }; -/** - * Count of the number of tests in each run state. - */ -export type TestStateCount = { [K in TestResultState]: number }; - -export const makeEmptyCounts = () => { - const o: Partial = {}; - for (const state of statesInOrder) { - o[state] = 0; - } - - return o as TestStateCount; -}; - export const maxCountPriority = (counts: Readonly) => { for (const state of statesInOrder) { if (counts[state] > 0) { @@ -266,7 +252,7 @@ export class LiveTestResult implements ITestResult { /** * @inheritdoc */ - public readonly counts: { [K in TestResultState]: number } = makeEmptyCounts(); + public readonly counts = makeEmptyCounts(); /** * @inheritdoc diff --git a/src/vs/workbench/contrib/testing/common/testingStates.ts b/src/vs/workbench/contrib/testing/common/testingStates.ts index 0ec1fb9143264..98b246bda2f0c 100644 --- a/src/vs/workbench/contrib/testing/common/testingStates.ts +++ b/src/vs/workbench/contrib/testing/common/testingStates.ts @@ -69,3 +69,13 @@ export const terminalStatePriorities: { [key in TestResultState]?: number } = { [TestResultState.Failed]: 2, [TestResultState.Errored]: 3, }; + +/** + * Count of the number of tests in each run state. + */ +export type TestStateCount = { [K in TestResultState]: number }; + +export const makeEmptyCounts = (): TestStateCount => { + // shh! don't tell anyone this is actually an array! + return new Uint32Array(statesInOrder.length) as any as { [K in TestResultState]: number }; +}; diff --git a/src/vs/workbench/contrib/testing/test/common/testResultService.test.ts b/src/vs/workbench/contrib/testing/test/common/testResultService.test.ts index 6acc4b8584749..6e3dbf4ee5485 100644 --- a/src/vs/workbench/contrib/testing/test/common/testResultService.test.ts +++ b/src/vs/workbench/contrib/testing/test/common/testResultService.test.ts @@ -10,10 +10,11 @@ import { MockContextKeyService } from 'vs/platform/keybinding/test/common/mockKe import { NullLogService } from 'vs/platform/log/common/log'; import { TestId } from 'vs/workbench/contrib/testing/common/testId'; import { TestProfileService } from 'vs/workbench/contrib/testing/common/testProfileService'; -import { HydratedTestResult, LiveTestResult, makeEmptyCounts, resultItemParents, TaskRawOutput, TestResultItemChange, TestResultItemChangeReason } from 'vs/workbench/contrib/testing/common/testResult'; +import { HydratedTestResult, LiveTestResult, resultItemParents, TaskRawOutput, TestResultItemChange, TestResultItemChangeReason } from 'vs/workbench/contrib/testing/common/testResult'; import { TestResultService } from 'vs/workbench/contrib/testing/common/testResultService'; import { InMemoryResultStorage, ITestResultStorage } from 'vs/workbench/contrib/testing/common/testResultStorage'; import { ITestTaskState, ResolvedTestRunRequest, TestResultItem, TestResultState, TestRunProfileBitset } from 'vs/workbench/contrib/testing/common/testTypes'; +import { makeEmptyCounts } from 'vs/workbench/contrib/testing/common/testingStates'; import { getInitializedMainTestCollection, testStubs, TestTestCollection } from 'vs/workbench/contrib/testing/test/common/testStubs'; import { TestStorageService } from 'vs/workbench/test/common/workbenchTestServices'; @@ -93,27 +94,24 @@ suite('Workbench - Test Results Service', () => { }); test('initializes with valid counts', () => { - assert.deepStrictEqual(r.counts, { - ...makeEmptyCounts(), - [TestResultState.Unset]: 4, - }); + const c = makeEmptyCounts(); + c[TestResultState.Unset] = 4; + assert.deepStrictEqual(r.counts, c); }); test('setAllToState', () => { changed.clear(); r.setAllToStatePublic(TestResultState.Queued, 't', (_, t) => t.item.label !== 'root'); - assert.deepStrictEqual(r.counts, { - ...makeEmptyCounts(), - [TestResultState.Unset]: 1, - [TestResultState.Queued]: 3, - }); + const c = makeEmptyCounts(); + c[TestResultState.Unset] = 1; + c[TestResultState.Queued] = 3; + assert.deepStrictEqual(r.counts, c); r.setAllToStatePublic(TestResultState.Failed, 't', (_, t) => t.item.label !== 'root'); - assert.deepStrictEqual(r.counts, { - ...makeEmptyCounts(), - [TestResultState.Unset]: 1, - [TestResultState.Failed]: 3, - }); + const c2 = makeEmptyCounts(); + c2[TestResultState.Unset] = 1; + c2[TestResultState.Failed] = 3; + assert.deepStrictEqual(r.counts, c2); assert.deepStrictEqual(r.getStateById(new TestId(['ctrlId', 'id-a']).toString())?.ownComputedState, TestResultState.Failed); assert.deepStrictEqual(r.getStateById(new TestId(['ctrlId', 'id-a']).toString())?.tasks[0].state, TestResultState.Failed); @@ -134,11 +132,10 @@ suite('Workbench - Test Results Service', () => { changed.clear(); const testId = new TestId(['ctrlId', 'id-a', 'id-aa']).toString(); r.updateState(testId, 't', TestResultState.Running); - assert.deepStrictEqual(r.counts, { - ...makeEmptyCounts(), - [TestResultState.Unset]: 3, - [TestResultState.Running]: 1, - }); + const c = makeEmptyCounts(); + c[TestResultState.Running] = 1; + c[TestResultState.Unset] = 3; + assert.deepStrictEqual(r.counts, c); assert.deepStrictEqual(r.getStateById(testId)?.ownComputedState, TestResultState.Running); // update computed state: assert.deepStrictEqual(r.getStateById(tests.root.id)?.computedState, TestResultState.Running); @@ -161,10 +158,9 @@ suite('Workbench - Test Results Service', () => { test('ignores outside run', () => { changed.clear(); r.updateState(new TestId(['ctrlId', 'id-b']).toString(), 't', TestResultState.Running); - assert.deepStrictEqual(r.counts, { - ...makeEmptyCounts(), - [TestResultState.Unset]: 4, - }); + const c = makeEmptyCounts(); + c[TestResultState.Unset] = 4; + assert.deepStrictEqual(r.counts, c); assert.deepStrictEqual(r.getStateById(new TestId(['ctrlId', 'id-b']).toString()), undefined); }); @@ -175,11 +171,10 @@ suite('Workbench - Test Results Service', () => { r.markComplete(); - assert.deepStrictEqual(r.counts, { - ...makeEmptyCounts(), - [TestResultState.Passed]: 1, - [TestResultState.Unset]: 3, - }); + const c = makeEmptyCounts(); + c[TestResultState.Unset] = 3; + c[TestResultState.Passed] = 1; + assert.deepStrictEqual(r.counts, c); assert.deepStrictEqual(r.getStateById(tests.root.id)?.ownComputedState, TestResultState.Unset); assert.deepStrictEqual(r.getStateById(new TestId(['ctrlId', 'id-a', 'id-aa']).toString())?.ownComputedState, TestResultState.Passed); From b1028c4e18122eacbb3a0bbe884921bea5f1518f Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Fri, 30 Jun 2023 15:37:04 -0700 Subject: [PATCH 2/2] testing: fix state not updating when failing peek opens Fixes #186376 --- .../ui/tree/compressedObjectTreeModel.ts | 2 +- .../testing/browser/testingOutputPeek.ts | 23 ++++++++++++++----- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/vs/base/browser/ui/tree/compressedObjectTreeModel.ts b/src/vs/base/browser/ui/tree/compressedObjectTreeModel.ts index 241c26daf23e1..2104ebeb01d52 100644 --- a/src/vs/base/browser/ui/tree/compressedObjectTreeModel.ts +++ b/src/vs/base/browser/ui/tree/compressedObjectTreeModel.ts @@ -146,7 +146,7 @@ export class CompressedObjectTreeModel, TFilterData e children: Iterable> = Iterable.empty(), options: IObjectTreeModelSetChildrenOptions, ): void { - // Diffs must be deem, since the compression can affect nested elements. + // Diffs must be deep, since the compression can affect nested elements. // @see https://github.com/microsoft/vscode/pull/114237#issuecomment-759425034 const diffIdentityProvider = options.diffIdentityProvider && wrapIdentityProvider(options.diffIdentityProvider); diff --git a/src/vs/workbench/contrib/testing/browser/testingOutputPeek.ts b/src/vs/workbench/contrib/testing/browser/testingOutputPeek.ts index 1a848d6ea4b37..134ec676516c9 100644 --- a/src/vs/workbench/contrib/testing/browser/testingOutputPeek.ts +++ b/src/vs/workbench/contrib/testing/browser/testingOutputPeek.ts @@ -1670,7 +1670,7 @@ class OutputPeekTree extends Disposable { taskChildrenToUpdate.clear(); }, 300)); - const handleNewResults = (result: LiveTestResult) => { + const attachToResults = (result: LiveTestResult) => { const resultNode = cc.get(result)! as TestResultElement; const disposable = new DisposableStore(); disposable.add(result.onNewTask(() => { @@ -1713,7 +1713,7 @@ class OutputPeekTree extends Disposable { disposable.dispose(); })); - this.tree.expand(resultNode, true); + return resultNode; }; this._register(results.onResultsChanged(e => { @@ -1737,7 +1737,7 @@ class OutputPeekTree extends Disposable { this.tree.collapse(child.element, false); } - handleNewResults(e.started); + this.tree.expand(attachToResults(e.started), true); } })); @@ -1749,7 +1749,7 @@ class OutputPeekTree extends Disposable { } }; - this._register(onDidReveal(({ subject, preserveFocus = false }) => { + this._register(onDidReveal(async ({ subject, preserveFocus = false }) => { if (subject instanceof TaskSubject) { const resultItem = this.tree.getNode(null).children.find(c => (c.element as TestResultElement)?.id === subject.resultId); if (resultItem) { @@ -1798,6 +1798,11 @@ class OutputPeekTree extends Disposable { this._register(this.tree.onContextMenu(e => this.onContextMenu(e))); this.tree.setChildren(null, getRootChildren()); + for (const result of results.results) { + if (!result.completedAt && result instanceof LiveTestResult) { + attachToResults(result); + } + } } public layout(height: number, width: number) { @@ -1880,8 +1885,6 @@ class TestRunElementRenderer implements ICompressibleTreeRenderer, _index: number, templateData: TemplateData): void { - templateData.elementDisposable.clear(); - templateData.elementDisposable.add(element.element.onDidChange(() => this.doRender(element.element, templateData))); this.doRender(element.element, templateData); } @@ -1890,7 +1893,15 @@ class TestRunElementRenderer implements ICompressibleTreeRenderer this.doRender(element, templateData))); + this.doRenderInner(element, templateData); + } + + /** Called, and may be re-called, to render or re-render an element */ + private doRenderInner(element: ITreeElement, templateData: TemplateData) { if (element.labelWithIcons) { dom.reset(templateData.label, ...element.labelWithIcons); } else if (element.description) {