Skip to content

Commit 5464a46

Browse files
esbenaesbena
authored
Merge pull request #3821 from github/tausbn/compare-perf-add-single-run-view-option
compare-perf: Add support for selecting a single run as input
2 parents 1f198bc + b862d14 commit 5464a46

File tree

5 files changed

+122
-35
lines changed

5 files changed

+122
-35
lines changed

‎extensions/ql-vscode/src/common/interface-types.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,7 @@ export interface SetPerformanceComparisonQueries {
403403
readonly t: "setPerformanceComparison";
404404
readonly from: PerformanceComparisonDataFromLog;
405405
readonly to: PerformanceComparisonDataFromLog;
406+
readonly comparison: boolean;
406407
}
407408

408409
export type FromComparePerformanceViewMessage = CommonFromViewMessages;

‎extensions/ql-vscode/src/compare-performance/compare-performance-view.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,10 @@ export class ComparePerformanceView extends AbstractWebview<
6969
}
7070

7171
const [fromPerf, toPerf] = await Promise.all([
72-
scanLogWithProgress(fromJsonLog, "1/2"),
73-
scanLogWithProgress(toJsonLog, "2/2"),
72+
fromJsonLog === ""
73+
? new PerformanceOverviewScanner()
74+
: scanLogWithProgress(fromJsonLog, "1/2"),
75+
scanLogWithProgress(toJsonLog, fromJsonLog === "" ? "1/1" : "2/2"),
7476
]);
7577

7678
// TODO: filter out irrelevant common predicates before transfer?
@@ -79,6 +81,7 @@ export class ComparePerformanceView extends AbstractWebview<
7981
t: "setPerformanceComparison",
8082
from: fromPerf.getData(),
8183
to: toPerf.getData(),
84+
comparison: fromJsonLog !== "",
8285
});
8386
}
8487

