From 3b0697771d9c33e4f1a941778aad0f890a419fae Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 26 Jun 2024 15:18:58 +0200 Subject: [PATCH 01/46] Add Compare Performance command (WIP) --- extensions/ql-vscode/package.json | 13 + extensions/ql-vscode/src/common/commands.ts | 1 + .../ql-vscode/src/common/interface-types.ts | 11 + .../src/common/vscode/abstract-webview.ts | 7 + .../src/common/vscode/webview-html.ts | 1 + .../compare-performance-view.ts | 93 ++++ extensions/ql-vscode/src/extension.ts | 34 ++ .../ql-vscode/src/log-insights/log-scanner.ts | 17 + .../ql-vscode/src/log-insights/log-summary.ts | 2 + .../log-insights/performance-comparison.ts | 177 +++++++ .../query-history/query-history-manager.ts | 40 ++ .../ql-vscode/src/view/common/WarningBox.tsx | 31 ++ extensions/ql-vscode/src/view/common/index.ts | 1 + .../ComparePerformance.tsx | 444 ++++++++++++++++++ .../compare-performance/RAPrettyPrinter.tsx | 151 ++++++ .../src/view/compare-performance/index.tsx | 8 + extensions/ql-vscode/src/view/webview.tsx | 2 + .../history-tree-data-provider.test.ts | 2 + .../query-history-manager.test.ts | 2 + .../variant-analysis-history.test.ts | 1 + 20 files changed, 1038 insertions(+) create mode 100644 extensions/ql-vscode/src/compare-performance/compare-performance-view.ts create mode 100644 extensions/ql-vscode/src/log-insights/performance-comparison.ts create mode 100644 extensions/ql-vscode/src/view/common/WarningBox.tsx create mode 100644 extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx create mode 100644 extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx create mode 100644 extensions/ql-vscode/src/view/compare-performance/index.tsx diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index 0f01bf4a5be..bb0bee713ac 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -959,6 +959,10 @@ "command": "codeQLQueryHistory.compareWith", "title": "Compare Results" }, + { + "command": "codeQLQueryHistory.comparePerformanceWith", + "title": "Compare Performance" + }, { "command": "codeQLQueryHistory.openOnGithub", "title": "View Logs" @@ -1230,6 +1234,11 @@ "group": "3_queryHistory@0", "when": "viewItem == rawResultsItem || viewItem == interpretedResultsItem" }, + { + "command": "codeQLQueryHistory.comparePerformanceWith", + "group": "3_queryHistory@1", + "when": "viewItem == rawResultsItem || viewItem == interpretedResultsItem" + }, { "command": "codeQLQueryHistory.showQueryLog", "group": "4_queryHistory@4", @@ -1733,6 +1742,10 @@ "command": "codeQLQueryHistory.compareWith", "when": "false" }, + { + "command": "codeQLQueryHistory.comparePerformanceWith", + "when": "false" + }, { "command": "codeQLQueryHistory.sortByName", "when": "false" diff --git a/extensions/ql-vscode/src/common/commands.ts b/extensions/ql-vscode/src/common/commands.ts index 302ca6fe0a9..2fd8a1995d4 100644 --- a/extensions/ql-vscode/src/common/commands.ts +++ b/extensions/ql-vscode/src/common/commands.ts @@ -180,6 +180,7 @@ export type QueryHistoryCommands = { "codeQLQueryHistory.removeHistoryItemContextInline": TreeViewContextMultiSelectionCommandFunction; "codeQLQueryHistory.renameItem": TreeViewContextMultiSelectionCommandFunction; "codeQLQueryHistory.compareWith": TreeViewContextMultiSelectionCommandFunction; + "codeQLQueryHistory.comparePerformanceWith": TreeViewContextMultiSelectionCommandFunction; "codeQLQueryHistory.showEvalLog": TreeViewContextMultiSelectionCommandFunction; "codeQLQueryHistory.showEvalLogSummary": TreeViewContextMultiSelectionCommandFunction; "codeQLQueryHistory.showEvalLogViewer": TreeViewContextMultiSelectionCommandFunction; diff --git a/extensions/ql-vscode/src/common/interface-types.ts b/extensions/ql-vscode/src/common/interface-types.ts index 81ef4e612e6..8d0681cbcd9 100644 --- a/extensions/ql-vscode/src/common/interface-types.ts +++ b/extensions/ql-vscode/src/common/interface-types.ts @@ -27,6 +27,7 @@ import type { } from "./raw-result-types"; import type { AccessPathSuggestionOptions } from "../model-editor/suggestions"; import type { ModelEvaluationRunState } from "../model-editor/shared/model-evaluation-run-state"; +import type { PerformanceComparisonDataFromLog } from "../log-insights/performance-comparison"; /** * This module contains types and code that are shared between @@ -396,6 +397,16 @@ export interface SetComparisonsMessage { readonly message: string | undefined; } +export type ToComparePerformanceViewMessage = SetPerformanceComparisonQueries; + +export interface SetPerformanceComparisonQueries { + readonly t: "setPerformanceComparison"; + readonly from: PerformanceComparisonDataFromLog; + readonly to: PerformanceComparisonDataFromLog; +} + +export type FromComparePerformanceViewMessage = CommonFromViewMessages; + export type QueryCompareResult = | RawQueryCompareResult | InterpretedQueryCompareResult; diff --git a/extensions/ql-vscode/src/common/vscode/abstract-webview.ts b/extensions/ql-vscode/src/common/vscode/abstract-webview.ts index c38590e4feb..87c0583af1d 100644 --- a/extensions/ql-vscode/src/common/vscode/abstract-webview.ts +++ b/extensions/ql-vscode/src/common/vscode/abstract-webview.ts @@ -41,6 +41,13 @@ export abstract class AbstractWebview< constructor(protected readonly app: App) {} + public hidePanel() { + if (this.panel !== undefined) { + this.panel.dispose(); + this.panel = undefined; + } + } + public async restoreView(panel: WebviewPanel): Promise { this.panel = panel; const config = await this.getPanelConfig(); diff --git a/extensions/ql-vscode/src/common/vscode/webview-html.ts b/extensions/ql-vscode/src/common/vscode/webview-html.ts index 7ad1f5d08e4..9a02714f726 100644 --- a/extensions/ql-vscode/src/common/vscode/webview-html.ts +++ b/extensions/ql-vscode/src/common/vscode/webview-html.ts @@ -7,6 +7,7 @@ import type { App } from "../app"; export type WebviewKind = | "results" | "compare" + | "compare-performance" | "variant-analysis" | "data-flow-paths" | "model-editor" diff --git a/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts b/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts new file mode 100644 index 00000000000..28925ba1afd --- /dev/null +++ b/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts @@ -0,0 +1,93 @@ +import { ViewColumn } from "vscode"; + +import type { App } from "../common/app"; +import { redactableError } from "../common/errors"; +import type { + FromComparePerformanceViewMessage, + ToComparePerformanceViewMessage, +} from "../common/interface-types"; +import type { Logger } from "../common/logging"; +import { showAndLogExceptionWithTelemetry } from "../common/logging"; +import { extLogger } from "../common/logging/vscode"; +import type { WebviewPanelConfig } from "../common/vscode/abstract-webview"; +import { AbstractWebview } from "../common/vscode/abstract-webview"; +import { telemetryListener } from "../common/vscode/telemetry"; +import type { HistoryItemLabelProvider } from "../query-history/history-item-label-provider"; +import { PerformanceOverviewScanner } from "../log-insights/performance-comparison"; +import { scanLog } from "../log-insights/log-scanner"; +import type { ResultsView } from "../local-queries"; + +export class ComparePerformanceView extends AbstractWebview< + ToComparePerformanceViewMessage, + FromComparePerformanceViewMessage +> { + constructor( + app: App, + public logger: Logger, + public labelProvider: HistoryItemLabelProvider, + private resultsView: ResultsView, + ) { + super(app); + } + + async showResults(fromJsonLog: string, toJsonLog: string) { + const panel = await this.getPanel(); + panel.reveal(undefined, false); + + // Close the results viewer as it will have opened when the user clicked the query in the history view + // (which they must do as part of the UI interaction for opening the performance view). + // The performance view generally needs a lot of width so it's annoying to have the result viewer open. + this.resultsView.hidePanel(); + + await this.waitForPanelLoaded(); + + // TODO: try processing in (async) parallel once readJsonl is streaming + const fromPerf = await scanLog( + fromJsonLog, + new PerformanceOverviewScanner(), + ); + const toPerf = await scanLog(toJsonLog, new PerformanceOverviewScanner()); + + await this.postMessage({ + t: "setPerformanceComparison", + from: fromPerf.getData(), + to: toPerf.getData(), + }); + } + + protected getPanelConfig(): WebviewPanelConfig { + return { + viewId: "comparePerformanceView", + title: "Compare CodeQL Performance", + viewColumn: ViewColumn.Active, + preserveFocus: true, + view: "compare-performance", + }; + } + + protected onPanelDispose(): void {} + + protected async onMessage( + msg: FromComparePerformanceViewMessage, + ): Promise { + switch (msg.t) { + case "viewLoaded": + this.onWebViewLoaded(); + break; + + case "telemetry": + telemetryListener?.sendUIInteraction(msg.action); + break; + + case "unhandledError": + void showAndLogExceptionWithTelemetry( + extLogger, + telemetryListener, + redactableError( + msg.error, + )`Unhandled error in performance comparison view: ${msg.error.message}`, + ); + break; + } + } +} diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index fde7cbec42a..979ee7b7e8d 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -135,6 +135,7 @@ import { LanguageContextStore } from "./language-context-store"; import { LanguageSelectionPanel } from "./language-selection-panel/language-selection-panel"; import { GitHubDatabasesModule } from "./databases/github-databases"; import { DatabaseFetcher } from "./databases/database-fetcher"; +import { ComparePerformanceView } from "./compare-performance/compare-performance-view"; /** * extension.ts @@ -924,6 +925,11 @@ async function activateWithInstalledDistribution( from: CompletedLocalQueryInfo, to: CompletedLocalQueryInfo, ): Promise => showResultsForComparison(compareView, from, to), + async ( + from: CompletedLocalQueryInfo, + to: CompletedLocalQueryInfo, + ): Promise => + showPerformanceComparison(comparePerformanceView, from, to), ); ctx.subscriptions.push(qhm); @@ -949,6 +955,15 @@ async function activateWithInstalledDistribution( ); ctx.subscriptions.push(compareView); + void extLogger.log("Initializing performance comparison view."); + const comparePerformanceView = new ComparePerformanceView( + app, + queryServerLogger, + labelProvider, + localQueryResultsView, + ); + ctx.subscriptions.push(comparePerformanceView); + void extLogger.log("Initializing source archive filesystem provider."); archiveFilesystemProvider_activate(ctx, dbm); @@ -1190,6 +1205,25 @@ async function showResultsForComparison( } } +async function showPerformanceComparison( + view: ComparePerformanceView, + from: CompletedLocalQueryInfo, + to: CompletedLocalQueryInfo, +): Promise { + const fromLog = from.evaluatorLogPaths?.jsonSummary; + const toLog = to.evaluatorLogPaths?.jsonSummary; + if (fromLog === undefined || toLog === undefined) { + return extLogger.showWarningMessage( + `Cannot compare performance as the structured logs are missing. Did they queries complete normally?`, + ); + } + await extLogger.log( + `Comparing performance of ${from.getQueryName()} and ${to.getQueryName()}`, + ); + + await view.showResults(fromLog, toLog); +} + function addUnhandledRejectionListener() { const handler = (error: unknown) => { // This listener will be triggered for errors from other extensions as diff --git a/extensions/ql-vscode/src/log-insights/log-scanner.ts b/extensions/ql-vscode/src/log-insights/log-scanner.ts index 7a8efcd5605..9324a5994f1 100644 --- a/extensions/ql-vscode/src/log-insights/log-scanner.ts +++ b/extensions/ql-vscode/src/log-insights/log-scanner.ts @@ -112,3 +112,20 @@ export class EvaluationLogScannerSet { scanners.forEach((scanner) => scanner.onDone()); } } + +/** + * Scan the evaluator summary log using the given scanner. For conveience, returns the scanner. + * + * @param jsonSummaryLocation The file path of the JSON summary log. + * @param scanner The scanner to process events from the log + */ +export async function scanLog( + jsonSummaryLocation: string, + scanner: T, +): Promise { + await readJsonlFile(jsonSummaryLocation, async (obj) => { + scanner.onEvent(obj); + }); + scanner.onDone(); + return scanner; +} diff --git a/extensions/ql-vscode/src/log-insights/log-summary.ts b/extensions/ql-vscode/src/log-insights/log-summary.ts index 50435d51e1e..5fa4bda58b8 100644 --- a/extensions/ql-vscode/src/log-insights/log-summary.ts +++ b/extensions/ql-vscode/src/log-insights/log-summary.ts @@ -33,6 +33,7 @@ interface ResultEventBase extends SummaryEventBase { export interface ComputeSimple extends ResultEventBase { evaluationStrategy: "COMPUTE_SIMPLE"; ra: Ra; + millis: number; pipelineRuns?: [PipelineRun]; queryCausingWork?: string; dependencies: { [key: string]: string }; @@ -42,6 +43,7 @@ export interface ComputeRecursive extends ResultEventBase { evaluationStrategy: "COMPUTE_RECURSIVE"; deltaSizes: number[]; ra: Ra; + millis: number; pipelineRuns: PipelineRun[]; queryCausingWork?: string; dependencies: { [key: string]: string }; diff --git a/extensions/ql-vscode/src/log-insights/performance-comparison.ts b/extensions/ql-vscode/src/log-insights/performance-comparison.ts new file mode 100644 index 00000000000..14f4d343b8a --- /dev/null +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -0,0 +1,177 @@ +import type { EvaluationLogScanner } from "./log-scanner"; +import type { SummaryEvent } from "./log-summary"; + +export interface PipelineSummary { + steps: string[]; + /** Total counts for each step in the RA array, across all iterations */ + counts: number[]; +} + +/** + * Data extracted from a log for the purpose of doing a performance comparison. + * + * Memory compactness is important since we keep this data in memory; once for + * each side of the comparison. + * + * This object must be able to survive a `postMessage` transfer from the extension host + * to a web view (which rules out `Map` values, for example). + */ +export interface PerformanceComparisonDataFromLog { + /** Names of predicates mentioned in the log */ + names: string[]; + + /** Number of milliseconds spent evaluating the `i`th predicate from the `names` array. */ + timeCosts: number[]; + + /** Number of tuples seen in pipelines evaluating the `i`th predicate from the `names` array. */ + tupleCosts: number[]; + + /** Number of iterations seen when evaluating the `i`th predicate from the `names` array. */ + iterationCounts: number[]; + + /** Number of executions of pipelines evaluating the `i`th predicate from the `names` array. */ + evaluationCounts: number[]; + + /** + * List of indices into the `names` array for which we have seen a cache hit. + * + * TODO: only count cache hits prior to first evaluation? + */ + cacheHitIndices: number[]; + + /** + * List of indices into the `names` array where the predicate was deemed empty due to a sentinel check. + */ + sentinelEmptyIndices: number[]; + + /** + * All the pipeline runs seen for the `i`th predicate from the `names` array. + * + * TODO: replace with more compact representation + */ + pipelineSummaryList: Array>; +} + +export class PerformanceOverviewScanner implements EvaluationLogScanner { + private readonly nameToIndex = new Map(); + private readonly data: PerformanceComparisonDataFromLog = { + names: [], + timeCosts: [], + tupleCosts: [], + cacheHitIndices: [], + sentinelEmptyIndices: [], + pipelineSummaryList: [], + evaluationCounts: [], + iterationCounts: [], + }; + + private getPredicateIndex(name: string): number { + const { nameToIndex } = this; + let index = nameToIndex.get(name); + if (index === undefined) { + index = nameToIndex.size; + nameToIndex.set(name, index); + const { + names, + timeCosts, + tupleCosts, + iterationCounts, + evaluationCounts, + pipelineSummaryList, + } = this.data; + names.push(name); + timeCosts.push(0); + tupleCosts.push(0); + iterationCounts.push(0); + evaluationCounts.push(0); + pipelineSummaryList.push({}); + } + return index; + } + + getData(): PerformanceComparisonDataFromLog { + return this.data; + } + + onEvent(event: SummaryEvent): void { + if ( + event.completionType !== undefined && + event.completionType !== "SUCCESS" + ) { + return; // Skip any evaluation that wasn't successful + } + + switch (event.evaluationStrategy) { + case "EXTENSIONAL": + case "COMPUTED_EXTENSIONAL": { + break; + } + case "CACHE_HIT": + case "CACHACA": { + this.data.cacheHitIndices.push( + this.getPredicateIndex(event.predicateName), + ); + break; + } + case "SENTINEL_EMPTY": { + this.data.sentinelEmptyIndices.push( + this.getPredicateIndex(event.predicateName), + ); + break; + } + case "COMPUTE_RECURSIVE": + case "COMPUTE_SIMPLE": + case "IN_LAYER": { + const index = this.getPredicateIndex(event.predicateName); + let totalTime = 0; + let totalTuples = 0; + if (event.evaluationStrategy !== "IN_LAYER") { + totalTime += event.millis; + } else { + // IN_LAYER events do no record of their total time. + // Make a best-effort estimate by adding up the positive iteration times (they can be negative). + for (const millis of event.predicateIterationMillis ?? []) { + if (millis > 0) { + totalTime += millis; + } + } + } + const { + timeCosts, + tupleCosts, + iterationCounts, + evaluationCounts, + pipelineSummaryList, + } = this.data; + const pipelineSummaries = pipelineSummaryList[index]; + for (const { counts, raReference } of event.pipelineRuns ?? []) { + // Get or create the pipeline summary for this RA + const pipelineSummary = (pipelineSummaries[raReference] ??= { + steps: event.ra[raReference], + counts: counts.map(() => 0), + }); + const { counts: totalTuplesPerStep } = pipelineSummary; + for (let i = 0, length = counts.length; i < length; ++i) { + // TODO: possibly exclude unions here + const count = counts[i]; + if (count < 0) { + // Empty RA lines have a tuple count of -1. Do not count them when aggregating. + // But retain the fact that this step had a negative count for rendering purposes. + totalTuplesPerStep[i] = count; + continue; + } + totalTuples += count; + totalTuplesPerStep[i] += count; + } + } + timeCosts[index] += totalTime; + tupleCosts[index] += totalTuples; + iterationCounts[index] += event.pipelineRuns?.length ?? 0; + evaluationCounts[index] += 1; + break; + } + } + } + + onDone(): void {} +} diff --git a/extensions/ql-vscode/src/query-history/query-history-manager.ts b/extensions/ql-vscode/src/query-history/query-history-manager.ts index 45d37e7eba9..0af42ba73d7 100644 --- a/extensions/ql-vscode/src/query-history/query-history-manager.ts +++ b/extensions/ql-vscode/src/query-history/query-history-manager.ts @@ -149,6 +149,10 @@ export class QueryHistoryManager extends DisposableObject { from: CompletedLocalQueryInfo, to: CompletedLocalQueryInfo, ) => Promise, + private readonly doComparePerformanceCallback: ( + from: CompletedLocalQueryInfo, + to: CompletedLocalQueryInfo, + ) => Promise, ) { super(); @@ -263,6 +267,8 @@ export class QueryHistoryManager extends DisposableObject { "query", ), "codeQLQueryHistory.compareWith": this.handleCompareWith.bind(this), + "codeQLQueryHistory.comparePerformanceWith": + this.handleComparePerformanceWith.bind(this), "codeQLQueryHistory.showEvalLog": createSingleSelectionCommand( this.app.logger, this.handleShowEvalLog.bind(this), @@ -679,6 +685,40 @@ export class QueryHistoryManager extends DisposableObject { } } + async handleComparePerformanceWith( + singleItem: QueryHistoryInfo, + multiSelect: QueryHistoryInfo[] | undefined, + ) { + // TODO: reduce duplication with 'handleCompareWith' + multiSelect ||= [singleItem]; + + if ( + !this.isSuccessfulCompletedLocalQueryInfo(singleItem) || + !multiSelect.every(this.isSuccessfulCompletedLocalQueryInfo) + ) { + // TODO: support performance comparison with partially-evaluated query (technically possible) + throw new Error( + "Please only select local queries that have completed successfully.", + ); + } + + const fromItem = this.getFromQueryToCompare(singleItem, multiSelect); + + let toItem: CompletedLocalQueryInfo | undefined = undefined; + try { + toItem = await this.findOtherQueryToCompare(fromItem, multiSelect); + } catch (e) { + void showAndLogErrorMessage( + this.app.logger, + `Failed to compare queries: ${getErrorMessage(e)}`, + ); + } + + if (toItem !== undefined) { + await this.doComparePerformanceCallback(fromItem, toItem); + } + } + async handleItemClicked(item: QueryHistoryInfo) { this.treeDataProvider.setCurrentItem(item); diff --git a/extensions/ql-vscode/src/view/common/WarningBox.tsx b/extensions/ql-vscode/src/view/common/WarningBox.tsx new file mode 100644 index 00000000000..b8003d83e57 --- /dev/null +++ b/extensions/ql-vscode/src/view/common/WarningBox.tsx @@ -0,0 +1,31 @@ +import { styled } from "styled-components"; +import { WarningIcon } from "./icon/WarningIcon"; + +const WarningBoxDiv = styled.div` + max-width: 100em; + padding: 0.5em 1em; + border: 1px solid var(--vscode-widget-border); + box-shadow: var(--vscode-widget-shadow) 0px 3px 8px; + display: flex; +`; + +const IconPane = styled.p` + width: 3em; + flex-shrink: 0; + text-align: center; +`; + +export interface WarningBoxProps { + children: React.ReactNode; +} + +export function WarningBox(props: WarningBoxProps) { + return ( + + + + +

{props.children}

+
+ ); +} diff --git a/extensions/ql-vscode/src/view/common/index.ts b/extensions/ql-vscode/src/view/common/index.ts index 7d3564bed43..fe1f4a6ec63 100644 --- a/extensions/ql-vscode/src/view/common/index.ts +++ b/extensions/ql-vscode/src/view/common/index.ts @@ -6,3 +6,4 @@ export * from "./HorizontalSpace"; export * from "./SectionTitle"; export * from "./VerticalSpace"; export * from "./ViewTitle"; +export * from "./WarningBox"; diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx new file mode 100644 index 00000000000..036585432c6 --- /dev/null +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -0,0 +1,444 @@ +import { useMemo, useState, Fragment } from "react"; +import type { + SetPerformanceComparisonQueries, + ToComparePerformanceViewMessage, +} from "../../common/interface-types"; +import { useMessageFromExtension } from "../common/useMessageFromExtension"; +import type { + PerformanceComparisonDataFromLog, + PipelineSummary, +} from "../../log-insights/performance-comparison"; +import { formatDecimal } from "../../common/number"; +import { styled } from "styled-components"; +import { Codicon, ViewTitle, WarningBox } from "../common"; +import { abbreviateRASteps } from "./RAPrettyPrinter"; + +const enum AbsentReason { + NotSeen = "NotSeen", + CacheHit = "CacheHit", + Sentinel = "Sentinel", +} + +interface OptionalValue { + absentReason: AbsentReason | undefined; + tuples: number; +} + +interface PredicateInfo extends OptionalValue { + pipelines: Record; +} + +class ComparisonDataset { + public nameToIndex = new Map(); + public cacheHitIndices: Set; + public sentinelEmptyIndices: Set; + + constructor(public data: PerformanceComparisonDataFromLog) { + const { names } = data; + const { nameToIndex } = this; + for (let i = 0; i < names.length; i++) { + nameToIndex.set(names[i], i); + } + this.cacheHitIndices = new Set(data.cacheHitIndices); + this.sentinelEmptyIndices = new Set(data.sentinelEmptyIndices); + } + + getTupleCountInfo(name: string): PredicateInfo { + const { data, nameToIndex, cacheHitIndices, sentinelEmptyIndices } = this; + const index = nameToIndex.get(name); + if (index == null) { + return { + tuples: 0, + absentReason: AbsentReason.NotSeen, + pipelines: {}, + }; + } + const tupleCost = data.tupleCosts[index]; + let absentReason: AbsentReason | undefined; + if (tupleCost === 0) { + if (sentinelEmptyIndices.has(index)) { + absentReason = AbsentReason.Sentinel; + } else if (cacheHitIndices.has(index)) { + absentReason = AbsentReason.CacheHit; + } + } + return { + tuples: tupleCost, + absentReason, + pipelines: data.pipelineSummaryList[index], + }; + } +} + +function renderAbsoluteValue(x: OptionalValue) { + switch (x.absentReason) { + case AbsentReason.NotSeen: + return n/a; + case AbsentReason.CacheHit: + return cache hit; + case AbsentReason.Sentinel: + return sentinel empty; + default: + return {formatDecimal(x.tuples)}; + } +} + +function renderDelta(x: number) { + const sign = x > 0 ? "+" : ""; + return ( + + {sign} + {formatDecimal(x)} + + ); +} + +function orderBy(fn: (x: T) => number | string) { + return (x: T, y: T) => { + const fx = fn(x); + const fy = fn(y); + return fx === fy ? 0 : fx < fy ? -1 : 1; + }; +} + +const ChevronCell = styled.td` + width: 1em !important; +`; + +const NameHeader = styled.th` + text-align: left; +`; + +const NumberHeader = styled.th` + text-align: right; + width: 10em !important; +`; + +const NameCell = styled.td``; + +const NumberCell = styled.td` + text-align: right; + width: 10em !important; +`; + +const AbsentNumberCell = styled.td` + text-align: right; + color: var(--vscode-disabledForeground); + + tr.expanded & { + color: inherit; + } + width: 10em !important; +`; + +const Table = styled.table` + border-collapse: collapse; + width: 100%; + border-spacing: 0; + background-color: var(--vscode-background); + color: var(--vscode-foreground); + & td { + padding: 0.5em; + } + & th { + padding: 0.5em; + } + &.expanded { + border: 1px solid var(--vscode-list-activeSelectionBackground); + margin-bottom: 1em; + } +`; + +const PredicateTR = styled.tr` + cursor: pointer; + + &.expanded { + background-color: var(--vscode-list-activeSelectionBackground); + color: var(--vscode-list-activeSelectionForeground); + position: sticky; + top: 0; + } + + & .codicon-chevron-right { + visibility: hidden; + } + + &:hover:not(.expanded) { + background-color: var(--vscode-list-hoverBackground); + & .codicon-chevron-right { + visibility: visible; + } + } +`; + +const PipelineStepTR = styled.tr` + & td { + padding-top: 0.3em; + padding-bottom: 0.3em; + } +`; + +interface PipelineStepProps { + before: number | undefined; + after: number | undefined; + step: string; +} + +function PipelineStep(props: PipelineStepProps) { + let { before, after, step } = props; + if (before != null && before < 0) { + before = undefined; + } + if (after != null && after < 0) { + after = undefined; + } + const delta = before != null && after != null ? after - before : undefined; + return ( + + + {before != null ? formatDecimal(before) : ""} + {after != null ? formatDecimal(after) : ""} + {delta != null ? renderDelta(delta) : } + {step} + + ); +} + +function Chevron({ expanded }: { expanded: boolean }) { + return ; +} + +function withToggledValue(set: Set, value: T) { + const result = new Set(set); + if (result.has(value)) { + result.delete(value); + } else { + result.add(value); + } + return result; +} + +export function ComparePerformance(_: Record) { + const [data, setData] = useState< + SetPerformanceComparisonQueries | undefined + >(); + + useMessageFromExtension( + (msg) => { + setData(msg); + }, + [setData], + ); + + const datasets = useMemo( + () => + data == null + ? undefined + : { + from: new ComparisonDataset(data.from), + to: new ComparisonDataset(data.to), + }, + [data], + ); + + const [expandedPredicates, setExpandedPredicates] = useState>( + () => new Set(), + ); + + const [hideCacheHits, setHideCacheHits] = useState(false); + + if (!datasets) { + return
Loading performance comparison...
; + } + + const { from, to } = datasets; + + const nameSet = new Set(from.data.names); + for (const name of to.data.names) { + nameSet.add(name); + } + + let hasCacheHitMismatch = false; + + const rows = Array.from(nameSet) + .map((name) => { + const before = from.getTupleCountInfo(name); + const after = to.getTupleCountInfo(name); + if (before.tuples === after.tuples) { + return undefined!; + } + if ( + before.absentReason === AbsentReason.CacheHit || + after.absentReason === AbsentReason.CacheHit + ) { + hasCacheHitMismatch = true; + if (hideCacheHits) { + return undefined!; + } + } + const diff = after.tuples - before.tuples; + return { name, before, after, diff }; + }) + .filter((x) => !!x) + .sort(orderBy((row) => row.diff)); + + let totalBefore = 0; + let totalAfter = 0; + let totalDiff = 0; + + for (const row of rows) { + totalBefore += row.before.tuples; + totalAfter += row.after.tuples; + totalDiff += row.diff; + } + + return ( + <> + Performance comparison + {hasCacheHitMismatch && ( + + Inconsistent cache hits +
+ Some predicates had a cache hit on one side but not the other. For + more accurate results, try running the{" "} + CodeQL: Clear Cache command before each query. +
+
+ +
+ )} + + + + + Before + After + Delta + Predicate + + +
+ {rows.map((row) => ( + + + + setExpandedPredicates( + withToggledValue(expandedPredicates, row.name), + ) + } + > + + + + {renderAbsoluteValue(row.before)} + {renderAbsoluteValue(row.after)} + {renderDelta(row.diff)} + {row.name} + + {expandedPredicates.has(row.name) && ( + <> + {collatePipelines( + row.before.pipelines, + row.after.pipelines, + ).map(({ name, first, second }, pipelineIndex) => ( + + + + {first != null && "Before"} + {second != null && "After"} + + {first != null && second != null && "Delta"} + + + Tuple counts for '{name}' pipeline + {first == null + ? " (after)" + : second == null + ? " (before)" + : ""} + + + {abbreviateRASteps(first?.steps ?? second!.steps).map( + (step, index) => ( + + ), + )} + + ))} + + )} + +
+ ))} + + + + + + + + {formatDecimal(totalBefore)} + {formatDecimal(totalAfter)} + {renderDelta(totalDiff)} + TOTAL + + +
+ + ); +} + +interface PipelinePair { + name: string; + first: PipelineSummary | undefined; + second: PipelineSummary | undefined; +} + +function collatePipelines( + before: Record, + after: Record, +): PipelinePair[] { + const result: PipelinePair[] = []; + + for (const [name, first] of Object.entries(before)) { + const second = after[name]; + if (second == null) { + result.push({ name, first, second: undefined }); + } else if (samePipeline(first.steps, second.steps)) { + result.push({ name, first, second }); + } else { + result.push({ name, first, second: undefined }); + result.push({ name, first: undefined, second }); + } + } + + for (const [name, second] of Object.entries(after)) { + if (before[name] == null) { + result.push({ name, first: undefined, second }); + } + } + + return result; +} + +function samePipeline(a: string[], b: string[]) { + return a.length === b.length && a.every((x, i) => x === b[i]); +} diff --git a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx new file mode 100644 index 00000000000..a2f59daa162 --- /dev/null +++ b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx @@ -0,0 +1,151 @@ +/** + * A set of names, for generating unambiguous abbreviations. + */ +class NameSet { + private readonly abbreviations = new Map(); + + constructor(readonly names: string[]) { + const qnames = names.map(parseName); + const builder = new TrieBuilder(); + qnames + .map((qname) => builder.visitQName(qname)) + .forEach((r, index) => { + this.abbreviations.set(names[index], r.abbreviate()); + }); + } + + public getAbbreviation(name: string) { + return this.abbreviations.get(name) ?? name; + } +} + +/** Name parsed into the form `prefix::name` */ +interface QualifiedName { + prefix?: QualifiedName; + name: string; + args?: QualifiedName[]; +} + +function tokeniseName(text: string) { + return Array.from(text.matchAll(/:+|<|>|,|"[^"]+"|`[^`]+`|[^:<>,"`]+/g)); +} + +function parseName(text: string): QualifiedName { + const tokens = tokeniseName(text); + + function next() { + return tokens.pop()![0]; + } + function peek() { + return tokens[tokens.length - 1][0]; + } + function skipToken(token: string) { + if (tokens.length > 0 && peek() === token) { + tokens.pop(); + return true; + } else { + return false; + } + } + + function parseQName(): QualifiedName { + let args: QualifiedName[] | undefined; + if (skipToken(">")) { + args = []; + while (peek() !== "<") { + args.push(parseQName()); + skipToken(","); + } + args.reverse(); + skipToken("<"); + } + const name = next(); + const prefix = skipToken("::") ? parseQName() : undefined; + return { + prefix, + name, + args, + }; + } + + const result = parseQName(); + if (tokens.length > 0) { + // It's a parse error if we did not consume all tokens. + // Just treat the whole text as the 'name'. + return { prefix: undefined, name: text, args: undefined }; + } + return result; +} + +class TrieNode { + children = new Map(); + constructor(readonly index: number) {} +} + +interface VisitResult { + node: TrieNode; + abbreviate: () => string; +} + +class TrieBuilder { + root = new TrieNode(0); + nextId = 1; + + getOrCreate(trieNode: TrieNode, child: string) { + const { children } = trieNode; + let node = children.get(child); + if (node == null) { + node = new TrieNode(this.nextId++); + children.set(child, node); + } + return node; + } + + visitQName(qname: QualifiedName): VisitResult { + const prefix = + qname.prefix != null ? this.visitQName(qname.prefix) : undefined; + const trieNodeBeforeArgs = this.getOrCreate( + prefix?.node ?? this.root, + qname.name, + ); + let trieNode = trieNodeBeforeArgs; + const args = qname.args?.map((arg) => this.visitQName(arg)); + if (args != null) { + const argKey = args.map((arg) => arg.node.index).join(","); + trieNode = this.getOrCreate(trieNodeBeforeArgs, argKey); + } + return { + node: trieNode, + abbreviate: () => { + let result = ""; + if (prefix != null) { + result += prefix.abbreviate(); + result += "::"; + } + result += qname.name; + if (args != null) { + result += "<"; + if (trieNodeBeforeArgs.children.size === 1) { + result += "..."; + } else { + result += args.map((arg) => arg.abbreviate()).join(","); + } + result += ">"; + } + return result; + }, + }; + } +} + +const nameTokenRegex = /\b[^ ]+::[^ (]+\b/g; + +export function abbreviateRASteps(steps: string[]): string[] { + const nameTokens = steps.flatMap((step) => + Array.from(step.matchAll(nameTokenRegex)).map((tok) => tok[0]), + ); + const nameSet = new NameSet(nameTokens); + return steps.map((step) => + step.replace(nameTokenRegex, (match) => nameSet.getAbbreviation(match)), + ); +} diff --git a/extensions/ql-vscode/src/view/compare-performance/index.tsx b/extensions/ql-vscode/src/view/compare-performance/index.tsx new file mode 100644 index 00000000000..ad0bb8f889a --- /dev/null +++ b/extensions/ql-vscode/src/view/compare-performance/index.tsx @@ -0,0 +1,8 @@ +import type { WebviewDefinition } from "../webview-definition"; +import { ComparePerformance } from "./ComparePerformance"; + +const definition: WebviewDefinition = { + component: , +}; + +export default definition; diff --git a/extensions/ql-vscode/src/view/webview.tsx b/extensions/ql-vscode/src/view/webview.tsx index d3adadf74a1..5081dbaffe3 100644 --- a/extensions/ql-vscode/src/view/webview.tsx +++ b/extensions/ql-vscode/src/view/webview.tsx @@ -6,6 +6,7 @@ import { registerUnhandledErrorListener } from "./common/errors"; import type { WebviewDefinition } from "./webview-definition"; import compareView from "./compare"; +import comparePerformance from "./compare-performance"; import dataFlowPathsView from "./data-flow-paths"; import methodModelingView from "./method-modeling"; import modelEditorView from "./model-editor"; @@ -18,6 +19,7 @@ import "@vscode/codicons/dist/codicon.css"; const views: Record = { compare: compareView, + "compare-performance": comparePerformance, "data-flow-paths": dataFlowPathsView, "method-modeling": methodModelingView, "model-editor": modelEditorView, diff --git a/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/history-tree-data-provider.test.ts b/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/history-tree-data-provider.test.ts index 7026e6c6684..a0305761e91 100644 --- a/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/history-tree-data-provider.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/history-tree-data-provider.test.ts @@ -38,6 +38,7 @@ describe("HistoryTreeDataProvider", () => { let app: App; let configListener: QueryHistoryConfigListener; const doCompareCallback = jest.fn(); + const doComparePerformanceCallback = jest.fn(); let queryHistoryManager: QueryHistoryManager; @@ -506,6 +507,7 @@ describe("HistoryTreeDataProvider", () => { }), languageContext, doCompareCallback, + doComparePerformanceCallback, ); (qhm.treeDataProvider as any).history = [...allHistory]; await workspace.saveAll(); diff --git a/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-manager.test.ts b/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-manager.test.ts index b5176590822..38eb471c918 100644 --- a/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-manager.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-manager.test.ts @@ -40,6 +40,7 @@ describe("QueryHistoryManager", () => { typeof variantAnalysisManagerStub.cancelVariantAnalysis >; const doCompareCallback = jest.fn(); + const doComparePerformanceCallback = jest.fn(); let executeCommand: jest.MockedFn< (commandName: string, ...args: any[]) => Promise @@ -939,6 +940,7 @@ describe("QueryHistoryManager", () => { }), new LanguageContextStore(mockApp), doCompareCallback, + doComparePerformanceCallback, ); (qhm.treeDataProvider as any).history = [...allHistory]; await workspace.saveAll(); diff --git a/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/variant-analysis-history.test.ts b/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/variant-analysis-history.test.ts index 1719ee00971..5442d5e2c38 100644 --- a/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/variant-analysis-history.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/variant-analysis-history.test.ts @@ -105,6 +105,7 @@ describe("Variant Analyses and QueryHistoryManager", () => { }), new LanguageContextStore(app), asyncNoop, + asyncNoop, ); disposables.push(qhm); From 96aa770e850e592cee1e56d0c3ee85da60723ce2 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 13 Nov 2024 11:50:42 +0100 Subject: [PATCH 02/46] Show evaluation and iteration counts in table --- .../ComparePerformance.tsx | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 036585432c6..8be813e6004 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -25,6 +25,8 @@ interface OptionalValue { } interface PredicateInfo extends OptionalValue { + evaluationCount: number; + iterationCount: number; pipelines: Record; } @@ -48,6 +50,8 @@ class ComparisonDataset { const index = nameToIndex.get(name); if (index == null) { return { + evaluationCount: 0, + iterationCount: 0, tuples: 0, absentReason: AbsentReason.NotSeen, pipelines: {}, @@ -63,6 +67,8 @@ class ComparisonDataset { } } return { + evaluationCount: data.evaluationCounts[index], + iterationCount: data.iterationCounts[index], tuples: tupleCost, absentReason, pipelines: data.pipelineSummaryList[index], @@ -184,6 +190,9 @@ interface PipelineStepProps { step: string; } +/** + * Row with details of a pipeline step, or one of the high-level stats appearing above the pipelines (evaluation/iteration counts). + */ function PipelineStep(props: PipelineStepProps) { let { before, after, step } = props; if (before != null && before < 0) { @@ -204,6 +213,46 @@ function PipelineStep(props: PipelineStepProps) { ); } +interface HighLevelStatsProps { + before: PredicateInfo; + after: PredicateInfo; +} + +function HighLevelStats(props: HighLevelStatsProps) { + const { before, after } = props; + const hasBefore = before.absentReason !== AbsentReason.NotSeen; + const hasAfter = after.absentReason !== AbsentReason.NotSeen; + const showEvaluationCount = + before.evaluationCount > 1 || after.evaluationCount > 1; + return ( + <> + + + {hasBefore ? "Before" : ""} + {hasAfter ? "After" : ""} + {hasBefore && hasAfter ? "Delta" : ""} + Stats + + {showEvaluationCount && ( + + )} + + + ); +} + function Chevron({ expanded }: { expanded: boolean }) { return ; } @@ -350,6 +399,7 @@ export function ComparePerformance(_: Record) { {expandedPredicates.has(row.name) && ( <> + {collatePipelines( row.before.pipelines, row.after.pipelines, From aaf23eae72f878622b5b1e4dc471e9c5c3a977dc Mon Sep 17 00:00:00 2001 From: Taus Date: Wed, 13 Nov 2024 13:45:02 +0000 Subject: [PATCH 03/46] compare-perf: Add support for sorting options Adds a dropdown with (at present) two options: sorting by delta and sorting by absolute delta. --- .../ComparePerformance.tsx | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 8be813e6004..88d2b0589db 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -1,3 +1,4 @@ +import type { ChangeEvent } from "react"; import { useMemo, useState, Fragment } from "react"; import type { SetPerformanceComparisonQueries, @@ -184,6 +185,8 @@ const PipelineStepTR = styled.tr` } `; +const SortOrderDropdown = styled.select``; + interface PipelineStepProps { before: number | undefined; after: number | undefined; @@ -253,6 +256,20 @@ function HighLevelStats(props: HighLevelStatsProps) { ); } +type TRow = { + name: string; + before: PredicateInfo; + after: PredicateInfo; + diff: number; +}; + +function getSortOrder(sortOrder: "delta" | "absDelta") { + if (sortOrder === "absDelta") { + return orderBy((row: TRow) => -Math.abs(row.diff)); + } + return orderBy((row: TRow) => row.diff); +} + function Chevron({ expanded }: { expanded: boolean }) { return ; } @@ -296,6 +313,8 @@ export function ComparePerformance(_: Record) { const [hideCacheHits, setHideCacheHits] = useState(false); + const [sortOrder, setSortOrder] = useState<"delta" | "absDelta">("absDelta"); + if (!datasets) { return
Loading performance comparison...
; } @@ -329,7 +348,7 @@ export function ComparePerformance(_: Record) { return { name, before, after, diff }; }) .filter((x) => !!x) - .sort(orderBy((row) => row.diff)); + .sort(getSortOrder(sortOrder)); let totalBefore = 0; let totalAfter = 0; @@ -363,6 +382,15 @@ export function ComparePerformance(_: Record) { )} + ) => + setSortOrder(e.target.value as "delta" | "absDelta") + } + value={sortOrder} + > + + + From 70ec5704c8f9670d3e814f18ed84753b03d3d833 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 13 Nov 2024 13:58:20 +0100 Subject: [PATCH 04/46] Make RAPrettyPrinter generate JSX fragments --- .../ComparePerformance.tsx | 2 +- .../compare-performance/RAPrettyPrinter.tsx | 59 ++++++++++++++----- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 88d2b0589db..6fffc4e8351 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -190,7 +190,7 @@ const SortOrderDropdown = styled.select``; interface PipelineStepProps { before: number | undefined; after: number | undefined; - step: string; + step: React.ReactNode; } /** diff --git a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx index a2f59daa162..d7fae2a3267 100644 --- a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx @@ -1,8 +1,10 @@ +import { Fragment } from "react"; + /** * A set of names, for generating unambiguous abbreviations. */ class NameSet { - private readonly abbreviations = new Map(); + private readonly abbreviations = new Map(); constructor(readonly names: string[]) { const qnames = names.map(parseName); @@ -14,7 +16,7 @@ class NameSet { }); } - public getAbbreviation(name: string) { + public getAbbreviation(name: string): React.ReactNode { return this.abbreviations.get(name) ?? name; } } @@ -84,7 +86,7 @@ class TrieNode { interface VisitResult { node: TrieNode; - abbreviate: () => string; + abbreviate: () => React.ReactNode; } class TrieBuilder { @@ -117,20 +119,28 @@ class TrieBuilder { return { node: trieNode, abbreviate: () => { - let result = ""; + const result: React.ReactNode[] = []; if (prefix != null) { - result += prefix.abbreviate(); - result += "::"; + result.push(prefix.abbreviate()); + result.push("::"); } - result += qname.name; + result.push(qname.name); if (args != null) { - result += "<"; + result.push("<"); if (trieNodeBeforeArgs.children.size === 1) { - result += "..."; + result.push("..."); } else { - result += args.map((arg) => arg.abbreviate()).join(","); + let first = true; + for (const arg of args) { + result.push(arg.abbreviate()); + if (first) { + first = false; + } else { + result.push(","); + } + } } - result += ">"; + result.push(">"); } return result; }, @@ -140,12 +150,31 @@ class TrieBuilder { const nameTokenRegex = /\b[^ ]+::[^ (]+\b/g; -export function abbreviateRASteps(steps: string[]): string[] { +export function abbreviateRASteps(steps: string[]): React.ReactNode[] { const nameTokens = steps.flatMap((step) => Array.from(step.matchAll(nameTokenRegex)).map((tok) => tok[0]), ); const nameSet = new NameSet(nameTokens); - return steps.map((step) => - step.replace(nameTokenRegex, (match) => nameSet.getAbbreviation(match)), - ); + return steps.map((step, index) => { + const matches = Array.from(step.matchAll(nameTokenRegex)); + const result: React.ReactNode[] = []; + for (let i = 0; i < matches.length; i++) { + const match = matches[i]; + const before = step.slice( + i === 0 ? 0 : matches[i - 1].index + matches[i - 1][0].length, + match.index, + ); + result.push(before); + result.push(nameSet.getAbbreviation(match[0])); + } + result.push( + matches.length === 0 + ? step + : step.slice( + matches[matches.length - 1].index + + matches[matches.length - 1][0].length, + ), + ); + return {result}; + }); } From 8268d6812f341cbbad4d0e64a500a2d34a3a5f96 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 13 Nov 2024 14:45:43 +0100 Subject: [PATCH 05/46] Apply styling to RA predicate names --- .../compare-performance/RAPrettyPrinter.tsx | 97 ++++++++++++++----- 1 file changed, 73 insertions(+), 24 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx index d7fae2a3267..010523194e8 100644 --- a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx @@ -1,4 +1,5 @@ import { Fragment } from "react"; +import { styled } from "styled-components"; /** * A set of names, for generating unambiguous abbreviations. @@ -12,7 +13,7 @@ class NameSet { qnames .map((qname) => builder.visitQName(qname)) .forEach((r, index) => { - this.abbreviations.set(names[index], r.abbreviate()); + this.abbreviations.set(names[index], r.abbreviate(true)); }); } @@ -86,7 +87,7 @@ class TrieNode { interface VisitResult { node: TrieNode; - abbreviate: () => React.ReactNode; + abbreviate: (isRoot?: boolean) => React.ReactNode; } class TrieBuilder { @@ -118,13 +119,21 @@ class TrieBuilder { } return { node: trieNode, - abbreviate: () => { + abbreviate: (isRoot = false) => { const result: React.ReactNode[] = []; if (prefix != null) { result.push(prefix.abbreviate()); result.push("::"); } - result.push(qname.name); + const { name } = qname; + const hash = name.indexOf("#"); + if (hash !== -1 && isRoot) { + const shortName = name.substring(0, hash); + result.push({shortName}); + result.push(name.substring(hash)); + } else { + result.push(isRoot ? {name} : name); + } if (args != null) { result.push("<"); if (trieNodeBeforeArgs.children.size === 1) { @@ -148,33 +157,73 @@ class TrieBuilder { } } -const nameTokenRegex = /\b[^ ]+::[^ (]+\b/g; +/** + * Span enclosing an entire qualified name. + * + * Can be used to gray out uninteresting parts of the name, though this looks worse than expected. + */ +const QNameSpan = styled.span` + /* color: var(--vscode-disabledForeground); */ +`; + +/** Span enclosing the innermost identifier, e.g. the `foo` in `A::B::foo#abc` */ +const IdentifierSpan = styled.span` + font-weight: 600; +`; + +/** Span enclosing keywords such as `JOIN` and `WITH`. */ +const KeywordSpan = styled.span` + font-weight: 500; +`; + +const nameTokenRegex = /\b[^ (]+\b/g; + +function traverseMatches( + text: string, + regex: RegExp, + callbacks: { + onMatch: (match: RegExpMatchArray) => void; + onText: (text: string) => void; + }, +) { + const matches = Array.from(text.matchAll(regex)); + let lastIndex = 0; + for (const match of matches) { + const before = text.substring(lastIndex, match.index); + if (before !== "") { + callbacks.onText(before); + } + callbacks.onMatch(match); + lastIndex = match.index + match[0].length; + } + const after = text.substring(lastIndex); + if (after !== "") { + callbacks.onText(after); + } +} export function abbreviateRASteps(steps: string[]): React.ReactNode[] { const nameTokens = steps.flatMap((step) => Array.from(step.matchAll(nameTokenRegex)).map((tok) => tok[0]), ); - const nameSet = new NameSet(nameTokens); + const nameSet = new NameSet(nameTokens.filter((name) => name.includes("::"))); return steps.map((step, index) => { - const matches = Array.from(step.matchAll(nameTokenRegex)); const result: React.ReactNode[] = []; - for (let i = 0; i < matches.length; i++) { - const match = matches[i]; - const before = step.slice( - i === 0 ? 0 : matches[i - 1].index + matches[i - 1][0].length, - match.index, - ); - result.push(before); - result.push(nameSet.getAbbreviation(match[0])); - } - result.push( - matches.length === 0 - ? step - : step.slice( - matches[matches.length - 1].index + - matches[matches.length - 1][0].length, - ), - ); + traverseMatches(step, nameTokenRegex, { + onMatch(match) { + const text = match[0]; + if (text.includes("::")) { + result.push({nameSet.getAbbreviation(text)}); + } else if (/[A-Z]+/.test(text)) { + result.push({text}); + } else { + result.push(match[0]); + } + }, + onText(text) { + result.push(text); + }, + }); return {result}; }); } From 58afeba1ac1a61d0379823465a9b3762119aa7f2 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 14 Nov 2024 10:30:32 +0100 Subject: [PATCH 06/46] Apply a background color to the pipeline header rows --- .../compare-performance/ComparePerformance.tsx | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 6fffc4e8351..59883cb43ff 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -216,6 +216,10 @@ function PipelineStep(props: PipelineStepProps) { ); } +const HeaderTR = styled.tr` + background-color: var(--vscode-sideBar-background); +`; + interface HighLevelStatsProps { before: PredicateInfo; after: PredicateInfo; @@ -229,13 +233,13 @@ function HighLevelStats(props: HighLevelStatsProps) { before.evaluationCount > 1 || after.evaluationCount > 1; return ( <> - + {hasBefore ? "Before" : ""} {hasAfter ? "After" : ""} {hasBefore && hasAfter ? "Delta" : ""} Stats - + {showEvaluationCount && ( ) {
- + Before After Delta Predicate - +
{rows.map((row) => ( @@ -433,7 +437,7 @@ export function ComparePerformance(_: Record) { row.after.pipelines, ).map(({ name, first, second }, pipelineIndex) => ( - + {first != null && "Before"} {second != null && "After"} @@ -448,7 +452,7 @@ export function ComparePerformance(_: Record) { ? " (before)" : ""} - + {abbreviateRASteps(first?.steps ?? second!.steps).map( (step, index) => ( Date: Thu, 14 Nov 2024 11:07:43 +0100 Subject: [PATCH 07/46] Make "..." clickable to reveal abbreviated name --- .../compare-performance/RAPrettyPrinter.tsx | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx index 010523194e8..d02ef1e9c3a 100644 --- a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx @@ -1,4 +1,4 @@ -import { Fragment } from "react"; +import { Fragment, useState } from "react"; import { styled } from "styled-components"; /** @@ -29,6 +29,21 @@ interface QualifiedName { args?: QualifiedName[]; } +function qnameToString(name: QualifiedName): string { + const parts: string[] = []; + if (name.prefix != null) { + parts.push(qnameToString(name.prefix)); + parts.push("::"); + } + parts.push(name.name); + if (name.args != null && name.args.length > 0) { + parts.push("<"); + parts.push(name.args.map(qnameToString).join(",")); + parts.push(">"); + } + return parts.join(""); +} + function tokeniseName(text: string) { return Array.from(text.matchAll(/:+|<|>|,|"[^"]+"|`[^`]+`|[^:<>,"`]+/g)); } @@ -137,7 +152,10 @@ class TrieBuilder { if (args != null) { result.push("<"); if (trieNodeBeforeArgs.children.size === 1) { - result.push("..."); + const argsText = qname + .args!.map((arg) => qnameToString(arg)) + .join(","); + result.push({argsText}); } else { let first = true; for (const arg of args) { @@ -157,6 +175,30 @@ class TrieBuilder { } } +const ExpandableTextButton = styled.button` + background: none; + border: none; + cursor: pointer; + padding: 0; + color: inherit; + &:hover { + background-color: rgba(128, 128, 128, 0.2); + } +`; + +interface ExpandableNamePartProps { + children: React.ReactNode; +} + +function ExpandableNamePart(props: ExpandableNamePartProps) { + const [isExpanded, setExpanded] = useState(false); + return ( + setExpanded(!isExpanded)}> + {isExpanded ? props.children : "..."} + + ); +} + /** * Span enclosing an entire qualified name. * From 317e52c0e71b4576133374f42f5b8a89d8c17396 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 14 Nov 2024 11:40:31 +0100 Subject: [PATCH 08/46] Also abbreviate RA names in predicate overview --- .../view/compare-performance/ComparePerformance.tsx | 8 +++++--- .../src/view/compare-performance/RAPrettyPrinter.tsx | 12 +++++++++++- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 59883cb43ff..b55f2cdff23 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -12,7 +12,7 @@ import type { import { formatDecimal } from "../../common/number"; import { styled } from "styled-components"; import { Codicon, ViewTitle, WarningBox } from "../common"; -import { abbreviateRASteps } from "./RAPrettyPrinter"; +import { abbreviateRANames, abbreviateRASteps } from "./RAPrettyPrinter"; const enum AbsentReason { NotSeen = "NotSeen", @@ -364,6 +364,8 @@ export function ComparePerformance(_: Record) { totalDiff += row.diff; } + const rowNames = abbreviateRANames(rows.map((row) => row.name)); + return ( <> Performance comparison @@ -406,7 +408,7 @@ export function ComparePerformance(_: Record) { - {rows.map((row) => ( + {rows.map((row, rowIndex) => ( ) { {renderAbsoluteValue(row.before)} {renderAbsoluteValue(row.after)} {renderDelta(row.diff)} - {row.name} + {rowNames[rowIndex]} {expandedPredicates.has(row.name) && ( <> diff --git a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx index d02ef1e9c3a..39038a8e520 100644 --- a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx @@ -193,7 +193,12 @@ interface ExpandableNamePartProps { function ExpandableNamePart(props: ExpandableNamePartProps) { const [isExpanded, setExpanded] = useState(false); return ( - setExpanded(!isExpanded)}> + { + setExpanded(!isExpanded); + event.stopPropagation(); + }} + > {isExpanded ? props.children : "..."} ); @@ -269,3 +274,8 @@ export function abbreviateRASteps(steps: string[]): React.ReactNode[] { return {result}; }); } + +export function abbreviateRANames(names: string[]): React.ReactNode[] { + const nameSet = new NameSet(names); + return names.map((name) => nameSet.getAbbreviation(name)); +} From 876c5b6091c94ce91b78e818489ba2e0b10158d3 Mon Sep 17 00:00:00 2001 From: Taus Date: Thu, 14 Nov 2024 13:19:28 +0000 Subject: [PATCH 09/46] Colorize positive/negative deltas --- .../compare-performance/ComparePerformance.tsx | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index b55f2cdff23..b566ec93a2e 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -93,7 +93,7 @@ function renderAbsoluteValue(x: OptionalValue) { function renderDelta(x: number) { const sign = x > 0 ? "+" : ""; return ( - + 0 ? "bad-value" : x < 0 ? "good-value" : ""}> {sign} {formatDecimal(x)} @@ -126,6 +126,19 @@ const NameCell = styled.td``; const NumberCell = styled.td` text-align: right; width: 10em !important; + + &.bad-value { + color: var(--vscode-problemsErrorIcon-foreground); + tr.expanded & { + color: inherit; + } + } + &.good-value { + color: var(--vscode-problemsInfoIcon-foreground); + tr.expanded & { + color: inherit; + } + } `; const AbsentNumberCell = styled.td` From 260bf0e8d1630b988f848ae35b980f83ba9cc512 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 14 Nov 2024 14:41:29 +0100 Subject: [PATCH 10/46] Add option to choose metric --- .../ComparePerformance.tsx | 87 ++++++++++++++++--- 1 file changed, 74 insertions(+), 13 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index b566ec93a2e..3a236616b2d 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -28,6 +28,7 @@ interface OptionalValue { interface PredicateInfo extends OptionalValue { evaluationCount: number; iterationCount: number; + timeCost: number; pipelines: Record; } @@ -54,6 +55,7 @@ class ComparisonDataset { evaluationCount: 0, iterationCount: 0, tuples: 0, + timeCost: 0, absentReason: AbsentReason.NotSeen, pipelines: {}, }; @@ -70,6 +72,7 @@ class ComparisonDataset { return { evaluationCount: data.evaluationCounts[index], iterationCount: data.iterationCounts[index], + timeCost: data.timeCosts[index], tuples: tupleCost, absentReason, pipelines: data.pipelineSummaryList[index], @@ -77,7 +80,7 @@ class ComparisonDataset { } } -function renderAbsoluteValue(x: OptionalValue) { +function renderAbsoluteValue(x: PredicateInfo, metric: Metric) { switch (x.absentReason) { case AbsentReason.NotSeen: return n/a; @@ -86,20 +89,30 @@ function renderAbsoluteValue(x: OptionalValue) { case AbsentReason.Sentinel: return sentinel empty; default: - return {formatDecimal(x.tuples)}; + return ( + + {formatDecimal(metric.get(x))} + {renderUnit(metric.unit)} + + ); } } -function renderDelta(x: number) { +function renderDelta(x: number, unit?: string) { const sign = x > 0 ? "+" : ""; return ( 0 ? "bad-value" : x < 0 ? "good-value" : ""}> {sign} {formatDecimal(x)} + {renderUnit(unit)} ); } +function renderUnit(unit: string | undefined) { + return unit == null ? "" : ` ${unit}`; +} + function orderBy(fn: (x: T) => number | string) { return (x: T, y: T) => { const fx = fn(x); @@ -198,7 +211,7 @@ const PipelineStepTR = styled.tr` } `; -const SortOrderDropdown = styled.select``; +const Dropdown = styled.select``; interface PipelineStepProps { before: number | undefined; @@ -287,6 +300,37 @@ function getSortOrder(sortOrder: "delta" | "absDelta") { return orderBy((row: TRow) => row.diff); } +interface Metric { + title: string; + get(info: PredicateInfo): number; + unit?: string; +} + +const metrics: Record = { + tuples: { + title: "Tuples in pipeline", + get: (info) => info.tuples, + }, + time: { + title: "Time spent (milliseconds)", + get: (info) => info.timeCost, + unit: "ms", + }, + evaluations: { + title: "Evaluations", + get: (info) => info.evaluationCount, + }, + iterations: { + title: "Iterations (per evaluation)", + get: (info) => + info.absentReason ? 0 : info.iterationCount / info.evaluationCount, + }, + iterationsTotal: { + title: "Iterations (total)", + get: (info) => info.iterationCount, + }, +}; + function Chevron({ expanded }: { expanded: boolean }) { return ; } @@ -332,6 +376,8 @@ export function ComparePerformance(_: Record) { const [sortOrder, setSortOrder] = useState<"delta" | "absDelta">("absDelta"); + const [metric, setMetric] = useState(metrics.tuples); + if (!datasets) { return
Loading performance comparison...
; } @@ -349,7 +395,9 @@ export function ComparePerformance(_: Record) { .map((name) => { const before = from.getTupleCountInfo(name); const after = to.getTupleCountInfo(name); - if (before.tuples === after.tuples) { + const beforeValue = metric.get(before); + const afterValue = metric.get(after); + if (beforeValue === afterValue) { return undefined!; } if ( @@ -361,7 +409,7 @@ export function ComparePerformance(_: Record) { return undefined!; } } - const diff = after.tuples - before.tuples; + const diff = afterValue - beforeValue; return { name, before, after, diff }; }) .filter((x) => !!x) @@ -372,8 +420,8 @@ export function ComparePerformance(_: Record) { let totalDiff = 0; for (const row of rows) { - totalBefore += row.before.tuples; - totalAfter += row.after.tuples; + totalBefore += metric.get(row.before); + totalAfter += metric.get(row.after); totalDiff += row.diff; } @@ -401,7 +449,20 @@ export function ComparePerformance(_: Record) { )} - ) => + setMetric(metrics[e.target.value]) + } + > + {Object.entries(metrics).map(([key, value]) => ( + + ))} + {" "} + sorted by{" "} + ) => setSortOrder(e.target.value as "delta" | "absDelta") } @@ -409,7 +470,7 @@ export function ComparePerformance(_: Record) { > - +
@@ -439,9 +500,9 @@ export function ComparePerformance(_: Record) { - {renderAbsoluteValue(row.before)} - {renderAbsoluteValue(row.after)} - {renderDelta(row.diff)} + {renderAbsoluteValue(row.before, metric)} + {renderAbsoluteValue(row.after, metric)} + {renderDelta(row.diff, metric.unit)} {rowNames[rowIndex]} {expandedPredicates.has(row.name) && ( From 453aa833f2ea1c5c3f1a4afd67f6cca33415cc5b Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 14 Nov 2024 15:29:00 +0100 Subject: [PATCH 11/46] Check for nullness of 'data' in a separate component This ensures we can use hooks after the check in the main component --- .../ComparePerformance.tsx | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 3a236616b2d..e88eaf9ce50 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -357,14 +357,23 @@ export function ComparePerformance(_: Record) { [setData], ); + if (!data) { + return
Loading performance comparison...
; + } + + return ; +} + +function ComparePerformanceWithData(props: { + data: SetPerformanceComparisonQueries; +}) { + const { data } = props; + const datasets = useMemo( - () => - data == null - ? undefined - : { - from: new ComparisonDataset(data.from), - to: new ComparisonDataset(data.to), - }, + () => ({ + from: new ComparisonDataset(data.from), + to: new ComparisonDataset(data.to), + }), [data], ); @@ -378,10 +387,6 @@ export function ComparePerformance(_: Record) { const [metric, setMetric] = useState(metrics.tuples); - if (!datasets) { - return
Loading performance comparison...
; - } - const { from, to } = datasets; const nameSet = new Set(from.data.names); From ccf2dc64acd4b1e9f5c3a14b81abbad4cf37603c Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 14 Nov 2024 15:30:34 +0100 Subject: [PATCH 12/46] Simplify datasets assignment --- .../src/view/compare-performance/ComparePerformance.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index e88eaf9ce50..7983180c86c 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -369,7 +369,7 @@ function ComparePerformanceWithData(props: { }) { const { data } = props; - const datasets = useMemo( + const { from, to } = useMemo( () => ({ from: new ComparisonDataset(data.from), to: new ComparisonDataset(data.to), @@ -387,8 +387,6 @@ function ComparePerformanceWithData(props: { const [metric, setMetric] = useState(metrics.tuples); - const { from, to } = datasets; - const nameSet = new Set(from.data.names); for (const name of to.data.names) { nameSet.add(name); From 412338c71726e96e3deb76b4aac99a322a91f556 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Thu, 14 Nov 2024 16:29:31 +0100 Subject: [PATCH 13/46] feat: parallel log scanning --- .../compare-performance-view.ts | 24 ++++++++++++++----- .../ql-vscode/src/log-insights/log-scanner.ts | 14 ++++++++--- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts b/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts index 28925ba1afd..aae1621b0dc 100644 --- a/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts +++ b/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts @@ -1,3 +1,4 @@ +import { statSync } from "fs"; import { ViewColumn } from "vscode"; import type { App } from "../common/app"; @@ -11,6 +12,7 @@ import { showAndLogExceptionWithTelemetry } from "../common/logging"; import { extLogger } from "../common/logging/vscode"; import type { WebviewPanelConfig } from "../common/vscode/abstract-webview"; import { AbstractWebview } from "../common/vscode/abstract-webview"; +import { withProgress } from "../common/vscode/progress"; import { telemetryListener } from "../common/vscode/telemetry"; import type { HistoryItemLabelProvider } from "../query-history/history-item-label-provider"; import { PerformanceOverviewScanner } from "../log-insights/performance-comparison"; @@ -41,12 +43,22 @@ export class ComparePerformanceView extends AbstractWebview< await this.waitForPanelLoaded(); - // TODO: try processing in (async) parallel once readJsonl is streaming - const fromPerf = await scanLog( - fromJsonLog, - new PerformanceOverviewScanner(), - ); - const toPerf = await scanLog(toJsonLog, new PerformanceOverviewScanner()); + function scanLogWithProgress(log: string, logDescription: string) { + const bytes = statSync(log).size; + return withProgress( + async (progress) => + scanLog(log, new PerformanceOverviewScanner(), progress), + + { + title: `Scanning evaluator log ${logDescription} (${(bytes / 1024 / 1024).toFixed(1)} MB)`, + }, + ); + } + + const [fromPerf, toPerf] = await Promise.all([ + scanLogWithProgress(fromJsonLog, "1/2"), + scanLogWithProgress(toJsonLog, "2/2"), + ]); await this.postMessage({ t: "setPerformanceComparison", diff --git a/extensions/ql-vscode/src/log-insights/log-scanner.ts b/extensions/ql-vscode/src/log-insights/log-scanner.ts index 9324a5994f1..55b655c2580 100644 --- a/extensions/ql-vscode/src/log-insights/log-scanner.ts +++ b/extensions/ql-vscode/src/log-insights/log-scanner.ts @@ -1,6 +1,7 @@ -import type { SummaryEvent } from "./log-summary"; -import { readJsonlFile } from "../common/jsonl-reader"; import type { Disposable } from "../common/disposable-object"; +import { readJsonlFile } from "../common/jsonl-reader"; +import type { ProgressCallback } from "../common/vscode/progress"; +import type { SummaryEvent } from "./log-summary"; /** * Callback interface used to report diagnostics from a log scanner. @@ -114,7 +115,7 @@ export class EvaluationLogScannerSet { } /** - * Scan the evaluator summary log using the given scanner. For conveience, returns the scanner. + * Scan the evaluator summary log using the given scanner. For convenience, returns the scanner. * * @param jsonSummaryLocation The file path of the JSON summary log. * @param scanner The scanner to process events from the log @@ -122,7 +123,14 @@ export class EvaluationLogScannerSet { export async function scanLog( jsonSummaryLocation: string, scanner: T, + progress?: ProgressCallback, ): Promise { + progress?.({ + // XXX all scans have step 1 - the backing progress tracker allows increments instead of steps - but for now we are happy with a tiny UI that says what is happening + message: `Scanning ...`, + step: 1, + maxStep: 2, + }); await readJsonlFile(jsonSummaryLocation, async (obj) => { scanner.onEvent(obj); }); From 6d4427e59ce2fcf3bc1a0fbcca1ce3bf3b646bd4 Mon Sep 17 00:00:00 2001 From: Taus Date: Thu, 14 Nov 2024 21:08:15 +0000 Subject: [PATCH 14/46] compare-perf: Add support for selecting a single run as input A very hacky implementation that simply instantiates an empty `PerformanceOverviewScanner` as the "from" column (i.e. with all values empty). To see it in action, select a single query run in the query history and pick "Compare Performance" from the context menu. Then select the "Single run" option when prompted. --- .../compare-performance-view.ts | 6 +- extensions/ql-vscode/src/extension.ts | 15 +++-- .../query-history/query-history-manager.ts | 66 +++++++++++++++++-- 3 files changed, 75 insertions(+), 12 deletions(-) diff --git a/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts b/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts index aae1621b0dc..d286161b1b9 100644 --- a/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts +++ b/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts @@ -56,8 +56,10 @@ export class ComparePerformanceView extends AbstractWebview< } const [fromPerf, toPerf] = await Promise.all([ - scanLogWithProgress(fromJsonLog, "1/2"), - scanLogWithProgress(toJsonLog, "2/2"), + fromJsonLog === "" + ? new PerformanceOverviewScanner() + : scanLogWithProgress(fromJsonLog, "1/2"), + scanLogWithProgress(toJsonLog, fromJsonLog === "" ? "1/1" : "2/2"), ]); await this.postMessage({ diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index 979ee7b7e8d..7d30ecb3a54 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -927,7 +927,7 @@ async function activateWithInstalledDistribution( ): Promise => showResultsForComparison(compareView, from, to), async ( from: CompletedLocalQueryInfo, - to: CompletedLocalQueryInfo, + to: CompletedLocalQueryInfo | undefined, ): Promise => showPerformanceComparison(comparePerformanceView, from, to), ); @@ -1208,17 +1208,22 @@ async function showResultsForComparison( async function showPerformanceComparison( view: ComparePerformanceView, from: CompletedLocalQueryInfo, - to: CompletedLocalQueryInfo, + to: CompletedLocalQueryInfo | undefined, ): Promise { - const fromLog = from.evaluatorLogPaths?.jsonSummary; - const toLog = to.evaluatorLogPaths?.jsonSummary; + let fromLog = from.evaluatorLogPaths?.jsonSummary; + let toLog = to?.evaluatorLogPaths?.jsonSummary; + + if (to === undefined) { + toLog = fromLog; + fromLog = ""; + } if (fromLog === undefined || toLog === undefined) { return extLogger.showWarningMessage( `Cannot compare performance as the structured logs are missing. Did they queries complete normally?`, ); } await extLogger.log( - `Comparing performance of ${from.getQueryName()} and ${to.getQueryName()}`, + `Comparing performance of ${from.getQueryName()} and ${to?.getQueryName() ?? "baseline"}`, ); await view.showResults(fromLog, toLog); diff --git a/extensions/ql-vscode/src/query-history/query-history-manager.ts b/extensions/ql-vscode/src/query-history/query-history-manager.ts index 0af42ba73d7..35f241c52de 100644 --- a/extensions/ql-vscode/src/query-history/query-history-manager.ts +++ b/extensions/ql-vscode/src/query-history/query-history-manager.ts @@ -151,7 +151,7 @@ export class QueryHistoryManager extends DisposableObject { ) => Promise, private readonly doComparePerformanceCallback: ( from: CompletedLocalQueryInfo, - to: CompletedLocalQueryInfo, + to: CompletedLocalQueryInfo | undefined, ) => Promise, ) { super(); @@ -706,7 +706,10 @@ export class QueryHistoryManager extends DisposableObject { let toItem: CompletedLocalQueryInfo | undefined = undefined; try { - toItem = await this.findOtherQueryToCompare(fromItem, multiSelect); + toItem = await this.findOtherQueryToComparePerformance( + fromItem, + multiSelect, + ); } catch (e) { void showAndLogErrorMessage( this.app.logger, @@ -714,9 +717,7 @@ export class QueryHistoryManager extends DisposableObject { ); } - if (toItem !== undefined) { - await this.doComparePerformanceCallback(fromItem, toItem); - } + await this.doComparePerformanceCallback(fromItem, toItem); } async handleItemClicked(item: QueryHistoryInfo) { @@ -1116,6 +1117,7 @@ export class QueryHistoryManager extends DisposableObject { detail: item.completedQuery.message, query: item, })); + if (comparableQueryLabels.length < 1) { throw new Error("No other queries available to compare with."); } @@ -1124,6 +1126,60 @@ export class QueryHistoryManager extends DisposableObject { return choice?.query; } + private async findOtherQueryToComparePerformance( + fromItem: CompletedLocalQueryInfo, + allSelectedItems: CompletedLocalQueryInfo[], + ): Promise { + const dbName = fromItem.databaseName; + + // If exactly 2 items are selected, return the one that + // isn't being used as the "from" item. + if (allSelectedItems.length === 2) { + const otherItem = + fromItem === allSelectedItems[0] + ? allSelectedItems[1] + : allSelectedItems[0]; + if (otherItem.databaseName !== dbName) { + throw new Error("Query databases must be the same."); + } + return otherItem; + } + + if (allSelectedItems.length > 2) { + throw new Error("Please select no more than 2 queries."); + } + + // Otherwise, present a dialog so the user can choose the item they want to use. + const comparableQueryLabels = this.treeDataProvider.allHistory + .filter(this.isSuccessfulCompletedLocalQueryInfo) + .filter( + (otherItem) => + otherItem !== fromItem && otherItem.databaseName === dbName, + ) + .map((item) => ({ + label: this.labelProvider.getLabel(item), + description: item.databaseName, + detail: item.completedQuery.message, + query: item, + })); + const comparableQueryLabelsWithDefault = [ + { + label: "Single run", + description: + "Look at the performance of this run, compared to a trivial baseline", + detail: undefined, + query: undefined, + }, + ...comparableQueryLabels, + ]; + if (comparableQueryLabelsWithDefault.length < 1) { + throw new Error("No other queries available to compare with."); + } + const choice = await window.showQuickPick(comparableQueryLabelsWithDefault); + + return choice?.query; + } + /** * Updates the compare with source query. This ensures that all compare command invocations * when exactly 2 queries are selected always have the proper _from_ query. Always use From e039f6bc52e8459dd1c4de2e6259329bebda6808 Mon Sep 17 00:00:00 2001 From: Taus Date: Fri, 15 Nov 2024 10:32:47 +0000 Subject: [PATCH 15/46] Simplify view when not in comparison mode --- .../ql-vscode/src/common/interface-types.ts | 1 + .../compare-performance-view.ts | 1 + .../ComparePerformance.tsx | 68 ++++++++++++------- 3 files changed, 47 insertions(+), 23 deletions(-) diff --git a/extensions/ql-vscode/src/common/interface-types.ts b/extensions/ql-vscode/src/common/interface-types.ts index 8d0681cbcd9..2a0fb24c811 100644 --- a/extensions/ql-vscode/src/common/interface-types.ts +++ b/extensions/ql-vscode/src/common/interface-types.ts @@ -403,6 +403,7 @@ export interface SetPerformanceComparisonQueries { readonly t: "setPerformanceComparison"; readonly from: PerformanceComparisonDataFromLog; readonly to: PerformanceComparisonDataFromLog; + readonly comparison: boolean; } export type FromComparePerformanceViewMessage = CommonFromViewMessages; diff --git a/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts b/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts index d286161b1b9..c1633801ed6 100644 --- a/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts +++ b/extensions/ql-vscode/src/compare-performance/compare-performance-view.ts @@ -66,6 +66,7 @@ export class ComparePerformanceView extends AbstractWebview< t: "setPerformanceComparison", from: fromPerf.getData(), to: toPerf.getData(), + comparison: fromJsonLog !== "", }); } diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 7983180c86c..98925170d18 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -216,6 +216,7 @@ const Dropdown = styled.select``; interface PipelineStepProps { before: number | undefined; after: number | undefined; + comparison: boolean; step: React.ReactNode; } @@ -223,7 +224,7 @@ interface PipelineStepProps { * Row with details of a pipeline step, or one of the high-level stats appearing above the pipelines (evaluation/iteration counts). */ function PipelineStep(props: PipelineStepProps) { - let { before, after, step } = props; + let { before, after, comparison, step } = props; if (before != null && before < 0) { before = undefined; } @@ -234,9 +235,11 @@ function PipelineStep(props: PipelineStepProps) { return ( - {before != null ? formatDecimal(before) : ""} + {comparison && ( + {before != null ? formatDecimal(before) : ""} + )} {after != null ? formatDecimal(after) : ""} - {delta != null ? renderDelta(delta) :
} + {comparison && (delta != null ? renderDelta(delta) : )} {step} ); @@ -249,10 +252,11 @@ const HeaderTR = styled.tr` interface HighLevelStatsProps { before: PredicateInfo; after: PredicateInfo; + comparison: boolean; } function HighLevelStats(props: HighLevelStatsProps) { - const { before, after } = props; + const { before, after, comparison } = props; const hasBefore = before.absentReason !== AbsentReason.NotSeen; const hasAfter = after.absentReason !== AbsentReason.NotSeen; const showEvaluationCount = @@ -261,21 +265,25 @@ function HighLevelStats(props: HighLevelStatsProps) { <> - {hasBefore ? "Before" : ""} + {comparison && {hasBefore ? "Before" : ""}} {hasAfter ? "After" : ""} - {hasBefore && hasAfter ? "Delta" : ""} + {comparison && ( + {hasBefore && hasAfter ? "Delta" : ""} + )} Stats {showEvaluationCount && ( )} >( () => new Set(), ); @@ -478,9 +488,9 @@ function ComparePerformanceWithData(props: { - Before - After - Delta + {comparison && Before} + {comparison ? "After" : "Value"} + {comparison && Delta} Predicate @@ -503,14 +513,18 @@ function ComparePerformanceWithData(props: { - {renderAbsoluteValue(row.before, metric)} + {comparison && renderAbsoluteValue(row.before, metric)} {renderAbsoluteValue(row.after, metric)} - {renderDelta(row.diff, metric.unit)} + {comparison && renderDelta(row.diff, metric.unit)} {rowNames[rowIndex]} {expandedPredicates.has(row.name) && ( <> - + {collatePipelines( row.before.pipelines, row.after.pipelines, @@ -518,18 +532,23 @@ function ComparePerformanceWithData(props: { - {first != null && "Before"} + {comparison && ( + {first != null && "Before"} + )} {second != null && "After"} - - {first != null && second != null && "Delta"} - + {comparison && ( + + {first != null && second != null && "Delta"} + + )} Tuple counts for '{name}' pipeline - {first == null - ? " (after)" - : second == null - ? " (before)" - : ""} + {comparison && + (first == null + ? " (after)" + : second == null + ? " (before)" + : "")} {abbreviateRASteps(first?.steps ?? second!.steps).map( @@ -538,6 +557,7 @@ function ComparePerformanceWithData(props: { key={index} before={first?.counts[index]} after={second?.counts[index]} + comparison={comparison} step={step} /> ), @@ -556,9 +576,11 @@ function ComparePerformanceWithData(props: { - {formatDecimal(totalBefore)} + {comparison && ( + {formatDecimal(totalBefore)} + )} {formatDecimal(totalAfter)} - {renderDelta(totalDiff)} + {comparison && renderDelta(totalDiff)} TOTAL From 1d2c2cfcf95cdaaba8b7afe4a462b7261d3935e9 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 15 Nov 2024 12:56:29 +0100 Subject: [PATCH 16/46] Allow word wrap to break anywhere --- .../src/view/compare-performance/ComparePerformance.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 98925170d18..05a235579c5 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -180,6 +180,7 @@ const Table = styled.table` border: 1px solid var(--vscode-list-activeSelectionBackground); margin-bottom: 1em; } + word-break: break-all; `; const PredicateTR = styled.tr` From b05ec33ba322b0097f4b23258cac3c355b586266 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 22 Nov 2024 11:30:30 +0100 Subject: [PATCH 17/46] Use useMemo for 'nameSet' --- .../compare-performance/ComparePerformance.tsx | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 05a235579c5..cada10beb39 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -1,5 +1,5 @@ import type { ChangeEvent } from "react"; -import { useMemo, useState, Fragment } from "react"; +import { Fragment, useMemo, useState } from "react"; import type { SetPerformanceComparisonQueries, ToComparePerformanceViewMessage, @@ -354,6 +354,14 @@ function withToggledValue(set: Set, value: T) { return result; } +function union(a: Set | T[], b: Set | T[]) { + const result = new Set(a); + for (const x of b) { + result.add(x); + } + return result; +} + export function ComparePerformance(_: Record) { const [data, setData] = useState< SetPerformanceComparisonQueries | undefined @@ -398,10 +406,10 @@ function ComparePerformanceWithData(props: { const [metric, setMetric] = useState(metrics.tuples); - const nameSet = new Set(from.data.names); - for (const name of to.data.names) { - nameSet.add(name); - } + const nameSet = useMemo( + () => union(from.data.names, to.data.names), + [from, to], + ); let hasCacheHitMismatch = false; From 20f6e3d45c0c01576a224751182b6a99c0079bea Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 22 Nov 2024 11:32:15 +0100 Subject: [PATCH 18/46] Use useMemo a few places to speed up UI interactions --- .../ComparePerformance.tsx | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index cada10beb39..1f91aa6c343 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -1,5 +1,5 @@ import type { ChangeEvent } from "react"; -import { Fragment, useMemo, useState } from "react"; +import { Fragment, useMemo, useRef, useState } from "react"; import type { SetPerformanceComparisonQueries, ToComparePerformanceViewMessage, @@ -411,9 +411,9 @@ function ComparePerformanceWithData(props: { [from, to], ); - let hasCacheHitMismatch = false; + const hasCacheHitMismatch = useRef(false); - const rows = Array.from(nameSet) + const rows = useMemo(() => Array.from(nameSet) .map((name) => { const before = from.getTupleCountInfo(name); const after = to.getTupleCountInfo(name); @@ -426,7 +426,7 @@ function ComparePerformanceWithData(props: { before.absentReason === AbsentReason.CacheHit || after.absentReason === AbsentReason.CacheHit ) { - hasCacheHitMismatch = true; + hasCacheHitMismatch.current = true; if (hideCacheHits) { return undefined!; } @@ -435,7 +435,9 @@ function ComparePerformanceWithData(props: { return { name, before, after, diff }; }) .filter((x) => !!x) - .sort(getSortOrder(sortOrder)); + .sort(getSortOrder(sortOrder)), + [nameSet, from, to, metric, hideCacheHits, sortOrder], + ); let totalBefore = 0; let totalAfter = 0; @@ -447,12 +449,15 @@ function ComparePerformanceWithData(props: { totalDiff += row.diff; } - const rowNames = abbreviateRANames(rows.map((row) => row.name)); + const rowNames = useMemo( + () => abbreviateRANames(rows.map((row) => row.name)), + [rows], + ); return ( <> Performance comparison - {hasCacheHitMismatch && ( + {hasCacheHitMismatch.current && ( Inconsistent cache hits
From 558d957eb7a42ad414150a2bb3a586c4e28b7ac1 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 22 Nov 2024 11:32:29 +0100 Subject: [PATCH 19/46] Reformat code --- .../ComparePerformance.tsx | 48 ++++++++++--------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 1f91aa6c343..76b252baf25 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -413,29 +413,31 @@ function ComparePerformanceWithData(props: { const hasCacheHitMismatch = useRef(false); - const rows = useMemo(() => Array.from(nameSet) - .map((name) => { - const before = from.getTupleCountInfo(name); - const after = to.getTupleCountInfo(name); - const beforeValue = metric.get(before); - const afterValue = metric.get(after); - if (beforeValue === afterValue) { - return undefined!; - } - if ( - before.absentReason === AbsentReason.CacheHit || - after.absentReason === AbsentReason.CacheHit - ) { - hasCacheHitMismatch.current = true; - if (hideCacheHits) { - return undefined!; - } - } - const diff = afterValue - beforeValue; - return { name, before, after, diff }; - }) - .filter((x) => !!x) - .sort(getSortOrder(sortOrder)), + const rows = useMemo( + () => + Array.from(nameSet) + .map((name) => { + const before = from.getTupleCountInfo(name); + const after = to.getTupleCountInfo(name); + const beforeValue = metric.get(before); + const afterValue = metric.get(after); + if (beforeValue === afterValue) { + return undefined!; + } + if ( + before.absentReason === AbsentReason.CacheHit || + after.absentReason === AbsentReason.CacheHit + ) { + hasCacheHitMismatch.current = true; + if (hideCacheHits) { + return undefined!; + } + } + const diff = afterValue - beforeValue; + return { name, before, after, diff }; + }) + .filter((x) => !!x) + .sort(getSortOrder(sortOrder)), [nameSet, from, to, metric, hideCacheHits, sortOrder], ); From 6568b569a130d698c5a9e9ca6f8197d0f8b1b03d Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 22 Nov 2024 11:41:13 +0100 Subject: [PATCH 20/46] Also useMemo the 'total' row computation --- .../ComparePerformance.tsx | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 76b252baf25..d4fdc329977 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -441,15 +441,17 @@ function ComparePerformanceWithData(props: { [nameSet, from, to, metric, hideCacheHits, sortOrder], ); - let totalBefore = 0; - let totalAfter = 0; - let totalDiff = 0; - - for (const row of rows) { - totalBefore += metric.get(row.before); - totalAfter += metric.get(row.after); - totalDiff += row.diff; - } + const { totalBefore, totalAfter, totalDiff } = useMemo(() => { + let totalBefore = 0; + let totalAfter = 0; + let totalDiff = 0; + for (const row of rows) { + totalBefore += metric.get(row.before); + totalAfter += metric.get(row.after); + totalDiff += row.diff; + } + return { totalBefore, totalAfter, totalDiff }; + }, [rows, metric]); const rowNames = useMemo( () => abbreviateRANames(rows.map((row) => row.name)), From 6f7eb74496b7830cd7bc28a84020dde50e78486f Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 22 Nov 2024 11:55:54 +0100 Subject: [PATCH 21/46] Reset hasCacheMismatch when rebuilding 'rows' --- .../src/view/compare-performance/ComparePerformance.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index d4fdc329977..3d39cd31a45 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -411,11 +411,11 @@ function ComparePerformanceWithData(props: { [from, to], ); - const hasCacheHitMismatch = useRef(false); + const hasCacheHitMismatch = useRef(false); - const rows = useMemo( - () => - Array.from(nameSet) + const rows = useMemo(() => { + hasCacheHitMismatch.current = false; + return Array.from(nameSet) .map((name) => { const before = from.getTupleCountInfo(name); const after = to.getTupleCountInfo(name); From 62f3b4f6961217e41c27155fd2ab55c481660fe9 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 22 Nov 2024 11:57:52 +0100 Subject: [PATCH 22/46] Reformat code again Only contains formatting changes --- .../ComparePerformance.tsx | 43 +++++++++---------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 3d39cd31a45..a807afda987 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -416,30 +416,29 @@ function ComparePerformanceWithData(props: { const rows = useMemo(() => { hasCacheHitMismatch.current = false; return Array.from(nameSet) - .map((name) => { - const before = from.getTupleCountInfo(name); - const after = to.getTupleCountInfo(name); - const beforeValue = metric.get(before); - const afterValue = metric.get(after); - if (beforeValue === afterValue) { + .map((name) => { + const before = from.getTupleCountInfo(name); + const after = to.getTupleCountInfo(name); + const beforeValue = metric.get(before); + const afterValue = metric.get(after); + if (beforeValue === afterValue) { + return undefined!; + } + if ( + before.absentReason === AbsentReason.CacheHit || + after.absentReason === AbsentReason.CacheHit + ) { + hasCacheHitMismatch.current = true; + if (hideCacheHits) { return undefined!; } - if ( - before.absentReason === AbsentReason.CacheHit || - after.absentReason === AbsentReason.CacheHit - ) { - hasCacheHitMismatch.current = true; - if (hideCacheHits) { - return undefined!; - } - } - const diff = afterValue - beforeValue; - return { name, before, after, diff }; - }) - .filter((x) => !!x) - .sort(getSortOrder(sortOrder)), - [nameSet, from, to, metric, hideCacheHits, sortOrder], - ); + } + const diff = afterValue - beforeValue; + return { name, before, after, diff }; + }) + .filter((x) => !!x) + .sort(getSortOrder(sortOrder)); + }, [nameSet, from, to, metric, hideCacheHits, sortOrder]); const { totalBefore, totalAfter, totalDiff } = useMemo(() => { let totalBefore = 0; From 9800fa13338d03664093fa4fd61b7f76c06ff11a Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 22 Nov 2024 12:46:09 +0100 Subject: [PATCH 23/46] Use interface instead of type alias for TRow --- .../src/view/compare-performance/ComparePerformance.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index a807afda987..c6975acfa1c 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -295,12 +295,12 @@ function HighLevelStats(props: HighLevelStatsProps) { ); } -type TRow = { +interface TRow { name: string; before: PredicateInfo; after: PredicateInfo; diff: number; -}; +} function getSortOrder(sortOrder: "delta" | "absDelta") { if (sortOrder === "absDelta") { @@ -413,7 +413,7 @@ function ComparePerformanceWithData(props: { const hasCacheHitMismatch = useRef(false); - const rows = useMemo(() => { + const rows: TRow[] = useMemo(() => { hasCacheHitMismatch.current = false; return Array.from(nameSet) .map((name) => { @@ -434,7 +434,7 @@ function ComparePerformanceWithData(props: { } } const diff = afterValue - beforeValue; - return { name, before, after, diff }; + return { name, before, after, diff } satisfies TRow; }) .filter((x) => !!x) .sort(getSortOrder(sortOrder)); From d008963602fe7b8b9c92aa1b1869c66ec3c51b87 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 22 Nov 2024 14:04:10 +0100 Subject: [PATCH 24/46] Refactor OptionalValue --- .../ComparePerformance.tsx | 89 ++++++++++--------- 1 file changed, 49 insertions(+), 40 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index c6975acfa1c..7996841e47a 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -20,12 +20,14 @@ const enum AbsentReason { Sentinel = "Sentinel", } -interface OptionalValue { - absentReason: AbsentReason | undefined; - tuples: number; +type Optional = AbsentReason | T; + +function isPresent(x: Optional): x is T { + return typeof x !== "string"; } -interface PredicateInfo extends OptionalValue { +interface PredicateInfo { + tuples: number; evaluationCount: number; iterationCount: number; timeCost: number; @@ -47,26 +49,18 @@ class ComparisonDataset { this.sentinelEmptyIndices = new Set(data.sentinelEmptyIndices); } - getTupleCountInfo(name: string): PredicateInfo { + getTupleCountInfo(name: string): Optional { const { data, nameToIndex, cacheHitIndices, sentinelEmptyIndices } = this; const index = nameToIndex.get(name); if (index == null) { - return { - evaluationCount: 0, - iterationCount: 0, - tuples: 0, - timeCost: 0, - absentReason: AbsentReason.NotSeen, - pipelines: {}, - }; + return AbsentReason.NotSeen; } const tupleCost = data.tupleCosts[index]; - let absentReason: AbsentReason | undefined; if (tupleCost === 0) { if (sentinelEmptyIndices.has(index)) { - absentReason = AbsentReason.Sentinel; + return AbsentReason.Sentinel; } else if (cacheHitIndices.has(index)) { - absentReason = AbsentReason.CacheHit; + return AbsentReason.CacheHit; } } return { @@ -74,14 +68,13 @@ class ComparisonDataset { iterationCount: data.iterationCounts[index], timeCost: data.timeCosts[index], tuples: tupleCost, - absentReason, pipelines: data.pipelineSummaryList[index], }; } } -function renderAbsoluteValue(x: PredicateInfo, metric: Metric) { - switch (x.absentReason) { +function renderAbsoluteValue(x: Optional, metric: Metric) { + switch (x) { case AbsentReason.NotSeen: return n/a; case AbsentReason.CacheHit: @@ -251,17 +244,18 @@ const HeaderTR = styled.tr` `; interface HighLevelStatsProps { - before: PredicateInfo; - after: PredicateInfo; + before: Optional; + after: Optional; comparison: boolean; } function HighLevelStats(props: HighLevelStatsProps) { const { before, after, comparison } = props; - const hasBefore = before.absentReason !== AbsentReason.NotSeen; - const hasAfter = after.absentReason !== AbsentReason.NotSeen; + const hasBefore = isPresent(before); + const hasAfter = isPresent(after); const showEvaluationCount = - before.evaluationCount > 1 || after.evaluationCount > 1; + (hasBefore && before.evaluationCount > 1) || + (hasAfter && after.evaluationCount > 1); return ( <> @@ -275,15 +269,19 @@ function HighLevelStats(props: HighLevelStatsProps) { {showEvaluationCount && ( )} ; + after: Optional; diff: number; } @@ -332,7 +330,9 @@ const metrics: Record = { iterations: { title: "Iterations (per evaluation)", get: (info) => - info.absentReason ? 0 : info.iterationCount / info.evaluationCount, + info.evaluationCount === 0 + ? 0 + : info.iterationCount / info.evaluationCount, }, iterationsTotal: { title: "Iterations (total)", @@ -340,6 +340,13 @@ const metrics: Record = { }, }; +function metricGetOptional( + metric: Metric, + value: Optional, +): Optional { + return isPresent(value) ? metric.get(value) : value; +} + function Chevron({ expanded }: { expanded: boolean }) { return ; } @@ -419,21 +426,23 @@ function ComparePerformanceWithData(props: { .map((name) => { const before = from.getTupleCountInfo(name); const after = to.getTupleCountInfo(name); - const beforeValue = metric.get(before); - const afterValue = metric.get(after); + const beforeValue = metricGetOptional(metric, before); + const afterValue = metricGetOptional(metric, after); if (beforeValue === afterValue) { return undefined!; } if ( - before.absentReason === AbsentReason.CacheHit || - after.absentReason === AbsentReason.CacheHit + before === AbsentReason.CacheHit || + after === AbsentReason.CacheHit ) { hasCacheHitMismatch.current = true; if (hideCacheHits) { return undefined!; } } - const diff = afterValue - beforeValue; + const diff = + (isPresent(afterValue) ? afterValue : 0) - + (isPresent(beforeValue) ? beforeValue : 0); return { name, before, after, diff } satisfies TRow; }) .filter((x) => !!x) @@ -445,8 +454,8 @@ function ComparePerformanceWithData(props: { let totalAfter = 0; let totalDiff = 0; for (const row of rows) { - totalBefore += metric.get(row.before); - totalAfter += metric.get(row.after); + totalBefore += isPresent(row.before) ? metric.get(row.before) : 0; + totalAfter += isPresent(row.after) ? metric.get(row.after) : 0; totalDiff += row.diff; } return { totalBefore, totalAfter, totalDiff }; @@ -543,8 +552,8 @@ function ComparePerformanceWithData(props: { comparison={comparison} /> {collatePipelines( - row.before.pipelines, - row.after.pipelines, + isPresent(row.before) ? row.before.pipelines : {}, + isPresent(row.after) ? row.after.pipelines : {}, ).map(({ name, first, second }, pipelineIndex) => ( From eec42c55328af6100d296992f1a791675dfb9e19 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 22 Nov 2024 14:09:27 +0100 Subject: [PATCH 25/46] Split renderAbsoluteValue into renderOptionalValue and renderPredicateMetric --- .../compare-performance/ComparePerformance.tsx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 7996841e47a..1ef632b044e 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -73,7 +73,7 @@ class ComparisonDataset { } } -function renderAbsoluteValue(x: Optional, metric: Metric) { +function renderOptionalValue(x: Optional, unit?: string) { switch (x) { case AbsentReason.NotSeen: return n/a; @@ -84,13 +84,17 @@ function renderAbsoluteValue(x: Optional, metric: Metric) { default: return ( - {formatDecimal(metric.get(x))} - {renderUnit(metric.unit)} + {formatDecimal(x)} + {renderUnit(unit)} ); } } +function renderPredicateMetric(x: Optional, metric: Metric) { + return renderOptionalValue(metricGetOptional(metric, x), metric.unit); +} + function renderDelta(x: number, unit?: string) { const sign = x > 0 ? "+" : ""; return ( @@ -539,8 +543,8 @@ function ComparePerformanceWithData(props: { - {comparison && renderAbsoluteValue(row.before, metric)} - {renderAbsoluteValue(row.after, metric)} + {comparison && renderPredicateMetric(row.before, metric)} + {renderPredicateMetric(row.after, metric)} {comparison && renderDelta(row.diff, metric.unit)} {rowNames[rowIndex]} From 9a0699f50ab9940b835909b3b001ccb13ba0e9f8 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 25 Nov 2024 09:16:42 +0100 Subject: [PATCH 26/46] Refactor predicate row into a separate component --- .../ComparePerformance.tsx | 172 +++++++++--------- 1 file changed, 86 insertions(+), 86 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 1ef632b044e..5ac4ea3c2a2 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -355,16 +355,6 @@ function Chevron({ expanded }: { expanded: boolean }) { return ; } -function withToggledValue(set: Set, value: T) { - const result = new Set(set); - if (result.has(value)) { - result.delete(value); - } else { - result.add(value); - } - return result; -} - function union(a: Set | T[], b: Set | T[]) { const result = new Set(a); for (const x of b) { @@ -407,10 +397,6 @@ function ComparePerformanceWithData(props: { const comparison = data?.comparison; - const [expandedPredicates, setExpandedPredicates] = useState>( - () => new Set(), - ); - const [hideCacheHits, setHideCacheHits] = useState(false); const [sortOrder, setSortOrder] = useState<"delta" | "absDelta">("absDelta"); @@ -526,78 +512,13 @@ function ComparePerformanceWithData(props: {
{rows.map((row, rowIndex) => ( - - - - setExpandedPredicates( - withToggledValue(expandedPredicates, row.name), - ) - } - > - - - - {comparison && renderPredicateMetric(row.before, metric)} - {renderPredicateMetric(row.after, metric)} - {comparison && renderDelta(row.diff, metric.unit)} - {rowNames[rowIndex]} - - {expandedPredicates.has(row.name) && ( - <> - - {collatePipelines( - isPresent(row.before) ? row.before.pipelines : {}, - isPresent(row.after) ? row.after.pipelines : {}, - ).map(({ name, first, second }, pipelineIndex) => ( - - - - {comparison && ( - {first != null && "Before"} - )} - {second != null && "After"} - {comparison && ( - - {first != null && second != null && "Delta"} - - )} - - Tuple counts for '{name}' pipeline - {comparison && - (first == null - ? " (after)" - : second == null - ? " (before)" - : "")} - - - {abbreviateRASteps(first?.steps ?? second!.steps).map( - (step, index) => ( - - ), - )} - - ))} - - )} - -
+ ))} @@ -619,6 +540,85 @@ function ComparePerformanceWithData(props: { ); } +interface PredicateRowProps { + renderedName: React.ReactNode; + row: TRow; + comparison: boolean; + metric: Metric; +} + +function PredicateRow(props: PredicateRowProps) { + const [isExpanded, setExpanded] = useState(false); + const { renderedName, row, comparison, metric } = props; + return ( +
+ + setExpanded(!isExpanded)} + > + + + + {comparison && renderPredicateMetric(row.before, metric)} + {renderPredicateMetric(row.after, metric)} + {comparison && renderDelta(row.diff, metric.unit)} + {renderedName} + + {isExpanded && ( + <> + + {collatePipelines( + isPresent(row.before) ? row.before.pipelines : {}, + isPresent(row.after) ? row.after.pipelines : {}, + ).map(({ name, first, second }, pipelineIndex) => ( + + + + {comparison && ( + {first != null && "Before"} + )} + {second != null && "After"} + {comparison && ( + + {first != null && second != null && "Delta"} + + )} + + Tuple counts for '{name}' pipeline + {comparison && + (first == null + ? " (after)" + : second == null + ? " (before)" + : "")} + + + {abbreviateRASteps(first?.steps ?? second!.steps).map( + (step, index) => ( + + ), + )} + + ))} + + )} + +
+ ); +} + interface PipelinePair { name: string; first: PipelineSummary | undefined; From 48954c7d22b555c610cf353cbdf0649cad5306d7 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 25 Nov 2024 09:22:12 +0100 Subject: [PATCH 27/46] Rename TRow -> Row --- .../view/compare-performance/ComparePerformance.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 5ac4ea3c2a2..fb6c12e1bf2 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -297,7 +297,7 @@ function HighLevelStats(props: HighLevelStatsProps) { ); } -interface TRow { +interface Row { name: string; before: Optional; after: Optional; @@ -306,9 +306,9 @@ interface TRow { function getSortOrder(sortOrder: "delta" | "absDelta") { if (sortOrder === "absDelta") { - return orderBy((row: TRow) => -Math.abs(row.diff)); + return orderBy((row: Row) => -Math.abs(row.diff)); } - return orderBy((row: TRow) => row.diff); + return orderBy((row: Row) => row.diff); } interface Metric { @@ -410,7 +410,7 @@ function ComparePerformanceWithData(props: { const hasCacheHitMismatch = useRef(false); - const rows: TRow[] = useMemo(() => { + const rows: Row[] = useMemo(() => { hasCacheHitMismatch.current = false; return Array.from(nameSet) .map((name) => { @@ -433,7 +433,7 @@ function ComparePerformanceWithData(props: { const diff = (isPresent(afterValue) ? afterValue : 0) - (isPresent(beforeValue) ? beforeValue : 0); - return { name, before, after, diff } satisfies TRow; + return { name, before, after, diff } satisfies Row; }) .filter((x) => !!x) .sort(getSortOrder(sortOrder)); @@ -542,7 +542,7 @@ function ComparePerformanceWithData(props: { interface PredicateRowProps { renderedName: React.ReactNode; - row: TRow; + row: Row; comparison: boolean; metric: Metric; } From ab00152ce29b9e56e1002fa81ab1d6315f17a779 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 25 Nov 2024 12:03:31 +0100 Subject: [PATCH 28/46] Group together rows by fingerprinting --- .../ComparePerformance.tsx | 139 ++++++++++++++++-- 1 file changed, 129 insertions(+), 10 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index fb6c12e1bf2..0aaa7619a8c 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -304,11 +304,22 @@ interface Row { diff: number; } +/** + * A set of predicates that have been grouped together because their names have the same fingerprint. + */ +interface RowGroup { + name: string; + rows: Row[]; + before: Optional; + after: Optional; + diff: number; +} + function getSortOrder(sortOrder: "delta" | "absDelta") { if (sortOrder === "absDelta") { - return orderBy((row: Row) => -Math.abs(row.diff)); + return orderBy((row: { diff: number }) => -Math.abs(row.diff)); } - return orderBy((row: Row) => row.diff); + return orderBy((row: { diff: number }) => row.diff); } interface Metric { @@ -351,6 +362,30 @@ function metricGetOptional( return isPresent(value) ? metric.get(value) : value; } +function addOptionals(a: Optional, b: Optional) { + if (isPresent(a) && isPresent(b)) { + return a + b; + } + if (isPresent(a)) { + return a; + } + if (isPresent(b)) { + return b; + } + if (a === b) { + return a; // If absent for the same reason, preserve that reason + } + return 0; // Otherwise collapse to zero +} + +/** + * Returns a "fingerprint" from the given name, which is used to group together similar names. + */ +export function getNameFingerprint(name: string) { + // For now just remove the hash from the name. We identify this as a '#' followed by exactly 8 hexadecimal characters. + return name.replace(/#[0-9a-f]{8}(?![0-9a-f])/g, ""); +} + function Chevron({ expanded }: { expanded: boolean }) { return ; } @@ -451,9 +486,40 @@ function ComparePerformanceWithData(props: { return { totalBefore, totalAfter, totalDiff }; }, [rows, metric]); - const rowNames = useMemo( - () => abbreviateRANames(rows.map((row) => row.name)), - [rows], + const rowGroups = useMemo(() => { + const groupedRows = new Map(); + for (const row of rows) { + const fingerprint = getNameFingerprint(row.name); + const rows = groupedRows.get(fingerprint); + if (rows) { + rows.push(row); + } else { + groupedRows.set(fingerprint, [row]); + } + } + return Array.from(groupedRows.entries()) + .map(([fingerprint, rows]) => { + const before = rows + .map((row) => metricGetOptional(metric, row.before)) + .reduce(addOptionals); + const after = rows + .map((row) => metricGetOptional(metric, row.after)) + .reduce(addOptionals); + return { + name: rows.length === 1 ? rows[0].name : fingerprint, + before, + after, + diff: + (isPresent(after) ? after : 0) - (isPresent(before) ? before : 0), + rows, + } satisfies RowGroup; + }) + .sort(getSortOrder(sortOrder)); + }, [rows, metric, sortOrder]); + + const rowGroupNames = useMemo( + () => abbreviateRANames(rowGroups.map((group) => group.name)), + [rowGroups], ); return ( @@ -511,11 +577,11 @@ function ComparePerformanceWithData(props: { - {rows.map((row, rowIndex) => ( - ( + @@ -540,6 +606,59 @@ function ComparePerformanceWithData(props: { ); } +interface PredicateRowGroupProps { + renderedName: React.ReactNode; + rowGroup: RowGroup; + comparison: boolean; + metric: Metric; +} + +function PredicateRowGroup(props: PredicateRowGroupProps) { + const { renderedName, rowGroup, comparison, metric } = props; + const [isExpanded, setExpanded] = useState(false); + const rowNames = useMemo( + () => abbreviateRANames(rowGroup.rows.map((row) => row.name)), + [rowGroup], + ); + if (rowGroup.rows.length === 1) { + return ; + } + return ( + + + setExpanded(!isExpanded)} + > + + + + {comparison && renderOptionalValue(rowGroup.before)} + {renderOptionalValue(rowGroup.after)} + {comparison && renderDelta(rowGroup.diff, metric.unit)} + + {renderedName} ({rowGroup.rows.length} predicates) + + + {isExpanded && + rowGroup.rows.map((row, rowIndex) => ( + + + + ))} + +
+ +
+ ); +} + interface PredicateRowProps { renderedName: React.ReactNode; row: Row; From 4a835b8711790efc17e6f9b94125632aebd94ef7 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 25 Nov 2024 14:34:53 +0100 Subject: [PATCH 29/46] Factor out rendering of table body to a memoized component --- .../ComparePerformance.tsx | 39 ++++++++++++++----- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 0aaa7619a8c..1b89b26a49d 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -1,5 +1,5 @@ import type { ChangeEvent } from "react"; -import { Fragment, useMemo, useRef, useState } from "react"; +import { Fragment, memo, useMemo, useRef, useState } from "react"; import type { SetPerformanceComparisonQueries, ToComparePerformanceViewMessage, @@ -577,15 +577,12 @@ function ComparePerformanceWithData(props: { - {rowGroups.map((rowGroup, rowGroupIndex) => ( - - ))} + @@ -606,6 +603,28 @@ function ComparePerformanceWithData(props: { ); } +interface PredicateTableProps { + rowGroups: RowGroup[]; + rowGroupNames: React.ReactNode[]; + comparison: boolean; + metric: Metric; +} + +function PredicateTableRaw(props: PredicateTableProps) { + const { comparison, metric, rowGroupNames, rowGroups } = props; + return rowGroups.map((rowGroup, rowGroupIndex) => ( + + )); +} + +const PredicateTable = memo(PredicateTableRaw); + interface PredicateRowGroupProps { renderedName: React.ReactNode; rowGroup: RowGroup; From 568f0827b277b90e9952c6cbb74157acc438e7ca Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 25 Nov 2024 16:36:03 +0100 Subject: [PATCH 30/46] Fix some crashes when pretty-printing an empty name Predicate names can't be empty, but it can happen with the renaming feature added in the next commit. --- .../src/view/compare-performance/RAPrettyPrinter.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx index 39038a8e520..f6dd7720e5a 100644 --- a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx @@ -70,14 +70,14 @@ function parseName(text: string): QualifiedName { let args: QualifiedName[] | undefined; if (skipToken(">")) { args = []; - while (peek() !== "<") { + while (tokens.length > 0 && peek() !== "<") { args.push(parseQName()); skipToken(","); } args.reverse(); skipToken("<"); } - const name = next(); + const name = tokens.length === 0 ? "" : next(); const prefix = skipToken("::") ? parseQName() : undefined; return { prefix, From 2cae71c657f46454d0e7f7e75556bf175e461b53 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 25 Nov 2024 16:37:26 +0100 Subject: [PATCH 31/46] Add predicate-renaming support --- .../ComparePerformance.tsx | 32 +++++- .../compare-performance/RenamingInput.tsx | 106 ++++++++++++++++++ 2 files changed, 132 insertions(+), 6 deletions(-) create mode 100644 extensions/ql-vscode/src/view/compare-performance/RenamingInput.tsx diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 1b89b26a49d..8c2f3989395 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -1,5 +1,12 @@ import type { ChangeEvent } from "react"; -import { Fragment, memo, useMemo, useRef, useState } from "react"; +import { + Fragment, + memo, + useDeferredValue, + useMemo, + useRef, + useState, +} from "react"; import type { SetPerformanceComparisonQueries, ToComparePerformanceViewMessage, @@ -13,6 +20,7 @@ import { formatDecimal } from "../../common/number"; import { styled } from "styled-components"; import { Codicon, ViewTitle, WarningBox } from "../common"; import { abbreviateRANames, abbreviateRASteps } from "./RAPrettyPrinter"; +import { Renaming, RenamingInput } from "./RenamingInput"; const enum AbsentReason { NotSeen = "NotSeen", @@ -381,9 +389,13 @@ function addOptionals(a: Optional, b: Optional) { /** * Returns a "fingerprint" from the given name, which is used to group together similar names. */ -export function getNameFingerprint(name: string) { - // For now just remove the hash from the name. We identify this as a '#' followed by exactly 8 hexadecimal characters. - return name.replace(/#[0-9a-f]{8}(?![0-9a-f])/g, ""); +export function getNameFingerprint(name: string, renamings: Renaming[]) { + for (const { patternRegexp, replacement } of renamings) { + if (patternRegexp != null) { + name = name.replace(patternRegexp, replacement); + } + } + return name; } function Chevron({ expanded }: { expanded: boolean }) { @@ -486,10 +498,17 @@ function ComparePerformanceWithData(props: { return { totalBefore, totalAfter, totalDiff }; }, [rows, metric]); + const [renamings, setRenamings] = useState(() => [ + new Renaming("#[0-9a-f]{8}(?![0-9a-f])", "#"), + ]); + + // Use deferred value to avoid expensive re-rendering for every keypress in the renaming editor + const deferredRenamings = useDeferredValue(renamings); + const rowGroups = useMemo(() => { const groupedRows = new Map(); for (const row of rows) { - const fingerprint = getNameFingerprint(row.name); + const fingerprint = getNameFingerprint(row.name, deferredRenamings); const rows = groupedRows.get(fingerprint); if (rows) { rows.push(row); @@ -515,7 +534,7 @@ function ComparePerformanceWithData(props: { } satisfies RowGroup; }) .sort(getSortOrder(sortOrder)); - }, [rows, metric, sortOrder]); + }, [rows, metric, sortOrder, deferredRenamings]); const rowGroupNames = useMemo( () => abbreviateRANames(rowGroups.map((group) => group.name)), @@ -544,6 +563,7 @@ function ComparePerformanceWithData(props: { )} + Compare{" "} ) => diff --git a/extensions/ql-vscode/src/view/compare-performance/RenamingInput.tsx b/extensions/ql-vscode/src/view/compare-performance/RenamingInput.tsx new file mode 100644 index 00000000000..6d86c7e8182 --- /dev/null +++ b/extensions/ql-vscode/src/view/compare-performance/RenamingInput.tsx @@ -0,0 +1,106 @@ +import type { ChangeEvent } from "react"; +import { styled } from "styled-components"; +import { + VSCodeButton, + VSCodeTextField, +} from "@vscode/webview-ui-toolkit/react"; +import { Codicon } from "../common"; + +export class Renaming { + patternRegexp: RegExp | undefined; + + constructor( + public pattern: string, + public replacement: string, + ) { + this.patternRegexp = tryCompilePattern(pattern); + } +} + +function tryCompilePattern(pattern: string): RegExp | undefined { + try { + return new RegExp(pattern, "i"); + } catch { + return undefined; + } +} + +const Input = styled(VSCodeTextField)` + width: 20em; +`; + +const Row = styled.div` + display: flex; + padding-bottom: 0.25em; +`; + +const Details = styled.details` + padding: 1em; +`; + +interface RenamingInputProps { + renamings: Renaming[]; + setRenamings: (renamings: Renaming[]) => void; +} + +export function RenamingInput(props: RenamingInputProps) { + const { renamings, setRenamings } = props; + return ( +
+ Predicate renaming +

+ The following regexp replacements are applied to every predicate name on + both sides. Predicates whose names clash after renaming are grouped + together. Can be used to correlate predicates that were renamed between + the two runs. +
+ Can also be used to group related predicates, for example, renaming{" "} + .*ssa.* to SSA will group all SSA-related + predicates together. +

+ {renamings.map((renaming, index) => ( + + ) => { + const newRenamings = [...renamings]; + newRenamings[index] = new Renaming( + e.target.value, + renaming.replacement, + ); + setRenamings(newRenamings); + }} + > + + + ) => { + const newRenamings = [...renamings]; + newRenamings[index] = new Renaming( + renaming.pattern, + e.target.value, + ); + setRenamings(newRenamings); + }} + > + + setRenamings(renamings.filter((_, i) => i !== index)) + } + > + + +
+
+ ))} + setRenamings([...renamings, new Renaming("", "")])} + > + Add renaming rule + +
+ ); +} From d37469fc94fd46157371e94408b7f4186a725c8d Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 26 Nov 2024 10:21:19 +0100 Subject: [PATCH 32/46] Add 'per evaluation' option next to metric --- .../ComparePerformance.tsx | 87 +++++++++++++------ 1 file changed, 60 insertions(+), 27 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 8c2f3989395..7f1a5ff7081 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -99,8 +99,15 @@ function renderOptionalValue(x: Optional, unit?: string) { } } -function renderPredicateMetric(x: Optional, metric: Metric) { - return renderOptionalValue(metricGetOptional(metric, x), metric.unit); +function renderPredicateMetric( + x: Optional, + metric: Metric, + isPerEvaluation: boolean, +) { + return renderOptionalValue( + metricGetOptional(metric, x, isPerEvaluation), + metric.unit, + ); } function renderDelta(x: number, unit?: string) { @@ -338,11 +345,11 @@ interface Metric { const metrics: Record = { tuples: { - title: "Tuples in pipeline", + title: "Tuple count", get: (info) => info.tuples, }, time: { - title: "Time spent (milliseconds)", + title: "Time spent", get: (info) => info.timeCost, unit: "ms", }, @@ -350,24 +357,22 @@ const metrics: Record = { title: "Evaluations", get: (info) => info.evaluationCount, }, - iterations: { - title: "Iterations (per evaluation)", - get: (info) => - info.evaluationCount === 0 - ? 0 - : info.iterationCount / info.evaluationCount, - }, iterationsTotal: { - title: "Iterations (total)", + title: "Iterations", get: (info) => info.iterationCount, }, }; function metricGetOptional( metric: Metric, - value: Optional, + info: Optional, + isPerEvaluation: boolean, ): Optional { - return isPresent(value) ? metric.get(value) : value; + if (!isPresent(info)) { + return info; + } + const value = metric.get(info); + return isPerEvaluation ? (value / info.evaluationCount) | 0 : value; } function addOptionals(a: Optional, b: Optional) { @@ -450,6 +455,8 @@ function ComparePerformanceWithData(props: { const [metric, setMetric] = useState(metrics.tuples); + const [isPerEvaluation, setPerEvaluation] = useState(false); + const nameSet = useMemo( () => union(from.data.names, to.data.names), [from, to], @@ -463,8 +470,8 @@ function ComparePerformanceWithData(props: { .map((name) => { const before = from.getTupleCountInfo(name); const after = to.getTupleCountInfo(name); - const beforeValue = metricGetOptional(metric, before); - const afterValue = metricGetOptional(metric, after); + const beforeValue = metricGetOptional(metric, before, isPerEvaluation); + const afterValue = metricGetOptional(metric, after, isPerEvaluation); if (beforeValue === afterValue) { return undefined!; } @@ -484,7 +491,7 @@ function ComparePerformanceWithData(props: { }) .filter((x) => !!x) .sort(getSortOrder(sortOrder)); - }, [nameSet, from, to, metric, hideCacheHits, sortOrder]); + }, [nameSet, from, to, metric, hideCacheHits, sortOrder, isPerEvaluation]); const { totalBefore, totalAfter, totalDiff } = useMemo(() => { let totalBefore = 0; @@ -519,10 +526,10 @@ function ComparePerformanceWithData(props: { return Array.from(groupedRows.entries()) .map(([fingerprint, rows]) => { const before = rows - .map((row) => metricGetOptional(metric, row.before)) + .map((row) => metricGetOptional(metric, row.before, isPerEvaluation)) .reduce(addOptionals); const after = rows - .map((row) => metricGetOptional(metric, row.after)) + .map((row) => metricGetOptional(metric, row.after, isPerEvaluation)) .reduce(addOptionals); return { name: rows.length === 1 ? rows[0].name : fingerprint, @@ -534,7 +541,7 @@ function ComparePerformanceWithData(props: { } satisfies RowGroup; }) .sort(getSortOrder(sortOrder)); - }, [rows, metric, sortOrder, deferredRenamings]); + }, [rows, metric, sortOrder, deferredRenamings, isPerEvaluation]); const rowGroupNames = useMemo( () => abbreviateRANames(rowGroups.map((group) => group.name)), @@ -576,6 +583,14 @@ function ComparePerformanceWithData(props: { ))}
{" "} + ) => + setPerEvaluation(e.target.value === "per-evaluation") + } + > + + + {" "} sorted by{" "} ) => @@ -602,6 +617,7 @@ function ComparePerformanceWithData(props: { rowGroupNames={rowGroupNames} comparison={comparison} metric={metric} + isPerEvaluation={isPerEvaluation} />
@@ -628,10 +644,12 @@ interface PredicateTableProps { rowGroupNames: React.ReactNode[]; comparison: boolean; metric: Metric; + isPerEvaluation: boolean; } function PredicateTableRaw(props: PredicateTableProps) { - const { comparison, metric, rowGroupNames, rowGroups } = props; + const { comparison, metric, rowGroupNames, rowGroups, isPerEvaluation } = + props; return rowGroups.map((rowGroup, rowGroupIndex) => ( )); } @@ -650,10 +669,11 @@ interface PredicateRowGroupProps { rowGroup: RowGroup; comparison: boolean; metric: Metric; + isPerEvaluation: boolean; } function PredicateRowGroup(props: PredicateRowGroupProps) { - const { renderedName, rowGroup, comparison, metric } = props; + const { renderedName, rowGroup, comparison, metric, isPerEvaluation } = props; const [isExpanded, setExpanded] = useState(false); const rowNames = useMemo( () => abbreviateRANames(rowGroup.rows.map((row) => row.name)), @@ -689,6 +709,7 @@ function PredicateRowGroup(props: PredicateRowGroupProps) { row={row} comparison={comparison} metric={metric} + isPerEvaluation={isPerEvaluation} /> @@ -703,11 +724,16 @@ interface PredicateRowProps { row: Row; comparison: boolean; metric: Metric; + isPerEvaluation: boolean; } function PredicateRow(props: PredicateRowProps) { const [isExpanded, setExpanded] = useState(false); - const { renderedName, row, comparison, metric } = props; + const { renderedName, row, comparison, metric, isPerEvaluation } = props; + const evaluationFactorBefore = + isPerEvaluation && isPresent(row.before) ? row.before.evaluationCount : 1; + const evaluationFactorAfter = + isPerEvaluation && isPresent(row.after) ? row.after.evaluationCount : 1; return (
@@ -719,8 +745,9 @@ function PredicateRow(props: PredicateRowProps) { - {comparison && renderPredicateMetric(row.before, metric)} - {renderPredicateMetric(row.after, metric)} + {comparison && + renderPredicateMetric(row.before, metric, isPerEvaluation)} + {renderPredicateMetric(row.after, metric, isPerEvaluation)} {comparison && renderDelta(row.diff, metric.unit)} {renderedName} @@ -761,8 +788,14 @@ function PredicateRow(props: PredicateRowProps) { (step, index) => ( From 8a58279e671fda7a45bd6257bf7f69109a759402 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 27 Nov 2024 10:20:21 +0100 Subject: [PATCH 33/46] Move "total" row to the top and render the metric --- .../ComparePerformance.tsx | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 7f1a5ff7081..411a8ed4bee 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -611,6 +611,20 @@ function ComparePerformanceWithData(props: { Predicate + + + + {comparison && renderOptionalValue(totalBefore, metric.unit)} + {renderOptionalValue(totalAfter, metric.unit)} + {comparison && renderDelta(totalDiff, metric.unit)} + + TOTAL + + + + + +
- - - - - - - - {comparison && ( - {formatDecimal(totalBefore)} - )} - {formatDecimal(totalAfter)} - {comparison && renderDelta(totalDiff)} - TOTAL - - -
); } From b9d15511cbefab1b5e129b9e580c57848a233bee Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 27 Nov 2024 10:49:32 +0100 Subject: [PATCH 34/46] Hide "sort by" dropdown when there is no delta to sort by --- .../ComparePerformance.tsx | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 411a8ed4bee..51aa32cb663 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -591,16 +591,20 @@ function ComparePerformanceWithData(props: { {" "} - sorted by{" "} - ) => - setSortOrder(e.target.value as "delta" | "absDelta") - } - value={sortOrder} - > - - - + {comparison && ( + <> + sorted by{" "} + ) => + setSortOrder(e.target.value as "delta" | "absDelta") + } + value={sortOrder} + > + + + + + )} From afa3d558c633f9278d329e7a2fb1ac5710a8c67d Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 27 Nov 2024 10:54:37 +0100 Subject: [PATCH 35/46] Factor header rows into a component --- .../ComparePerformance.tsx | 86 +++++++++++-------- 1 file changed, 49 insertions(+), 37 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 51aa32cb663..97d132b3ac2 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -262,6 +262,32 @@ const HeaderTR = styled.tr` background-color: var(--vscode-sideBar-background); `; +interface HeaderRowProps { + hasBefore?: boolean; + hasAfter?: boolean; + comparison: boolean; + title: React.ReactNode; +} + +function HeaderRow(props: HeaderRowProps) { + const { comparison, hasBefore, hasAfter, title } = props; + return ( + + + {comparison ? ( + <> + {hasBefore ? "Before" : ""} + {hasAfter ? "After" : ""} + {hasBefore && hasAfter ? "Delta" : ""} + + ) : ( + Value + )} + {title} + + ); +} + interface HighLevelStatsProps { before: Optional; after: Optional; @@ -277,15 +303,12 @@ function HighLevelStats(props: HighLevelStatsProps) { (hasAfter && after.evaluationCount > 1); return ( <> - - - {comparison && {hasBefore ? "Before" : ""}} - {hasAfter ? "After" : ""} - {comparison && ( - {hasBefore && hasAfter ? "Delta" : ""} - )} - Stats - + {showEvaluationCount && ( - - - {comparison && Before} - {comparison ? "After" : "Value"} - {comparison && Delta} - Predicate - + @@ -765,27 +782,22 @@ function PredicateRow(props: PredicateRowProps) { isPresent(row.after) ? row.after.pipelines : {}, ).map(({ name, first, second }, pipelineIndex) => ( - - - {comparison && ( - {first != null && "Before"} - )} - {second != null && "After"} - {comparison && ( - - {first != null && second != null && "Delta"} - - )} - - Tuple counts for '{name}' pipeline - {comparison && - (first == null - ? " (after)" - : second == null - ? " (before)" - : "")} - - + + Tuple counts for '{name}' pipeline + {comparison && + (first == null + ? " (after)" + : second == null + ? " (before)" + : "")} + + } + /> {abbreviateRASteps(first?.steps ?? second!.steps).map( (step, index) => ( Date: Wed, 27 Nov 2024 11:55:18 +0100 Subject: [PATCH 36/46] Only warn about cache hits in comparison mode --- .../src/view/compare-performance/ComparePerformance.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 97d132b3ac2..2c7f53043a4 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -574,7 +574,7 @@ function ComparePerformanceWithData(props: { return ( <> Performance comparison - {hasCacheHitMismatch.current && ( + {comparison && hasCacheHitMismatch.current && ( Inconsistent cache hits
From aa528c60379b3f1348d0351951272fe5bd920dd7 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 27 Nov 2024 11:55:48 +0100 Subject: [PATCH 37/46] Remove unused export --- .../src/view/compare-performance/ComparePerformance.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 2c7f53043a4..cff08fb3881 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -417,7 +417,7 @@ function addOptionals(a: Optional, b: Optional) { /** * Returns a "fingerprint" from the given name, which is used to group together similar names. */ -export function getNameFingerprint(name: string, renamings: Renaming[]) { +function getNameFingerprint(name: string, renamings: Renaming[]) { for (const { patternRegexp, replacement } of renamings) { if (patternRegexp != null) { name = name.replace(patternRegexp, replacement); From 6f461e75a775a2c05d1b90ad61b88d92a3c50600 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 17 Jan 2025 10:40:09 +0100 Subject: [PATCH 38/46] Update extensions/ql-vscode/package.json Restrict to Canary Co-authored-by: Andrew Eisenberg --- extensions/ql-vscode/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index bb0bee713ac..9fab7bbc172 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -1237,7 +1237,7 @@ { "command": "codeQLQueryHistory.comparePerformanceWith", "group": "3_queryHistory@1", - "when": "viewItem == rawResultsItem || viewItem == interpretedResultsItem" + "when": "viewItem == rawResultsItem && config.codeQL.canary || viewItem == interpretedResultsItem && config.codeQL.canary" }, { "command": "codeQLQueryHistory.showQueryLog", From 2293cc35370580e177bfffe27a4d65cf3bbaf502 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 17 Jan 2025 10:43:15 +0100 Subject: [PATCH 39/46] Update extensions/ql-vscode/src/log-insights/log-scanner.ts Co-authored-by: Andrew Eisenberg --- extensions/ql-vscode/src/log-insights/log-scanner.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/log-insights/log-scanner.ts b/extensions/ql-vscode/src/log-insights/log-scanner.ts index 55b655c2580..0ad775e6ec1 100644 --- a/extensions/ql-vscode/src/log-insights/log-scanner.ts +++ b/extensions/ql-vscode/src/log-insights/log-scanner.ts @@ -126,7 +126,7 @@ export async function scanLog( progress?: ProgressCallback, ): Promise { progress?.({ - // XXX all scans have step 1 - the backing progress tracker allows increments instead of steps - but for now we are happy with a tiny UI that says what is happening + // all scans have step 1 - the backing progress tracker allows increments instead of steps - but for now we are happy with a tiny UI that says what is happening message: `Scanning ...`, step: 1, maxStep: 2, From 08bffab05f162fea879e13a97ffa3e5f87af7cfc Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 17 Jan 2025 10:51:49 +0100 Subject: [PATCH 40/46] Add more description of the "struct of arrays" layout --- .../ql-vscode/src/log-insights/performance-comparison.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/log-insights/performance-comparison.ts b/extensions/ql-vscode/src/log-insights/performance-comparison.ts index 14f4d343b8a..2867ae6b5d2 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -17,7 +17,14 @@ export interface PipelineSummary { * to a web view (which rules out `Map` values, for example). */ export interface PerformanceComparisonDataFromLog { - /** Names of predicates mentioned in the log */ + /** + * Names of predicates mentioned in the log. + * + * For compactness, details of these predicates are stored in a "struct of arrays" style. + * + * All fields (except those ending with `Indices`) should contain an array of the same length as `names`; + * details of a given predicate should be stored at the same index in each of those arrays. + */ names: string[]; /** Number of milliseconds spent evaluating the `i`th predicate from the `names` array. */ From f09210b03374298da278ebbcf241db9d8e826b83 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 17 Jan 2025 11:26:40 +0100 Subject: [PATCH 41/46] Only record cache hits prior to first evaluation --- .../src/log-insights/performance-comparison.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/extensions/ql-vscode/src/log-insights/performance-comparison.ts b/extensions/ql-vscode/src/log-insights/performance-comparison.ts index 2867ae6b5d2..894c318363e 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -115,9 +115,13 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { } case "CACHE_HIT": case "CACHACA": { - this.data.cacheHitIndices.push( - this.getPredicateIndex(event.predicateName), - ); + // Record a cache hit, but only if the predicate has not been seen before. + // We're mainly interested in the reuse of caches from an earlier query run as they can distort comparisons. + if (!this.nameToIndex.has(event.predicateName)) { + this.data.cacheHitIndices.push( + this.getPredicateIndex(event.predicateName), + ); + } break; } case "SENTINEL_EMPTY": { From 37dcd0822bd86a9f02cf3a6ca43e47cf1f2771c5 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 17 Jan 2025 11:34:54 +0100 Subject: [PATCH 42/46] Remove TODO that was just resolved --- extensions/ql-vscode/src/log-insights/performance-comparison.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/extensions/ql-vscode/src/log-insights/performance-comparison.ts b/extensions/ql-vscode/src/log-insights/performance-comparison.ts index 894c318363e..ca5f24782f0 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -41,8 +41,6 @@ export interface PerformanceComparisonDataFromLog { /** * List of indices into the `names` array for which we have seen a cache hit. - * - * TODO: only count cache hits prior to first evaluation? */ cacheHitIndices: number[]; From 370b17c0f50a86b1c6d36338c7fb2aa6d2754bbd Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 17 Jan 2025 11:35:16 +0100 Subject: [PATCH 43/46] Remove TODOs --- .../ql-vscode/src/log-insights/performance-comparison.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/extensions/ql-vscode/src/log-insights/performance-comparison.ts b/extensions/ql-vscode/src/log-insights/performance-comparison.ts index ca5f24782f0..46634cf8bf3 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -51,8 +51,6 @@ export interface PerformanceComparisonDataFromLog { /** * All the pipeline runs seen for the `i`th predicate from the `names` array. - * - * TODO: replace with more compact representation */ pipelineSummaryList: Array>; } @@ -161,7 +159,6 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { }); const { counts: totalTuplesPerStep } = pipelineSummary; for (let i = 0, length = counts.length; i < length; ++i) { - // TODO: possibly exclude unions here const count = counts[i]; if (count < 0) { // Empty RA lines have a tuple count of -1. Do not count them when aggregating. From 8b8d17478170ca7c33a941448714e78285264ca2 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 17 Jan 2025 11:36:08 +0100 Subject: [PATCH 44/46] Clean up some more TODOs --- extensions/ql-vscode/src/query-history/query-history-manager.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/extensions/ql-vscode/src/query-history/query-history-manager.ts b/extensions/ql-vscode/src/query-history/query-history-manager.ts index 35f241c52de..0296b32d503 100644 --- a/extensions/ql-vscode/src/query-history/query-history-manager.ts +++ b/extensions/ql-vscode/src/query-history/query-history-manager.ts @@ -689,14 +689,12 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - // TODO: reduce duplication with 'handleCompareWith' multiSelect ||= [singleItem]; if ( !this.isSuccessfulCompletedLocalQueryInfo(singleItem) || !multiSelect.every(this.isSuccessfulCompletedLocalQueryInfo) ) { - // TODO: support performance comparison with partially-evaluated query (technically possible) throw new Error( "Please only select local queries that have completed successfully.", ); From bba31c030a61e14afcf9b5d61cd777edb3cdf990 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 17 Jan 2025 11:47:35 +0100 Subject: [PATCH 45/46] Add comment to clarify reverse parsing --- .../ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx index f6dd7720e5a..b2595dd4c38 100644 --- a/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx @@ -67,6 +67,7 @@ function parseName(text: string): QualifiedName { } function parseQName(): QualifiedName { + // Note that the tokens stream is parsed in reverse order. This is simpler, but may look confusing initially. let args: QualifiedName[] | undefined; if (skipToken(">")) { args = []; From 666c26e6a1709f158c991aa57543cda3f4bfdbc5 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 17 Jan 2025 12:31:46 +0100 Subject: [PATCH 46/46] Permit performance comparisons across DBs The snippet seems to have been copied from 'findOtherQueryToCompare' where it makes sense, but in the context of a performance comparison we don't need this restriction. --- .../src/query-history/query-history-manager.ts | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/extensions/ql-vscode/src/query-history/query-history-manager.ts b/extensions/ql-vscode/src/query-history/query-history-manager.ts index 0296b32d503..3074ceb3bb9 100644 --- a/extensions/ql-vscode/src/query-history/query-history-manager.ts +++ b/extensions/ql-vscode/src/query-history/query-history-manager.ts @@ -1128,8 +1128,6 @@ export class QueryHistoryManager extends DisposableObject { fromItem: CompletedLocalQueryInfo, allSelectedItems: CompletedLocalQueryInfo[], ): Promise { - const dbName = fromItem.databaseName; - // If exactly 2 items are selected, return the one that // isn't being used as the "from" item. if (allSelectedItems.length === 2) { @@ -1137,9 +1135,6 @@ export class QueryHistoryManager extends DisposableObject { fromItem === allSelectedItems[0] ? allSelectedItems[1] : allSelectedItems[0]; - if (otherItem.databaseName !== dbName) { - throw new Error("Query databases must be the same."); - } return otherItem; } @@ -1150,10 +1145,7 @@ export class QueryHistoryManager extends DisposableObject { // Otherwise, present a dialog so the user can choose the item they want to use. const comparableQueryLabels = this.treeDataProvider.allHistory .filter(this.isSuccessfulCompletedLocalQueryInfo) - .filter( - (otherItem) => - otherItem !== fromItem && otherItem.databaseName === dbName, - ) + .filter((otherItem) => otherItem !== fromItem) .map((item) => ({ label: this.labelProvider.getLabel(item), description: item.databaseName,