Skip to content

Commit 5387602

Browse files
gartunggartung
committed
Remove max(abs) as agregation. Apply threshold coloring instead of red for posittive and green for negative.
1 parent 78fb46a commit 5387602

2 files changed

Lines changed: 27 additions & 38 deletions

File tree

‎comparisons/compare-maxmem-summary.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,8 @@ def build_summary_payload(workflows, results_url):
8080
"orderedWorkflows": ordered_workflows,
8181
"orderedSteps": ordered_steps,
8282
"resultsURL": results_url,
83-
"defaultWarnThreshold": maxmem_threshold.WARN_THRESHOLD,
84-
"defaultErrorThreshold": maxmem_threshold.ERROR_THRESHOLD,
83+
"defaultWarnThreshold": float(maxmem_threshold.WARN_THRESHOLD),
84+
"defaultErrorThreshold": float(maxmem_threshold.ERROR_THRESHOLD),
8585
}
8686

8787

@@ -111,6 +111,7 @@ def compare_maxmem_summary(**kwargs):
111111
max_memory_pdiff_dict = jsonDict["max memory pdiffs"]
112112
workflow = jsonDict["workflow"]
113113
threshold = float(jsonDict["threshold"])
114+
error_threshold = float(jsonDict["error_threshold"])
114115
if workflow not in workflows:
115116
workflows[workflow] = {}
116117
for step in max_memory_pr_dict.keys():
@@ -194,6 +195,7 @@ def compare_maxmem_summary(**kwargs):
194195
"leaked alloc adiff": nlallocated_adiff,
195196
"leaked alloc pdiff": nlallocated_pdiff,
196197
"threshold": threshold,
198+
"error_threshold": error_threshold
197199
}
198200
dumpfile = "maxmem-summary.json"
199201
with open(dumpfile, "w") as f:

‎comparisons/maxmem_summary_viewer.html‎

Lines changed: 23 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -564,7 +564,6 @@ <h1>Max Memory Summary Viewer</h1>
564564
const AGGREGATIONS = [
565565
{ key: "max", label: "max" },
566566
{ key: "min", label: "min" },
567-
{ key: "abs-max", label: "abs(max)" }
568567
];
569568

570569
const state = {
@@ -637,9 +636,6 @@ <h1>Max Memory Summary Viewer</h1>
637636
if (typeof value !== "number" || Number.isNaN(value)) {
638637
return value;
639638
}
640-
if (state.aggregationKey === "abs-max") {
641-
return Math.abs(value);
642-
}
643639
return value;
644640
}
645641

@@ -785,15 +781,6 @@ <h1>Max Memory Summary Viewer</h1>
785781
}
786782
continue;
787783
}
788-
789-
if (state.aggregationKey === "abs-max") {
790-
if (Math.abs(value) > Math.abs(bestValue)) {
791-
bestValue = Math.abs(value);
792-
bestStepData = stepData;
793-
}
794-
continue;
795-
}
796-
797784
if (value > bestValue) {
798785
bestValue = value;
799786
bestStepData = stepData;
@@ -810,25 +797,25 @@ <h1>Max Memory Summary Viewer</h1>
810797
return classes.join(" ");
811798
}
812799

