Skip to content

Commit a155289

Browse files
nickrolfenickrolfe
committed
Actually report bad join orders in recursions
1 parent 7cdcf9d commit a155289

File tree

4 files changed

+114
-111
lines changed

4 files changed

+114
-111
lines changed

‎extensions/ql-vscode/src/log-insights/join-order.ts

Lines changed: 64 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -19,23 +19,6 @@ function safeMax(it?: Iterable<number>) {
1919
return Number.isFinite(m) ? m : 0;
2020
}
2121

22-
/**
23-
* Compute a key for the maps that that is sent to report generation.
24-
* Should only be used on events that are known to define queryCausingWork.
25-
*/
26-
function makeKey(
27-
queryCausingWork: string | undefined,
28-
predicate: string,
29-
suffix = "",
30-
): string {
31-
if (queryCausingWork === undefined) {
32-
throw new Error(
33-
"queryCausingWork was not defined on an event we expected it to be defined for!",
34-
);
35-
}
36-
return `${queryCausingWork}:${predicate}${suffix ? ` ${suffix}` : ""}`;
37-
}
38-
3922
function getDependentPredicates(operations: string[]): string[] {
4023
const id = String.raw`[0-9a-zA-Z:#_\./]+`;
4124
const idWithAngleBrackets = String.raw`[0-9a-zA-Z:#_<>\./]+`;
@@ -128,14 +111,6 @@ function pointwiseSum(
128111
return result;
129112
}
130113

131-
function pushValue<K, V>(m: Map<K, V[]>, k: K, v: V) {
132-
if (!m.has(k)) {
133-
m.set(k, []);
134-
}
135-
m.get(k)!.push(v);
136-
return m;
137-
}
138-
139114
function computeJoinOrderBadness(
140115
maxTupleCount: number,
141116
maxDependentPredicateSize: number,
@@ -161,11 +136,6 @@ class JoinOrderScanner implements EvaluationLogScanner {
161136
string,
162137
Array<ComputeRecursive | InLayer>
163138
>();
164-
// Map a key of the form 'query-with-demand : predicate name' to its badness input.
165-
private readonly maxTupleCountMap = new Map<string, number[]>();
166-
private readonly resultSizeMap = new Map<string, number[]>();
167-
private readonly maxDependentPredicateSizeMap = new Map<string, number[]>();
168-
private readonly joinOrderMetricMap = new Map<string, number>();
169139

170140
constructor(
171141
private readonly problemReporter: EvaluationLogProblemReporter,
@@ -216,27 +186,6 @@ class JoinOrderScanner implements EvaluationLogScanner {
216186
}
217187
}
218188

219-
private reportProblemIfNecessary(
220-
event: SummaryEvent,
221-
iteration: number,
222-
metric: number,
223-
): void {
224-
if (metric >= this.warningThreshold) {
225-
this.problemReporter.reportProblem(
226-
event.predicateName,
227-
event.raHash,
228-
iteration,
229-
`Relation '${
230-
event.predicateName
231-
}' has an inefficient join order. Its join order metric is ${metric.toFixed(
232-
2,
233-
)}, which is larger than the threshold of ${this.warningThreshold.toFixed(
234-
2,
235-
)}.`,
236-
);
237-
}
238-
}
239-
240189
private computeBadnessMetric(event: SummaryEvent): void {
241190
if (
242191
event.completionType !== undefined &&
@@ -252,28 +201,32 @@ class JoinOrderScanner implements EvaluationLogScanner {
252201
}
253202
// Compute the badness metric for a non-recursive predicate. The metric in this case is defined as:
254203
// badness = (max tuple count in the pipeline) / (largest predicate this pipeline depends on)
255-
const key = makeKey(event.queryCausingWork, event.predicateName);
256204
const resultSize = event.resultSize;
257205

258206
// There is only one entry in `pipelineRuns` if it's a non-recursive predicate.
259207
const { maxTupleCount, maxDependentPredicateSize } =
260208
this.badnessInputsForNonRecursiveDelta(event.pipelineRuns[0], event);
261209

262210
if (maxDependentPredicateSize > 0) {
263-
pushValue(this.maxTupleCountMap, key, maxTupleCount);
264-
pushValue(this.resultSizeMap, key, resultSize);
265-
pushValue(
266-
this.maxDependentPredicateSizeMap,
267-
key,
268-
maxDependentPredicateSize,
269-
);
270211
const metric = computeJoinOrderBadness(
271212
maxTupleCount,
272213
maxDependentPredicateSize,
273214
resultSize!,
274215
);
275-
this.joinOrderMetricMap.set(key, metric);
276-
this.reportProblemIfNecessary(event, 0, metric);
216+
if (metric >= this.warningThreshold) {
217+
const message = `'${
218+
event.predicateName
219+
}' has an inefficient join order. Its join order metric is ${metric.toFixed(
220+
2,
221+
)}, which is larger than the threshold of ${this.warningThreshold.toFixed(
222+
2,
223+
)}.`;
224+
this.problemReporter.reportProblemNonRecursive(
225+
event.predicateName,
226+
event.raHash,
227+
message,
228+
);
229+
}
277230
}
278231
break;
279232
}
@@ -282,39 +235,39 @@ class JoinOrderScanner implements EvaluationLogScanner {
282235
// Compute the badness metric for a recursive predicate for each ordering.
283236
const sccMetricInput = this.badnessInputsForRecursiveDelta(event);
284237
// Loop through each predicate in the SCC
285-
sccMetricInput.forEach((buckets, predicate) => {
286-
// Loop through each ordering of the predicate
287-
buckets.forEach((bucket, raReference) => {
288-
// Format the key as demanding-query:name (ordering)
289-
const key = makeKey(
290-
event.queryCausingWork,
291-
predicate,
292-
`(${raReference})`,
293-
);
294-
const maxTupleCount = Math.max(...bucket.tupleCounts);
295-
const resultSize = bucket.resultSize;
296-
const maxDependentPredicateSize = Math.max(
297-
...bucket.dependentPredicateSizes.values(),
298-
);
299-
300-
if (maxDependentPredicateSize > 0) {
301-
pushValue(this.maxTupleCountMap, key, maxTupleCount);
302-
pushValue(this.resultSizeMap, key, resultSize);
303-
pushValue(
304-
this.maxDependentPredicateSizeMap,
305-
key,
306-
maxDependentPredicateSize,
238+
sccMetricInput.forEach((hashToOrderToBucket, predicateName) => {
239+
hashToOrderToBucket.forEach((orderToBucket, raHash) => {
240+
// Loop through each ordering of the predicate.
241+
orderToBucket.forEach((bucket, raReference) => {
242+
const maxDependentPredicateSize = Math.max(
243+
...bucket.dependentPredicateSizes.values(),
307244
);
308-
const metric = computeJoinOrderBadness(
309-
maxTupleCount,
310-
maxDependentPredicateSize,
311-
resultSize,
312-
);
313-
const oldMetric = this.joinOrderMetricMap.get(key);
314-
if (oldMetric === undefined || metric > oldMetric) {
315-
this.joinOrderMetricMap.set(key, metric);
245+
246+
if (maxDependentPredicateSize > 0) {
247+
const maxTupleCount = Math.max(...bucket.tupleCounts);
248+
const resultSize = bucket.resultSize;
249+
const metric = computeJoinOrderBadness(
250+
maxTupleCount,
251+
maxDependentPredicateSize,
252+
resultSize,
253+
);
254+
if (metric >= this.warningThreshold) {
255+
const message = `The ${raReference} pipeline for '${
256+
predicateName
257+
}' has an inefficient join order. Its join order metric is ${metric.toFixed(
258+
2,
259+
)}, which is larger than the threshold of ${this.warningThreshold.toFixed(
260+
2,
261+
)}.`;
262+
this.problemReporter.reportProblemForRecursionSummary(
263+
predicateName,
264+
raHash,
265+
raReference,
266+
message,
267+
);
268+
}
316269
}
317-
}
270+
});
318271
});
319272
});
320273
break;
@@ -457,20 +410,28 @@ class JoinOrderScanner implements EvaluationLogScanner {
457410
*/
458411
private badnessInputsForRecursiveDelta(
459412
event: ComputeRecursive,
460-
): Map<string, Map<string, Bucket>> {
461-
// nameToOrderToBucket : predicate name -> ordering (i.e., standard, order_500000, etc.) -> bucket
462-
const nameToOrderToBucket = new Map<string, Map<string, Bucket>>();
413+
): Map<string, Map<string, Map<string, Bucket>>> {
414+
// nameToHashToOrderToBucket : predicate name -> RA hash -> ordering (i.e., standard, order_500000, etc.) -> bucket
415+
const nameToHashToOrderToBucket = new Map<
416+
string,
417+
Map<string, Map<string, Bucket>>
418+
>();
463419

464420
// Iterate through the SCC and compute the metric inputs
465421
this.iterateSCC(event, (inLayerEvent, run, iteration) => {
466422
const raReference = run.raReference;
467423
const predicateName = inLayerEvent.predicateName;
468-
if (!nameToOrderToBucket.has(predicateName)) {
469-
nameToOrderToBucket.set(predicateName, new Map());
424+
if (!nameToHashToOrderToBucket.has(predicateName)) {
425+
nameToHashToOrderToBucket.set(predicateName, new Map());
426+
}
427+
const hashToOrderToBucket = nameToHashToOrderToBucket.get(predicateName)!;
428+
const raHash = inLayerEvent.raHash;
429+
if (!hashToOrderToBucket.has(raHash)) {
430+
hashToOrderToBucket.set(raHash, new Map());
470431
}
471-
const orderTobucket = nameToOrderToBucket.get(predicateName)!;
472-
if (!orderTobucket.has(raReference)) {
473-
orderTobucket.set(raReference, {
432+
const orderToBucket = hashToOrderToBucket.get(raHash)!;
433+
if (!orderToBucket.has(raReference)) {
434+
orderToBucket.set(raReference, {
474435
tupleCounts: new Int32Array(0),
475436
resultSize: 0,
476437
dependentPredicateSizes: new Map(),
@@ -484,7 +445,7 @@ class JoinOrderScanner implements EvaluationLogScanner {
484445
iteration,
485446
);
486447

487-
const bucket = orderTobucket.get(raReference)!;
448+
const bucket = orderToBucket.get(raReference)!;
488449
// Pointwise sum the tuple counts
489450
const newTupleCounts = pointwiseSum(
490451
bucket.tupleCounts,
@@ -504,13 +465,13 @@ class JoinOrderScanner implements EvaluationLogScanner {
504465
);
505466
}
506467

507-
orderTobucket.set(raReference, {
468+
orderToBucket.set(raReference, {
508469
tupleCounts: newTupleCounts,
509470
resultSize,
510471
dependentPredicateSizes: newDependentPredicateSizes,
511472
});
512473
});
513-
return nameToOrderToBucket;
474+
return nameToHashToOrderToBucket;
514475
}
515476
}
516477

‎extensions/ql-vscode/src/log-insights/log-scanner-service.ts

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,41 @@ class ProblemReporter implements EvaluationLogProblemReporter {
2828

2929
constructor(private readonly symbols: SummarySymbols | undefined) {}
3030

31-
public reportProblem(
31+
public reportProblemNonRecursive(
3232
predicateName: string,
3333
raHash: string,
34-
iteration: number,
3534
message: string,
3635
): void {
3736
const nameWithHash = predicateSymbolKey(predicateName, raHash);
3837
const predicateSymbol = this.symbols?.predicates[nameWithHash];
3938
let predicateInfo: PipelineInfo | undefined = undefined;
4039
if (predicateSymbol !== undefined) {
41-
predicateInfo = predicateSymbol.iterations[iteration];
40+
predicateInfo = predicateSymbol.iterations[0];
41+
}
42+
if (predicateInfo !== undefined) {
43+
const range = new Range(
44+
predicateInfo.raStartLine,
45+
0,
46+
predicateInfo.raEndLine + 1,
47+
0,
48+
);
49+
this.diagnostics.push(
50+
new Diagnostic(range, message, DiagnosticSeverity.Error),
51+
);
52+
}
53+
}
54+
55+
public reportProblemForRecursionSummary(
56+
predicateName: string,
57+
raHash: string,
58+
order: string,
59+
message: string,
60+
): void {
61+
const nameWithHash = predicateSymbolKey(predicateName, raHash);
62+
const predicateSymbol = this.symbols?.predicates[nameWithHash];
63+
let predicateInfo: PipelineInfo | undefined = undefined;
64+
if (predicateSymbol !== undefined) {
65+
predicateInfo = predicateSymbol.recursionSummaries[order];
4266
}
4367
if (predicateInfo !== undefined) {
4468
const range = new Range(

‎extensions/ql-vscode/src/log-insights/log-scanner.ts

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,18 +8,30 @@ import type { SummaryEvent } from "./log-summary";
88
*/
99
export interface EvaluationLogProblemReporter {
1010
/**
11-
* Report a potential problem detected in the evaluation log.
11+
* Report a potential problem detected in the evaluation log for a non-recursive predicate.
1212
*
1313
* @param predicateName The mangled name of the predicate with the problem.
1414
* @param raHash The RA hash of the predicate with the problem.
15-
* @param iteration The iteration number with the problem. For a non-recursive predicate, this
16-
* must be zero.
1715
* @param message The problem message.
1816
*/
19-
reportProblem(
17+
reportProblemNonRecursive(
2018
predicateName: string,
2119
raHash: string,
22-
iteration: number,
20+
message: string,
21+
): void;
22+
23+
/**
24+
* Report a potential problem detected in the evaluation log for the summary of a recursive pipeline.
25+
*
26+
* @param predicateName The mangled name of the predicate with the problem.
27+
* @param raHash The RA hash of the predicate with the problem.
28+
* @param order The particular order (pipeline name) that had the problem.
29+
* @param message The problem message.
30+
*/
31+
reportProblemForRecursionSummary(
32+
predicateName: string,
33+
raHash: string,
34+
order: string,
2335
message: string,
2436
): void;
2537

‎extensions/ql-vscode/src/log-insights/summary-parser.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ interface PredicateSymbol {
1818
* `PipelineInfo` for each iteration. A non-recursive predicate will have a single iteration `0`.
1919
*/
2020
iterations: Record<number, PipelineInfo>;
21+
22+
/**
23+
* `PipelineInfo` for each order, summarised for all iterations that used that order. Empty for non-recursive predicates.
24+
*/
25+
recursionSummaries: Record<string, PipelineInfo>;
2126
}
2227

2328
/**
@@ -105,6 +110,7 @@ async function generateSummarySymbols(
105110
if (symbol === undefined) {
106111
symbol = {
107112
iterations: {},
113+
recursionSummaries: {},
108114
};
109115
symbols.predicates[predicateName] = symbol;
110116
}

0 commit comments

Comments
 (0)