[perf] Show diff of text in summary bar options for better readability. - The summary bar now shows the common title & diff of each trace as options. - We have used newer apis originally curated for google charts but fits here well. - For a single trace we hide the summary dropdown since there's no choice to be made. Bug: b/376930870 Change-Id: I1156f148317d438ab0870a74612d0006cd028230 Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/923364 Reviewed-by: Ashwin Verleker <ashwinpv@google.com> Commit-Queue: Vidit Chitkara <viditchitkara@google.com>
diff --git a/perf/configs/demo.json b/perf/configs/demo.json index 4f2a2eb..7cc0788 100644 --- a/perf/configs/demo.json +++ b/perf/configs/demo.json
@@ -60,7 +60,7 @@ "useTestPicker": "true", "showZero": "false", "enable_chart_tooltip": "true", - "use_test_picker_query": "false", + "use_test_picker_query": "true", "use_titles": "true" } },
diff --git a/perf/modules/explore-simple-sk/BUILD.bazel b/perf/modules/explore-simple-sk/BUILD.bazel index 6e53260..1daf91f 100644 --- a/perf/modules/explore-simple-sk/BUILD.bazel +++ b/perf/modules/explore-simple-sk/BUILD.bazel
@@ -66,6 +66,7 @@ "//:node_modules/@material/web", "//:node_modules/lit-html", # keep "//perf/modules/dataframe:dataframe_context_ts_lib", + "//perf/modules/dataframe:traceset_ts_lib", ], ts_srcs = [ "explore-simple-sk.ts", @@ -79,12 +80,15 @@ src = "explore-simple-sk_test.ts", deps = [ ":explore-simple-sk", + "//:node_modules/@google-web-components/google-chart", "//:node_modules/@types/chai", "//:node_modules/chai", "//:node_modules/fetch-mock", "//infra-sk/modules:object_ts_lib", "//infra-sk/modules:test_util_ts_lib", + "//perf/modules/common:plot-builder_ts_lib", + "//perf/modules/dataframe:test_utils_ts_lib", "//perf/modules/json:index_ts_lib", - "//perf/modules/paramtools:index_ts_lib", + "//perf/modules/plot-google-chart-sk", ], )
diff --git a/perf/modules/explore-simple-sk/explore-simple-sk.scss b/perf/modules/explore-simple-sk/explore-simple-sk.scss index 0dcb15c..b2419be 100644 --- a/perf/modules/explore-simple-sk/explore-simple-sk.scss +++ b/perf/modules/explore-simple-sk/explore-simple-sk.scss
@@ -471,7 +471,6 @@ flex-direction: row; align-items: end; padding-bottom: 16px; - height: 55px; box-sizing: content-box; label {
diff --git a/perf/modules/explore-simple-sk/explore-simple-sk.ts b/perf/modules/explore-simple-sk/explore-simple-sk.ts index 1c40ff9..23259dd 100644 --- a/perf/modules/explore-simple-sk/explore-simple-sk.ts +++ b/perf/modules/explore-simple-sk/explore-simple-sk.ts
@@ -143,10 +143,11 @@ PlotSelectionEventDetails, PlotShowTooltipEventDetails, } from '../plot-google-chart-sk/plot-google-chart-sk'; -import { DataFrameRepository } from '../dataframe/dataframe_context'; +import { DataFrameRepository, DataTable } from '../dataframe/dataframe_context'; import { ExistingBugDialogSk } from '../existing-bug-dialog-sk/existing-bug-dialog-sk'; import { generateSubDataframe } from '../dataframe/index'; import { SplitChartSelectionEventDetails } from '../split-chart-menu-sk/split-chart-menu-sk'; +import { getLegend, getTitle, isSingleTrace } from '../dataframe/traceset'; /** The type of trace we are adding to a plot. */ type addPlotType = 'query' | 'formula' | 'pivot'; @@ -571,7 +572,13 @@ private summaryOptionsField: Ref<PickerFieldSk> = createRef(); - private traceNameIdMap = new Map(); + // Map with displayed summary bar option as key and trace key + // as value. For example, {'...k1=v1,k2=v2' => ',ck1=>cv1,ck2=>cv2,k1=v1,k2=v2,'} + private summaryOptionTraceMap: Map<string, string> = new Map(); + + // Map with displayed trace key as key and summary bar option + // as value. For example, {',ck1=>cv1,ck2=>cv2,k1=v1,k2=v2,' => '...k1=v1,k2=v2'} + private traceIdSummaryOptionMap: Map<string, string> = new Map(); private pointToCommitDetailMap = new Map(); @@ -1067,7 +1074,7 @@ private onSummaryPickerChanged(e: CustomEvent) { const selectedTrace = e.detail.value; - this.traceKeyForSummary = this.traceNameIdMap.get(selectedTrace) || ''; + this.traceKeyForSummary = this.summaryOptionTraceMap.get(selectedTrace) || ''; const plot = this.plotSummary.value; if (plot) { @@ -1931,8 +1938,8 @@ // If traces are rendered and summary bar is enabled, show // summary for the trace clicked on the graph. if (this.summaryOptionsField.value) { - const formattedTraceId = this.traceFormatter!.formatTrace(fromKey(detail.name)); - this.summaryOptionsField.value!.setValue(formattedTraceId); + const option = this.traceIdSummaryOptionMap.get(detail.name) || ''; + this.summaryOptionsField.value!.setValue(option); } const selected = this.selectedRange?.begin || 0; @@ -2198,8 +2205,12 @@ */ private AddPlotLines(traceSet: { [key: string]: number[] }, labels: tick[]) { this.plotSimple.value?.addLines(traceSet, labels); - if (this._state.plotSummary) { - this.addPlotSummaryOptions(); + + const dt = this.dfRepo.value?.data; + const df = this.dfRepo.value?.dataframe; + const shouldAddSummaryOptions = this._state.plotSummary && df !== undefined && dt !== undefined; + if (shouldAddSummaryOptions) { + this.addPlotSummaryOptions(df, dt); } } @@ -2216,25 +2227,66 @@ /** * Adds the option list for the plot summary selection. + * @param df The dataframe object from dataframe context. + * @param df The dataTable object from dataframe context. */ - private addPlotSummaryOptions() { - if (!this.dfRepo.value?.dataframe) { + private addPlotSummaryOptions(df: DataFrame, dt: DataTable) { + if (!this.summaryOptionsField.value) { return; } - const traceIds: string[] = []; - Object.keys(this.dfRepo.value.dataframe.traceset!).forEach((traceId) => { - // Ignore the zero trace if present. - if (traceId !== ZERO_NAME) { - const traceName = this.traceFormatter!.formatTrace(fromKey(traceId)); - this.traceNameIdMap.set(traceName, traceId); - traceIds.push(traceName); - } + + if (isSingleTrace(dt)) { + this.summaryOptionsField.value!.style.display = 'none'; + return; + } + + const titleObj = getTitle(dt); + let commonTitle = ''; + for (const [key, value] of Object.entries(titleObj)) { + commonTitle += `${key}=${value},`; + } + + // getLegend returns trace Ids which are not common in all the graphs. + // Since it is an object we convert it to the standard key format a=A,b=B, + // so that it could be fed to the traceFormatter. + // + // Also note that some values in the legend object has "-" as value which is + // used to signify a trace not having any values for a particular param. + // We ignore this. + const shortTraceIds: string[] = []; + getLegend(dt).forEach((traceObject: { [key: string]: any }) => { + let shortTraceId = ''; + Object.keys(traceObject).forEach((k) => { + const v = String(traceObject[k]); + shortTraceId += v !== '-' ? `${k}=${v},` : ''; + }); + + let formattedShortTrace = this.traceFormatter!.formatTrace(fromKey(shortTraceId)); + // Since this text is just the uncommon part of trace Id we want to remove + // the label from the formatted trace id so the user doesn't get confused. + formattedShortTrace = formattedShortTrace.replace('Trace ID: ', ''); + shortTraceIds.push(formattedShortTrace); }); - if (this.summaryOptionsField.value) { - this.summaryOptionsField.value!.options = traceIds; - this.summaryOptionsField.value!.label = 'Trace for summary bar'; - } + const displayOptions: string[] = []; + const traceIds = Object.keys(df.traceset); + shortTraceIds.forEach((shortTraceId, i) => { + const traceId = traceIds[i]; + const op = `...${shortTraceId}`; + // These maps are useful to find summary drop down options from the trace key + // and vice versa. The trace key's format is constant across the tool. + // Hence these maps come in handy for that purpose. They're used primarily + // when the summary drop down changes and when a trace is selected from + // the graph. + this.summaryOptionTraceMap.set(op, traceId); + this.traceIdSummaryOptionMap.set(traceId, op); + displayOptions.push(op); + }); + + const formattedTitle = this.traceFormatter!.formatTrace(fromKey(commonTitle)); + this.summaryOptionsField.value!.helperText = formattedTitle; + this.summaryOptionsField.value!.options = displayOptions; + this.summaryOptionsField.value!.label = 'Trace for summary bar'; } private paramsetKeyValueClick(e: CustomEvent<ParamSetSkClickEventDetail>) {
diff --git a/perf/modules/explore-simple-sk/explore-simple-sk_test.ts b/perf/modules/explore-simple-sk/explore-simple-sk_test.ts index cd1323a..d799e26 100644 --- a/perf/modules/explore-simple-sk/explore-simple-sk_test.ts +++ b/perf/modules/explore-simple-sk/explore-simple-sk_test.ts
@@ -23,10 +23,16 @@ updateShortcut, } from './explore-simple-sk'; import { setUpElementUnderTest } from '../../../infra-sk/modules/test_util'; -import { fromKey } from '../paramtools'; +import { generateFullDataFrame } from '../dataframe/test_utils'; +import { PlotGoogleChartSk } from '../plot-google-chart-sk/plot-google-chart-sk'; +import { load } from '@google-web-components/google-chart/loader'; +import { convertFromDataframe } from '../common/plot-builder'; fetchMock.config.overwriteRoutes = true; +const now = 1726081856; // an arbitrary UNIX time; +const timeSpan = 89; // an arbitrary prime number for time span between commits . + describe('calculateRangeChange', () => { const offsets: CommitRange = [100, 120] as CommitRange; @@ -259,46 +265,114 @@ }); describe('traceSelected', () => { - it('should update the summary dropdown value', () => { + it('should update the summary dropdown value', async () => { + const keys = [ + ',benchmark=JetStream2,bot=MacM1,ref_mode=head,subtest=Average,test=Total,v8_mode=pgo,', + ',benchmark=JetStream2,bot=MacM1,ref_mode=ref,subtest=Average,test=Total,v8_mode=default,', + ',benchmark=JetStream2,bot=MacM1,ref_mode=ref,subtest=Normal,test=Total,v8_mode=default,', + ',benchmark=JetStream2,bot=MacM1,ref_mode=ref,subtest=Normal,test=Total,', + ]; + const df = generateFullDataFrame( + { begin: 90, end: 120 }, + now, + keys.length, + [timeSpan], + [Array.from({ length: 4 }, (_, k) => k)], + keys + ); + + setUpElementUnderTest<PlotGoogleChartSk>('plot-google-chart-sk'); + await load(); + const dt = google.visualization.arrayToDataTable(convertFromDataframe(df, 'both')!); const explore = setUpElementUnderTest<ExploreSimpleSk>('explore-simple-sk')(); explore.state.plotSummary = true; explore['tracesRendered'] = true; explore.render(); - const traceIds = [ - ',config=8888,arch=x86,', - ',config=8888,arch=arm,', - ',config=565,arch=x86,', - ',config=565,arch=arm,', - ]; - explore['summaryOptionsField'].value!.options = Object.keys(traceIds); - - const fakePoints = [ - [99, 1], - [100, 2], - [101, 3], - ]; - const header: ColumnHeader[] = []; - fakePoints.forEach((p) => { - header.push({ - offset: CommitNumber(p[0]), - timestamp: TimestampSeconds(p[1]), - }); - }); + explore['addPlotSummaryOptions'](df, dt); + explore.render(); const p: PointSelected = { commit: CommitNumber(100), - name: traceIds[1], + name: keys[1], }; - const ev = selectionToEvent(p, header); + const ev = selectionToEvent(p, df.header); explore.traceSelected(ev); - const expected = explore['traceFormatter']!.formatTrace(fromKey(traceIds[1])); + const expected = '...,ref_mode=ref,subtest=Average,v8_mode=default,'; const actual = explore['summaryOptionsField'].value!.getValue(); assert.equal(actual, expected); }); }); +describe('addSummaryOptions', () => { + it('should hide if single trace', async () => { + const keys = [ + ',benchmark=JetStream2,bot=MacM1,ref_mode=head,subtest=Average,test=Total,v8_mode=pgo,', + ]; + const df = generateFullDataFrame( + { begin: 90, end: 120 }, + now, + keys.length, + [timeSpan], + [null], + keys + ); + + setUpElementUnderTest<PlotGoogleChartSk>('plot-google-chart-sk'); + await load(); + const dt = google.visualization.arrayToDataTable(convertFromDataframe(df, 'both')!); + const explore = setUpElementUnderTest<ExploreSimpleSk>('explore-simple-sk')(); + explore.state.plotSummary = true; + explore['tracesRendered'] = true; + explore.render(); + + explore['addPlotSummaryOptions'](df, dt); + + assert.equal(explore['summaryOptionsField'].value?.style.display, 'none'); + assert.equal(explore['summaryOptionsField'].value?.helperText, ''); + assert.deepEqual(explore['summaryOptionsField'].value?.options, []); + }); + + it('should show short names for multiple traces', async () => { + const keys = [ + ',benchmark=JetStream2,bot=MacM1,ref_mode=head,subtest=Average,test=Total,v8_mode=pgo,', + ',benchmark=JetStream2,bot=MacM1,ref_mode=ref,subtest=Average,test=Total,v8_mode=default,', + ',benchmark=JetStream2,bot=MacM1,ref_mode=ref,subtest=Normal,test=Total,v8_mode=default,', + ',benchmark=JetStream2,bot=MacM1,ref_mode=ref,subtest=Normal,test=Total,', + ]; + const df = generateFullDataFrame( + { begin: 90, end: 120 }, + now, + keys.length, + [timeSpan], + [null], + keys + ); + + setUpElementUnderTest<PlotGoogleChartSk>('plot-google-chart-sk'); + await load(); + const dt = google.visualization.arrayToDataTable(convertFromDataframe(df, 'both')!); + const explore = setUpElementUnderTest<ExploreSimpleSk>('explore-simple-sk')(); + explore.state.plotSummary = true; + explore['tracesRendered'] = true; + explore.render(); + + explore['addPlotSummaryOptions'](df, dt); + + const expectedHelperText = 'Trace ID: ,benchmark=JetStream2,bot=MacM1,test=Total,'; + assert.equal(explore['summaryOptionsField'].value?.helperText, expectedHelperText); + + const expectedOptions = [ + '...,ref_mode=head,subtest=Average,v8_mode=pgo,', + '...,ref_mode=ref,subtest=Average,v8_mode=default,', + '...,ref_mode=ref,subtest=Normal,v8_mode=default,', + '...,ref_mode=ref,subtest=Normal,', + ]; + assert.deepEqual(explore['summaryOptionsField'].value?.options, expectedOptions); + }); +}); + describe('createGraphConfigs', () => { it('traceset without formulas', () => { const traceset = TraceSet({
diff --git a/perf/modules/picker-field-sk/picker-field-sk.scss b/perf/modules/picker-field-sk/picker-field-sk.scss index 055a34b..e7af108 100644 --- a/perf/modules/picker-field-sk/picker-field-sk.scss +++ b/perf/modules/picker-field-sk/picker-field-sk.scss
@@ -22,6 +22,10 @@ transition: color 0.3s ease; color: var(--on-disabled); } + vaadin-combo-box::part(helper-text) { + transition: color 0.3s ease; + color: var(--on-disabled); + } vaadin-combo-box:hover::part(label) { transition: color 0.3s ease; color: var(--on-surface);
diff --git a/perf/modules/picker-field-sk/picker-field-sk.ts b/perf/modules/picker-field-sk/picker-field-sk.ts index 5523799..e70b9bd 100644 --- a/perf/modules/picker-field-sk/picker-field-sk.ts +++ b/perf/modules/picker-field-sk/picker-field-sk.ts
@@ -31,6 +31,8 @@ export class PickerFieldSk extends ElementSk { private _label: string = ''; + private _helper_text: string = ''; + private _options: string[] = []; private _comboBox: HTMLElement | null = null; @@ -44,9 +46,10 @@ <div> <vaadin-combo-box label="${ele.label}" + helper-text="${ele.helperText}" placeholder="${ele.label}" .items=${ele.options} - theme="small" + theme="small helper-above-field" clear-button-visible @value-changed=${ele.onValueChanged} autoselect> @@ -139,6 +142,15 @@ this._label = v; this._render(); } + + get helperText(): string { + return this._helper_text; + } + + set helperText(v: string) { + this._helper_text = v; + this._render(); + } } define('picker-field-sk', PickerFieldSk);