Add performance improvements to alert comment

Author: 414owen

src/write.ts

@@ -94,10 +94,11 @@ interface Alert {
     ratio: number;
 }
 
-function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number): Alert[] {
+function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number): [Alert[], Alert[]] {
     core.debug(`Comparing current:${curSuite.commit.id} and prev:${prevSuite.commit.id} for alert`);
 
-    const alerts = [];
+    const losses = [];
+    const gains = [];
     for (const current of curSuite.benches) {
         const prev = prevSuite.benches.find((b) => b.name === current.name);
         if (prev === undefined) {
@@ -107,16 +108,18 @@ function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number
 
         const ratio = getRatio(curSuite.tool, prev, current);
 
-        if (ratio > threshold) {
+        if (threshold === 0 || ratio > threshold) {
             core.warning(
                 `Performance alert! Previous value was ${prev.value} and current value is ${current.value}.` +
-                    ` It is ${ratio}x worse than previous exceeding a ratio threshold ${threshold}`,
+                    ` It is ${ratio}x worse than previous, exceeding ratio threshold ${threshold}`,
             );
-            alerts.push({ current, prev, ratio });
+            losses.push({ current, prev, ratio });
+        } else if (ratio < 1 / threshold) {
+            gains.push({ current, prev, ratio });
         }
     }
 
-    return alerts;
+    return [losses, gains];
 }
 
 function getCurrentRepoMetadata() {
@@ -175,7 +178,10 @@ export function buildComment(
         '',
         expandableDetails ? '<details>' : '',
         '',
-        `| Benchmark suite | Current: ${curSuite.commit.id} | Previous: ${prevSuite.commit.id} | Ratio |`,
+        `Previous: ${prevSuite.commit.id}`,
+        `Current: ${curSuite.commit.id}`,
+        '',
+        `| Benchmark suite | Current | Previous | Ratio |`,
         '|-|-|-|-|',
     ];
 
@@ -200,32 +206,77 @@ export function buildComment(
     return lines.join('\n');
 }
 
+function pushResultLines(results: Alert[], output: string[]) {
+    results.sort((a, b) => a.ratio - b.ratio);
+    for (const alert of results) {
+        const { current, prev, ratio } = alert;
+        const line = `| \`${current.name}\` | ${strVal(current)} | ${strVal(prev)} | \`${floatStr(ratio)}\` |`;
+        output.push(line);
+    }
+}
+
+const RESULT_TABLE_HEADER = ['', `| Benchmark suite | Current | Previous | Ratio |`, '|-|-|-|-|'];
+
+function buildReportComment(
+    results: Alert[],
+    benchName: string,
+    curSuite: Benchmark,
+    prevSuite: Benchmark,
+    cc: string[],
+): string {
+    // Do not show benchmark name if it is the default value 'Benchmark'.
+    const benchmarkText = benchName === 'Benchmark' ? '' : ` **'${benchName}'**`;
+    const lines = [
+        '# Performance Report',
+        '',
+        `For benchmark${benchmarkText}.`,
+        '',
+        `Previous commit: ${prevSuite.commit.id}`,
+        `Current commit: ${curSuite.commit.id}`,
+    ];
+
+    lines.push(...RESULT_TABLE_HEADER);
+    pushResultLines(results, lines);
+    lines.push('', commentFooter());
+
+    if (cc.length > 0) {
+        lines.push('', `CC: ${cc.join(' ')}`);
+    }
+
+    return lines.join('\n');
+}
+
 function buildAlertComment(
-    alerts: Alert[],
+    losses: Alert[],
+    gains: Alert[],
     benchName: string,
     curSuite: Benchmark,
     prevSuite: Benchmark,
     threshold: number,
     cc: string[],
 ): string {
     // Do not show benchmark name if it is the default value 'Benchmark'.
-    const benchmarkText = benchName === 'Benchmark' ? '' : ` **'${benchName}'**`;
-    const title = threshold === 0 ? '# Performance Report' : '# :warning: **Performance Alert** :warning:';
+    const benchmarkText = benchName === 'Benchmark' ? '' : ` for **'${benchName}'**`;
     const thresholdString = floatStr(threshold);
     const lines = [
-        title,
+        `# Performance Report${benchmarkText}`,
         '',
-        `Possible performance regression was detected for benchmark${benchmarkText}.`,
-        `Benchmark result of this commit is worse than the previous benchmark result exceeding threshold \`${thresholdString}\`.`,
+        `Benchmark result(s) exceed ratio of \`${thresholdString}\`.`,
         '',
-        `| Benchmark suite | Current: ${curSuite.commit.id} | Previous: ${prevSuite.commit.id} | Ratio |`,
-        '|-|-|-|-|',
+        `Previous commit: ${prevSuite.commit.id}`,
+        `Current commit: ${curSuite.commit.id}`,
     ];
 
-    for (const alert of alerts) {
-        const { current, prev, ratio } = alert;
-        const line = `| \`${current.name}\` | ${strVal(current)} | ${strVal(prev)} | \`${floatStr(ratio)}\` |`;
-        lines.push(line);
+    if (losses.length > 0) {
+        lines.push(...['', '### :snail: The following benchmarks show regressions:']);
+        lines.push(...RESULT_TABLE_HEADER);
+        pushResultLines(losses, lines);
+    }
+
+    if (gains.length > 0) {
+        lines.push(...['', '### :rocket: The following benchmarks show improvements:']);
+        lines.push(...RESULT_TABLE_HEADER);
+        pushResultLines(gains, lines);
     }
 
     // Footer
@@ -276,14 +327,22 @@ async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Be
         return;
     }
 
-    const alerts = findAlerts(curSuite, prevSuite, alertThreshold);
+    const [losses, gains] = findAlerts(curSuite, prevSuite, alertThreshold);
+    const alerts = [...losses, ...gains];
     if (alerts.length === 0) {
         core.debug('No performance alert found happily');
         return;
     }
 
-    core.debug(`Found ${alerts.length} alerts`);
-    const body = buildAlertComment(alerts, benchName, curSuite, prevSuite, alertThreshold, alertCommentCcUsers);
+    let body = '';
+    if (alertThreshold === 0) {
+        core.debug(`Alert threshold is 0. Leaving report with ${alerts.length} alerts`);
+        body = buildReportComment(alerts, benchName, curSuite, prevSuite, alertCommentCcUsers);
+    } else {
+        core.debug(`Found ${alerts.length} alerts`);
+        body = buildAlertComment(losses, gains, benchName, curSuite, prevSuite, alertThreshold, alertCommentCcUsers);
+    }
+
     let message = body;
 
     if (commentOnAlert) {
@@ -297,9 +356,9 @@ async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Be
 
     if (failOnAlert) {
         // Note: alertThreshold is smaller than failThreshold. It was checked in config.ts
-        const len = alerts.length;
+        const len = losses.length;
         const threshold = floatStr(failThreshold);
-        const failures = alerts.filter((a) => a.ratio > failThreshold);
+        const failures = losses.filter((a) => a.ratio > failThreshold);
         if (failures.length > 0) {
             core.debug('Mark this workflow as fail since one or more fatal alerts found');
             if (failThreshold !== alertThreshold) {

test/buildComment.test.ts

@@ -108,8 +108,11 @@ describe('buildComment', () => {
             # TestSuite
             
             <details>
+
+            Previous: testCommitIdPrevious
+            Current: testCommitIdCurrent
             
-            | Benchmark suite | Current: testCommitIdCurrent | Previous: testCommitIdPrevious | Ratio |
+            | Benchmark suite | Current | Previous | Ratio |
             |-|-|-|-|
             | \`TestBench<1>\` | \`0\` testUnit | \`0\` testUnit | \`1\` |
             | \`TestBench<2>\` | \`1\` testUnit | \`0\` testUnit | \`+∞\` |
@@ -195,7 +198,10 @@ describe('buildComment', () => {
             
             <details>
             
-            | Benchmark suite | Current: testCommitIdCurrent | Previous: testCommitIdPrevious | Ratio |
+            Previous: testCommitIdPrevious
+            Current: testCommitIdCurrent
+
+            | Benchmark suite | Current | Previous | Ratio |
             |-|-|-|-|
             | \`TestBench<1>\` | \`0\` testUnit | \`0\` testUnit | \`1\` |
             | \`TestBench<2>\` | \`0\` testUnit | \`1\` testUnit | \`+∞\` |