Reduce CoreMark benchmark noise in CI

.github/workflows/ci.yml

@@ -205,6 +205,11 @@ jobs:
     runs-on: ubuntu-latest
     permissions:
       contents: read
+    env:
+      # How many times each side is timed. The reported figure is the median, so
+      # a single unlucky run no longer decides the result. Kept low because the
+      # instruction count below, not the score, is what resolves small changes.
+      BENCH_SAMPLES: 3
     steps:
       - name: Checkout code
         uses: actions/checkout@v4
@@ -222,50 +227,212 @@ jobs:
           tar -xzf wabt-${WABT_VERSION}-${WABT_PLATFORM}.tar.gz
           sudo cp wabt-${WABT_VERSION}/bin/* /usr/local/bin/
 
+      - name: Install Valgrind
+        # Counts the instructions each side executes. Hardware counters are not
+        # an option here: GitHub runners are Azure VMs whose hypervisor does not
+        # expose the PMU, so perf reports hardware events as unsupported.
+        # Valgrind needs none, being pure user-space instrumentation.
+        run: |
+          sudo apt-get update
+          sudo apt-get install --assume-yes --no-install-recommends valgrind
+
       - name: Cache Rust dependencies
         # Pinned commit resolved from the annotated Swatinem/rust-cache@v2 tag.
         uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32
         with:
           save-if: ${{ github.ref == 'refs/heads/main' }}
           cache-bin: false
 
-      - name: Run CoreMark benchmark
-        id: benchmark
+      - name: Build benchmark binaries
         run: |
-          # Build and run the benchmark from workspace root, capturing output
-          output=$(cargo bench -p spacewasm_std --bench coremark --no-fail-fast 2>&1)
-          echo "$output"
-
-          # Extract the CoreMark score from the output
-          score=$(echo "$output" | grep "CoreMark Score:" | awk '{print $3}')
-          if [ -z "$score" ]; then
-            echo "Failed to extract CoreMark score"
-            exit 1
-          fi
+          set -euo pipefail
+
+          # Stage the bench binary next to the wasm it loads, so a side can be
+          # re-run later without checking its revision back out.
+          stage() {
+            local dest="$1"
+            local exe
+            exe=$(cargo bench -p spacewasm_std --bench coremark --no-run --message-format=json \
+              | jq -r 'select(.reason == "compiler-artifact")
+                       | select(.target.kind | index("bench"))
+                       | select(.target.name == "coremark")
+                       | .executable' \
+              | tail -n 1)
+            if [ -z "$exe" ] || [ ! -x "$exe" ]; then
+              echo "Could not locate the CoreMark benchmark executable"
+              return 1
+            fi
+            mkdir -p "$dest/benches"
+            cp "$exe" "$dest/coremark"
+            cp crates/spacewasm_std/benches/coremark-minimal.wasm "$dest/benches/"
+          }
 
-          echo "score=$score" >> $GITHUB_OUTPUT
-          echo "Benchmark score: $score"
+          # Keep the measurement harness and workload identical for both builds.
+          # The baseline must vary only the implementation under measurement;
+          # otherwise older main branches cannot pin the workload and the two
+          # instruction counts are not comparable.
+          harness="$RUNNER_TEMP/coremark-harness"
+          mkdir -p "$harness"
+          cp crates/spacewasm_std/benches/coremark.rs "$harness/"
+          cp crates/spacewasm_std/benches/coremark-minimal.wasm "$harness/"
 
-      - name: Download baseline benchmark
-        id: baseline
-        continue-on-error: true
-        run: |
-          # Try to get baseline from main branch
+          stage "$RUNNER_TEMP/pr"
+
+          # The baseline stays best effort. A harness written against an
+          # interface main does not have yet will not build there, and a PR
+          # should still get a score of its own when that happens.
           if [ "${{ github.event_name }}" == "pull_request" ]; then
-            git fetch origin main:main
-            git checkout main
-            baseline_output=$(cargo bench -p spacewasm_std --bench coremark --no-fail-fast 2>&1) || true
-            baseline_score=$(echo "$baseline_output" | grep "CoreMark Score:" | awk '{print $3}')
-            git checkout -
+            head=$(git rev-parse HEAD)
+            if git fetch --no-tags origin main && git checkout --detach FETCH_HEAD; then
+              cp "$harness/coremark.rs" crates/spacewasm_std/benches/coremark.rs
+              cp "$harness/coremark-minimal.wasm" \
+                crates/spacewasm_std/benches/coremark-minimal.wasm
+              stage "$RUNNER_TEMP/base" \
+                || echo "::warning::could not build the baseline against this branch's harness"
+              git restore --source=HEAD -- \
+                crates/spacewasm_std/benches/coremark.rs \
+                crates/spacewasm_std/benches/coremark-minimal.wasm
+              git checkout --detach "$head"
+            else
+              echo "::warning::could not check out main to build a baseline"
+            fi
+          fi
+
+      - name: Count instructions
+        id: instructions
+        run: |
+          set -euo pipefail
+
+          # Callgrind instruction counts do not depend on the runner's CPU model,
+          # its neighbours, or the hypervisor. With the same harness and pinned
+          # workload on both sides, they provide a repeatable work proxy for the
+          # implementation change.
+          #
+          # COREMARK_FIXED_CLOCK is what makes the two counts comparable:
+          # without it the module sizes its own workload from wall-clock time,
+          # in steps of nearly 2x, and the count would describe the runner
+          # rather than the build.
+          sides=(pr)
+          if [ -x "$RUNNER_TEMP/base/coremark" ]; then
+            sides+=(base)
+          fi
 
-            if [ -n "$baseline_score" ]; then
-              echo "baseline=$baseline_score" >> $GITHUB_OUTPUT
-              echo "Baseline score: $baseline_score"
+          # A pinned run scores exactly 10.000 and takes a fraction of a second,
+          # which is a cheaper way to find out that a side cannot pin than the
+          # callgrind run it would spoil. A pr side that cannot pin is this
+          # branch's own doing and fails the job. A baseline that cannot pin is
+          # not, so the counts are skipped and the timed comparison stands.
+          for side in "${sides[@]}"; do
+            log="$RUNNER_TEMP/$side.pin.log"
+            if ! ( cd "$RUNNER_TEMP/$side" && COREMARK_FIXED_CLOCK=1 ./coremark ) > "$log" 2>&1 \
+              || ! grep -q '^CoreMark Score: 10.000$' "$log"; then
+              cat "$log"
+              if [ "$side" = pr ]; then
+                echo "The pr benchmark did not produce the pinned workload"
+                exit 1
+              fi
+              echo "::warning::the baseline did not produce a pinned run; skipping instruction counts"
+              exit 0
+            fi
+          done
+
+          for side in "${sides[@]}"; do
+            out="$RUNNER_TEMP/callgrind.$side"
+            log="$RUNNER_TEMP/$side.count.log"
+
+            if ! (
+              cd "$RUNNER_TEMP/$side"
+              COREMARK_FIXED_CLOCK=1 valgrind --tool=callgrind \
+                --cache-sim=no --branch-sim=no \
+                --callgrind-out-file="$out" ./coremark
+            ) > "$log" 2>&1; then
+              cat "$log"
+              echo "Failed to count instructions for $side"
+              exit 1
+            fi
+
+            # Whole-process total, so parsing and compiling the module count as
+            # well as interpreting it. Ir is the only event collected here, so
+            # the summary carries a single figure.
+            ir=$(awk '/^(summary|totals):/ { print $2; exit }' "$out")
+            if ! [[ "$ir" =~ ^[0-9]+$ ]]; then
+              cat "$log"
+              echo "No instruction count in $out"
+              exit 1
+            fi
+
+            if [ "$side" = pr ]; then
+              echo "instructions=$ir" >> "$GITHUB_OUTPUT"
+              echo "Instructions: $ir"
             else
-              echo "baseline=" >> $GITHUB_OUTPUT
+              echo "baseline_instructions=$ir" >> "$GITHUB_OUTPUT"
+              echo "Baseline instructions: $ir"
             fi
+          done
+
+      - name: Run CoreMark benchmark
+        id: benchmark
+        run: |
+          set -euo pipefail
+
+          run_once() {
+            ( cd "$1" && ./coremark ) | awk '/CoreMark Score:/ { print $3 }'
+          }
+
+          summarize() {
+            sort -g "$1" | awk '
+              { v[NR] = $1 }
+              END {
+                median = (NR % 2) ? v[(NR + 1) / 2] : (v[NR / 2] + v[NR / 2 + 1]) / 2
+                printf "%.3f %.3f %.3f", median, v[1], v[NR]
+              }'
+          }
+
+          sides=(pr)
+          : > "$RUNNER_TEMP/pr.scores"
+          if [ -x "$RUNNER_TEMP/base/coremark" ]; then
+            sides+=(base)
+            : > "$RUNNER_TEMP/base.scores"
+          fi
+
+          for i in $(seq 1 "$BENCH_SAMPLES"); do
+            # Swap the order on every other sample. Measuring one side first
+            # every time hands it a systematically different machine state.
+            order=("${sides[@]}")
+            if [ "${#sides[@]}" -eq 2 ] && [ $((i % 2)) -eq 0 ]; then
+              order=(base pr)
+            fi
+
+            for side in "${order[@]}"; do
+              score=$(run_once "$RUNNER_TEMP/$side")
+              if [ -z "$score" ]; then
+                echo "Failed to extract CoreMark score for $side"
+                exit 1
+              fi
+              echo "$score" >> "$RUNNER_TEMP/$side.scores"
+              echo "sample $i ($side): $score"
+            done
+          done
+
+          read -r score min max <<< "$(summarize "$RUNNER_TEMP/pr.scores")"
+          {
+            echo "samples=$BENCH_SAMPLES"
+            echo "score=$score"
+            echo "min=$min"
+            echo "max=$max"
+          } >> "$GITHUB_OUTPUT"
+          echo "Benchmark score: $score (range $min-$max over $BENCH_SAMPLES samples)"
+
+          if [ -s "${RUNNER_TEMP}/base.scores" ]; then
+            read -r baseline baseline_min baseline_max <<< "$(summarize "$RUNNER_TEMP/base.scores")"
+            {
+              echo "baseline=$baseline"
+              echo "baseline_min=$baseline_min"
+              echo "baseline_max=$baseline_max"
+            } >> "$GITHUB_OUTPUT"
+            echo "Baseline score: $baseline (range $baseline_min-$baseline_max over $BENCH_SAMPLES samples)"
           else
-            echo "baseline=" >> $GITHUB_OUTPUT
+            echo "baseline=" >> "$GITHUB_OUTPUT"
           fi
 
       - name: Save benchmark results
@@ -274,8 +441,15 @@ jobs:
           cat > benchmark-results.json <<EOF
           {
             "prNumber": "${{ github.event.pull_request.number }}",
+            "currentInstructions": "${{ steps.instructions.outputs.instructions }}",
+            "baselineInstructions": "${{ steps.instructions.outputs.baseline_instructions }}",
+            "samples": "${{ steps.benchmark.outputs.samples }}",
             "currentScore": "${{ steps.benchmark.outputs.score }}",
-            "baselineScore": "${{ steps.baseline.outputs.baseline }}"
+            "currentMin": "${{ steps.benchmark.outputs.min }}",
+            "currentMax": "${{ steps.benchmark.outputs.max }}",
+            "baselineScore": "${{ steps.benchmark.outputs.baseline }}",
+            "baselineMin": "${{ steps.benchmark.outputs.baseline_min }}",
+            "baselineMax": "${{ steps.benchmark.outputs.baseline_max }}"
           }
           EOF
 

.github/workflows/comment.yml

@@ -74,22 +74,83 @@ jobs:
 
             const benchmark = readResults('benchmark-results');
             if (benchmark) {
+              const samples = parseInt(benchmark.samples, 10) || 1;
               const currentScore = parseFloat(benchmark.currentScore);
               const baselineScore = parseFloat(benchmark.baselineScore);
 
+              // Range covered by the raw samples of one side.
+              function spread(min, max) {
+                const lo = parseFloat(min);
+                const hi = parseFloat(max);
+                return isNaN(lo) || isNaN(hi) ? null : { lo, hi };
+              }
+
+              function describe(score, range) {
+                if (!range || samples < 2) {
+                  return score.toFixed(3);
+                }
+                return `${score.toFixed(3)} ` +
+                  `(median of ${samples}, range ${range.lo.toFixed(3)}-${range.hi.toFixed(3)})`;
+              }
+
+              const currentRange = spread(benchmark.currentMin, benchmark.currentMax);
+              const baselineRange = spread(benchmark.baselineMin, benchmark.baselineMax);
+
               let comment = '## CoreMark Benchmark Results\n\n';
-              comment += `**Current Score:** ${currentScore.toFixed(3)}\n`;
+
+              // Instruction counts, when the CI run that produced this artifact
+              // was new enough to measure them.
+              const currentInsns = parseInt(benchmark.currentInstructions, 10);
+              const baselineInsns = parseInt(benchmark.baselineInstructions, 10);
+
+              if (currentInsns) {
+                comment += `**Instructions:** ${currentInsns.toLocaleString('en-US')}\n`;
+
+                if (baselineInsns) {
+                  const diff = currentInsns - baselineInsns;
+                  const percentChange = (diff / baselineInsns) * 100;
+
+                  comment += `**Baseline Instructions (main):** ` +
+                    `${baselineInsns.toLocaleString('en-US')}\n`;
+                  comment += `**Difference:** ${diff >= 0 ? '+' : ''}` +
+                    `${diff.toLocaleString('en-US')} (${percentChange.toFixed(2)}%)\n\n`;
+
+                  // The count is exact, so this is a reading aid rather than a
+                  // threshold: it puts a large difference in front of a reviewer
+                  // instead of leaving it to be picked out of the figures.
+                  if (percentChange >= 1.0) {
+                    comment += '**Note:** this PR executes more than 1% more instructions ' +
+                      'than the baseline.\n\n';
+                  }
+                } else {
+                  comment += '\n_No baseline available for comparison_\n\n';
+                }
+
+                comment += '_Counted under callgrind with the same benchmark harness and ' +
+                  'a workload pinned by `COREMARK_FIXED_CLOCK`. This is a repeatable ' +
+                  'instruction-count proxy, not a timing measurement; it excludes cache ' +
+                  'and branch behaviour._\n\n';
+              }
+
+              comment += `**Current Score:** ${describe(currentScore, currentRange)}\n`;
 
               if (baselineScore && !isNaN(baselineScore)) {
                 const diff = currentScore - baselineScore;
-                const percentChange = ((diff / baselineScore) * 100).toFixed(2);
+                const percentChange = (diff / baselineScore) * 100;
 
-                comment += `**Baseline Score (main):** ${baselineScore.toFixed(3)}\n`;
-                comment += `**Difference:** ${diff >= 0 ? '+' : ''}${diff.toFixed(3)} (${percentChange}%)\n\n`;
+                comment += `**Baseline Score (main):** ${describe(baselineScore, baselineRange)}\n`;
+                comment += `**Difference:** ${diff >= 0 ? '+' : ''}${diff.toFixed(3)} ` +
+                  `(${percentChange.toFixed(2)}%)\n\n`;
               } else {
                 comment += '\n_No baseline available for comparison_\n';
               }
 
+              comment += `\n_The timed-score ranges are descriptive only. With ${samples} ` +
+                'samples they are not confidence bounds or regression thresholds._\n';
+              comment += '\n_Scores are only comparable within a single run, which measures ' +
+                'both sides on one runner. GitHub-hosted runners differ by more than 2x ' +
+                'between CPU models._\n';
+
               await upsertComment(
                 parseInt(benchmark.prNumber, 10),
                 'CoreMark Benchmark Results',