fix: time units are not normalized (issue #122) (#318)

This PR fixes an issue when the new benchmark results are using a different time unit than the previous one. This happens specifically when the benchmark values are very close to the boundary between 2 units, ex. `990 ns/iter` and `1.1 us/iter`. In that case, we need to normalize the newly coming results to the previously used unit so that the values are comparable.

* extract addBenchmarkEntry function
* normalize new entry values to match last entry units
* handle `<time>`, `<time>/iter` and `ops/<time>` units

Author: ktrz

src/addBenchmarkEntry.ts

@@ -0,0 +1,41 @@
+import { Benchmark } from './extract';
+import * as core from '@actions/core';
+import { BenchmarkSuites } from './write';
+import { normalizeBenchmark } from './normalizeBenchmark';
+
+export function addBenchmarkEntry(
+    benchName: string,
+    benchEntry: Benchmark,
+    entries: BenchmarkSuites,
+    maxItems: number | null,
+): { prevBench: Benchmark | null; normalizedCurrentBench: Benchmark } {
+    let prevBench: Benchmark | null = null;
+    let normalizedCurrentBench: Benchmark = benchEntry;
+
+    // Add benchmark result
+    if (entries[benchName] === undefined) {
+        entries[benchName] = [benchEntry];
+        core.debug(`No suite was found for benchmark '${benchName}' in existing data. Created`);
+    } else {
+        const suites = entries[benchName];
+        // Get the last suite which has different commit ID for alert comment
+        for (const e of [...suites].reverse()) {
+            if (e.commit.id !== benchEntry.commit.id) {
+                prevBench = e;
+                break;
+            }
+        }
+
+        normalizedCurrentBench = normalizeBenchmark(prevBench, benchEntry);
+
+        suites.push(normalizedCurrentBench);
+
+        if (maxItems !== null && suites.length > maxItems) {
+            suites.splice(0, suites.length - maxItems);
+            core.debug(
+                `Number of data items for '${benchName}' was truncated to ${maxItems} due to max-items-in-charts`,
+            );
+        }
+    }
+    return { prevBench, normalizedCurrentBench };
+}

src/canonicalizeUnit.ts

@@ -0,0 +1,3 @@
+export function canonicalizeUnit(u: string): string {
+    return u.trim().toLowerCase().replace(/µs|μs/g, 'us');
+}

src/extractRangeInfo.ts

@@ -0,0 +1,24 @@
+export function extractRangeInfo(range: string | undefined): { prefix: string; value: number } | undefined {
+    if (!range) {
+        return undefined;
+    }
+
+    const matches = range.match(/(?<prefix>(\+-|±)\s*)(?<value>\d.*)/);
+
+    if (!matches || !matches.groups) {
+        return undefined;
+    }
+
+    const valueString = matches.groups.value;
+
+    const value = Number(valueString);
+
+    if (isNaN(value)) {
+        return undefined;
+    }
+
+    return {
+        value,
+        prefix: matches.groups.prefix ?? '',
+    };
+}

src/normalizeBenchmark.ts

@@ -0,0 +1,20 @@
+import { Benchmark } from './extract';
+import { normalizeBenchmarkResult } from './normalizeBenchmarkResult';
+
+export function normalizeBenchmark(prevBench: Benchmark | null, currentBench: Benchmark): Benchmark {
+    if (!prevBench) {
+        return currentBench;
+    }
+
+    return {
+        ...currentBench,
+        benches: currentBench.benches
+            .map((currentBenchResult) => ({
+                currentBenchResult,
+                prevBenchResult: prevBench.benches.find((result) => result.name === currentBenchResult.name),
+            }))
+            .map(({ currentBenchResult, prevBenchResult }) =>
+                normalizeBenchmarkResult(prevBenchResult, currentBenchResult),
+            ),
+    };
+}

src/normalizeBenchmarkResult.ts

@@ -0,0 +1,33 @@
+import { BenchmarkResult } from './extract';
+import { normalizeValueByUnit } from './normalizeValueByUnit';
+import { extractRangeInfo } from './extractRangeInfo';
+
+export function normalizeBenchmarkResult(
+    prevBenchResult: BenchmarkResult | undefined | null,
+    currentBenchResult: BenchmarkResult,
+): BenchmarkResult {
+    if (!prevBenchResult) {
+        return currentBenchResult;
+    }
+
+    const prevUnit = prevBenchResult.unit;
+    const currentUnit = currentBenchResult.unit;
+    const currentRange = currentBenchResult.range;
+    const currentRangeInfo = extractRangeInfo(currentRange);
+
+    const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currentBenchResult.value);
+    const normalizedUnit = currentBenchResult.value !== normalizedValue ? prevUnit : currentUnit;
+    const normalizedRangeInfo = currentRangeInfo
+        ? {
+              prefix: currentRangeInfo.prefix,
+              value: normalizeValueByUnit(prevUnit, currentUnit, currentRangeInfo.value),
+          }
+        : undefined;
+
+    return {
+        ...currentBenchResult,
+        value: normalizedValue,
+        unit: normalizedUnit,
+        range: normalizedRangeInfo ? `${normalizedRangeInfo.prefix}${normalizedRangeInfo.value}` : currentRange,
+    };
+}

