From b87d4fef3e794bb05faee7bb9f5c8bc309826d3e Mon Sep 17 00:00:00 2001 From: Nancy Li Date: Tue, 9 May 2023 10:47:06 +0000 Subject: [PATCH] [RPP] Deduplicate |appendEventAtLevel| function of track appenders Move appendEventAtLevel to CompatibilityBuilder to reduce the duplicate codes. Bug: 1427378 Change-Id: I3fd4c2b513e0022e7122c3420a788a79c62554c5 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/4512963 Reviewed-by: Andres Olivares Commit-Queue: Nancy Li --- .../timeline/CompatibilityTracksAppender.ts | 48 +++++++++++++++---- front_end/panels/timeline/GPUTrackAppender.ts | 26 ++-------- .../timeline/InteractionsTrackAppender.ts | 23 ++------- .../timeline/LayoutShiftsTrackAppender.ts | 15 ++---- .../panels/timeline/TimingsTrackAppender.ts | 22 ++------- .../timeline/layout_shifts_track_test.ts | 2 +- 6 files changed, 54 insertions(+), 82 deletions(-) diff --git a/front_end/panels/timeline/CompatibilityTracksAppender.ts b/front_end/panels/timeline/CompatibilityTracksAppender.ts index f0478c524b..8ebd0a1b7b 100644 --- a/front_end/panels/timeline/CompatibilityTracksAppender.ts +++ b/front_end/panels/timeline/CompatibilityTracksAppender.ts @@ -6,7 +6,11 @@ import * as TraceEngine from '../../models/trace/trace.js'; import type * as PerfUI from '../../ui/legacy/components/perf_ui/perf_ui.js'; import * as SDK from '../../core/sdk/sdk.js'; import type * as TimelineModel from '../../models/timeline_model/timeline_model.js'; -import {type TimelineFlameChartEntry, type EntryType} from './TimelineFlameChartDataProvider.js'; +import { + type TimelineFlameChartEntry, + EntryType, + InstantEventVisibleDurationMs, +} from './TimelineFlameChartDataProvider.js'; import {TimingsTrackAppender} from './TimingsTrackAppender.js'; import {InteractionsTrackAppender} from './InteractionsTrackAppender.js'; import {GPUTrackAppender} from './GPUTrackAppender.js'; @@ -33,7 +37,7 @@ export type HighlightedEntryInfo = { * removed. This processes of doing this for a track is referred to as * "migrating the track" to the new system. * - * The migration implementation will result benefitial among other + * The migration implementation will result beneficial among other * things because the complexity of rendering the details of each track * is distributed among multiple standalone modules. * Read more at go/rpp-flamechart-arch @@ -118,22 +122,21 @@ export class CompatibilityTracksAppender { this.#legacyEntryTypeByLevel = legacyEntryTypeByLevel; this.#legacyTimelineModel = legacyTimelineModel; - this.#timingsTrackAppender = new TimingsTrackAppender( - this, this.#flameChartData, this.#traceParsedData, this.#entryData, this.#legacyEntryTypeByLevel); + this.#timingsTrackAppender = + new TimingsTrackAppender(this, this.#flameChartData, this.#traceParsedData, this.#legacyEntryTypeByLevel); this.#allTrackAppenders.push(this.#timingsTrackAppender); - this.#interactionsTrackAppender = new InteractionsTrackAppender( - this, this.#flameChartData, this.#traceParsedData, this.#entryData, this.#legacyEntryTypeByLevel); + this.#interactionsTrackAppender = + new InteractionsTrackAppender(this, this.#flameChartData, this.#traceParsedData, this.#legacyEntryTypeByLevel); this.#allTrackAppenders.push(this.#interactionsTrackAppender); - this.#gpuTrackAppender = new GPUTrackAppender( - this, this.#flameChartData, this.#traceParsedData, this.#entryData, this.#legacyEntryTypeByLevel); + this.#gpuTrackAppender = new GPUTrackAppender(this, this.#traceParsedData, this.#legacyEntryTypeByLevel); this.#allTrackAppenders.push(this.#gpuTrackAppender); // Layout Shifts track in OPP was called the "Experience" track even though // all it shows are layout shifts. - this.#layoutShiftsTrackAppender = new LayoutShiftsTrackAppender( - this, this.#flameChartData, this.#traceParsedData, this.#entryData, this.#legacyEntryTypeByLevel); + this.#layoutShiftsTrackAppender = + new LayoutShiftsTrackAppender(this, this.#flameChartData, this.#traceParsedData, this.#legacyEntryTypeByLevel); this.#allTrackAppenders.push(this.#layoutShiftsTrackAppender); } @@ -306,9 +309,34 @@ export class CompatibilityTracksAppender { * style, title, etc.) is needed. */ registerTrackForLevel(level: number, appender: TrackAppender): void { + // TODO(crbug.com/1442454) Figure out how to avoid the circular calls. this.#trackForLevel.set(level, appender); } + /** + * Adds an event to the flame chart data at a defined level. + * @param event the event to be appended, + * @param level the level to append the event, + * @param appender the track which the event belongs to. + * @returns the index of the event in all events to be rendered in the flamechart. + */ + appendEventAtLevel(event: TraceEngine.Types.TraceEvents.TraceEventData, level: number, appender: TrackAppender): + number { + // TODO(crbug.com/1442454) Figure out how to avoid the circular calls. + this.#trackForLevel.set(level, appender); + + const index = this.#entryData.length; + this.#entryData.push(event); + this.#legacyEntryTypeByLevel[level] = EntryType.TrackAppender; + this.#flameChartData.entryLevels[index] = level; + this.#flameChartData.entryStartTimes[index] = TraceEngine.Helpers.Timing.microSecondsToMilliseconds(event.ts); + const msDuration = event.dur || + TraceEngine.Helpers.Timing.millisecondsToMicroseconds( + InstantEventVisibleDurationMs as TraceEngine.Types.Timing.MilliSeconds); + this.#flameChartData.entryTotalTimes[index] = TraceEngine.Helpers.Timing.microSecondsToMilliseconds(msDuration); + return index; + } + /** * Gets the all track appenders that have been set to be visible. */ diff --git a/front_end/panels/timeline/GPUTrackAppender.ts b/front_end/panels/timeline/GPUTrackAppender.ts index 18e28f2ea2..3a730bd2f6 100644 --- a/front_end/panels/timeline/GPUTrackAppender.ts +++ b/front_end/panels/timeline/GPUTrackAppender.ts @@ -1,9 +1,8 @@ // Copyright 2023 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import * as TraceEngine from '../../models/trace/trace.js'; -import type * as PerfUI from '../../ui/legacy/components/perf_ui/perf_ui.js'; import * as i18n from '../../core/i18n/i18n.js'; +import * as TraceEngine from '../../models/trace/trace.js'; import { type TrackAppender, @@ -13,8 +12,6 @@ import { } from './CompatibilityTracksAppender.js'; import { EntryType, - InstantEventVisibleDurationMs, - type TimelineFlameChartEntry, } from './TimelineFlameChartDataProvider.js'; import {buildGroupStyle, buildTrackHeader, getFormattedTime, getSyncEventLevel} from './AppenderUtils.js'; @@ -32,9 +29,7 @@ export class GPUTrackAppender implements TrackAppender { readonly appenderName: TrackAppenderName = 'GPU'; #compatibilityBuilder: CompatibilityTracksAppender; - #flameChartData: PerfUI.FlameChart.FlameChartTimelineData; #traceParsedData: Readonly; - #entryData: TimelineFlameChartEntry[]; // TODO(crbug.com/1416533) // This is used only for compatibility with the legacy flame chart // architecture of the panel. Once all tracks have been migrated to @@ -43,13 +38,11 @@ export class GPUTrackAppender implements TrackAppender { #legacyEntryTypeByLevel: EntryType[]; constructor( - compatibilityBuilder: CompatibilityTracksAppender, flameChartData: PerfUI.FlameChart.FlameChartTimelineData, + compatibilityBuilder: CompatibilityTracksAppender, traceParsedData: TraceEngine.TraceModel.PartialTraceParseDataDuringMigration, - entryData: TimelineFlameChartEntry[], legacyEntryTypeByLevel: EntryType[]) { + legacyEntryTypeByLevel: EntryType[]) { this.#compatibilityBuilder = compatibilityBuilder; - this.#flameChartData = flameChartData; this.#traceParsedData = traceParsedData; - this.#entryData = entryData; this.#legacyEntryTypeByLevel = legacyEntryTypeByLevel; } @@ -108,6 +101,7 @@ export class GPUTrackAppender implements TrackAppender { this.#legacyEntryTypeByLevel.length = trackStartLevel + maxStackDepth; this.#legacyEntryTypeByLevel.fill(EntryType.TrackAppender, trackStartLevel); + return trackStartLevel + maxStackDepth; } @@ -117,17 +111,7 @@ export class GPUTrackAppender implements TrackAppender { * array, which contains all the events in the timeline. */ #appendEventAtLevel(event: TraceEngine.Types.TraceEvents.TraceEventData, level: number): number { - this.#compatibilityBuilder.registerTrackForLevel(level, this); - const index = this.#entryData.length; - this.#entryData.push(event); - this.#legacyEntryTypeByLevel[level] = EntryType.TrackAppender; - this.#flameChartData.entryLevels[index] = level; - this.#flameChartData.entryStartTimes[index] = TraceEngine.Helpers.Timing.microSecondsToMilliseconds(event.ts); - const msDuration = event.dur || - TraceEngine.Helpers.Timing.millisecondsToMicroseconds( - InstantEventVisibleDurationMs as TraceEngine.Types.Timing.MilliSeconds); - this.#flameChartData.entryTotalTimes[index] = TraceEngine.Helpers.Timing.microSecondsToMilliseconds(msDuration); - return index; + return this.#compatibilityBuilder.appendEventAtLevel(event, level, this); } /* diff --git a/front_end/panels/timeline/InteractionsTrackAppender.ts b/front_end/panels/timeline/InteractionsTrackAppender.ts index cebf1d79c6..e94ccdc466 100644 --- a/front_end/panels/timeline/InteractionsTrackAppender.ts +++ b/front_end/panels/timeline/InteractionsTrackAppender.ts @@ -6,7 +6,6 @@ import type * as PerfUI from '../../ui/legacy/components/perf_ui/perf_ui.js'; import { EntryType, - type TimelineFlameChartEntry, } from './TimelineFlameChartDataProvider.js'; import { type CompatibilityTracksAppender, @@ -38,7 +37,6 @@ export class InteractionsTrackAppender implements TrackAppender { #compatibilityBuilder: CompatibilityTracksAppender; #flameChartData: PerfUI.FlameChart.FlameChartTimelineData; #traceParsedData: Readonly; - #entryData: TimelineFlameChartEntry[]; // TODO(crbug.com/1416533) // This is used only for compatibility with the legacy flame chart // architecture of the panel. Once all tracks have been migrated to @@ -49,7 +47,7 @@ export class InteractionsTrackAppender implements TrackAppender { constructor( compatibilityBuilder: CompatibilityTracksAppender, flameChartData: PerfUI.FlameChart.FlameChartTimelineData, traceParsedData: TraceEngine.TraceModel.PartialTraceParseDataDuringMigration, - entryData: TimelineFlameChartEntry[], legacyEntryTypeByLevel: EntryType[]) { + legacyEntryTypeByLevel: EntryType[]) { this.#compatibilityBuilder = compatibilityBuilder; this.#colorGenerator = new Common.Color.Generator( { @@ -60,7 +58,6 @@ export class InteractionsTrackAppender implements TrackAppender { {min: 70, max: 100, count: 6}, 50, 0.7); this.#flameChartData = flameChartData; this.#traceParsedData = traceParsedData; - this.#entryData = entryData; this.#legacyEntryTypeByLevel = legacyEntryTypeByLevel; } @@ -127,26 +124,12 @@ export class InteractionsTrackAppender implements TrackAppender { * @returns the position occupied by the new event in the entryData * array, which contains all the events in the timeline. */ - #appendEventAtLevel(syntheticEvent: TraceEngine.Types.TraceEvents.SyntheticInteractionEvent, level: number): number { - this.#compatibilityBuilder.registerTrackForLevel(level, this); - const index = this.#entryData.length; - - // Although the event is a SyntheticInteractionEvent, it extends - // TraceEventData, so we can safely push it onto entryData. - this.#entryData.push(syntheticEvent); - this.#legacyEntryTypeByLevel[level] = EntryType.TrackAppender; - this.#flameChartData.entryLevels[index] = level; - this.#flameChartData.entryStartTimes[index] = - TraceEngine.Helpers.Timing.microSecondsToMilliseconds(syntheticEvent.ts); + #appendEventAtLevel(syntheticEvent: TraceEngine.Types.TraceEvents.SyntheticInteractionEvent, level: number): void { + const index = this.#compatibilityBuilder.appendEventAtLevel(syntheticEvent, level, this); const eventDurationMicroSeconds = syntheticEvent.dur || TraceEngine.Types.Timing.MicroSeconds(0); - if (eventDurationMicroSeconds > LONG_INTERACTION_THRESHOLD) { this.#addCandyStripingForLongInteraction(index); } - - this.#flameChartData.entryTotalTimes[index] = - TraceEngine.Helpers.Timing.microSecondsToMilliseconds(eventDurationMicroSeconds); - return index; } #addCandyStripingForLongInteraction(eventIndex: number): void { diff --git a/front_end/panels/timeline/LayoutShiftsTrackAppender.ts b/front_end/panels/timeline/LayoutShiftsTrackAppender.ts index 024cf98e09..0369e41cd8 100644 --- a/front_end/panels/timeline/LayoutShiftsTrackAppender.ts +++ b/front_end/panels/timeline/LayoutShiftsTrackAppender.ts @@ -6,7 +6,6 @@ import type * as PerfUI from '../../ui/legacy/components/perf_ui/perf_ui.js'; import { EntryType, - type TimelineFlameChartEntry, } from './TimelineFlameChartDataProvider.js'; import { type CompatibilityTracksAppender, @@ -33,7 +32,6 @@ export class LayoutShiftsTrackAppender implements TrackAppender { #compatibilityBuilder: CompatibilityTracksAppender; #flameChartData: PerfUI.FlameChart.FlameChartTimelineData; #traceParsedData: Readonly; - #entryData: TimelineFlameChartEntry[]; // TODO(crbug.com/1416533) // This is used only for compatibility with the legacy flame chart // architecture of the panel. Once all tracks have been migrated to @@ -44,11 +42,10 @@ export class LayoutShiftsTrackAppender implements TrackAppender { constructor( compatibilityBuilder: CompatibilityTracksAppender, flameChartData: PerfUI.FlameChart.FlameChartTimelineData, traceParsedData: TraceEngine.TraceModel.PartialTraceParseDataDuringMigration, - entryData: TimelineFlameChartEntry[], legacyEntryTypeByLevel: EntryType[]) { + legacyEntryTypeByLevel: EntryType[]) { this.#compatibilityBuilder = compatibilityBuilder; this.#flameChartData = flameChartData; this.#traceParsedData = traceParsedData; - this.#entryData = entryData; this.#legacyEntryTypeByLevel = legacyEntryTypeByLevel; } @@ -121,13 +118,8 @@ export class LayoutShiftsTrackAppender implements TrackAppender { * @returns the position occupied by the new event in the entryData * array, which contains all the events in the timeline. */ - #appendEventAtLevel(event: TraceEngine.Types.TraceEvents.TraceEventData, level: number): number { - this.#compatibilityBuilder.registerTrackForLevel(level, this); - const index = this.#entryData.length; - this.#entryData.push(event); - this.#legacyEntryTypeByLevel[level] = EntryType.TrackAppender; - this.#flameChartData.entryLevels[index] = level; - this.#flameChartData.entryStartTimes[index] = TraceEngine.Helpers.Timing.microSecondsToMilliseconds(event.ts); + #appendEventAtLevel(event: TraceEngine.Types.TraceEvents.TraceEventData, level: number): void { + const index = this.#compatibilityBuilder.appendEventAtLevel(event, level, this); // Bit of a hack: LayoutShifts are instant events, so have no duration. But // OPP doesn't do well at making tiny events easy to spot and click. So we // set it to a small duration so that the user is able to see and click @@ -135,7 +127,6 @@ export class LayoutShiftsTrackAppender implements TrackAppender { // allow us to do this properly and not hack around it. const msDuration = TraceEngine.Types.Timing.MicroSeconds(5_000); this.#flameChartData.entryTotalTimes[index] = TraceEngine.Helpers.Timing.microSecondsToMilliseconds(msDuration); - return index; } /* diff --git a/front_end/panels/timeline/TimingsTrackAppender.ts b/front_end/panels/timeline/TimingsTrackAppender.ts index 6d9e3446fc..6b43860391 100644 --- a/front_end/panels/timeline/TimingsTrackAppender.ts +++ b/front_end/panels/timeline/TimingsTrackAppender.ts @@ -6,8 +6,6 @@ import type * as PerfUI from '../../ui/legacy/components/perf_ui/perf_ui.js'; import { EntryType, - InstantEventVisibleDurationMs, - type TimelineFlameChartEntry, } from './TimelineFlameChartDataProvider.js'; import { type CompatibilityTracksAppender, @@ -42,7 +40,6 @@ export class TimingsTrackAppender implements TrackAppender { #compatibilityBuilder: CompatibilityTracksAppender; #flameChartData: PerfUI.FlameChart.FlameChartTimelineData; #traceParsedData: Readonly; - #entryData: TimelineFlameChartEntry[]; // TODO(crbug.com/1416533) // This is used only for compatibility with the legacy flame chart // architecture of the panel. Once all tracks have been migrated to @@ -53,7 +50,7 @@ export class TimingsTrackAppender implements TrackAppender { constructor( compatibilityBuilder: CompatibilityTracksAppender, flameChartData: PerfUI.FlameChart.FlameChartTimelineData, traceParsedData: TraceEngine.TraceModel.PartialTraceParseDataDuringMigration, - entryData: TimelineFlameChartEntry[], legacyEntryTypeByLevel: EntryType[]) { + legacyEntryTypeByLevel: EntryType[]) { this.#compatibilityBuilder = compatibilityBuilder; this.#colorGenerator = new Common.Color.Generator( { @@ -64,7 +61,6 @@ export class TimingsTrackAppender implements TrackAppender { {min: 70, max: 100, count: 6}, 50, 0.7); this.#flameChartData = flameChartData; this.#traceParsedData = traceParsedData; - this.#entryData = entryData; this.#legacyEntryTypeByLevel = legacyEntryTypeByLevel; } @@ -163,8 +159,8 @@ export class TimingsTrackAppender implements TrackAppender { */ #appendConsoleTimings(currentLevel: number): number { let newLevel = currentLevel; - for (const timestamp of this.#traceParsedData.UserTimings.timestampEvents) { - this.#appendEventAtLevel(timestamp, newLevel); + for (const timestampEvent of this.#traceParsedData.UserTimings.timestampEvents) { + this.#appendEventAtLevel(timestampEvent, newLevel); } if (this.#traceParsedData.UserTimings.timestampEvents.length !== 0) { // Add console.time events on the next level, but only if the @@ -221,17 +217,7 @@ export class TimingsTrackAppender implements TrackAppender { * array, which contains all the events in the timeline. */ #appendEventAtLevel(event: TraceEngine.Types.TraceEvents.TraceEventData, level: number): number { - this.#compatibilityBuilder.registerTrackForLevel(level, this); - const index = this.#entryData.length; - this.#entryData.push(event); - this.#legacyEntryTypeByLevel[level] = EntryType.TrackAppender; - this.#flameChartData.entryLevels[index] = level; - this.#flameChartData.entryStartTimes[index] = TraceEngine.Helpers.Timing.microSecondsToMilliseconds(event.ts); - const msDuration = event.dur || - TraceEngine.Helpers.Timing.millisecondsToMicroseconds( - InstantEventVisibleDurationMs as TraceEngine.Types.Timing.MilliSeconds); - this.#flameChartData.entryTotalTimes[index] = TraceEngine.Helpers.Timing.microSecondsToMilliseconds(msDuration); - return index; + return this.#compatibilityBuilder.appendEventAtLevel(event, level, this); } /* diff --git a/test/interactions/panels/performance/timeline/layout_shifts_track_test.ts b/test/interactions/panels/performance/timeline/layout_shifts_track_test.ts index f82479f019..098a4f0082 100644 --- a/test/interactions/panels/performance/timeline/layout_shifts_track_test.ts +++ b/test/interactions/panels/performance/timeline/layout_shifts_track_test.ts @@ -13,6 +13,6 @@ describe('Layout shifts track', () => { itScreenshot('renders the layout shifts track correctly', async () => { await loadComponentDocExample('performance_panel/track_example.html?track=LayoutShifts&fileName=cls-single-frame'); const flameChart = await waitFor('.flame-chart-main-pane'); - await assertElementScreenshotUnchanged(flameChart, 'performance/layout_shifts_track.png', 3); + await assertElementScreenshotUnchanged(flameChart, 'performance/layout_shifts_track.png', 2); }); });