813-
if (legend.key === "adiff" || legend.key === "pdiff") {
814-
if (value > 0) {
815-
classes.push("delta-up");
816-
} else if (value < 0) {
817-
classes.push("delta-down");
800+
const thresholdEligibleQuantity = quantity.key !== "nallocated" && quantity.key !== "leaked alloc";
801+
if ((legend.key === "adiff" || legend.key === "pdiff") && thresholdEligibleQuantity) {
802+
const warnThreshold = Number((stepData && stepData.threshold) ?? state.payload.defaultWarnThreshold);
803+
const errorThreshold = Number((stepData && stepData.error_threshold) ?? state.payload.defaultErrorThreshold);
804+
let comparisonValue = value;
805+
if (legend.key === "pdiff") {
806+
const baseValue = Number(stepData && stepData[fieldName(quantity.key, "base")]);
807+
comparisonValue = Number.isFinite(baseValue) ? (value * baseValue) / 100 : NaN;
818808
}
819-
}
820-
821-
if (quantity.key === "max memory" && legend.key === "adiff") {
822-
const warnThreshold = stepData.threshold || state.payload.defaultWarnThreshold;
823-
const errorThreshold = stepData.error_threshold || state.payload.defaultErrorThreshold;
824-
if (value > errorThreshold) {
825-
classes.push("threshold-error");
826-
} else if (value > warnThreshold) {
827-
classes.push("threshold-warn");
828-
} else if (value < -errorThreshold) {
829-
classes.push("threshold-great");
830-
} else if (value < -warnThreshold) {
831-
classes.push("threshold-good");
809+
if (Number.isFinite(comparisonValue)) {
810+
if (Number.isFinite(errorThreshold) && comparisonValue > errorThreshold) {
811+
classes.push("threshold-error");
812+
} else if (Number.isFinite(warnThreshold) && comparisonValue > warnThreshold) {
813+
classes.push("threshold-warn");
814+
} else if (Number.isFinite(errorThreshold) && comparisonValue < -errorThreshold) {
815+
classes.push("threshold-great");
816+
} else if (Number.isFinite(warnThreshold) && comparisonValue < -warnThreshold) {
817+
classes.push("threshold-good");
818+
}
832819
}
833820
}
834821

@@ -874,6 +861,7 @@ <h1>Max Memory Summary Viewer</h1>
874861
workflow: workflowName,
875862
step,
876863
stepOrder: stepNumber(step),
864+
stepData,
877865
baseline: aggregatedNumericValue(stepData[fieldName(quantity.key, "base")]),
878866
pullRequest: aggregatedNumericValue(stepData[fieldName(quantity.key, "pr")]),
879867
absoluteDiff: aggregatedNumericValue(stepData[fieldName(quantity.key, "adiff")]),
@@ -936,6 +924,7 @@ <h1>Max Memory Summary Viewer</h1>
936924
const aggregation = currentAggregation();
937925
const detailRows = getDetailRows(workflowName, steps);
938926
const percentLegend = LEGENDS.find((legend) => legend.key === "pdiff");
927+
const adiffLegend = LEGENDS.find((legend) => legend.key === "adiff");
939928
const numberLegend = { key: "base", unit: quantity.unit };
940929
return `
941930
<div class="detail-wrap">
@@ -959,8 +948,8 @@ <h1>Max Memory Summary Viewer</h1>
959948
<td class="td-step">${escapeHtml(detailRow.step.replace("step", "Step "))}</td>
960949
<td class="td-num ${detailRow.baseline == null ? "cell-empty" : ""}">${formatValue(detailRow.baseline, quantity, numberLegend)}</td>
961950
<td class="td-num ${detailRow.pullRequest == null ? "cell-empty" : ""}">${formatValue(detailRow.pullRequest, quantity, numberLegend)}</td>
962-
<td class="td-num ${detailRow.absoluteDiff == null ? "cell-empty" : detailRow.absoluteDiff > 0 ? "delta-up" : detailRow.absoluteDiff < 0 ? "delta-down" : ""}">${formatValue(detailRow.absoluteDiff, quantity, numberLegend)}</td>
963-
<td class="td-num ${detailRow.percentDiff == null ? "cell-empty" : detailRow.percentDiff > 0 ? "delta-up" : detailRow.percentDiff < 0 ? "delta-down" : ""}">${formatValue(detailRow.percentDiff, quantity, percentLegend)}</td>
951+
<td class="td-num ${cellClass(quantity, adiffLegend, detailRow.stepData, detailRow.absoluteDiff)}">${formatValue(detailRow.absoluteDiff, quantity, numberLegend)}</td>
952+
<td class="td-num ${cellClass(quantity, percentLegend, detailRow.stepData, detailRow.percentDiff)}">${formatValue(detailRow.percentDiff, quantity, percentLegend)}</td>
964953
</tr>
965954
`).join("")}
966955
</tbody>
@@ -997,9 +986,7 @@ <h1>Max Memory Summary Viewer</h1>
997986
? "-"
998987
: `${formatValue(peak.value, quantity, adiffLegend)}${adiffLegend.unit || quantity.unit ? ` ${adiffLegend.unit || quantity.unit}` : ""} @ ${peak.workflow} ${peak.step}`;
999988

1000-
el.thresholdNote.textContent = quantity.key === "max memory"
1001-
? `Threshold highlighting uses warn ${state.payload.defaultWarnThreshold} MB and error ${state.payload.defaultErrorThreshold} MB defaults unless a step overrides them.`
1002-
: "Threshold highlighting is applied to max memory PR - baseline values.";
989+
el.thresholdNote.textContent = `Threshold highlighting uses warn ${state.payload.defaultWarnThreshold} and error ${state.payload.defaultErrorThreshold} defaults unless a step overrides them. For percent diff, thresholds are applied after converting percent back to absolute delta using baseline.`;
1003990

1004991
el.tableHeadRow.innerHTML = "";
1005992
const workflowHead = document.createElement("th");

0 commit comments

Comments
 (0)