‎extensions/ql-vscode/src/extension.ts‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -927,7 +927,7 @@ async function activateWithInstalledDistribution(
927927
): Promise<void> => showResultsForComparison(compareView, from, to),
928928
async (
929929
from: CompletedLocalQueryInfo,
930-
to: CompletedLocalQueryInfo,
930+
to: CompletedLocalQueryInfo | undefined,
931931
): Promise<void> =>
932932
showPerformanceComparison(comparePerformanceView, from, to),
933933
);
@@ -1210,17 +1210,22 @@ async function showResultsForComparison(
12101210
async function showPerformanceComparison(
12111211
view: ComparePerformanceView,
12121212
from: CompletedLocalQueryInfo,
1213-
to: CompletedLocalQueryInfo,
1213+
to: CompletedLocalQueryInfo | undefined,
12141214
): Promise<void> {
1215-
const fromLog = from.evalutorLogPaths?.jsonSummary;
1216-
const toLog = to.evalutorLogPaths?.jsonSummary;
1215+
let fromLog = from.evalutorLogPaths?.jsonSummary;
1216+
let toLog = to?.evalutorLogPaths?.jsonSummary;
1217+
1218+
if (to === undefined) {
1219+
toLog = fromLog;
1220+
fromLog = "";
1221+
}
12171222
if (fromLog === undefined || toLog === undefined) {
12181223
return extLogger.showWarningMessage(
12191224
`Cannot compare performance as the structured logs are missing. Did they queries complete normally?`,
12201225
);
12211226
}
12221227
await extLogger.log(
1223-
`Comparing performance of ${from.getQueryName()} and ${to.getQueryName()}`,
1228+
`Comparing performance of ${from.getQueryName()} and ${to?.getQueryName() ?? "baseline"}`,
12241229
);
12251230

12261231
await view.showResults(fromLog, toLog);

‎extensions/ql-vscode/src/query-history/query-history-manager.ts‎

Lines changed: 61 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ export class QueryHistoryManager extends DisposableObject {
151151
) => Promise<void>,
152152
private readonly doComparePerformanceCallback: (
153153
from: CompletedLocalQueryInfo,
154-
to: CompletedLocalQueryInfo,
154+
to: CompletedLocalQueryInfo | undefined,
155155
) => Promise<void>,
156156
) {
157157
super();
@@ -706,17 +706,18 @@ export class QueryHistoryManager extends DisposableObject {
706706

707707
let toItem: CompletedLocalQueryInfo | undefined = undefined;
708708
try {
709-
toItem = await this.findOtherQueryToCompare(fromItem, multiSelect);
709+
toItem = await this.findOtherQueryToComparePerformance(
710+
fromItem,
711+
multiSelect,
712+
);
710713
} catch (e) {
711714
void showAndLogErrorMessage(
712715
this.app.logger,
713716
`Failed to compare queries: ${getErrorMessage(e)}`,
714717
);
715718
}
716719

717-
if (toItem !== undefined) {
718-
await this.doComparePerformanceCallback(fromItem, toItem);
719-
}
720+
await this.doComparePerformanceCallback(fromItem, toItem);
720721
}
721722

722723
async handleItemClicked(item: QueryHistoryInfo) {
@@ -1116,6 +1117,7 @@ export class QueryHistoryManager extends DisposableObject {
11161117
detail: item.completedQuery.message,
11171118
query: item,
11181119
}));
1120+
11191121
if (comparableQueryLabels.length < 1) {
11201122
throw new Error("No other queries available to compare with.");
11211123
}
@@ -1124,6 +1126,60 @@ export class QueryHistoryManager extends DisposableObject {
11241126
return choice?.query;
11251127
}
11261128

1129+
private async findOtherQueryToComparePerformance(
1130+
fromItem: CompletedLocalQueryInfo,
1131+
allSelectedItems: CompletedLocalQueryInfo[],
1132+
): Promise<CompletedLocalQueryInfo | undefined> {
1133+
const dbName = fromItem.databaseName;
1134+
1135+
// If exactly 2 items are selected, return the one that
1136+
// isn't being used as the "from" item.
1137+
if (allSelectedItems.length === 2) {
1138+
const otherItem =
1139+
fromItem === allSelectedItems[0]
1140+
? allSelectedItems[1]
1141+
: allSelectedItems[0];
1142+
if (otherItem.databaseName !== dbName) {
1143+
throw new Error("Query databases must be the same.");
1144+
}
1145+
return otherItem;
1146+
}
1147+
1148+
if (allSelectedItems.length > 2) {
1149+
throw new Error("Please select no more than 2 queries.");
1150+
}
1151+
1152+
// Otherwise, present a dialog so the user can choose the item they want to use.
1153+
const comparableQueryLabels = this.treeDataProvider.allHistory
1154+
.filter(this.isSuccessfulCompletedLocalQueryInfo)
1155+
.filter(
1156+
(otherItem) =>
1157+
otherItem !== fromItem && otherItem.databaseName === dbName,
1158+
)
1159+
.map((item) => ({
1160+
label: this.labelProvider.getLabel(item),
1161+
description: item.databaseName,
1162+
detail: item.completedQuery.message,
1163+
query: item,
1164+
}));
1165+
const comparableQueryLabelsWithDefault = [
1166+
{
1167+
label: "Single run",
1168+
description:
1169+
"Look at the performance of this run, compared to a trivial baseline",
1170+
detail: undefined,
1171+
query: undefined,
1172+
},
1173+
...comparableQueryLabels,
1174+
];
1175+
if (comparableQueryLabelsWithDefault.length < 1) {
1176+
throw new Error("No other queries available to compare with.");
1177+
}
1178+
const choice = await window.showQuickPick(comparableQueryLabelsWithDefault);
1179+
1180+
return choice?.query;
1181+
}
1182+
11271183
/**
11281184
* Updates the compare with source query. This ensures that all compare command invocations
11291185
* when exactly 2 queries are selected always have the proper _from_ query. Always use

‎extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx‎

Lines changed: 45 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -216,14 +216,15 @@ const Dropdown = styled.select``;
216216
interface PipelineStepProps {
217217
before: number | undefined;
218218
after: number | undefined;
219+
comparison: boolean;
219220
step: React.ReactNode;
220221
}
221222

222223
/**
223224
* Row with details of a pipeline step, or one of the high-level stats appearing above the pipelines (evaluation/iteration counts).
224225
*/
225226
function PipelineStep(props: PipelineStepProps) {
226-
let { before, after, step } = props;
227+
let { before, after, comparison, step } = props;
227228
if (before != null && before < 0) {
228229
before = undefined;
229230
}
@@ -234,9 +235,11 @@ function PipelineStep(props: PipelineStepProps) {
234235
return (
235236
<PipelineStepTR>
236237
<ChevronCell />
237-
<NumberCell>{before != null ? formatDecimal(before) : ""}</NumberCell>
238+
{comparison && (
239+
<NumberCell>{before != null ? formatDecimal(before) : ""}</NumberCell>
240+
)}
238241
<NumberCell>{after != null ? formatDecimal(after) : ""}</NumberCell>
239-
{delta != null ? renderDelta(delta) : <td></td>}
242+
{comparison && (delta != null ? renderDelta(delta) : <td></td>)}
240243
<NameCell>{step}</NameCell>
241244
</PipelineStepTR>
242245
);
@@ -249,10 +252,11 @@ const HeaderTR = styled.tr`
249252
interface HighLevelStatsProps {
250253
before: PredicateInfo;
251254
after: PredicateInfo;
255+
comparison: boolean;
252256
}
253257

254258
function HighLevelStats(props: HighLevelStatsProps) {
255-
const { before, after } = props;
259+
const { before, after, comparison } = props;
256260
const hasBefore = before.absentReason !== AbsentReason.NotSeen;
257261
const hasAfter = after.absentReason !== AbsentReason.NotSeen;
258262
const showEvaluationCount =
@@ -261,21 +265,25 @@ function HighLevelStats(props: HighLevelStatsProps) {
261265
<>
262266
<HeaderTR>
263267
<ChevronCell></ChevronCell>
264-
<NumberHeader>{hasBefore ? "Before" : ""}</NumberHeader>
268+
{comparison && <NumberHeader>{hasBefore ? "Before" : ""}</NumberHeader>}
265269
<NumberHeader>{hasAfter ? "After" : ""}</NumberHeader>
266-
<NumberHeader>{hasBefore && hasAfter ? "Delta" : ""}</NumberHeader>
270+
{comparison && (
271+
<NumberHeader>{hasBefore && hasAfter ? "Delta" : ""}</NumberHeader>
272+
)}
267273
<NameHeader>Stats</NameHeader>
268274
</HeaderTR>
269275
{showEvaluationCount && (
270276
<PipelineStep
271277
before={before.evaluationCount || undefined}
272278
after={after.evaluationCount || undefined}
279+
comparison={comparison}
273280
step="Number of evaluations"
274281
/>
275282
)}
276283
<PipelineStep
277284
before={before.iterationCount / before.evaluationCount || undefined}
278285
after={after.iterationCount / after.evaluationCount || undefined}
286+
comparison={comparison}
279287
step={
280288
showEvaluationCount
281289
? "Number of iterations per evaluation"
@@ -377,6 +385,8 @@ function ComparePerformanceWithData(props: {
377385
[data],
378386
);
379387

388+
const comparison = data?.comparison;
389+
380390
const [expandedPredicates, setExpandedPredicates] = useState<Set<string>>(
381391
() => new Set<string>(),
382392
);
@@ -478,9 +488,9 @@ function ComparePerformanceWithData(props: {
478488
<thead>
479489
<HeaderTR>
480490
<ChevronCell />
481-
<NumberHeader>Before</NumberHeader>
482-
<NumberHeader>After</NumberHeader>
483-
<NumberHeader>Delta</NumberHeader>
491+
{comparison && <NumberHeader>Before</NumberHeader>}
492+
<NumberHeader>{comparison ? "After" : "Value"}</NumberHeader>
493+
{comparison && <NumberHeader>Delta</NumberHeader>}
484494
<NameHeader>Predicate</NameHeader>
485495
</HeaderTR>
486496
</thead>
@@ -503,33 +513,42 @@ function ComparePerformanceWithData(props: {
503513
<ChevronCell>
504514
<Chevron expanded={expandedPredicates.has(row.name)} />
505515
</ChevronCell>
506-
{renderAbsoluteValue(row.before, metric)}
516+
{comparison && renderAbsoluteValue(row.before, metric)}
507517
{renderAbsoluteValue(row.after, metric)}
508-
{renderDelta(row.diff, metric.unit)}
518+
{comparison && renderDelta(row.diff, metric.unit)}
509519
<NameCell>{rowNames[rowIndex]}</NameCell>
510520
</PredicateTR>
511521
{expandedPredicates.has(row.name) && (
512522
<>
513-
<HighLevelStats before={row.before} after={row.after} />
523+
<HighLevelStats
524+
before={row.before}
525+
after={row.after}
526+
comparison={comparison}
527+
/>
514528
{collatePipelines(
515529
row.before.pipelines,
516530
row.after.pipelines,
517531
).map(({ name, first, second }, pipelineIndex) => (
518532
<Fragment key={pipelineIndex}>
519533
<HeaderTR>
520534
<td></td>
521-
<NumberHeader>{first != null && "Before"}</NumberHeader>
535+
{comparison && (
536+
<NumberHeader>{first != null && "Before"}</NumberHeader>
537+
)}
522538
<NumberHeader>{second != null && "After"}</NumberHeader>
523-
<NumberHeader>
524-
{first != null && second != null && "Delta"}
525-
</NumberHeader>
539+
{comparison && (
540+
<NumberHeader>
541+
{first != null && second != null && "Delta"}
542+
</NumberHeader>
543+
)}
526544
<NameHeader>
527545
Tuple counts for &apos;{name}&apos; pipeline
528-
{first == null
529-
? " (after)"
530-
: second == null
531-
? " (before)"
532-
: ""}
546+
{comparison &&
547+
(first == null
548+
? " (after)"
549+
: second == null
550+
? " (before)"
551+
: "")}
533552
</NameHeader>
534553
</HeaderTR>
535554
{abbreviateRASteps(first?.steps ?? second!.steps).map(
@@ -538,6 +557,7 @@ function ComparePerformanceWithData(props: {
538557
key={index}
539558
before={first?.counts[index]}
540559
after={second?.counts[index]}
560+
comparison={comparison}
541561
step={step}
542562
/>
543563
),
@@ -556,9 +576,11 @@ function ComparePerformanceWithData(props: {
556576
</tr>
557577
<tr key="total">
558578
<ChevronCell />
559-
<NumberCell>{formatDecimal(totalBefore)}</NumberCell>
579+
{comparison && (
580+
<NumberCell>{formatDecimal(totalBefore)}</NumberCell>
581+
)}
560582
<NumberCell>{formatDecimal(totalAfter)}</NumberCell>
561-
{renderDelta(totalDiff)}
583+
{comparison && renderDelta(totalDiff)}
562584
<NameCell>TOTAL</NameCell>
563585
</tr>
564586
</tfoot>

0 commit comments

Comments
 (0)