Revert "Perf: Clear graphs when count is too large"

This reverts commit 27b508c.

Reason for revert: breaks opening from shortcuts (b/467935356)

Bug: 467935356

Original change's description:
> Perf: Clear graphs when count is too large
>
> Adds a feature to clear all graphs when the trace count exceeds a maximum limit in the Test Picker.
> It also fixes a potential crash in explore-multi-sk when handling remove-explore events without details.
>
> Change-Id: I15477f7eb8030bc15000a07f436ee9cdd8d088c6
> Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/1056545
> Commit-Queue: Tony Seaward <[email protected]>
> Reviewed-by: Farid (Mojtaba) Faridzad <[email protected]>

Change-Id: I208e0d2c907bc6efe0dbd7cd324f15f26d3f1ea4
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/1123236
Auto-Submit: Anri Sidorov <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
Commit-Queue: Rubber Stamper <[email protected]>

Author: nevedaren

perf/modules/explore-multi-sk/BUILD.bazel

@@ -19,15 +19,15 @@ sk_element(
         "//elements-sk/modules:define_ts_lib",
         "//infra-sk/modules:hintable_ts_lib",
         "//infra-sk/modules:jsonorthrow_ts_lib",
-        "//infra-sk/modules:query_ts_lib",
         "//infra-sk/modules:statereflector_ts_lib",
         "//infra-sk/modules/ElementSk:index_ts_lib",
         "//infra-sk/modules/json:index_ts_lib",
         "//perf/modules/errorMessage:index_ts_lib",
         "//perf/modules/json:index_ts_lib",
         "//perf/modules/paramtools:index_ts_lib",
-        "//perf/modules/telemetry:telemetry_ts_lib",
         "//perf/modules/window:window_ts_lib",
+        "//infra-sk/modules:query_ts_lib",
+        "//perf/modules/telemetry:telemetry_ts_lib",
     ],
     ts_srcs = [
         "explore-multi-sk.ts",

perf/modules/explore-multi-sk/explore-multi-sk.ts

@@ -316,28 +316,20 @@ export class ExploreMultiSk extends ElementSk {
   private _onRemoveExplore = (e: Event) => {
     this._dataLoading = true;
     this.testPicker?.setReadOnly(true);
-    const detail = (e as CustomEvent).detail;
-    const exploreElemToRemove = detail ? (detail.elem as ExploreSimpleSk) : null;
-    if (exploreElemToRemove === null) {
-      // Remove all explore elements.
-      this.resetGraphs();
+    const exploreElemToRemove = (e as CustomEvent).detail.elem as ExploreSimpleSk;
+    if (this.exploreElements.length === 1) {
+      this.removeExplore(exploreElemToRemove);
+      e.stopPropagation();
     } else {
-      if (this.exploreElements.length === 1) {
-        this.removeExplore(exploreElemToRemove);
-        e.stopPropagation();
-      } else {
-        const param = this.state.splitByKeys[0];
-        if (exploreElemToRemove.state.queries.length > 0) {
-          const query = exploreElemToRemove.state.queries[0];
-          const valueToRemove = new URLSearchParams(query).get(param);
-          if (valueToRemove) {
-            this.testPicker?.removeItemFromChart(param, [valueToRemove]);
-          }
+      const param = this.state.splitByKeys[0];
+      if (exploreElemToRemove.state.queries.length > 0) {
+        const query = exploreElemToRemove.state.queries[0];
+        const valueToRemove = new URLSearchParams(query).get(param);
+        if (valueToRemove) {
+          this.testPicker?.removeItemFromChart(param, [valueToRemove]);
         }
       }
     }
-    this._dataLoading = false;
-    this.testPicker?.setReadOnly(false);
   };
 
   // Event listener for when the Test Picker plot button is clicked.
@@ -569,7 +561,7 @@ export class ExploreMultiSk extends ElementSk {
 
     // Remove the traces from the current page explore elements.
     const elemsToRemove: ExploreSimpleSk[] = [];
-    const updatePromises = this.exploreElements.map(async (elem) => {
+    const updatePromises = this.exploreElements.map((elem) => {
       const hasQueryToRemove =
         elem.state.queries && queriesToRemove.some((qr) => elem.state.queries.includes(qr));
 
@@ -597,7 +589,7 @@ export class ExploreMultiSk extends ElementSk {
         }
         if (elem.state.queries.length === 0) {
           elemsToRemove.push(elem);
-          return await Promise.resolve();
+          return Promise.resolve();
         }
         const params: ParamSet = ParamSet({});
         Object.keys(traceset).forEach((trace) => {
@@ -628,7 +620,7 @@ export class ExploreMultiSk extends ElementSk {
           msg: '',
           skps: [],
         };
-        return await elem.UpdateWithFrameResponse(
+        return elem.UpdateWithFrameResponse(
           updatedResponse,
           updatedRequest,
           /* switchToTab= */ true,
@@ -637,7 +629,7 @@ export class ExploreMultiSk extends ElementSk {
           /* replaceAnomalies= */ true
         );
       }
-      return await Promise.resolve();
+      return Promise.resolve();
     });
 
     await Promise.all(updatePromises);
@@ -1085,7 +1077,7 @@ export class ExploreMultiSk extends ElementSk {
       }
 
       const readOnly = this.exploreElements.length > 0;
-      await this.testPicker!.initializeTestPicker(testPickerParams!, defaultParams, readOnly);
+      this.testPicker!.initializeTestPicker(testPickerParams!, defaultParams, readOnly);
       this._render();
     }
 

perf/modules/explore-multi-sk/explore-multi-sk_test.ts

@@ -335,22 +335,6 @@ describe('ExploreMultiSk', () => {
       assert.equal(element['exploreElements'].length, initialGraphCount - 1);
     });
 
-    it('removes all graphs when a remove-explore event is dispatched with null detail', async () => {
-      await element['initializeTestPicker'](); // Initialize to attach the listener.
-      element['addEmptyGraph']();
-      const initialGraphCount = element['exploreElements'].length;
-      assert.isAbove(initialGraphCount, 0);
-
-      const event = new CustomEvent('remove-explore', {
-        bubbles: true,
-        composed: true,
-        // No detail provided, simulating the bug/feature
-      });
-      element.dispatchEvent(event);
-
-      assert.equal(element['exploreElements'].length, 0);
-    });
-
     it('resets pagination when the last graph is removed', async () => {
       await element['initializeTestPicker']();
       const graph1 = element['addEmptyGraph']()!;
@@ -806,7 +790,7 @@ describe('ExploreMultiSk', () => {
       loadExtendedSpy = sinon.stub(mainGraph, 'loadExtendedRangeData').resolves();
       sinon.stub(mainGraph, 'getSelectedRange').returns({ begin: 0, end: 1 });
       // Ensure that awaiting 'requestComplete' doesn't hang the test.
-      sinon.stub(mainGraph, 'requestComplete').get(async () => await Promise.resolve());
+      sinon.stub(mainGraph, 'requestComplete').get(() => Promise.resolve());
 
       // We stub 'addEmptyGraph' to ensure it returns our controlled instance
       // of ExploreSimpleSk, allowing us to spy on its methods.

perf/modules/test-picker-sk/test-picker-sk.ts

@@ -121,10 +121,7 @@ export class TestPickerSk extends ElementSk {
 
     this._containerDiv = this.querySelector('#fieldContainer');
     this._plotButton = this.querySelector('#plot-button');
-    // Using document.querySelector('#graphContainer') fails when ExploreMultiSk uses Shadow DOM.
-    // Instead, find the graph container in the same root (shadow or light) as this element.
-    const root = this.getRootNode() as ParentNode;
-    this._graphDiv = root.querySelector ? root.querySelector('#graphContainer') : null;
+    this._graphDiv = document.querySelector('#graphContainer');
 
     window.addEventListener('data-loaded', () => {
       this._dataLoading = false;
@@ -138,7 +135,7 @@ export class TestPickerSk extends ElementSk {
    * initializes and populates the field, focuses it, and sets up event
    * listeners.
    */
-  private async addChildField(readOnly: boolean): Promise<void> {
+  private addChildField(readOnly: boolean): Promise<void> {
     const currentIndex = this._currentIndex;
     const currentFieldInfo = this._fieldData[currentIndex];
     const param = currentFieldInfo.param;
@@ -193,7 +190,7 @@ export class TestPickerSk extends ElementSk {
       }
       this._render();
     };
-    return await this.callNextParamList(handler, currentIndex);
+    return this.callNextParamList(handler, currentIndex);
   }
 
   /**
@@ -279,7 +276,7 @@ export class TestPickerSk extends ElementSk {
    * @param handler - A callback function to handle the JSON response.
    * @param index - The index of the current field.
    */
-  private async callNextParamList(
+  private callNextParamList(
     handler: (json: NextParamListHandlerResponse) => void,
     index: number
   ): Promise<void> {
@@ -296,7 +293,7 @@ export class TestPickerSk extends ElementSk {
       q: fieldData,
     };
 
-    return await fetch('/_/nextParamList/', {
+    return fetch('/_/nextParamList/', {
       method: 'POST',
       body: JSON.stringify(body),
       headers: {
@@ -693,7 +690,7 @@ export class TestPickerSk extends ElementSk {
    *
    * @param index
    */
-  private async fetchExtraOptions(index: number): Promise<void> {
+  private fetchExtraOptions(index: number): Promise<void> {
     const handler = async (json: NextParamListHandlerResponse) => {
       const param = Object.keys(json.paramset)[0];
       const count: number = json.count || -1;
@@ -756,7 +753,7 @@ export class TestPickerSk extends ElementSk {
       }
       this._render();
     };
-    return await this.callNextParamList(handler, index);
+    return this.callNextParamList(handler, index);
   }
 
   createParamSetFromFieldData(): ParamSet {
@@ -863,33 +860,19 @@ export class TestPickerSk extends ElementSk {
     }
 
     this.setReadOnly(false);
-
-    // 1. Check if we need to clear existing graphs due to high count.
-    if (this._graphDiv && this._graphDiv.children.length > 0 && count > PLOT_MAXIMUM) {
-      this.dispatchEvent(
-        new CustomEvent('remove-explore', {
-          bubbles: true,
-          composed: true,
-        })
-      );
-    }
-
     this._count = count;
-
-    // 2. Handle invalid count (too many or none).
     if (count > PLOT_MAXIMUM || count <= 0) {
+      // Disable plotting if there are too many or no traces.
       this.autoAddTrace = false;
-      this._plotButton!.title = count > PLOT_MAXIMUM ? 'Too many traces.' : 'No traces.';
+      this._plotButton!.title = this._count > PLOT_MAXIMUM ? 'Too many traces.' : 'No traces.';
       this._plotButton!.disabled = true;
       return;
     }
-
-    // 3. Handle valid count.
     if (this._graphDiv && this._graphDiv.children.length > 0) {
-      // Graph loaded, auto-add changes.
+      // Graph is already loaded, so allow new changes automatically.
       this.autoAddTrace = true;
     } else {
-      // No graph, ready to plot.
+      // No graph loaded yet, so allow plotting.
       this._plotButton!.title = 'Plot graph';
       this.autoAddTrace = false;
       this._plotButton!.disabled = false;
@@ -958,15 +941,15 @@ export class TestPickerSk extends ElementSk {
    * queries will automatically get "bot=linux-perf" appended. The exception
    * is if bot is already specified in the query, then no defaults are applied.
    */
-  async initializeTestPicker(
+  initializeTestPicker(
     params: string[],
     defaultParams: { [key: string]: string[] | null },
     readOnly: boolean
   ): Promise<void> {
     this._defaultParams = defaultParams;
     this.initializeFieldData(params);
     this._render();
-    return await this.addChildField(readOnly);
+    return this.addChildField(readOnly);
   }
 
   /**

perf/modules/test-picker-sk/test-picker-sk_test.ts

@@ -14,7 +14,7 @@ describe('test-picker-sk', () => {
 
   beforeEach(async () => {
     // Mock the fetch function.
-    fetchMock = async (_url: RequestInfo | URL, request: RequestInit | undefined) => {
+    fetchMock = (_url: RequestInfo | URL, request: RequestInit | undefined) => {
       const req = JSON.parse(request!.body as string) as NextParamListHandlerRequest;
       const params = toParamSet(req.q!);
       const paramset: any = {};
@@ -27,7 +27,7 @@ describe('test-picker-sk', () => {
         paramset: paramset,
         count: 10,
       };
-      return await Promise.resolve(new Response(JSON.stringify(response)));
+      return Promise.resolve(new Response(JSON.stringify(response)));
     };
     window.fetch = fetchMock;
 
@@ -88,33 +88,6 @@ describe('test-picker-sk', () => {
     const e = await eventPromise;
     expect(e.detail.query).to.equal('');
   });
-
-  it('should emit remove-explore event when count exceeds limit and graph is present', async () => {
-    // Mock the presence of a graph in the container.
-    const graphDiv = document.createElement('div');
-    graphDiv.id = 'graphContainer';
-    graphDiv.appendChild(document.createElement('div')); // Add a child to simulate a loaded graph.
-    document.body.appendChild(graphDiv);
-
-    // Re-initialize element to pick up the graphDiv.
-    element = newInstance((_el: TestPickerSk) => {});
-    element.initializeTestPicker(['benchmark'], {}, false);
-    await new Promise((resolve) => setTimeout(resolve, 100));
-
-    // Listen for the event.
-    let eventCaught = false;
-    element.addEventListener('remove-explore', () => {
-      eventCaught = true;
-    });
-
-    // Manually trigger updateCount with a large number.
-    (element as any).updateCount(201);
-
-    expect(eventCaught).to.be.true;
-
-    // Cleanup
-    document.body.removeChild(graphDiv);
-  });
 });
 
 describe('test-picker-sk conditional defaults', () => {
@@ -129,7 +102,7 @@ describe('test-picker-sk conditional defaults', () => {
     document.body.appendChild(mockExplore);
 
     // Mock fetch
-    window.fetch = async (_url: RequestInfo | URL, request: RequestInit | undefined) => {
+    window.fetch = (_url: RequestInfo | URL, request: RequestInit | undefined) => {
       const req = JSON.parse(request!.body as string) as NextParamListHandlerRequest;
       const params = toParamSet(req.q!);
       const paramset: any = {};
@@ -145,7 +118,7 @@ describe('test-picker-sk conditional defaults', () => {
         paramset: paramset,
         count: 10,
       };
-      return await Promise.resolve(new Response(JSON.stringify(response)));
+      return Promise.resolve(new Response(JSON.stringify(response)));
     };
 
     element = newInstance((_el: TestPickerSk) => {});