RPP: refactor initiator code

When reviewing a bug fix in this code this week I found that the code
was quite hard to follow and with no tests I got confused a few times.
This CL is my attempt to refactor the code slightly. I haven't done
anything too wild but instead have just renamed some things and changed
how we cache + track the last selected index vs the current one.

Bug: none
Change-Id: I119cf6a50839e19c6004e34b55984789358662d9
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6432453
Reviewed-by: Andres Olivares <[email protected]>
Commit-Queue: Jack Franklin <[email protected]>

Author: jackfranklin

front_end/panels/timeline/TimelineFlameChartDataProvider.test.ts

@@ -63,6 +63,46 @@ describeWithEnvironment('TimelineFlameChartDataProvider', function() {
     assert.lengthOf(timelineData3.initiatorsData, 0);
   });
 
+  it('caches initiator arrows for the same event', async function() {
+    const dataProvider = new Timeline.TimelineFlameChartDataProvider.TimelineFlameChartDataProvider();
+    const {parsedTrace} = await TraceLoader.traceEngine(this, 'scheduler-post-task.json.gz');
+    const entityMapper = new Timeline.Utils.EntityMapper.EntityMapper(parsedTrace);
+    dataProvider.setModel(parsedTrace, entityMapper);
+    dataProvider.timelineData();
+    // a postTask scheduled event - picked as it has an initiator
+    const event = parsedTrace.Renderer.allTraceEntries.find(event => {
+      return event.name === Trace.Types.Events.Name.RUN_POST_TASK_CALLBACK && event.ts === 512724961655;
+    });
+    assert.exists(event);
+    const index = dataProvider.indexForEvent(event);
+    assert.isNotNull(index);
+    dataProvider.buildFlowForInitiator(index);
+    const initiatorDataBefore = dataProvider.timelineData().initiatorsData;
+    dataProvider.buildFlowForInitiator(-1);
+    dataProvider.buildFlowForInitiator(index);
+    const initiatorDataAfter = dataProvider.timelineData().initiatorsData;
+    assert.strictEqual(initiatorDataBefore, initiatorDataAfter);
+  });
+
+  it('does not trigger a redraw if there are no initiators for the old and new selection', async function() {
+    const dataProvider = new Timeline.TimelineFlameChartDataProvider.TimelineFlameChartDataProvider();
+    const {parsedTrace} = await TraceLoader.traceEngine(this, 'scheduler-post-task.json.gz');
+    const entityMapper = new Timeline.Utils.EntityMapper.EntityMapper(parsedTrace);
+    dataProvider.setModel(parsedTrace, entityMapper);
+    dataProvider.timelineData();
+    // a RunTask event with no initiators
+    const event = parsedTrace.Renderer.allTraceEntries.find(event => {
+      return event.name === Trace.Types.Events.Name.RUN_TASK && event.ts === 512724754996;
+    });
+    assert.exists(event);
+    const index = dataProvider.indexForEvent(event);
+    assert.isNotNull(index);
+    const shouldRedraw = dataProvider.buildFlowForInitiator(index);
+    assert.isFalse(shouldRedraw);  // this event has no initiators
+    const shouldRedrawAgain = dataProvider.buildFlowForInitiator(-1);
+    assert.isFalse(shouldRedrawAgain);  // previous event has no initiators & user has selected no event
+  });
+
   describe('groupTreeEvents', function() {
     it('returns the correct events for tree views given a flame chart group', async function() {
       const dataProvider = new Timeline.TimelineFlameChartDataProvider.TimelineFlameChartDataProvider();

front_end/panels/timeline/TimelineFlameChartDataProvider.ts

@@ -132,14 +132,20 @@ export class TimelineFlameChartDataProvider extends Common.ObjectWrapper.ObjectW
 
   private entryTypeByLevel: EntryType[] = [];
   private entryIndexToTitle: string[] = [];
-  private lastInitiatorEntry = -1;
-  private lastInitiatorsData: PerfUI.FlameChart.FlameChartInitiatorData[] = [];
+  #lastInitiatorEntryIndex = -1;
 
   private lastSelection: Selection|null = null;
   readonly #font = `${PerfUI.Font.DEFAULT_FONT_SIZE} ${PerfUI.Font.getFontFamilyForCanvas()}`;
   #eventIndexByEvent = new WeakMap<Trace.Types.Events.Event, number|null>();
   #entityMapper: Utils.EntityMapper.EntityMapper|null = null;
 
+  /**
+   * When we create initiator chains for a selected event, we store those
+   * chains in this map so that if the user reselects the same event we do not
+   * have to recalculate. This is reset when the trace changes.
+   */
+  #initiatorsCache = new Map<number, PerfUI.FlameChart.FlameChartInitiatorData[]>();
+
   constructor() {
     super();
     this.reset();
@@ -543,6 +549,8 @@ export class TimelineFlameChartDataProvider extends Common.ObjectWrapper.ObjectW
     this.timelineDataInternal = null;
     this.parsedTrace = null;
     this.#entityMapper = null;
+    this.#lastInitiatorEntryIndex = -1;
+    this.#initiatorsCache.clear();
   }
 
   maxStackDepth(): number {
@@ -1243,45 +1251,52 @@ export class TimelineFlameChartDataProvider extends Common.ObjectWrapper.ObjectW
    * @returns if we should re-render the flame chart (canvas)
    */
   buildFlowForInitiator(entryIndex: number): boolean {
-    if (!this.parsedTrace) {
+    if (!this.parsedTrace || !this.compatibilityTracksAppender || !this.timelineDataInternal) {
       return false;
     }
-    if (!this.timelineDataInternal) {
+
+    if (this.#lastInitiatorEntryIndex === entryIndex) {
+      // If the user clicks on an entry twice by mistake, this can fire. But if
+      // the entry matches the selected entry, then there is nothing more for
+      // us to do.
       return false;
     }
-    // Remove all previously assigned decorations indicating that the flow event entries are hidden
+
+    this.#lastInitiatorEntryIndex = entryIndex;
+
     const previousInitiatorsDataLength = this.timelineDataInternal.initiatorsData.length;
-    // |entryIndex| equals -1 means there is no entry selected, just clear the
-    // initiator cache if there is any previous arrow and return true to
-    // re-render.
+
     if (entryIndex === -1) {
-      this.lastInitiatorEntry = entryIndex;
-      if (previousInitiatorsDataLength === 0) {
-        // This means there is no arrow before, so we don't need to re-render.
+      // User has deselected an event, so if it had any initiators we need to clear them.
+      if (this.timelineDataInternal.initiatorsData.length === 0) {
+        // The previous selected entry had no initiators, so we can early exit and not redraw anything.
         return false;
       }
-      // Reset to clear any previous arrows from the last event.
-      this.timelineDataInternal.resetFlowData();
+      // Clear initiator data and trigger a re-render.
+      this.timelineDataInternal.emptyInitiators();
       return true;
     }
-    if (this.lastInitiatorEntry === entryIndex) {
-      if (this.lastInitiatorsData) {
-        this.timelineDataInternal.initiatorsData = this.lastInitiatorsData;
-      }
-      return false;
-    }
-    if (!this.compatibilityTracksAppender) {
-      return false;
-    }
 
+    // If the user hasn't clicked on an event, bail, as there are no initiators
+    // for screenshots or frames.
     const entryType = this.#entryTypeForIndex(entryIndex);
     if (entryType !== EntryType.TRACK_APPENDER) {
       return false;
     }
+
+    // Avoid re-building the initiators if we already did it previously.
+    const cached = this.#initiatorsCache.get(entryIndex);
+    if (cached) {
+      this.timelineDataInternal.initiatorsData = cached;
+      return true;
+    }
+
+    // At this point, we know we:
+    // 1. Have an event to build initiators for.
+    // 2. Know that it's not an event with initiators that are cached.
     const event = this.entryData[entryIndex];
     // Reset to clear any previous arrows from the last event.
-    this.timelineDataInternal.resetFlowData();
-    this.lastInitiatorEntry = entryIndex;
+    this.timelineDataInternal.emptyInitiators();
 
     const hiddenEvents: Trace.Types.Events.Event[] =
         ModificationsManager.activeManager()?.getEntriesFilter().invisibleEntries() ?? [];
@@ -1294,10 +1309,19 @@ export class TimelineFlameChartDataProvider extends Common.ObjectWrapper.ObjectW
         hiddenEvents,
         expandableEntries,
     );
-    // This means there is no change for arrows.
+
+    if (initiatorsData.length === 0) {
+      // Small optimization: cache if this entry has 0 initiators, meaning if
+      // it gets reselected we don't redo the work to find out it has 0
+      // initiators.
+      this.#initiatorsCache.set(entryIndex, []);
+    }
+
+    // Previous event had 0 initiators, new event has 0, therefore exit early and don't render.
     if (previousInitiatorsDataLength === 0 && initiatorsData.length === 0) {
       return false;
     }
+
     for (const initiatorData of initiatorsData) {
       const eventIndex = this.indexForEvent(initiatorData.event);
       const initiatorIndex = this.indexForEvent(initiatorData.initiator);
@@ -1311,7 +1335,7 @@ export class TimelineFlameChartDataProvider extends Common.ObjectWrapper.ObjectW
         isEntryHidden: initiatorData.isEntryHidden,
       });
     }
-    this.lastInitiatorsData = this.timelineDataInternal.initiatorsData;
+    this.#initiatorsCache.set(entryIndex, this.timelineDataInternal.initiatorsData);
     return true;
   }
 

front_end/panels/timeline/TimelineFlameChartNetworkDataProvider.ts

@@ -554,13 +554,13 @@ export class TimelineFlameChartNetworkDataProvider implements PerfUI.FlameChart.
         return false;
       }
       // Reset to clear any previous arrows from the last event.
-      this.#timelineDataInternal.resetFlowData();
+      this.#timelineDataInternal.emptyInitiators();
       return true;
     }
 
     const event = this.#events[entryIndex];
     // Reset to clear any previous arrows from the last event.
-    this.#timelineDataInternal.resetFlowData();
+    this.#timelineDataInternal.emptyInitiators();
     this.#lastInitiatorEntry = entryIndex;
 
     const initiatorsData = initiatorsDataToDrawForNetwork(this.#parsedTrace, event);

front_end/ui/legacy/components/perf_ui/FlameChart.ts

@@ -1130,7 +1130,7 @@ export class FlameChart extends Common.ObjectWrapper.eventMixin<EventTypes, type
 
   private deselectAllEntries(): void {
     this.selectedEntryIndex = -1;
-    this.rawTimelineData?.resetFlowData();
+    this.rawTimelineData?.emptyInitiators();
     this.draw();
   }
 
@@ -1218,7 +1218,7 @@ export class FlameChart extends Common.ObjectWrapper.eventMixin<EventTypes, type
             (groupIndex >= groups.length - 1 || groups[groupIndex + 1].startLevel > level)) {
           this.selectedEntryIndex = -1;
           // Reset all flow arrows when we deselect the entry.
-          this.rawTimelineData.resetFlowData();
+          this.rawTimelineData.emptyInitiators();
         }
       }
     }
@@ -4150,7 +4150,7 @@ export class FlameChartTimelineData {
     );
   }
 
-  resetFlowData(): void {
+  emptyInitiators(): void {
     this.initiatorsData = [];
   }
 }