src/normalizeValueByUnit.ts

@@ -0,0 +1,24 @@
+import { canonicalizeUnit } from './canonicalizeUnit';
+
+export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number {
+    const prev = canonicalizeUnit(prevUnit);
+    const current = canonicalizeUnit(currentUnit);
+    for (const units of SUPPORTED_UNITS) {
+        const prevUnitIndex = units.indexOf(prev);
+        const currentUnitIndex = units.indexOf(current);
+
+        if (prevUnitIndex >= 0 && currentUnitIndex >= 0) {
+            const unitDiff = prevUnitIndex - currentUnitIndex;
+
+            return value * UNIT_CONVERSION_MULTIPLIER ** unitDiff;
+        }
+    }
+
+    return value;
+}
+
+const UNIT_CONVERSION_MULTIPLIER = 1000;
+const TIME_UNITS = ['s', 'ms', 'us', 'ns'];
+const ITER_UNITS = TIME_UNITS.map((unit) => `${unit}/iter`);
+const OPS_PER_TIME_UNIT = [...TIME_UNITS].reverse().map((unit) => `ops/${unit}`);
+const SUPPORTED_UNITS = [TIME_UNITS, ITER_UNITS, OPS_PER_TIME_UNIT];

src/write.ts

@@ -9,6 +9,7 @@ import { Config, ToolType } from './config';
 import { DEFAULT_INDEX_HTML } from './default_index_html';
 import { leavePRComment } from './comment/leavePRComment';
 import { leaveCommitComment } from './comment/leaveCommitComment';
