From 435b3eb0185bcb5ece9900195db310881fdb2227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Olivares?= Date: Mon, 17 Oct 2022 11:46:39 +0000 Subject: [PATCH] Make PerformanceModel processing async In preparation to resolve function names in the perfomance panel using source maps, this CL implements two functional changes: 1. The loadingComplete function of the PerformanceModel is set to be async. This is because the name resolving will require the result of async invocations and this function is ideal for adding the calls into (since this function commands the trace processing after all trace events are available). With this change, other functions in the call chain are updated accordingly (this includes calls in the legacy TimlineTestRunner) 2. To avoid race conditions, the PerformanceModel instance owned by the TimelinePanel is not used until the TimelineController has finished the processing pipeline. Before this CL, this waiting didn't happen made the code subject to race conditions, which were made evident with the implementation of (1). Tests in Chromium land that make use of the updated helpers in the test runner need to be updated before this CL can land. Bug: 1364601 Change-Id: I2b7586337793eafd570e98947dcfbb574e6e43e5 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3941705 Reviewed-by: Jack Franklin Commit-Queue: Andres Olivares --- .../TimelineTestRunner.js | 24 ++++++----------- front_end/models/bindings/FileUtils.ts | 6 ++--- front_end/panels/timeline/PerformanceModel.ts | 2 +- .../panels/timeline/TimelineController.ts | 14 +++++++--- front_end/panels/timeline/TimelineLoader.ts | 21 +++++++-------- front_end/panels/timeline/TimelinePanel.ts | 27 +++++++++++-------- .../performance/performance-pane-a11y-test.js | 3 +-- .../performance_event_log_a11y_test.js | 2 +- 8 files changed, 50 insertions(+), 49 deletions(-) diff --git a/front_end/legacy_test_runner/performance_test_runner/TimelineTestRunner.js b/front_end/legacy_test_runner/performance_test_runner/TimelineTestRunner.js index 248e7f4c94..4c4a0f35c9 100644 --- a/front_end/legacy_test_runner/performance_test_runner/TimelineTestRunner.js +++ b/front_end/legacy_test_runner/performance_test_runner/TimelineTestRunner.js @@ -102,8 +102,7 @@ PerformanceTestRunner.invokeWithTracing = function(functionName, callback, addit } function onPageActionsDone() { - PerformanceTestRunner.runWhenTimelineIsReady(callback); - timelineController.stopRecording(); + timelineController.stopRecording().then(callback); } }; @@ -119,12 +118,12 @@ PerformanceTestRunner.timelineFrameModel = function() { return PerformanceTestRunner.performanceModel().frameModel(); }; -PerformanceTestRunner.createPerformanceModelWithEvents = function(events) { +PerformanceTestRunner.createPerformanceModelWithEvents = async function(events) { const tracingModel = new SDK.TracingModel(new Bindings.TempFileBackingStorage('tracing')); tracingModel.addEvents(events); tracingModel.tracingComplete(); const performanceModel = new Timeline.PerformanceModel(); - performanceModel.setTracingModel(tracingModel); + await performanceModel.setTracingModel(tracingModel); UI.panels.timeline.performanceModel = performanceModel; UI.panels.timeline.applyFilters(performanceModel); return performanceModel; @@ -137,7 +136,7 @@ PerformanceTestRunner.createTimelineController = function() { }; PerformanceTestRunner.runWhenTimelineIsReady = function(callback) { - TestRunner.addSniffer(UI.panels.timeline, 'loadingComplete', () => callback()); + TestRunner.addSniffer(UI.panels.timeline, 'loadingCompleteForTest', () => callback()); }; PerformanceTestRunner.startTimeline = function() { @@ -146,11 +145,8 @@ PerformanceTestRunner.startTimeline = function() { return TestRunner.addSnifferPromise(panel, 'recordingStarted'); }; -PerformanceTestRunner.stopTimeline = function() { - return new Promise(resolve => { - PerformanceTestRunner.runWhenTimelineIsReady(resolve); - UI.panels.timeline.toggleRecording(); - }); +PerformanceTestRunner.stopTimeline = async function() { + await UI.panels.timeline.toggleRecording(); }; PerformanceTestRunner.runPerfTraceWithReload = async function() { @@ -405,12 +401,8 @@ PerformanceTestRunner.dumpTimelineFlameChart = function(includeGroups) { PerformanceTestRunner.dumpFlameChartProvider(provider, includeGroups); }; -PerformanceTestRunner.loadTimeline = function(timelineData) { - const promise = new Promise(fulfill => PerformanceTestRunner.runWhenTimelineIsReady(fulfill)); - - UI.panels.timeline.loadFromFile(new Blob([timelineData], {type: 'text/plain'})); - - return promise; +PerformanceTestRunner.loadTimeline = async function(timelineData) { + await UI.panels.timeline.loadFromFile(new Blob([timelineData], {type: 'text/plain'})); }; TestRunner.deprecatedInitAsync(` diff --git a/front_end/models/bindings/FileUtils.ts b/front_end/models/bindings/FileUtils.ts index 8e2b7d3ea5..1fda4c89eb 100644 --- a/front_end/models/bindings/FileUtils.ts +++ b/front_end/models/bindings/FileUtils.ts @@ -163,19 +163,19 @@ export class ChunkedFileReader implements ChunkedReader { } if (endOfFile) { - this.finishRead(); + void this.finishRead(); return; } void this.loadChunk(); } - private finishRead(): void { + private async finishRead(): Promise { if (!this.#output) { return; } this.#file = null; this.#reader = null; - void this.#output.close(); + await this.#output.close(); this.#transferFinished(!this.#errorInternal); } diff --git a/front_end/panels/timeline/PerformanceModel.ts b/front_end/panels/timeline/PerformanceModel.ts index 89efcc5ffc..2e4bfc13f2 100644 --- a/front_end/panels/timeline/PerformanceModel.ts +++ b/front_end/panels/timeline/PerformanceModel.ts @@ -71,7 +71,7 @@ export class PerformanceModel extends Common.ObjectWrapper.ObjectWrapper f.accept(event)); } - setTracingModel(model: SDK.TracingModel.TracingModel): void { + async setTracingModel(model: SDK.TracingModel.TracingModel): Promise { this.tracingModelInternal = model; this.timelineModelInternal.setEvents(model); diff --git a/front_end/panels/timeline/TimelineController.ts b/front_end/panels/timeline/TimelineController.ts index 7a808a98ea..e9f9e98a5b 100644 --- a/front_end/panels/timeline/TimelineController.ts +++ b/front_end/panels/timeline/TimelineController.ts @@ -142,7 +142,11 @@ export class TimelineController implements SDK.TargetManager.SDKModelObserver { this.client.processingStarted(); - window.setTimeout(() => this.finalizeTrace(), 0); + await this.finalizeTrace(); } private async finalizeTrace(): Promise { this.injectCpuProfileEvents(); await SDK.TargetManager.TargetManager.instance().resumeAllTargets(); this.tracingModel.tracingComplete(); - this.client.loadingComplete(this.tracingModel); + await this.client.loadingComplete(this.tracingModel); + this.client.loadingCompleteForTest(); } private injectCpuProfileEvent(pid: number, tid: number, cpuProfile: Protocol.Profiler.Profile|null): void { @@ -381,6 +386,7 @@ export interface Client { processingStarted(): void; loadingProgress(progress?: number): void; loadingComplete(tracingModel: SDK.TracingModel.TracingModel|null): void; + loadingCompleteForTest(): void; } export interface RecordingOptions { enableJSSampling?: boolean; diff --git a/front_end/panels/timeline/TimelineLoader.ts b/front_end/panels/timeline/TimelineLoader.ts index 6bb1fe74b4..ef861cbe28 100644 --- a/front_end/panels/timeline/TimelineLoader.ts +++ b/front_end/panels/timeline/TimelineLoader.ts @@ -63,18 +63,17 @@ export class TimelineLoader implements Common.StringOutputStream.OutputStream { this.jsonTokenizer = new TextUtils.TextUtils.BalancedJSONTokenizer(this.writeBalancedJSON.bind(this), true); } - static loadFromFile(file: File, client: Client): TimelineLoader { + static async loadFromFile(file: File, client: Client): Promise { const loader = new TimelineLoader(client); const fileReader = new Bindings.FileUtils.ChunkedFileReader(file, TransferChunkLengthBytes); loader.canceledCallback = fileReader.cancel.bind(fileReader); loader.totalSize = file.size; - void fileReader.read(loader).then(success => { - if (!success && fileReader.error()) { - // TODO(crbug.com/1172300) Ignored during the jsdoc to ts migration - // eslint-disable-next-line @typescript-eslint/no-explicit-any - loader.reportErrorAndCancelLoading((fileReader.error() as any).message); - } - }); + const success = await fileReader.read(loader); + if (!success && fileReader.error()) { + // TODO(crbug.com/1172300) Ignored during the jsdoc to ts migration + // eslint-disable-next-line @typescript-eslint/no-explicit-any + loader.reportErrorAndCancelLoading((fileReader.error() as any).message); + } return loader; } @@ -247,16 +246,16 @@ export class TimelineLoader implements Common.StringOutputStream.OutputStream { return; } this.client.processingStarted(); - window.setTimeout(() => this.finalizeTrace(), 0); + await this.finalizeTrace(); } - private finalizeTrace(): void { + private async finalizeTrace(): Promise { if (this.state === State.LoadingCPUProfileFormat) { this.parseCPUProfileFormat(this.buffer); this.buffer = ''; } (this.tracingModel as SDK.TracingModel.TracingModel).tracingComplete(); - (this.client as Client).loadingComplete(this.tracingModel); + await (this.client as Client).loadingComplete(this.tracingModel); } private parseCPUProfileFormat(text: string): void { diff --git a/front_end/panels/timeline/TimelinePanel.ts b/front_end/panels/timeline/TimelinePanel.ts index d9810746e1..6a8e4f7b28 100644 --- a/front_end/panels/timeline/TimelinePanel.ts +++ b/front_end/panels/timeline/TimelinePanel.ts @@ -719,12 +719,12 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod } } - private loadFromFile(file: File): void { + private async loadFromFile(file: File): Promise { if (this.state !== State.Idle) { return; } this.prepareToLoadTimeline(); - this.loader = TimelineLoader.loadFromFile(file, this); + this.loader = await TimelineLoader.loadFromFile(file, this); this.createFileSelector(); } @@ -889,8 +889,8 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod .then(widget => widget.stopRecording()); } if (this.controller) { - const model = await this.controller.stopRecording(); - this.performanceModel = model; + this.performanceModel = this.controller.getPerformanceModel(); + await this.controller.stopRecording(); this.setUIControlsEnabled(true); this.controller.dispose(); this.controller = null; @@ -963,13 +963,13 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod this.saveButton.setEnabled(this.state === state.Idle && Boolean(this.performanceModel)); } - toggleRecording(): void { + async toggleRecording(): Promise { if (this.state === State.Idle) { this.recordingPageReload = false; - void this.startRecording(); + await this.startRecording(); Host.userMetrics.actionTaken(Host.UserMetrics.Action.TimelineStarted); } else if (this.state === State.Recording) { - void this.stopRecording(); + await this.stopRecording(); } } @@ -1148,7 +1148,7 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod } } - loadingComplete(tracingModel: SDK.TracingModel.TracingModel|null): void { + async loadingComplete(tracingModel: SDK.TracingModel.TracingModel|null): Promise { delete this.loader; this.setState(State.Idle); @@ -1165,7 +1165,8 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod if (!this.performanceModel) { this.performanceModel = new PerformanceModel(); } - this.performanceModel.setTracingModel(tracingModel); + + await this.performanceModel.setTracingModel(tracingModel); this.setModel(this.performanceModel); this.historyManager.addRecording(this.performanceModel); @@ -1178,6 +1179,10 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod } } + loadingCompleteForTest(): void { + // Not implemented, added only for allowing the TimelineTestRunner + // to be in sync when a trace load is finished. + } private showRecordingStarted(): void { if (this.statusPane) { return; @@ -1330,7 +1335,7 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod if (!file) { return; } - this.loadFromFile(file); + void this.loadFromFile(file); } } } @@ -1588,7 +1593,7 @@ export class ActionDelegate implements UI.ActionRegistration.ActionDelegate { console.assert(panel && panel instanceof TimelinePanel); switch (actionId) { case 'timeline.toggle-recording': - panel.toggleRecording(); + void panel.toggleRecording(); return true; case 'timeline.record-reload': panel.recordReload(); diff --git a/test/webtests/http/tests/devtools/a11y-axe-core/performance/performance-pane-a11y-test.js b/test/webtests/http/tests/devtools/a11y-axe-core/performance/performance-pane-a11y-test.js index ad93799e9d..9c4ebd8580 100644 --- a/test/webtests/http/tests/devtools/a11y-axe-core/performance/performance-pane-a11y-test.js +++ b/test/webtests/http/tests/devtools/a11y-axe-core/performance/performance-pane-a11y-test.js @@ -37,8 +37,7 @@ ]; // create dummy data for test - const model = - PerformanceTestRunner.createPerformanceModelWithEvents(testData); + const model = await PerformanceTestRunner.createPerformanceModelWithEvents(testData); const detailsView = UI.panels.timeline.flameChart.detailsView; diff --git a/test/webtests/http/tests/devtools/a11y-axe-core/performance/performance_event_log_a11y_test.js b/test/webtests/http/tests/devtools/a11y-axe-core/performance/performance_event_log_a11y_test.js index b5a1c6aa79..deb0db3d52 100644 --- a/test/webtests/http/tests/devtools/a11y-axe-core/performance/performance_event_log_a11y_test.js +++ b/test/webtests/http/tests/devtools/a11y-axe-core/performance/performance_event_log_a11y_test.js @@ -14,7 +14,7 @@ TestRunner.addResult('Loading a performance model.'); const view = tabbedPane.visibleView; - const model = PerformanceTestRunner.createPerformanceModelWithEvents([{}]); + const model = await PerformanceTestRunner.createPerformanceModelWithEvents([{}]); view.setModel(model, PerformanceTestRunner.mainTrack()); view.updateContents(Timeline.TimelineSelection.fromRange( model.timelineModel().minimumRecordTime(), model.timelineModel().maximumRecordTime()));