Allow comparison across different run ids, but show a warning that it…

… may be invalid
smarr · Jul 18, 2023 · 710c819 · 710c819
1 parent 080e621
commit 710c819
Show file tree

Hide file tree

Showing 8 changed files with 83 additions and 21 deletions.
diff --git a/resources/style.css b/resources/style.css
@@ -228,6 +228,13 @@ table.benchmark-details .btn-sm {
   padding: 0 !important;
 }
 
+.benchmark-details .alert {
+  display: inline-block;
+  padding: 0rem 0.2rem;
+  margin: 0.1rem 0.5rem;
+  line-height: 1rem;
+}
+
 .warmup-benchmark {
   padding-right: 1.5ex;
   font-weight: bold;

diff --git a/src/backend/compare/html/index.html b/src/backend/compare/html/index.html
@@ -11,6 +11,11 @@
   <script src="/static/compare.js" type="module"></script>
 </head>
 <body class="compare timeline-multi">
+<svg xmlns="http://www.w3.org/2000/svg" style="display: none;">
+  <symbol id="exclamation-triangle-fill" fill="currentColor" viewBox="0 0 16 16">
+    <path d="M8.982 1.566a1.13 1.13 0 0 0-1.96 0L.165 13.233c-.457.778.091 1.767.98 1.767h13.713c.889 0 1.438-.99.98-1.767L8.982 1.566zM8 5c.535 0 .954.462.9.995l-.35 3.507a.552.552 0 0 1-1.1 0L7.1 5.995A.905.905 0 0 1 8 5zm.002 6a1 1 0 1 1 0 2 1 1 0 0 1 0-2z"/>
+  </symbol>
+</svg>
 
 <header>
 <div class="jumbotron compare">

diff --git a/src/backend/compare/html/stats-row.html b/src/backend/compare/html/stats-row.html
@@ -39,8 +39,16 @@
 
 
     if (hasTotal) {
+      let inconsistent = '';
+      if (stats.inconsistentRunIds) {
+        inconsistent = `<div class="alert alert-warning" role="alert"
+            title="The run configuration changed between the two versions. Comparison is likely invalid.">
+          <svg class="bi flex-shrink-0 me-2" width="16" height="16" role="img"
+            aria-label="The run configuration changed between the two versions. Comparison is likely invalid."><use xlink:href="#exclamation-triangle-fill"/></svg>
+        </div>`;
+      }
 %}<tr>
-<th scope="row">{%= stats.benchId.b %}{%- args %}</th>
+<th scope="row">{%= stats.benchId.b %}{%- args %}{%- inconsistent %}</th>
   <td class="inline-cmp"><img src="{%= it.config.reportsUrl %}/{%= stats.inlinePlot %}"></td>
 {%    if (stats.exeStats) {
 %}{%-   include('stats-row-across-exes.html', {

diff --git a/src/backend/compare/prep-data.ts b/src/backend/compare/prep-data.ts
@@ -69,24 +69,24 @@ export function compareToSortForSinglePassChangeStats(
   a: Measurements,
   b: Measurements
 ): number {
-  const r = compareToSortForSinglePassChangeStatsWithoutCommitId(a, b);
+  let r = compareToSortForSinglePassChangeStatsNonStrict(a, b);
+  if (r !== 0) {
+    return r;
+  }
+
+  r = a.runId - b.runId;
   if (r !== 0) {
     return r;
   }
 
   return a.commitId.localeCompare(b.commitId);
 }
 
-export function compareToSortForSinglePassChangeStatsWithoutCommitId(
+export function compareToSortForSinglePassChangeStatsNonStrict(
   a: Measurements,
   b: Measurements
 ): number {
-  let r = a.runId - b.runId;
-  if (r !== 0) {
-    return r;
-  }
-
-  r = a.envId - b.envId;
+  let r = a.envId - b.envId;
   if (r !== 0) {
     return r;
   }
@@ -374,9 +374,7 @@ export function countVariantsAndDropMissing(
 
     const change = measurements[i + 1];
 
-    if (
-      compareToSortForSinglePassChangeStatsWithoutCommitId(base, change) !== 0
-    ) {
+    if (compareToSortForSinglePassChangeStatsNonStrict(base, change) !== 0) {
       dropAsMissing(i);
     } else {
       i += 2;
@@ -499,6 +497,9 @@ async function computeStatisticsAndInlinePlot(
 
     // add the various details
     row.details.hasWarmup = siteConfig.canShowWarmup(change.values);
+    if (row.inconsistentRunIds === undefined || row.inconsistentRunIds) {
+      row.inconsistentRunIds = base.runId !== change.runId;
+    }
 
     if (!row.versionStats) {
       row.versionStats = {};

diff --git a/src/shared/view-types.ts b/src/shared/view-types.ts
@@ -113,6 +113,8 @@ export interface CompareStatsRow {
 
   inlinePlot?: string;
 
+  inconsistentRunIds?: boolean;
+
   /** Statistics per criterion, comparing base and change. */
   versionStats?: CompareStatsRowAcrossVersions;
   exeStats?: CompareStatsRowAcrossExes[];

diff --git a/tests/backend/compare/prep-data.test.ts b/tests/backend/compare/prep-data.test.ts
@@ -82,14 +82,20 @@ const runSettings: RunSettings = {
   simplifiedCmdline: 'Exec TestBenchmark1'
 };
 
-function makeM(criterion, unit, envId, commitId) {
+function makeM(
+  criterion: string,
+  unit: string,
+  envId: number,
+  commitId: string,
+  runId = 1
+) {
   return {
     criterion: { name: criterion, unit },
     values: [[]],
     envId,
     commitId,
     runSettings,
-    runId: 1,
+    runId,
     trialId: 1,
     expId: 1
   };
@@ -210,6 +216,31 @@ describe('compareToSortForSinglePassChangeStats()', () => {
     expect(data[5].envId).toBe(2);
     expect(data[5].criterion.name).toBe('total');
   });
+
+  it(
+    'should place same criteria next to each other, ' +
+      'even when runId differs',
+    () => {
+      const data: Measurements[] = [
+        makeM('total', 'ms', 1, 'a', 1),
+        makeM('alloc', 'by', 1, 'b', 2),
+        makeM('total', 'ms', 1, 'b', 2),
+        makeM('alloc', 'by', 1, 'a', 1)
+      ];
+
+      data.sort(compareToSortForSinglePassChangeStats);
+
+      expect(data[0].commitId).toBe('a');
+      expect(data[0].criterion.name).toBe('alloc');
+      expect(data[1].commitId).toBe('b');
+      expect(data[1].criterion.name).toBe('alloc');
+
+      expect(data[2].commitId).toBe('a');
+      expect(data[2].criterion.name).toBe('total');
+      expect(data[3].commitId).toBe('b');
+      expect(data[3].criterion.name).toBe('total');
+    }
+  );
 });
 
 describe('countVariantsAndDropMissing()', () => {
@@ -318,19 +349,17 @@ describe('countVariantsAndDropMissing()', () => {
     expect(mc2[0].criterion.name).toEqual('total');
   });
 
-  it('should consider different runIds as incompatible', () => {
+  it('should not drop different runIds', () => {
     const data: Measurements[] = [
-      makeM('total', 'ms', 1, 'a'),
-      makeM('total', 'ms', 1, 'a')
+      makeM('total', 'ms', 1, 'a', 1),
+      makeM('total', 'ms', 1, 'a', 2)
     ];
-    data[0].runId = 2;
 
     const result = countVariantsAndDropMissing(makeProRes(data), 'a', 'b');
 
-    expect(data).toHaveLength(0);
+    expect(data).toHaveLength(2);
     expect(result.missing).toBeDefined();
-    expect(result.missing.size).toEqual(1);
-    expect([...result.missing.values()][0].missing).toHaveLength(2);
+    expect(result.missing.size).toEqual(0);
   });
 });
 

diff --git a/tests/data/expected-results/stats-data-prep/compare-view-jssom.html b/tests/data/expected-results/stats-data-prep/compare-view-jssom.html
@@ -22,6 +22,11 @@
   <script src="/static/compare.js" type="module"></script>
 </head>
 <body class="compare timeline-multi">
+<svg xmlns="http://www.w3.org/2000/svg" style="display: none;">
+  <symbol id="exclamation-triangle-fill" fill="currentColor" viewBox="0 0 16 16">
+    <path d="M8.982 1.566a1.13 1.13 0 0 0-1.96 0L.165 13.233c-.457.778.091 1.767.98 1.767h13.713c.889 0 1.438-.99.98-1.767L8.982 1.566zM8 5c.535 0 .954.462.9.995l-.35 3.507a.552.552 0 0 1-1.1 0L7.1 5.995A.905.905 0 0 1 8 5zm.002 6a1 1 0 1 1 0 2 1 1 0 0 1 0-2z"/>
+  </symbol>
+</svg>
 
 <header>
 <div class="jumbotron compare">

diff --git a/tests/data/expected-results/stats-data-prep/compare-view-tsom.html b/tests/data/expected-results/stats-data-prep/compare-view-tsom.html
@@ -22,6 +22,11 @@
   <script src="/static/compare.js" type="module"></script>
 </head>
 <body class="compare timeline-multi">
+<svg xmlns="http://www.w3.org/2000/svg" style="display: none;">
+  <symbol id="exclamation-triangle-fill" fill="currentColor" viewBox="0 0 16 16">
+    <path d="M8.982 1.566a1.13 1.13 0 0 0-1.96 0L.165 13.233c-.457.778.091 1.767.98 1.767h13.713c.889 0 1.438-.99.98-1.767L8.982 1.566zM8 5c.535 0 .954.462.9.995l-.35 3.507a.552.552 0 0 1-1.1 0L7.1 5.995A.905.905 0 0 1 8 5zm.002 6a1 1 0 1 1 0 2 1 1 0 0 1 0-2z"/>
+  </symbol>
+</svg>
 
 <header>
 <div class="jumbotron compare">