+import { addBenchmarkEntry } from './addBenchmarkEntry';
 
 export type BenchmarkSuites = { [name: string]: Benchmark[] };
 export interface DataJson {
@@ -321,39 +322,16 @@ function addBenchmarkToDataJson(
     bench: Benchmark,
     data: DataJson,
     maxItems: number | null,
-): Benchmark | null {
+): { prevBench: Benchmark | null; normalizedCurrentBench: Benchmark } {
     const repoMetadata = getCurrentRepoMetadata();
     const htmlUrl = repoMetadata.html_url ?? '';
 
-    let prevBench: Benchmark | null = null;
     data.lastUpdate = Date.now();
     data.repoUrl = htmlUrl;
 
-    // Add benchmark result
-    if (data.entries[benchName] === undefined) {
-        data.entries[benchName] = [bench];
-        core.debug(`No suite was found for benchmark '${benchName}' in existing data. Created`);
-    } else {
-        const suites = data.entries[benchName];
-        // Get last suite which has different commit ID for alert comment
-        for (const e of suites.slice().reverse()) {
-            if (e.commit.id !== bench.commit.id) {
-                prevBench = e;
-                break;
-            }
-        }
+    const { prevBench, normalizedCurrentBench } = addBenchmarkEntry(benchName, bench, data.entries, maxItems);
 
-        suites.push(bench);
-
-        if (maxItems !== null && suites.length > maxItems) {
-            suites.splice(0, suites.length - maxItems);
-            core.debug(
-                `Number of data items for '${benchName}' was truncated to ${maxItems} due to max-items-in-charts`,
-            );
-        }
-    }
-
-    return prevBench;
+    return { prevBench, normalizedCurrentBench };
 }
 
 function isRemoteRejectedError(err: unknown): err is Error {
@@ -367,7 +345,7 @@ async function writeBenchmarkToGitHubPagesWithRetry(
     bench: Benchmark,
     config: Config,
     retry: number,
-): Promise<Benchmark | null> {
+): Promise<{ prevBench: Benchmark | null; normalizedCurrentBench: Benchmark }> {
     const {
         name,
         tool,
@@ -421,7 +399,7 @@ async function writeBenchmarkToGitHubPagesWithRetry(
     await io.mkdirP(benchmarkDataDirFullPath);
 
     const data = await loadDataJs(dataPath);
-    const prevBench = addBenchmarkToDataJson(name, bench, data, maxItemsInChart);
+    const { prevBench, normalizedCurrentBench } = addBenchmarkToDataJson(name, bench, data, maxItemsInChart);
 
     await storeDataJs(dataPath, data);
 
@@ -469,10 +447,13 @@ async function writeBenchmarkToGitHubPagesWithRetry(
         );
     }
 
-    return prevBench;
+    return { prevBench, normalizedCurrentBench };
 }
 
-async function writeBenchmarkToGitHubPages(bench: Benchmark, config: Config): Promise<Benchmark | null> {
+async function writeBenchmarkToGitHubPages(
+    bench: Benchmark,
+    config: Config,
+): Promise<{ prevBench: Benchmark | null; normalizedCurrentBench: Benchmark }> {
     const { ghPagesBranch, skipFetchGhPages, ghRepository, githubToken } = config;
     if (!ghRepository) {
         if (!skipFetchGhPages) {
@@ -508,14 +489,14 @@ async function writeBenchmarkToExternalJson(
     bench: Benchmark,
     jsonFilePath: string,
     config: Config,
-): Promise<Benchmark | null> {
+): Promise<{ prevBench: Benchmark | null; normalizedCurrentBench: Benchmark }> {
     const { name, maxItemsInChart, saveDataFile } = config;
     const data = await loadDataJson(jsonFilePath);
-    const prevBench = addBenchmarkToDataJson(name, bench, data, maxItemsInChart);
+    const { prevBench, normalizedCurrentBench } = addBenchmarkToDataJson(name, bench, data, maxItemsInChart);
 
     if (!saveDataFile) {
         core.debug('Skipping storing benchmarks in external data file');
-        return prevBench;
+        return { prevBench, normalizedCurrentBench };
     }
 
     try {
@@ -526,12 +507,12 @@ async function writeBenchmarkToExternalJson(
         throw new Error(`Could not store benchmark data as JSON at ${jsonFilePath}: ${err}`);
     }
 
-    return prevBench;
+    return { prevBench, normalizedCurrentBench };
 }
 
 export async function writeBenchmark(bench: Benchmark, config: Config) {
     const { name, externalDataJsonPath } = config;
-    const prevBench = externalDataJsonPath
+    const { prevBench, normalizedCurrentBench } = externalDataJsonPath
         ? await writeBenchmarkToExternalJson(bench, externalDataJsonPath, config)
         : await writeBenchmarkToGitHubPages(bench, config);
 
@@ -540,9 +521,9 @@ export async function writeBenchmark(bench: Benchmark, config: Config) {
     if (prevBench === null) {
         core.debug('Alert check was skipped because previous benchmark result was not found');
     } else {
-        await handleComment(name, bench, prevBench, config);
-        await handleSummary(name, bench, prevBench, config);
-        await handleAlert(name, bench, prevBench, config);
+        await handleComment(name, normalizedCurrentBench, prevBench, config);
+        await handleSummary(name, normalizedCurrentBench, prevBench, config);
+        await handleAlert(name, normalizedCurrentBench, prevBench, config);
     }
 }
 

test/extractRangeInfo.spec.ts

@@ -0,0 +1,43 @@
+import { extractRangeInfo } from '../src/extractRangeInfo';
+
+describe('extractRangeInfo', () => {
+    it("should extract range with '±'", () => {
+        expect(extractRangeInfo('±20')).toEqual({ value: 20, prefix: '±' });
+    });
+
+    it("should extract range with '± '", () => {
+        expect(extractRangeInfo('± 20')).toEqual({ value: 20, prefix: '± ' });
+    });
+
+    it("should extract range with '+-'", () => {
+        expect(extractRangeInfo('+-20')).toEqual({ value: 20, prefix: '+-' });
+    });
+
+    it("should extract range with '+- '", () => {
+        expect(extractRangeInfo('+- 20')).toEqual({ value: 20, prefix: '+- ' });
+    });
+
+    it('should extract single-digit integer', () => {
+        expect(extractRangeInfo('±2')).toEqual({ value: 2, prefix: '±' });
+    });
+    it('should extract decimal and preserve prefix space', () => {
+        expect(extractRangeInfo('± 0.5')).toEqual({ value: 0.5, prefix: '± ' });
+    });
+    it('should extract scientific notation', () => {
+        expect(extractRangeInfo('+-1e-3')).toEqual({ value: 1e-3, prefix: '+-' });
+    });
+    it('should tolerate surrounding whitespace', () => {
+        expect(extractRangeInfo('  ±20  ')).toEqual({ value: 20, prefix: '±' });
+    });
+
+    it('should NOT extract range with unknown prefix', () => {
+        expect(extractRangeInfo('unknown prefix 20')).toBeUndefined();
+    });
+
+    it('should NOT extract range with invalid number', () => {
+        expect(extractRangeInfo('± boo')).toBeUndefined();
+        expect(extractRangeInfo('+- boo')).toBeUndefined();
+        expect(extractRangeInfo('± 1boo')).toBeUndefined();
+        expect(extractRangeInfo('+- 1boo')).toBeUndefined();
+    });
+});