Revert "[perf] Performance Improvements"
This reverts commit 20d0b632c7c70238b6fef1d6119a3f20a28fb70a.
Reason for revert: Seeing issues with dataframe, going to revert and investigate
Original change's description:
> [perf] Performance Improvements
>
> Refactor ExploreMultiSk to improve graph management and data loading efficiency.
>
> - Fix to correctly reset exploreElements, resolving memory leak and state issues with graph splitting.
> - Use allGraphConfigs for centralized configuration management.
> - Optimize graph rendering and data fetching coordination via pagination.
>
> Change-Id: I10c745f387af59d6f012b4b0c39447e83e7d92f8
> Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/1037197
> Reviewed-by: Farid (Mojtaba) Faridzad <faridzad@google.com>
> Commit-Queue: Tony Seaward <seawardt@google.com>
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Change-Id: I691a55488d764cdf5bbee000f51f8b5b367c5cd4
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/1120456
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Tony Seaward <seawardt@google.com>
diff --git a/.gitignore b/.gitignore
index 4500eb1..31394ab 100644
--- a/.gitignore
+++ b/.gitignore
@@ -123,4 +123,3 @@
# Dotenv
.env
-tsconfig.json
diff --git a/perf/modules/alerts-page-sk/alerts-page-sk.ts b/perf/modules/alerts-page-sk/alerts-page-sk.ts
index 723d6c6..c9b13eb 100644
--- a/perf/modules/alerts-page-sk/alerts-page-sk.ts
+++ b/perf/modules/alerts-page-sk/alerts-page-sk.ts
@@ -244,7 +244,7 @@
}
private startEditing(cfg: Alert) {
- this.origCfg = structuredClone(this._cfg);
+ this.origCfg = JSON.parse(JSON.stringify(this._cfg)) as Alert;
this.cfg = cfg;
this.dialog!.showModal();
}
@@ -291,7 +291,7 @@
}
set cfg(val) {
- this._cfg = structuredClone(val);
+ this._cfg = JSON.parse(JSON.stringify(val)) as Alert;
if (this._cfg && !this._cfg.owner) {
this._cfg.owner = this.email;
}
diff --git a/perf/modules/dataframe/dataframe_context.ts b/perf/modules/dataframe/dataframe_context.ts
index 120b016..d3d1757 100644
--- a/perf/modules/dataframe/dataframe_context.ts
+++ b/perf/modules/dataframe/dataframe_context.ts
@@ -39,8 +39,6 @@
import { formatSpecialFunctions } from '../paramtools';
import { MISSING_DATA_SENTINEL } from '../const/const';
-const MAX_DATAPOINTS = 5000;
-
// Holds issue data for a single data point.
// x and y corresponds to the location of the data point on the chart
export interface IssueDetail {
@@ -278,21 +276,6 @@
.concat(traceTail[key] || []) as Trace;
});
- if (this._header.length > MAX_DATAPOINTS) {
- const excess = this._header.length - MAX_DATAPOINTS;
- if (isAfter) {
- this._header.splice(0, excess);
- Object.keys(this._traceset).forEach((key) => {
- (this._traceset[key] as number[]).splice(0, excess);
- });
- } else {
- this._header.splice(MAX_DATAPOINTS);
- Object.keys(this._traceset).forEach((key) => {
- (this._traceset[key] as number[]).splice(MAX_DATAPOINTS);
- });
- }
- }
-
if (traceMetadata !== null) {
if (this._traceMetadata === null) {
this._traceMetadata = [];
diff --git a/perf/modules/explore-multi-sk/explore-multi-sk.ts b/perf/modules/explore-multi-sk/explore-multi-sk.ts
index 03f1cbd..1c97219 100644
--- a/perf/modules/explore-multi-sk/explore-multi-sk.ts
+++ b/perf/modules/explore-multi-sk/explore-multi-sk.ts
@@ -113,14 +113,6 @@
}
export class ExploreMultiSk extends ElementSk {
- private allGraphConfigs: GraphConfig[] = [];
-
- private allFrameResponses: FrameResponse[] = [];
-
- private allFrameRequests: FrameRequest[] = [];
-
- private mainGraphSelectedRange: { begin: number; end: number } | null = null;
-
private graphConfigs: GraphConfig[] = [];
private exploreElements: ExploreSimpleSk[] = [];
@@ -290,7 +282,7 @@
}
await load();
- await this.renderCurrentPage();
+ await this.addGraphsToCurrentPage();
// If a key is specified on initial load, we must wait for the
// shortcut's graphs to load their data before we can split them.
if (this.state.splitByKeys.length > 0 && this.exploreElements.length > 0) {
@@ -369,7 +361,7 @@
check();
});
}
- this.renderCurrentPage(false);
+ this.addGraphsToCurrentPage(false);
const query = this.testPicker!.createQueryFromFieldData();
await newExplore.addFromQueryOrFormula(true, 'query', query, '');
} else {
@@ -397,7 +389,7 @@
return;
}
await mainGraph.requestComplete;
- this.renderCurrentPage(false);
+ this.addGraphsToCurrentPage(false);
const CHUNK_SIZE = 5;
const groupdToLoadInChunks = Math.min(this.state.pageSize, groups.length);
@@ -499,7 +491,7 @@
check();
});
}
- this.renderCurrentPage(true);
+ this.addGraphsToCurrentPage(true);
explore = newExplore;
} else {
return;
@@ -902,8 +894,8 @@
traces.forEach((trace) => {
queries.push(queryFromKey(trace));
});
- } else if (this.allGraphConfigs.length > 0) {
- queries.push(...this.allGraphConfigs[0].queries);
+ } else {
+ queries.push(...this.graphConfigs[0].queries);
}
const request: FrameRequest = {
queries: queries,
@@ -1011,24 +1003,16 @@
}
*/
- this.mainGraphSelectedRange = this.exploreElements[0].getSelectedRange();
+ const selectedRange = this.exploreElements[0].getSelectedRange();
// Create the main graph config containing all trace data.
const mainRequest: FrameRequest = this.createFrameRequest();
const mainResponse: FrameResponse = this.createFrameResponse();
- this.allFrameRequests = [mainRequest];
- this.allFrameResponses = [mainResponse];
+ const frameRequests: FrameRequest[] = [mainRequest];
+ const frameResponses: FrameResponse[] = [mainResponse];
this.clearGraphs();
-
- // Add the main graph back so it occupies index 0.
- this.addEmptyGraph();
- // Ensure the main graph config (index 0) has the queries for all data.
- if (this.allGraphConfigs.length > 0) {
- this.allGraphConfigs[0].queries = mainRequest.queries || [];
- }
-
// Create the graph configs for each group.
Array.from(groupedTraces.values()).forEach((traces, i) => {
this.addEmptyGraph();
@@ -1040,13 +1024,31 @@
// Main graph config is always at index 0.
this.graphConfigs[i + 1] = graphConfig;
- this.allFrameRequests.push(exploreRequest);
- this.allFrameResponses.push(exploreResponse);
+ frameRequests.push(exploreRequest);
+ frameResponses.push(exploreResponse);
});
// Now add the graphs that have been configured to the page.
- this.renderCurrentPage();
+ this.addGraphsToCurrentPage(true);
+ const isSplitChart: boolean = this.exploreElements.length > 1;
+ // Limit page size to the number of graphs available.
+ const limit = Math.min(
+ this.state.pageSize + this.state.pageOffset + 1,
+ this.exploreElements.length
+ );
+
+ // If graph is being split, skip the primary graph (index 0), as it contains all the data.
+ // This is to avoid displaying the primary graph in the pagination view.
+ const offset = isSplitChart ? this.state.pageOffset + 1 : 0;
+ for (let i = offset; i < limit; i++) {
+ this.exploreElements[i].UpdateWithFrameResponse(
+ frameResponses[i],
+ frameRequests[i],
+ false,
+ selectedRange
+ );
+ }
if (this.stateHasChanged) {
this.stateHasChanged();
}
@@ -1105,7 +1107,7 @@
const checkTracesets = () => {
const currentTracesets = this.getTracesets();
if (
- currentTracesets.length === this.currentPageExploreElements.length &&
+ currentTracesets.length === this.exploreElements.length &&
currentTracesets.some((ts) => ts.length > 0)
) {
resolve(currentTracesets);
@@ -1137,7 +1139,7 @@
this.testPicker!.populateFieldDataFromParamSet(paramSets, paramSet);
this.testPicker!.setReadOnly(false);
- this.currentPageExploreElements[0].useBrowserURL(false);
+ this.exploreElements[0].useBrowserURL(false);
this.testPicker!.scrollIntoView();
}
@@ -1169,28 +1171,26 @@
const numPages = Math.ceil(this.state.totalGraphs / this.state.pageSize);
const maxValidPageOffset = Math.max(0, (numPages - 1) * this.state.pageSize);
this.state.pageOffset = Math.min(this.state.pageOffset, maxValidPageOffset);
- this.renderCurrentPage();
+ this.addGraphsToCurrentPage(true);
}
this.updateShortcutMultiview();
} else {
- this.state.totalGraphs = this.exploreElements.length;
+ const numElements = this.exploreElements.length;
+ this.state.totalGraphs = numElements > 1 ? numElements - 1 : 1;
if (this.stateHasChanged) this.stateHasChanged();
- this.renderCurrentPage();
+ this.addGraphsToCurrentPage(true);
}
}
private resetGraphs() {
this.emptyCurrentPage();
- this.currentPageExploreElements = [];
- this.allGraphConfigs = [];
this.exploreElements = [];
- this.allFrameRequests = [];
- this.allFrameResponses = [];
+ this.graphConfigs = [];
}
private clearGraphs() {
- this.allGraphConfigs = [];
- this.exploreElements = [];
+ this.exploreElements.splice(1);
+ this.graphConfigs.splice(1);
}
private emptyCurrentPage(): void {
@@ -1199,36 +1199,30 @@
this.currentPageGraphConfigs = [];
}
- private renderCurrentPage(doNotQueryData: boolean = true): void {
- this.state.totalGraphs = this.allGraphConfigs.length > 1 ? this.allGraphConfigs.length - 1 : 1;
+ private addGraphsToCurrentPage(doNotQueryData: boolean = false): void {
+ this.state.totalGraphs = this.exploreElements.length > 1 ? this.exploreElements.length - 1 : 1;
this.emptyCurrentPage();
- let startIndex = this.allGraphConfigs.length > 1 ? this.state.pageOffset : 0;
+ let startIndex = this.exploreElements.length > 1 ? this.state.pageOffset : 0;
- if (this.allGraphConfigs.length > 1) {
+ if (this.exploreElements.length > 1) {
startIndex++;
}
let endIndex = startIndex + this.state.pageSize - 1;
- if (this.allGraphConfigs.length <= endIndex) {
- endIndex = this.allGraphConfigs.length - 1;
+ if (this.exploreElements.length <= endIndex) {
+ endIndex = this.exploreElements.length - 1;
+ }
+
+ for (let i = startIndex; i <= endIndex; i++) {
+ this.currentPageExploreElements.push(this.exploreElements[i]);
+ this.currentPageGraphConfigs.push(this.graphConfigs[i]);
}
const fragment = document.createDocumentFragment();
- for (let i = startIndex; i <= endIndex; i++) {
- const graphConfig = this.allGraphConfigs[i];
- if (!graphConfig) {
- continue;
- }
- const explore = this.createExploreSimpleSk();
- this.currentPageExploreElements.push(explore);
- this.addStateToExplore(explore, graphConfig, doNotQueryData, i);
- fragment.appendChild(explore);
- explore.UpdateWithFrameResponse(
- this.allFrameResponses[i],
- this.allFrameRequests[i],
- false,
- this.mainGraphSelectedRange
- );
- }
+ this.currentPageExploreElements.forEach((elem, i) => {
+ const graphConfig = this.currentPageGraphConfigs[i];
+ this.addStateToExplore(elem, graphConfig, doNotQueryData);
+ fragment.appendChild(elem);
+ });
this.graphDiv!.appendChild(fragment);
this.updateChartHeights();
@@ -1244,14 +1238,12 @@
}
private async syncRange(e: CustomEvent<PlotSelectionEventDetails>): Promise<void> {
- const graphs = this.currentPageExploreElements;
+ const graphs = this.exploreElements;
const offset = e.detail.offsetInSeconds;
const range = e.detail.value;
// It is possible when loading split graphs on start that the first element
// hasnt selected a range yet.
- const selectedRange = this.currentPageExploreElements
- .map((e) => e.getSelectedRange())
- .find((r) => !!r);
+ const selectedRange = this.exploreElements.map((e) => e.getSelectedRange()).find((r) => !!r);
// Sets dataLoading state across all graphs since the main graph is only one doing work.
graphs.forEach((graph, i) => {
@@ -1263,7 +1255,7 @@
// Extend range of primary graph first, so that the other graphs can use
// the updated range when they are updated.
- await this.currentPageExploreElements[0].extendRange(range, offset);
+ await this.exploreElements[0].extendRange(range, offset);
// Once extended, then update each split graph.
graphs.forEach((graph, i) => {
@@ -1288,7 +1280,7 @@
}
private async syncChartSelection(e: CustomEvent<PlotSelectionEventDetails>): Promise<void> {
- const graphs = this.currentPageExploreElements;
+ const graphs = this.exploreElements;
if (!e.detail.value) {
return;
}
@@ -1342,9 +1334,9 @@
private addStateToExplore(
explore: ExploreSimpleSk,
graphConfig: GraphConfig,
- doNotQueryData: boolean,
- index: number
+ doNotQueryData: boolean
) {
+ const index = this.exploreElements.indexOf(explore);
const newState: ExploreState = {
formulas: graphConfig.formulas || [],
queries: graphConfig.queries || [],
@@ -1375,15 +1367,13 @@
hide_paramset: true,
graph_index: index,
doNotQueryData: doNotQueryData,
- dots: this.state.dots,
- autoRefresh: false,
- show_google_plot: this.state.show_google_plot,
};
explore.state = newState;
}
- private createExploreSimpleSk(): ExploreSimpleSk {
+ private addEmptyGraph(unshift?: boolean): ExploreSimpleSk | null {
const explore: ExploreSimpleSk = new ExploreSimpleSk(this.useTestPicker);
+ const graphConfig = new GraphConfig();
explore.defaults = this.defaults;
explore.openQueryByDefault = false;
explore.navOpen = false;
@@ -1391,25 +1381,33 @@
if (this.userEmail) {
explore.user = this.userEmail;
}
+ if (unshift) {
+ this.exploreElements.unshift(explore);
+ this.graphConfigs.unshift(graphConfig);
+ } else {
+ this.exploreElements.push(explore);
+ this.graphConfigs.push(graphConfig);
+ }
explore.addEventListener('state_changed', () => {
const elemState = explore.state;
let stateChanged = false;
- if (this.allGraphConfigs[elemState.graph_index].formulas !== elemState.formulas) {
- this.allGraphConfigs[elemState.graph_index].formulas = elemState.formulas || [];
+ if (this.graphConfigs[elemState.graph_index].formulas !== elemState.formulas) {
+ graphConfig.formulas = elemState.formulas || [];
stateChanged = true;
}
- if (this.allGraphConfigs[elemState.graph_index].queries[0] !== elemState.queries[0]) {
- this.allGraphConfigs[elemState.graph_index].queries = elemState.queries || [];
+ if (this.graphConfigs[elemState.graph_index].queries[0] !== elemState.queries[0]) {
+ graphConfig.queries = elemState.queries || [];
stateChanged = true;
}
- if (this.allGraphConfigs[elemState.graph_index].keys !== elemState.keys) {
- this.allGraphConfigs[elemState.graph_index].keys = elemState.keys || '';
+ if (this.graphConfigs[elemState.graph_index].keys !== elemState.keys) {
+ graphConfig.keys = elemState.keys || '';
stateChanged = true;
}
if (stateChanged) {
+ this.graphConfigs[elemState.graph_index] = graphConfig;
this.updateShortcutMultiview();
}
});
@@ -1534,10 +1532,7 @@
* @returns
*/
private getHeader(): (ColumnHeader | null)[] | null {
- if (this.currentPageExploreElements.length === 0) {
- return null;
- }
- return this.currentPageExploreElements[0].getHeader();
+ return this.exploreElements[0].getHeader();
}
/**
@@ -1548,7 +1543,7 @@
private getCompleteTraceset(): { [key: string]: number[] } {
const fullTraceSet: { [key: string]: number[] } = {};
- this.currentPageExploreElements.forEach((elem) => {
+ this.exploreElements.forEach((elem) => {
const headerLength = elem.getHeader()?.length;
// Check that header lengths are the same, otherwise ignore.
if (headerLength === this.getHeader()?.length) {
@@ -1571,7 +1566,7 @@
*/
private getAllCommitLinks(): (CommitLinks | null)[] {
const commitLinks: (CommitLinks | null)[] = [];
- this.currentPageExploreElements.forEach((elem) => {
+ this.exploreElements.forEach((elem) => {
const elemLinks = elem.getCommitLinks();
if (elemLinks.length > 0) {
commitLinks.push(...elemLinks);
@@ -1583,7 +1578,7 @@
private getFullAnomalyMap(): AnomalyMap {
const anomalyMap: AnomalyMap = {};
- this.currentPageExploreElements.forEach((elem) => {
+ this.exploreElements.forEach((elem) => {
const anomalies = elem.getAnomalyMap();
Object.keys(anomalies!).forEach((traceId) => {
const existingEntry = anomalyMap[traceId];
@@ -1638,7 +1633,7 @@
*
*/
private updateShortcutMultiview() {
- updateShortcut(this.allGraphConfigs)
+ updateShortcut(this.graphConfigs)
.then((shortcut) => {
if (shortcut === '') {
this.state.shortcut = '';
@@ -1659,7 +1654,7 @@
this.state.pageOffset + e.detail.delta * this.state.pageSize
);
this.stateHasChanged!();
- this.renderCurrentPage();
+ this.splitGraphs();
}
private pageSizeChanged(e: MouseEvent) {
@@ -1667,20 +1662,7 @@
this.testPicker?.setReadOnly(true);
this.state.pageSize = +(e.target! as HTMLInputElement).value;
this.stateHasChanged!();
- this.renderCurrentPage();
- }
-
- private addEmptyGraph(unshift?: boolean): ExploreSimpleSk {
- const explore = this.createExploreSimpleSk();
- const graphConfig = new GraphConfig();
- if (unshift) {
- this.exploreElements.unshift(explore);
- this.allGraphConfigs.unshift(graphConfig);
- } else {
- this.exploreElements.push(explore);
- this.allGraphConfigs.push(graphConfig);
- }
- return explore;
+ this.splitGraphs();
}
private async loadAllCharts() {
@@ -1697,4 +1679,5 @@
}
}
}
+
define('explore-multi-sk', ExploreMultiSk);
diff --git a/perf/modules/explore-multi-sk/explore-multi-sk_test.ts b/perf/modules/explore-multi-sk/explore-multi-sk_test.ts
index 7b69c3a..b42e73c 100644
--- a/perf/modules/explore-multi-sk/explore-multi-sk_test.ts
+++ b/perf/modules/explore-multi-sk/explore-multi-sk_test.ts
@@ -200,7 +200,7 @@
const initialGraphCount = element['exploreElements'].length;
element['addEmptyGraph']();
assert.equal(element['exploreElements'].length, initialGraphCount + 1);
- assert.equal(element['allGraphConfigs'].length, initialGraphCount + 1);
+ assert.equal(element['graphConfigs'].length, initialGraphCount + 1);
});
it('removes a graph when a remove-explore event is dispatched', async () => {
@@ -216,7 +216,6 @@
element.dispatchEvent(event);
assert.equal(element['exploreElements'].length, initialGraphCount - 1);
- assert.equal(element['allGraphConfigs'].length, initialGraphCount - 1);
});
it('resets pagination when the last graph is removed', async () => {
@@ -232,7 +231,6 @@
element.dispatchEvent(event);
assert.equal(element['exploreElements'].length, 0);
- assert.equal(element['allGraphConfigs'].length, 0);
assert.equal(element.state.totalGraphs, 0);
// However, the page offset is correctly reset to 0 by a different
@@ -285,7 +283,7 @@
const newShortcutId = 'new-shortcut-id';
fetchMock.post('/_/shortcut/update', { id: newShortcutId });
- element['allGraphConfigs'] = [{ queries: ['config=new'], formulas: [], keys: '' }];
+ element['graphConfigs'] = [{ queries: ['config=new'], formulas: [], keys: '' }];
// stateHasChanged needs to be non-null for the update to be pushed.
element['stateHasChanged'] = () => {};
element['updateShortcutMultiview']();
@@ -349,34 +347,6 @@
beforeEach(async () => {
await setupElement();
});
-
- it('does not leak exploreElements on repeated splitGraphs', async () => {
- // Initial state: 0 graphs (connectedCallback might not add one if no shortcut/params)
- // Let's add one manually to start with a known state.
- element['addEmptyGraph']();
- assert.equal(element['exploreElements'].length, 1);
-
- // Mock grouping to always return 1 group to simulate steady state
- sinon.stub(element, 'groupTracesBySplitKey' as any).returns(new Map([['key', ['trace1']]]));
-
- // Mock methods called by splitGraphs
- sinon.stub(element, 'createFrameRequest' as any).returns({});
- sinon.stub(element, 'createFrameResponse' as any).returns({});
- sinon.stub(element, 'renderCurrentPage' as any);
- sinon.stub(element, 'checkDataLoaded' as any);
-
- // Force totalGraphs to > 1 so splitGraphs doesn't return early
- element.state.totalGraphs = 2;
-
- await element['splitGraphs'](false, true);
-
- // Should be reset to 2 (1 Master + 1 group)
- assert.equal(element['exploreElements'].length, 2, 'exploreElements should be reset');
-
- // Run again
- await element['splitGraphs'](false, true);
- assert.equal(element['exploreElements'].length, 2, 'exploreElements should remain stable');
- });
});
describe('Synchronization', () => {
@@ -463,7 +433,7 @@
const spy2 = sinon.spy(graph2, 'updateSelectedRangeWithPlotSummary');
// Manually set the internal state to use our mocks.
- element['currentPageExploreElements'] = [graph1 as any, graph2 as any];
+ element['exploreElements'] = [graph1 as any, graph2 as any];
const detail: PlotSelectionEventDetails = {
domain: 'commit',
@@ -572,12 +542,9 @@
horizontal_zoom: false,
graph_index: 0,
doNotQueryData: false,
- dots: true,
- autoRefresh: false,
- show_google_plot: false,
};
- element['addStateToExplore'](simpleSk, new GraphConfig(), false, 0);
+ element['addStateToExplore'](simpleSk, new GraphConfig(), false);
assert.equal(simpleSk.state.begin, 1000);
assert.equal(simpleSk.state.end, 2000);
@@ -644,7 +611,6 @@
{ queries: ['config=test1'], formulas: [], keys: '' },
{ queries: ['config=test2'], formulas: [], keys: '' },
];
- element['allGraphConfigs'] = element['graphConfigs'];
element.state.splitByKeys = ['config'];
await element['loadAllCharts']();
@@ -808,16 +774,14 @@
// After the initial chunked load, the `exploreElements` array is populated.
// Let's simulate the state after `_onPlotButtonClicked` has run.
- // The stub for `splitGraphs` prevents `renderCurrentPage` from being called
+ // The stub for `splitGraphs` prevents `addGraphsToCurrentPage` from being called
// in a way that's useful for this test's setup, so we call it manually.
element['exploreElements'] = [
mainGraph,
...Array(totalSplitGraphs).fill(new ExploreSimpleSk()),
];
element['graphConfigs'] = Array(totalSplitGraphs + 1).fill(new GraphConfig());
- element['allGraphConfigs'] = element['graphConfigs'];
- element.state.pageSize = 2;
- element['renderCurrentPage'](true); // This will respect the small pageSize.
+ element['addGraphsToCurrentPage'](true); // This will respect the small pageSize.
assert.equal(
element['currentPageExploreElements'].length,
@@ -849,7 +813,7 @@
assert.equal(element.state.pageOffset, 0, 'Page offset should be reset');
assert.isTrue(splitGraphsSpy.called, 'splitGraphs should be called to reload');
- element['renderCurrentPage']();
+ element['addGraphsToCurrentPage']();
assert.equal(
element['currentPageExploreElements'].length,
totalSplitGraphs,
@@ -903,8 +867,8 @@
state.shortcut = shortcutId;
state.splitByKeys = ['someKey']; // Enable splitting to trigger aggregation.
- // Mock renderCurrentPage to avoid DOM issues in this test
- sinon.stub(element, 'renderCurrentPage' as any).returns(undefined);
+ // Mock addGraphsToCurrentPage to avoid DOM issues in this test
+ sinon.stub(element, 'addGraphsToCurrentPage' as any).returns(undefined);
// Mock splitGraphs as well since it's called after graph loading
sinon.stub(element, 'splitGraphs' as any).resolves();
// Mock checkDataLoaded to prevent side effects
diff --git a/perf/modules/explore-simple-sk/explore-simple-sk.ts b/perf/modules/explore-simple-sk/explore-simple-sk.ts
index 57eb46e..a952fe5 100644
--- a/perf/modules/explore-simple-sk/explore-simple-sk.ts
+++ b/perf/modules/explore-simple-sk/explore-simple-sk.ts
@@ -183,10 +183,6 @@
// max number of charts to show on a page
const chartsPerPage = 11;
-const REFRESH_TIMEOUT = 30 * 1000;
-
-type RequestFrameCallback = (frameResponse: FrameResponse) => Promise<void>;
-
export interface ZoomWithDelta {
zoom: CommitRange;
delta: CommitNumber;
@@ -315,12 +311,6 @@
showZero: boolean = false;
- dots: boolean = true;
-
- autoRefresh: boolean = false;
-
- show_google_plot = false;
-
numCommits: number = 250;
requestType: RequestType = 1; // 0 to use begin/end, 1 to use numCommits.
@@ -475,8 +465,6 @@
// is no pending request.
private _requestId = '';
- private _refreshId = -1;
-
// All the data converted into a CVS blob to download.
private _csvBlobURL: string = '';
@@ -957,10 +945,7 @@
};
private showZero = () => {
- this.setState({
- ...this._state,
- showZero: !this._state.showZero,
- });
+ this._state.showZero = !this._state.showZero;
this.googleChartPlot.value!.showZero = this._state.showZero;
this.render();
};
@@ -1117,15 +1102,12 @@
const position = chart.getPositionByIndex(index);
const commit = this.getCommitDetails(commitPos);
const prevCommit = this.getCommitDetails(this.getPreviousCommit(index.tableRow, traceName));
- this.setState({
- ...this._state,
- selected: {
- name: traceName,
- commit: commitPos,
- tableCol: index.tableCol,
- tableRow: index.tableRow,
- },
- });
+ this.state.selected = {
+ name: traceName,
+ commit: commitPos,
+ tableCol: index.tableCol,
+ tableRow: index.tableRow,
+ };
this.enableTooltip(
{
@@ -1544,15 +1526,9 @@
private switchZoom(target: MdSwitch | null) {
this.zoomDirectionSwitch = !this.zoomDirectionSwitch;
if (target!.selected) {
- this.setState({
- ...this._state,
- horizontal_zoom: true,
- });
+ this.state.horizontal_zoom = true;
} else {
- this.setState({
- ...this._state,
- horizontal_zoom: false,
- });
+ this.state.horizontal_zoom = false;
}
this.render();
const detail = {
@@ -1708,12 +1684,10 @@
})
.then(jsonOrThrow)
.then((json: ShiftResponse) => {
- this.setState({
- ...this._state,
- begin: json.begin,
- end: json.end,
- requestType: 0,
- });
+ this._state.begin = json.begin;
+ this._state.end = json.end;
+ this._state.requestType = 0;
+ this._stateHasChanged();
this.rangeChangeImpl();
})
.catch(errorMessage);
@@ -2459,10 +2433,8 @@
if (this.logEntry) {
this.logEntry!.textContent = '';
}
- this.setState({
- ...this._state,
- selected: defaultPointSelected(),
- });
+ this._state.selected = defaultPointSelected();
+ this._stateHasChanged();
}
/**
@@ -2759,15 +2731,17 @@
this.commitTime!.textContent = '';
}
const switchToTab = body.formulas!.length > 0 || body.queries!.length > 0 || body.keys !== '';
- this.requestFrame(body, (json) => {
- if (json === null || json === undefined) {
+ this.requestFrame(body)
+ .then((json) => {
+ if (json === null || json === undefined) {
+ errorMessage('Failed to find any matching traces.');
+ return;
+ }
+ return this.UpdateWithFrameResponse(json, body, switchToTab);
+ })
+ .catch(() => {
errorMessage('Failed to find any matching traces.');
- return Promise.resolve();
- }
- return this.UpdateWithFrameResponse(json, body, switchToTab);
- }).catch(() => {
- errorMessage('Failed to find any matching traces.');
- });
+ });
}
/**
@@ -2840,40 +2814,6 @@
* otherwise replace them all with the new ones.
* @param {Boolean} tab - If true then switch to the Params tab.
*/
- private toggleDotsHandler() {
- this.setState({
- ...this._state,
- dots: !this._state.dots,
- });
- }
-
- private autoRefreshHandler(target: MdSwitch | null) {
- this.setState({
- ...this._state,
- autoRefresh: target!.selected,
- });
- this.autoRefreshChanged();
- }
-
- private autoRefreshChanged() {
- if (!this._state.autoRefresh) {
- if (this._refreshId !== -1) {
- clearInterval(this._refreshId);
- }
- } else {
- this._refreshId = window.setInterval(() => this.autoRefresh(), REFRESH_TIMEOUT);
- }
- }
-
- private autoRefresh() {
- const body = this.requestFrameBodyFullFromState();
- const switchToTab = body.formulas!.length > 0 || body.queries!.length > 0 || body.keys !== '';
- this.requestFrame(body, (json) => {
- return this.UpdateWithFrameResponse(json, body, switchToTab);
- });
- this.setState(this._state);
- }
-
private async addTraces(
json: FrameResponse,
tab: boolean,
@@ -3036,10 +2976,10 @@
*
* @param plotType - The type of traces being added.
*/
- async add(replace: boolean, plotType: addPlotType): Promise<void> {
+ add(replace: boolean, plotType: addPlotType): void {
const q = this.query!.current_query;
const f = this.formula!.value;
- await this.addFromQueryOrFormula(replace, plotType, q, f);
+ this.addFromQueryOrFormula(replace, plotType, q, f);
}
/** Create a FrameRequest for just a single query or formula. */
@@ -3153,7 +3093,7 @@
try {
if (createFromScratch) {
const body = this.requestFrameBodyFullFromState();
- return this.requestFrame(body, (json) => {
+ return this.requestFrame(body).then((json) => {
return this.UpdateWithFrameResponse(
json,
body,
@@ -3165,7 +3105,7 @@
} else {
const requestNewData = this.requestFrameBodyForTrace(plotType, q, f);
const requestAllData = this.requestFrameBodyFullFromState();
- return this.requestFrame(requestNewData, (json) => {
+ return this.requestFrame(requestNewData).then((json) => {
// Merge new data with the existing state.
const frameResponse = json as FrameResponse;
const newDf = frameResponse.dataframe!;
@@ -3392,12 +3332,10 @@
})
.then(jsonOrThrow)
.then((json) => {
- this.setState({
- ...this._state,
- keys: json.id,
- queries: [],
- });
+ this._state.keys = json.id;
+ this._state.queries = [];
this.clearSelectedState();
+ this._stateHasChanged();
this.render();
})
.catch(errorMessage);
@@ -3538,6 +3476,16 @@
}
}
+ /** Common catch function for _requestFrame and _checkFrameRequestStatus. */
+ private catch(msg: any) {
+ this._requestId = '';
+ if (msg) {
+ errorMessage(msg);
+ }
+ this.percent!.textContent = '';
+ this.spinning = false;
+ }
+
/** @prop spinning - True if we are waiting to retrieve data from
* the server.
*/
@@ -3566,58 +3514,43 @@
* tz: "America/New_York"
* };
*
- * The 'cb' callback function will be called with the decoded JSON body
+ * @returns A Promise that resolves with the decoded JSON body
* of the response once it's available.
*/
- private requestFrame(body: FrameRequest, cb: RequestFrameCallback): Promise<void> {
+ private async requestFrame(body: FrameRequest): Promise<FrameResponse> {
if (this._requestId !== '') {
const err = new Error('There is a pending query already running.');
errorMessage(err.message);
- return Promise.reject(err);
+ throw err;
}
- this._requestId = 'About to make request';
+ // _requestId is used as a lock, any non-empty string works for that.
+ this._requestId = 'Request in progress';
this.spinning = true;
- return new Promise<void>((resolve, reject) => {
- this.sendFrameRequest(body, async (json) => {
- // make this inner callback async
- try {
- await cb(json); // await the execution of the main callback
- resolve();
- } catch (e: any) {
- // Catch errors from cb
- errorMessage(e.message || e.toString());
- reject(e);
- }
- })
- .catch((msg: any) => {
- // Catch errors from sendFrameRequest itself
- if (msg) {
- errorMessage(msg.message || msg.toString());
- }
- if (this.percent) {
- // Check if percent is not null
- this.percent.textContent = '';
- }
- reject(msg);
- })
- .finally(() => {
- this.spinning = false;
- this._requestId = '';
- });
- });
+ this.dataLoading = true;
+
+ try {
+ const jsonResponse = await this.sendFrameRequest(body);
+ return jsonResponse;
+ } catch (err: any) {
+ // This catches errors from sendFrameRequest (e.g., status !== 'Finished')
+ if (this.percent) {
+ this.percent.textContent = '';
+ }
+ // Re-throw the error for the caller to handle
+ throw err;
+ } finally {
+ // Ensuring the state is always cleaned up.
+ this.spinning = false;
+ this.dataLoading = false;
+ this._requestId = '';
+ }
}
/**
* Starts the frame request and returns the resulting data.
*/
- private async sendFrameRequest(body: FrameRequest, cb: RequestFrameCallback) {
- // Fill in a default time range if either of the values are -1.
- if (this._state.begin === -1 || this._state.end === -1) {
- const now = Math.floor(Date.now() / 1000);
- this._state.begin = now - DEFAULT_RANGE_S;
- this._state.end = now;
- }
+ private async sendFrameRequest(body: FrameRequest): Promise<FrameResponse> {
body.tz = Intl.DateTimeFormat().resolvedOptions().timeZone;
const finishedProg = await startRequest(
@@ -3640,7 +3573,7 @@
errorMessage(msg);
}
- await cb(finishedProg.results as FrameResponse);
+ return finishedProg.results as FrameResponse;
}
get state(): State {
@@ -3648,10 +3581,6 @@
}
set state(state: State) {
- this.setState(state);
- }
-
- setState(state: State): void {
const prevBegin = this._state?.begin;
const prevEnd = this._state?.end;
const prevRequestType = this._state?.requestType;
@@ -3659,13 +3588,15 @@
const prevFormulas = [...(this._state?.formulas || [])];
const prevKeys = this._state?.keys;
- if (state.begin === -1 && state.end === -1) {
- const now = Math.floor(Date.now() / 1000);
- state.end = now;
- state.begin = now - this.getDefaultRange();
- }
state = this.rationalizeTimeRange(state);
this._state = state;
+
+ // Synchronize the xAxisSwitch with the state's domain
+ const isDateDomain = this._state.domain === 'date';
+ if (this.xAxisSwitch !== isDateDomain) {
+ this.xAxisSwitch = isDateDomain;
+ }
+
if (this.range) {
this.range!.state = {
begin: this._state.begin,
@@ -3715,8 +3646,7 @@
this.updateTestPickerUrl();
}
- this.autoRefreshChanged();
- if (!state.doNotQueryData && this.getHeader() !== null) {
+ if (!state.doNotQueryData) {
this.rangeChangeImpl();
}
}
diff --git a/perf/modules/plot-google-chart-sk/plot-google-chart-sk.ts b/perf/modules/plot-google-chart-sk/plot-google-chart-sk.ts
index 07d991b..299f636 100644
--- a/perf/modules/plot-google-chart-sk/plot-google-chart-sk.ts
+++ b/perf/modules/plot-google-chart-sk/plot-google-chart-sk.ts
@@ -712,19 +712,6 @@
});
}
- disconnectedCallback(): void {
- super.disconnectedCallback();
- this.clear();
- window.removeEventListener('mousemove', this.onWindowMouseMove);
- window.removeEventListener('mouseup', this.onWindowMouseUp);
- }
-
- public clear(): void {
- if (this.chart) {
- this.chart.clearChart();
- }
- }
-
private onSidePanelToggle() {
this.plotElement.value?.redraw();
}
diff --git a/perf/modules/plot-google-chart-sk/plot-google-chart-sk_test.ts b/perf/modules/plot-google-chart-sk/plot-google-chart-sk_test.ts
index fd3a6a9..799dbf1 100644
--- a/perf/modules/plot-google-chart-sk/plot-google-chart-sk_test.ts
+++ b/perf/modules/plot-google-chart-sk/plot-google-chart-sk_test.ts
@@ -51,11 +51,6 @@
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
expect(element).to.not.be.null;
});
-
- it('some result', () => {
- // eslint-disable-next-line @typescript-eslint/no-unused-expressions
- expect(element).to.not.be.null;
- });
});
describe('determineYAxisTitle', () => {
diff --git a/perf/modules/plot-summary-sk/BUILD.bazel b/perf/modules/plot-summary-sk/BUILD.bazel
index 4fdd95f..560b0ae 100644
--- a/perf/modules/plot-summary-sk/BUILD.bazel
+++ b/perf/modules/plot-summary-sk/BUILD.bazel
@@ -102,18 +102,14 @@
"//:node_modules/lit",
"//perf/modules/dataframe:index_ts_lib",
],
- ts_srcs = [
- "h_resizable_box_sk.ts",
- ],
+ ts_srcs = ["h_resizable_box_sk.ts"],
visibility = ["//visibility:public"],
)
sk_element(
name = "plot-summary-sk.css_ts_lib",
ts_deps = ["//:node_modules/lit"],
- ts_srcs = [
- "plot-summary-sk.css.ts",
- ],
+ ts_srcs = ["plot-summary-sk.css.ts"],
visibility = ["//visibility:public"],
)
diff --git a/perf/modules/plot-summary-sk/plot-summary-sk.ts b/perf/modules/plot-summary-sk/plot-summary-sk.ts
index 845693c..b02d29d 100644
--- a/perf/modules/plot-summary-sk/plot-summary-sk.ts
+++ b/perf/modules/plot-summary-sk/plot-summary-sk.ts
@@ -113,18 +113,6 @@
const options = SummaryChartOptions(getComputedStyle(this), this.domain);
options.colors = [];
- // Downsample the data if there are too many points.
- const numRows = dt.getNumberOfRows();
- const maxPoints = 1000;
- if (numRows > maxPoints) {
- const sampleRate = Math.ceil(numRows / maxPoints);
- const rowsToKeep = [];
- for (let i = 0; i < numRows; i += sampleRate) {
- rowsToKeep.push(i);
- }
- view.setRows(rowsToKeep);
- }
-
// The first two columns are the commit position and the date.
const cols = [this.domain === 'commit' ? 0 : 1];
for (let i = 2; i < dt.getNumberOfColumns(); i++) {
diff --git a/perf/modules/report-page-sk/BUILD.bazel b/perf/modules/report-page-sk/BUILD.bazel
index 814c90a..0df10b7 100644
--- a/perf/modules/report-page-sk/BUILD.bazel
+++ b/perf/modules/report-page-sk/BUILD.bazel
@@ -23,13 +23,13 @@
"//:node_modules/lit",
"//elements-sk/modules:define_ts_lib",
"//elements-sk/modules:upgradeproperty_ts_lib",
- "//infra-sk/modules:jsonorthrow_ts_lib",
"//infra-sk/modules/ElementSk:index_ts_lib",
"//perf/modules/cid:cid_ts_lib",
"//perf/modules/errorMessage:index_ts_lib",
"//perf/modules/json:index_ts_lib",
- "//perf/modules/telemetry:telemetry_ts_lib",
"//perf/modules/trace-details-formatter:traceformatter_ts_lib",
+ "//perf/modules/telemetry:telemetry_ts_lib",
+ "//infra-sk/modules:jsonorthrow_ts_lib",
],
ts_srcs = [
"report-page-sk.ts",
diff --git a/perf/modules/report-page-sk/report-page-sk.ts b/perf/modules/report-page-sk/report-page-sk.ts
index b2e2be4..cb4b8e4 100644
--- a/perf/modules/report-page-sk/report-page-sk.ts
+++ b/perf/modules/report-page-sk/report-page-sk.ts
@@ -381,7 +381,6 @@
const query = this.getQueryFromAnomaly(anomaly);
const state = new State();
- state.plotSummary = true;
const timerange = this.anomalyTracker.getAnomaly(anomaly.id)!.timerange;
explore.state = {
...state,
diff --git a/perf/modules/report-page-sk/report-page-sk_puppeteer_test.ts b/perf/modules/report-page-sk/report-page-sk_puppeteer_test.ts
index dbdb876..ad914f5 100644
--- a/perf/modules/report-page-sk/report-page-sk_puppeteer_test.ts
+++ b/perf/modules/report-page-sk/report-page-sk_puppeteer_test.ts
@@ -52,86 +52,6 @@
const apiMocks = [
{
- name: 'Frame Start',
- matches: (url: string) => url.includes('/_/frame/start'),
- response: {
- status: 200,
- contentType: 'application/json',
- body: JSON.stringify({
- status: 'Finished',
- results: {
- dataframe: {
- traceset: {
- ',config=test,': [1, 2, 3, 4],
- },
- header: [
- {
- offset: 100,
- timestamp: 1676307170,
- hash: 'h1',
- author: 'a',
- message: 'm',
- url: 'u',
- },
- {
- offset: 101,
- timestamp: 1676307171,
- hash: 'h2',
- author: 'a',
- message: 'm',
- url: 'u',
- },
- {
- offset: 102,
- timestamp: 1676307172,
- hash: 'h3',
- author: 'a',
- message: 'm',
- url: 'u',
- },
- {
- offset: 103,
- timestamp: 1676307173,
- hash: 'h4',
- author: 'a',
- message: 'm',
- url: 'u',
- },
- ],
- paramset: {
- config: ['test'],
- },
- skip: 0,
- traceMetadata: [],
- },
- anomalymap: {},
- skps: [],
- msg: '',
- display_mode: 'display_plot',
- },
- messages: [],
- }),
- },
- },
- {
- name: 'User Issues',
- matches: (url: string) => url.includes('/_/user_issues/'),
- response: {
- status: 200,
- contentType: 'application/json',
- body: JSON.stringify({ UserIssues: [] }),
- },
- },
- {
- name: 'Telemetry',
- matches: (url: string) => url.includes('/_/fe_telemetry'),
- response: {
- status: 200,
- contentType: 'text/plain',
- body: '',
- },
- },
- {
name: 'CID Details',
matches: (url: string) => url.endsWith('/_/cid/'),
response: {
diff --git a/perf/modules/user-issue-sk/BUILD.bazel b/perf/modules/user-issue-sk/BUILD.bazel
index b7ca4cb..cbfb081 100644
--- a/perf/modules/user-issue-sk/BUILD.bazel
+++ b/perf/modules/user-issue-sk/BUILD.bazel
@@ -12,9 +12,9 @@
"//:node_modules/lit",
"//:node_modules/lit-html", # keep
"//infra-sk/modules/json:index_ts_lib",
- "//infra-sk/modules:jsonorthrow_ts_lib",
- "//perf/modules/errorMessage:index_ts_lib",
"//perf/modules/json:index_ts_lib",
+ "//perf/modules/errorMessage:index_ts_lib",
+ "//infra-sk/modules:jsonorthrow_ts_lib",
],
ts_srcs = [
"user-issue-sk.ts",
