From 55c808e1c2b71994ea78ab18601cf2f13231fb1b Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 18 Feb 2025 21:34:49 +0100 Subject: [PATCH 01/15] Destructure event object --- .../log-insights/performance-comparison.ts | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/extensions/ql-vscode/src/log-insights/performance-comparison.ts b/extensions/ql-vscode/src/log-insights/performance-comparison.ts index 46634cf8bf3..d22fc14f578 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -97,14 +97,12 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { } onEvent(event: SummaryEvent): void { - if ( - event.completionType !== undefined && - event.completionType !== "SUCCESS" - ) { + const { completionType, evaluationStrategy, predicateName } = event; + if (completionType !== undefined && completionType !== "SUCCESS") { return; // Skip any evaluation that wasn't successful } - switch (event.evaluationStrategy) { + switch (evaluationStrategy) { case "EXTENSIONAL": case "COMPUTED_EXTENSIONAL": { break; @@ -113,26 +111,24 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { case "CACHACA": { // 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), - ); + if (!this.nameToIndex.has(predicateName)) { + this.data.cacheHitIndices.push(this.getPredicateIndex(predicateName)); } break; } case "SENTINEL_EMPTY": { this.data.sentinelEmptyIndices.push( - this.getPredicateIndex(event.predicateName), + this.getPredicateIndex(predicateName), ); break; } case "COMPUTE_RECURSIVE": case "COMPUTE_SIMPLE": case "IN_LAYER": { - const index = this.getPredicateIndex(event.predicateName); + const index = this.getPredicateIndex(predicateName); let totalTime = 0; let totalTuples = 0; - if (event.evaluationStrategy !== "IN_LAYER") { + if (evaluationStrategy !== "IN_LAYER") { totalTime += event.millis; } else { // IN_LAYER events do no record of their total time. From bcd72a9cec2b2e319c06aba1f4469d8ed4fc97b8 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 22 Apr 2025 15:52:09 +0200 Subject: [PATCH 02/15] Do not use "millis" field for COMPUTE_RECURSIVE events --- .../ql-vscode/src/log-insights/performance-comparison.ts | 7 ++++--- 1 file changed, 4 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 d22fc14f578..a906cf13136 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -128,11 +128,12 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { const index = this.getPredicateIndex(predicateName); let totalTime = 0; let totalTuples = 0; - if (evaluationStrategy !== "IN_LAYER") { + if (evaluationStrategy === "COMPUTE_SIMPLE") { 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). + // Make a best-effort estimate of the total time by adding up the positive iteration times (they can be negative). + // Note that for COMPUTE_RECURSIVE the "millis" field contain the total time of the SCC, not just that predicate, + // but we don't have a good way to show that in the UI, so we rely on the accumulated iteration times. for (const millis of event.predicateIterationMillis ?? []) { if (millis > 0) { totalTime += millis; From 06a2513a118412e45e08bf56ef9da242f3c401c9 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 22 Apr 2025 15:58:18 +0200 Subject: [PATCH 03/15] Also handle NAMED_LOCAL event type --- .../ql-vscode/src/log-insights/log-summary.ts | 16 ++++++++++++++-- .../src/log-insights/performance-comparison.ts | 1 + 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/src/log-insights/log-summary.ts b/extensions/ql-vscode/src/log-insights/log-summary.ts index 5fa4bda58b8..fb6c8acff27 100644 --- a/extensions/ql-vscode/src/log-insights/log-summary.ts +++ b/extensions/ql-vscode/src/log-insights/log-summary.ts @@ -16,7 +16,8 @@ type EvaluationStrategy = | "EXTENSIONAL" | "SENTINEL_EMPTY" | "CACHACA" - | "CACHE_HIT"; + | "CACHE_HIT" + | "NAMED_LOCAL"; interface SummaryEventBase { evaluationStrategy: EvaluationStrategy; @@ -60,6 +61,16 @@ export interface InLayer extends ResultEventBase { predicateIterationMillis: number[]; } +export interface NamedLocal extends ResultEventBase { + evaluationStrategy: "NAMED_LOCAL"; + deltaSizes: number[]; + ra: Ra; + pipelineRuns: PipelineRun[]; + queryCausingWork?: string; + dependencies: { [key: string]: string }; + predicateIterationMillis: number[]; +} + interface ComputedExtensional extends ResultEventBase { evaluationStrategy: "COMPUTED_EXTENSIONAL"; queryCausingWork?: string; @@ -92,4 +103,5 @@ export type SummaryEvent = | Extensional | SentinelEmpty | Cachaca - | CacheHit; + | CacheHit + | NamedLocal; diff --git a/extensions/ql-vscode/src/log-insights/performance-comparison.ts b/extensions/ql-vscode/src/log-insights/performance-comparison.ts index a906cf13136..85d10410c30 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -124,6 +124,7 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { } case "COMPUTE_RECURSIVE": case "COMPUTE_SIMPLE": + case "NAMED_LOCAL": case "IN_LAYER": { const index = this.getPredicateIndex(predicateName); let totalTime = 0; From 116575294db1f31ccab12f2d29b334b4650c64fb Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 22 Apr 2025 17:31:27 +0200 Subject: [PATCH 04/15] Associate predicate with their RA hash --- .../log-insights/performance-comparison.ts | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/extensions/ql-vscode/src/log-insights/performance-comparison.ts b/extensions/ql-vscode/src/log-insights/performance-comparison.ts index 85d10410c30..d4368b5f0ef 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -27,6 +27,9 @@ export interface PerformanceComparisonDataFromLog { */ names: string[]; + /** RA hash of the `i`th predicate event */ + raHashes: string[]; + /** Number of milliseconds spent evaluating the `i`th predicate from the `names` array. */ timeCosts: number[]; @@ -56,9 +59,9 @@ export interface PerformanceComparisonDataFromLog { } export class PerformanceOverviewScanner implements EvaluationLogScanner { - private readonly nameToIndex = new Map(); private readonly data: PerformanceComparisonDataFromLog = { names: [], + raHashes: [], timeCosts: [], tupleCosts: [], cacheHitIndices: [], @@ -66,28 +69,33 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { pipelineSummaryList: [], evaluationCounts: [], iterationCounts: [], + dependencyLists: [], }; + private readonly raToIndex = new Map(); - private getPredicateIndex(name: string): number { - const { nameToIndex } = this; - let index = nameToIndex.get(name); + private getPredicateIndex(name: string, ra: string): number { + let index = this.raToIndex.get(ra); if (index === undefined) { - index = nameToIndex.size; - nameToIndex.set(name, index); + index = this.raToIndex.size; + this.raToIndex.set(ra, index); const { names, + raHashes, timeCosts, tupleCosts, iterationCounts, evaluationCounts, pipelineSummaryList, + dependencyLists, } = this.data; names.push(name); + raHashes.push(ra); timeCosts.push(0); tupleCosts.push(0); iterationCounts.push(0); evaluationCounts.push(0); pipelineSummaryList.push({}); + dependencyLists.push([]); } return index; } @@ -97,7 +105,7 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { } onEvent(event: SummaryEvent): void { - const { completionType, evaluationStrategy, predicateName } = event; + const { completionType, evaluationStrategy, predicateName, raHash } = event; if (completionType !== undefined && completionType !== "SUCCESS") { return; // Skip any evaluation that wasn't successful } @@ -111,14 +119,16 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { case "CACHACA": { // 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(predicateName)) { - this.data.cacheHitIndices.push(this.getPredicateIndex(predicateName)); + if (!this.raToIndex.has(raHash)) { + this.data.cacheHitIndices.push( + this.getPredicateIndex(predicateName, raHash), + ); } break; } case "SENTINEL_EMPTY": { this.data.sentinelEmptyIndices.push( - this.getPredicateIndex(predicateName), + this.getPredicateIndex(predicateName, raHash), ); break; } @@ -126,7 +136,7 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { case "COMPUTE_SIMPLE": case "NAMED_LOCAL": case "IN_LAYER": { - const index = this.getPredicateIndex(predicateName); + const index = this.getPredicateIndex(predicateName, raHash); let totalTime = 0; let totalTuples = 0; if (evaluationStrategy === "COMPUTE_SIMPLE") { From 6329d239edf16fc9582c7dc082af03b17d386d38 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 22 Apr 2025 17:31:52 +0200 Subject: [PATCH 05/15] Store dependencies of predicates --- extensions/ql-vscode/src/log-insights/log-summary.ts | 2 +- .../src/log-insights/performance-comparison.ts | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/log-insights/log-summary.ts b/extensions/ql-vscode/src/log-insights/log-summary.ts index fb6c8acff27..d8723bc8a5b 100644 --- a/extensions/ql-vscode/src/log-insights/log-summary.ts +++ b/extensions/ql-vscode/src/log-insights/log-summary.ts @@ -29,6 +29,7 @@ interface SummaryEventBase { interface ResultEventBase extends SummaryEventBase { resultSize: number; + dependencies?: { [key: string]: string }; } export interface ComputeSimple extends ResultEventBase { @@ -67,7 +68,6 @@ export interface NamedLocal extends ResultEventBase { ra: Ra; pipelineRuns: PipelineRun[]; queryCausingWork?: string; - dependencies: { [key: string]: string }; predicateIterationMillis: number[]; } diff --git a/extensions/ql-vscode/src/log-insights/performance-comparison.ts b/extensions/ql-vscode/src/log-insights/performance-comparison.ts index d4368b5f0ef..72f38bae942 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -56,6 +56,9 @@ export interface PerformanceComparisonDataFromLog { * All the pipeline runs seen for the `i`th predicate from the `names` array. */ pipelineSummaryList: Array>; + + /** All dependencies of the `i`th predicate from the `names` array, encoded as a list of indices in `names`. */ + dependencyLists: number[][]; } export class PerformanceOverviewScanner implements EvaluationLogScanner { @@ -157,8 +160,10 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { iterationCounts, evaluationCounts, pipelineSummaryList, + dependencyLists, } = this.data; const pipelineSummaries = pipelineSummaryList[index]; + const dependencyList = dependencyLists[index]; for (const { counts, raReference } of event.pipelineRuns ?? []) { // Get or create the pipeline summary for this RA const pipelineSummary = (pipelineSummaries[raReference] ??= { @@ -178,6 +183,12 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { totalTuplesPerStep[i] += count; } } + for (const dependencyHash of Object.values(event.dependencies ?? {})) { + const dependencyIndex = this.raToIndex.get(dependencyHash); + if (dependencyIndex != null) { + dependencyList.push(dependencyIndex); + } + } timeCosts[index] += totalTime; tupleCosts[index] += totalTuples; iterationCounts[index] += event.pipelineRuns?.length ?? 0; From 12a342efeacde7e019f3f315091cac9bd9cc656d Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 23 Apr 2025 00:13:31 +0200 Subject: [PATCH 06/15] Record implicit dependency for 'cached' predicates --- .../src/log-insights/performance-comparison.ts | 17 ++++++++++++++++- 1 file changed, 16 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 72f38bae942..5211693c78e 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -75,6 +75,7 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { dependencyLists: [], }; private readonly raToIndex = new Map(); + private readonly nameToIndex = new Map(); private getPredicateIndex(name: string, ra: string): number { let index = this.raToIndex.get(ra); @@ -114,8 +115,21 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { } switch (evaluationStrategy) { - case "EXTENSIONAL": + case "EXTENSIONAL": { + break; + } case "COMPUTED_EXTENSIONAL": { + if (predicateName.startsWith("cached_")) { + // Add a dependency from a cached COMPUTED_EXTENSIONAL to the predicate with the actual contents. + // The raHash of the this event may appear in a CACHE_HIT events in the other event log. The dependency + // we're adding here is needed in order to associate the original predicate with such a cache hit. + const originalName = predicateName.substring("cached_".length); + const originalIndex = this.nameToIndex.get(originalName); + if (originalIndex != null) { + const index = this.getPredicateIndex(predicateName, raHash); + this.data.dependencyLists[index].push(originalIndex); + } + } break; } case "CACHE_HIT": @@ -140,6 +154,7 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { case "NAMED_LOCAL": case "IN_LAYER": { const index = this.getPredicateIndex(predicateName, raHash); + this.nameToIndex.set(predicateName, index); let totalTime = 0; let totalTuples = 0; if (evaluationStrategy === "COMPUTE_SIMPLE") { From 833f67971de8d68570f5fe769a566d042a6dc2f1 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 23 Apr 2025 10:27:12 +0200 Subject: [PATCH 07/15] Record dependencies from sentinels --- .../ql-vscode/src/log-insights/performance-comparison.ts | 9 ++++++--- 1 file changed, 6 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 5211693c78e..4a034cdcbba 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -144,9 +144,12 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { break; } case "SENTINEL_EMPTY": { - this.data.sentinelEmptyIndices.push( - this.getPredicateIndex(predicateName, raHash), - ); + const index = this.getPredicateIndex(predicateName, raHash); + this.data.sentinelEmptyIndices.push(index); + const sentinelIndex = this.raToIndex.get(event.sentinelRaHash); + if (sentinelIndex != null) { + this.data.dependencyLists[index].push(sentinelIndex); // needed for matching up cache hits + } break; } case "COMPUTE_RECURSIVE": From 570f63e731d882594ad2e88ae08402079e2b058e Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 23 Apr 2025 10:27:26 +0200 Subject: [PATCH 08/15] Record dependencies inside SCCs --- .../ql-vscode/src/log-insights/log-summary.ts | 1 + .../src/log-insights/performance-comparison.ts | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/extensions/ql-vscode/src/log-insights/log-summary.ts b/extensions/ql-vscode/src/log-insights/log-summary.ts index d8723bc8a5b..31e582a06bc 100644 --- a/extensions/ql-vscode/src/log-insights/log-summary.ts +++ b/extensions/ql-vscode/src/log-insights/log-summary.ts @@ -30,6 +30,7 @@ interface SummaryEventBase { interface ResultEventBase extends SummaryEventBase { resultSize: number; dependencies?: { [key: string]: string }; + mainHash?: string; } export interface ComputeSimple extends ResultEventBase { diff --git a/extensions/ql-vscode/src/log-insights/performance-comparison.ts b/extensions/ql-vscode/src/log-insights/performance-comparison.ts index 4a034cdcbba..f04d7cc3cbb 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -75,6 +75,7 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { dependencyLists: [], }; private readonly raToIndex = new Map(); + private readonly mainHashToRepr = new Map(); private readonly nameToIndex = new Map(); private getPredicateIndex(name: string, ra: string): number { @@ -207,6 +208,19 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { dependencyList.push(dependencyIndex); } } + // For predicates in the same SCC, add two-way dependencies with an arbitrary SCC member + const sccHash = + event.mainHash ?? + (evaluationStrategy === "COMPUTE_RECURSIVE" ? raHash : null); + if (sccHash != null) { + const mainIndex = this.mainHashToRepr.get(sccHash); + if (mainIndex == null) { + this.mainHashToRepr.set(sccHash, index); + } else { + dependencyLists[index].push(mainIndex); + dependencyLists[mainIndex].push(index); + } + } timeCosts[index] += totalTime; tupleCosts[index] += totalTuples; iterationCounts[index] += event.pipelineRuns?.length ?? 0; From c72457785e43b26055dad077408fbc5b3602bc06 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 22 Apr 2025 17:32:16 +0200 Subject: [PATCH 09/15] Store pipeline hash on PipelineSummary --- .../src/log-insights/performance-comparison.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/extensions/ql-vscode/src/log-insights/performance-comparison.ts b/extensions/ql-vscode/src/log-insights/performance-comparison.ts index f04d7cc3cbb..2922951479b 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -1,3 +1,4 @@ +import { createHash } from "crypto"; import type { EvaluationLogScanner } from "./log-scanner"; import type { SummaryEvent } from "./log-summary"; @@ -5,6 +6,7 @@ export interface PipelineSummary { steps: string[]; /** Total counts for each step in the RA array, across all iterations */ counts: number[]; + hash: string; } /** @@ -188,6 +190,7 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { const pipelineSummary = (pipelineSummaries[raReference] ??= { steps: event.ra[raReference], counts: counts.map(() => 0), + hash: getPipelineHash(event.ra[raReference]), }); const { counts: totalTuplesPerStep } = pipelineSummary; for (let i = 0, length = counts.length; i < length; ++i) { @@ -232,3 +235,11 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { onDone(): void {} } + +function getPipelineHash(steps: string[]) { + const md5 = createHash("md5"); + for (const step of steps) { + md5.write(step); + } + return md5.digest("base64"); +} From e8bf7e3ccd7c263b9f346ad60c3fa2d216698019 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 25 Apr 2025 10:36:58 +0200 Subject: [PATCH 10/15] Associate rows with RA-hash and a name-with-pipeline hash --- .../ComparePerformance.tsx | 66 ++++++++++++++----- 1 file changed, 49 insertions(+), 17 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 5708b65faf0..9bc38bc5c25 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -35,6 +35,8 @@ function isPresent(x: Optional): x is T { } interface PredicateInfo { + name: string; + raHash: string; tuples: number; evaluationCount: number; iterationCount: number; @@ -43,23 +45,37 @@ interface PredicateInfo { } class ComparisonDataset { + /** + * Predicates indexed by a key consisting of the name and its pipeline hash. + * Unlike the RA hash, the pipeline hash only depends on the predicate's own pipeline. + */ + public keyToIndex = new Map(); + public raToIndex = new Map(); public nameToIndex = new Map(); public cacheHitIndices: Set; public sentinelEmptyIndices: Set; - constructor(public data: PerformanceComparisonDataFromLog) { - const { names } = data; - const { nameToIndex } = this; + constructor(private data: PerformanceComparisonDataFromLog) { + const { names, raHashes, pipelineSummaryList } = data; + const { keyToIndex, raToIndex, nameToIndex } = this; for (let i = 0; i < names.length; i++) { - nameToIndex.set(names[i], i); + const name = names[i]; + const pipelineHash = getPipelineSummaryHash(pipelineSummaryList[i]); + keyToIndex.set(`${name}@${pipelineHash}`, i); + nameToIndex.set(name, i); + raToIndex.set(raHashes[i], i); } this.cacheHitIndices = new Set(data.cacheHitIndices); this.sentinelEmptyIndices = new Set(data.sentinelEmptyIndices); } - getTupleCountInfo(name: string): Optional { - const { data, nameToIndex, cacheHitIndices, sentinelEmptyIndices } = this; - const index = nameToIndex.get(name); + keys() { + return Array.from(this.keyToIndex.keys()); + } + + getTupleCountInfo(key: string): Optional { + const { data, keyToIndex, cacheHitIndices, sentinelEmptyIndices } = this; + const index = keyToIndex.get(key); if (index == null) { return AbsentReason.NotSeen; } @@ -72,6 +88,8 @@ class ComparisonDataset { } } return { + name: data.names[index], + raHash: data.raHashes[index], evaluationCount: data.evaluationCounts[index], iterationCount: data.iterationCounts[index], timeCost: data.timeCosts[index], @@ -336,6 +354,7 @@ function HighLevelStats(props: HighLevelStatsProps) { } interface Row { + key: string; name: string; before: Optional; after: Optional; @@ -480,19 +499,16 @@ function ComparePerformanceWithData(props: { const [isPerEvaluation, setPerEvaluation] = useState(false); - const nameSet = useMemo( - () => union(from.data.names, to.data.names), - [from, to], - ); + const keySet = useMemo(() => union(from.keys(), to.keys()), [from, to]); const hasCacheHitMismatch = useRef(false); const rows: Row[] = useMemo(() => { hasCacheHitMismatch.current = false; - return Array.from(nameSet) - .map((name) => { - const before = from.getTupleCountInfo(name); - const after = to.getTupleCountInfo(name); + return Array.from(keySet) + .map((key) => { + const before = from.getTupleCountInfo(key); + const after = to.getTupleCountInfo(key); const beforeValue = metricGetOptional(metric, before, isPerEvaluation); const afterValue = metricGetOptional(metric, after, isPerEvaluation); if (beforeValue === afterValue) { @@ -510,11 +526,16 @@ function ComparePerformanceWithData(props: { const diff = (isPresent(afterValue) ? afterValue : 0) - (isPresent(beforeValue) ? beforeValue : 0); - return { name, before, after, diff } satisfies Row; + const name = isPresent(before) + ? before.name + : isPresent(after) + ? after.name + : key; + return { key, name, before, after, diff } satisfies Row; }) .filter((x) => !!x) .sort(getSortOrder(sortOrder)); - }, [nameSet, from, to, metric, hideCacheHits, sortOrder, isPerEvaluation]); + }, [keySet, from, to, metric, hideCacheHits, sortOrder, isPerEvaluation]); const { totalBefore, totalAfter, totalDiff } = useMemo(() => { let totalBefore = 0; @@ -860,3 +881,14 @@ function collatePipelines( function samePipeline(a: string[], b: string[]) { return a.length === b.length && a.every((x, i) => x === b[i]); } + +function getPipelineSummaryHash(pipelines: Record) { + // Note: we can't import "crypto" here because it is not available in the browser, + // so we just concatenate the hashes of the individual pipelines. + const keys = Object.keys(pipelines).sort(); + let result = ""; + for (const key of keys) { + result += `${pipelines[key].hash};`; + } + return result; +} From 06fcd6fc47454092a249396c23d03c4199eb6a9b Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 25 Apr 2025 10:56:46 +0200 Subject: [PATCH 11/15] Detect "shadowed" cache hits --- .../ComparePerformance.tsx | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 9bc38bc5c25..cb5aaf43f7f 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -97,6 +97,75 @@ class ComparisonDataset { pipelines: data.pipelineSummaryList[index], }; } + + /** + * Returns the RA hashes of all predicates that were evaluated in this data set, but not seen in `other`, + * because in `other` the dependency upon these predicates was cut off by a cache hit. + * + * For example, suppose predicate `A` depends on `B`, which depends on `C`, and the + * predicates were evaluated in the first log but not the second: + * ``` + * first eval. log second eval. log + * predicate A seen evaluation seen cache hit + * | + * V + * predicate B seen evaluation not seen + * | + * V + * predicate C seen evaluation not seen + * ``` + * + * To ensure a meaningful comparison, we want to omit `predicate A` from the comparison view because of the cache hit. + * + * But predicates B and C did not have a recorded cache hit in the second log, because they were never scheduled for evaluation. + * Given the dependency graph, the most likely explanation is that they would have been evaluated if `A` had not been a cache hit. + * We therefore say that B and C are "shadowed" by the cache hit on A. + * + * The dependency graph is only visible in the first evaluation log, because `B` and `C` do not exist in the second log. + * So to compute this, we use the dependency graph from one log together with the set of cache hits in the other log. + */ + getPredicatesShadowedByCacheHit(other: ComparisonDataset) { + const { + data: { dependencyLists, raHashes, names }, + raToIndex, + } = this; + const cacheHits = new Set(); + + function visit(index: number, raHash: string) { + if (cacheHits.has(raHash)) { + return; + } + cacheHits.add(raHash); + const dependencies = dependencyLists[index]; + for (const dep of dependencies) { + const name = names[dep]; + if (!other.nameToIndex.has(name)) { + visit(dep, raHashes[dep]); + } + } + } + + for (const otherCacheHit of other.cacheHitIndices) { + { + // Look up by RA hash + const raHash = other.data.raHashes[otherCacheHit]; + const ownIndex = raToIndex.get(raHash); + if (ownIndex != null) { + visit(ownIndex, raHash); + } + } + { + // Look up by name + const name = other.data.names[otherCacheHit]; + const ownIndex = this.nameToIndex.get(name); + if (ownIndex != null) { + visit(ownIndex, this.data.raHashes[ownIndex]); + } + } + } + + return cacheHits; + } } function renderOptionalValue(x: Optional, unit: string | undefined) { @@ -503,6 +572,17 @@ function ComparePerformanceWithData(props: { const hasCacheHitMismatch = useRef(false); + const shadowedCacheHitsFrom = useMemo( + () => + hideCacheHits ? from.getPredicatesShadowedByCacheHit(to) : new Set(), + [from, to, hideCacheHits], + ); + const shadowedCacheHitsTo = useMemo( + () => + hideCacheHits ? to.getPredicatesShadowedByCacheHit(from) : new Set(), + [from, to, hideCacheHits], + ); + const rows: Row[] = useMemo(() => { hasCacheHitMismatch.current = false; return Array.from(keySet) @@ -523,6 +603,16 @@ function ComparePerformanceWithData(props: { return undefined!; } } + if ( + (isPresent(before) && + !isPresent(after) && + shadowedCacheHitsFrom.has(before.raHash)) || + (isPresent(after) && + !isPresent(before) && + shadowedCacheHitsTo.has(after.raHash)) + ) { + return undefined!; + } const diff = (isPresent(afterValue) ? afterValue : 0) - (isPresent(beforeValue) ? beforeValue : 0); From e718ea630c36c253846be585f2d14a4768e54874 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 25 Apr 2025 11:07:46 +0200 Subject: [PATCH 12/15] Default to hiding cache hits, and omit warning The warning doesn't seem necessary anymore since shadowed cache hits are detected now --- .../ComparePerformance.tsx | 29 +++++++------------ 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index cb5aaf43f7f..ea787de13d7 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -18,7 +18,7 @@ import type { } from "../../log-insights/performance-comparison"; import { formatDecimal } from "../../common/number"; import { styled } from "styled-components"; -import { Codicon, ViewTitle, WarningBox } from "../common"; +import { Codicon, ViewTitle } from "../common"; import { abbreviateRANames, abbreviateRASteps } from "./RAPrettyPrinter"; import { Renaming, RenamingInput } from "./RenamingInput"; @@ -560,7 +560,7 @@ function ComparePerformanceWithData(props: { const comparison = data?.comparison; - const [hideCacheHits, setHideCacheHits] = useState(false); + const [hideCacheHits, setHideCacheHits] = useState(true); const [sortOrder, setSortOrder] = useState<"delta" | "absDelta">("absDelta"); @@ -686,23 +686,14 @@ function ComparePerformanceWithData(props: { <> Performance comparison {comparison && hasCacheHitMismatch.current && ( - - 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. -
-
- - + )} Compare{" "} From 0da5e5b7574cf62b11ebdd77d605c18733974ead Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 25 Apr 2025 11:41:38 +0200 Subject: [PATCH 13/15] Update dependencies passed to useMemo --- .../view/compare-performance/ComparePerformance.tsx | 12 +++++++++++- 1 file changed, 11 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 ea787de13d7..5e015c25b23 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -625,7 +625,17 @@ function ComparePerformanceWithData(props: { }) .filter((x) => !!x) .sort(getSortOrder(sortOrder)); - }, [keySet, from, to, metric, hideCacheHits, sortOrder, isPerEvaluation]); + }, [ + keySet, + from, + to, + metric, + hideCacheHits, + sortOrder, + isPerEvaluation, + shadowedCacheHitsFrom, + shadowedCacheHitsTo, + ]); const { totalBefore, totalAfter, totalDiff } = useMemo(() => { let totalBefore = 0; From 7b113a2b3833291a69863e3869c6fc5983357b51 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 28 Apr 2025 13:01:22 +0200 Subject: [PATCH 14/15] Do not export NamedLocal as it is never imported. There's a linting check that fails if we export something without importing it anywhere. --- extensions/ql-vscode/src/log-insights/log-summary.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/log-insights/log-summary.ts b/extensions/ql-vscode/src/log-insights/log-summary.ts index 31e582a06bc..6919210f98c 100644 --- a/extensions/ql-vscode/src/log-insights/log-summary.ts +++ b/extensions/ql-vscode/src/log-insights/log-summary.ts @@ -63,7 +63,7 @@ export interface InLayer extends ResultEventBase { predicateIterationMillis: number[]; } -export interface NamedLocal extends ResultEventBase { +interface NamedLocal extends ResultEventBase { evaluationStrategy: "NAMED_LOCAL"; deltaSizes: number[]; ra: Ra; From f9f92462b7ee6fcc217af1b98eb6e4ccf8a8afd3 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 29 Apr 2025 09:07:04 +0200 Subject: [PATCH 15/15] Apply suggestions from code review Co-authored-by: Andrew Eisenberg --- extensions/ql-vscode/src/log-insights/performance-comparison.ts | 2 +- .../src/view/compare-performance/ComparePerformance.tsx | 2 +- 2 files changed, 2 insertions(+), 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 2922951479b..8f00e9d2dbf 100644 --- a/extensions/ql-vscode/src/log-insights/performance-comparison.ts +++ b/extensions/ql-vscode/src/log-insights/performance-comparison.ts @@ -124,7 +124,7 @@ export class PerformanceOverviewScanner implements EvaluationLogScanner { case "COMPUTED_EXTENSIONAL": { if (predicateName.startsWith("cached_")) { // Add a dependency from a cached COMPUTED_EXTENSIONAL to the predicate with the actual contents. - // The raHash of the this event may appear in a CACHE_HIT events in the other event log. The dependency + // The raHash of the this event may appear in a CACHE_HIT event in the other event log. The dependency // we're adding here is needed in order to associate the original predicate with such a cache hit. const originalName = predicateName.substring("cached_".length); const originalIndex = this.nameToIndex.get(originalName); diff --git a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx index 5e015c25b23..c9fac0a09dc 100644 --- a/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx +++ b/extensions/ql-vscode/src/view/compare-performance/ComparePerformance.tsx @@ -159,7 +159,7 @@ class ComparisonDataset { const name = other.data.names[otherCacheHit]; const ownIndex = this.nameToIndex.get(name); if (ownIndex != null) { - visit(ownIndex, this.data.raHashes[ownIndex]); + visit(ownIndex, raHashes[ownIndex]); } } }