From f0f94e000035843c0332e11a51c284ada400ae8c Mon Sep 17 00:00:00 2001 From: suthat Date: Sat, 25 Jul 2026 12:34:21 +0700 Subject: [PATCH 1/6] Reduce CoreMark benchmark noise in CI The benchmark job measured the PR and main once each and reported the difference to three decimal places, which reads as a precise result but is not one. Across the 41 PRs that carry a bot comment, the reported delta ranges from -13.23% to +13.39%, and PRs that only touch docs, the README, the logo or CI config still move it by as much as 3.80%. Two things make the measurement worse than it needs to be. A single CoreMark run scores itself from wall-clock time over roughly fifteen seconds on a shared runner, so one sample carries several percent of noise on its own. On top of that main was always measured second: the mean delta over those 41 PRs is -1.13% and 27 of them are negative, which a sign test puts at p = 0.03 against an even split. Build both bench binaries up front, stage each one next to the wasm it loads, then run them alternately and report the median of five samples per side. Swapping which side goes first on every other sample keeps the ordering from favoring either one. The comment now carries the observed sample range and marks a delta that falls inside it as noise. This does not make scores comparable between runs and cannot: baseline scores for main span 230 to 498 depending on which CPU model the runner lands on. Co-authored-by: Cursor --- .github/workflows/ci.yml | 129 ++++++++++++++++++++++++++-------- .github/workflows/comment.yml | 49 +++++++++++-- 2 files changed, 143 insertions(+), 35 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5356bae..dbcc344 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -205,6 +205,10 @@ jobs: runs-on: ubuntu-latest permissions: contents: read + env: + # How many times each side is measured. The reported figure is the + # median, so a single unlucky run no longer decides the result. + BENCH_SAMPLES: 5 steps: - name: Checkout code uses: actions/checkout@v4 @@ -229,43 +233,103 @@ jobs: save-if: ${{ github.ref == 'refs/heads/main' }} cache-bin: false + - name: Build benchmark binaries + run: | + 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) + mkdir -p "$dest/benches" + cp "$exe" "$dest/coremark" + cp crates/spacewasm_std/benches/coremark-minimal.wasm "$dest/benches/" + } + + stage "$RUNNER_TEMP/pr" + + # The baseline is best effort: a PR should still get a score of its + # own if main cannot be built here. + if [ "${{ github.event_name }}" == "pull_request" ]; then + head=$(git rev-parse HEAD) + if git fetch --no-tags origin main && git checkout --detach FETCH_HEAD; then + stage "$RUNNER_TEMP/base" || echo "::warning::could not build the baseline benchmark" + git checkout --detach "$head" + else + echo "::warning::could not check out main to build a baseline" + fi + fi + - name: Run CoreMark benchmark id: benchmark 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 - echo "score=$score" >> $GITHUB_OUTPUT - echo "Benchmark score: $score" + run_once() { + ( cd "$1" && ./coremark ) | awk '/CoreMark Score:/ { print $3 }' + } - - name: Download baseline benchmark - id: baseline - continue-on-error: true - run: | - # Try to get baseline from main branch - 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 - + 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] + }' + } - if [ -n "$baseline_score" ]; then - echo "baseline=$baseline_score" >> $GITHUB_OUTPUT - echo "Baseline score: $baseline_score" - else - echo "baseline=" >> $GITHUB_OUTPUT + 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 +338,13 @@ jobs: cat > benchmark-results.json <= 0 ? '+' : ''}${diff.toFixed(3)} (${percentChange}%)\n\n`; + const percentChange = (diff / baselineScore) * 100; + + comment += `**Baseline Score (main):** ${describe(baselineScore, baselineRange)}\n`; + comment += `**Difference:** ${diff >= 0 ? '+' : ''}${diff.toFixed(3)} ` + + `(${percentChange.toFixed(2)}%)\n\n`; + + // The widest sample range seen in this job is the smallest + // difference this runner was able to resolve, so treat anything + // below it as noise rather than as a result. + const widest = Math.max( + currentRange ? currentRange.width : 0, + baselineRange ? baselineRange.width : 0 + ); + const noise = (widest / baselineScore) * 100; + + if (noise > 0) { + comment += Math.abs(percentChange) <= noise + ? `_Within the ${noise.toFixed(2)}% sample-to-sample noise of this run._\n` + : `_Larger than the ${noise.toFixed(2)}% sample-to-sample noise of this run._\n`; + } } else { comment += '\n_No baseline available for comparison_\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', From ffa1c7c550324b459fada3a4e29829b6d1ec7c85 Mon Sep 17 00:00:00 2001 From: suthat Date: Wed, 29 Jul 2026 22:11:40 +0700 Subject: [PATCH 2/6] Compare instruction counts, not just wall-clock scores A CoreMark score mostly measures how fast the runner is. What a PR can change is how much work the interpreter does, and instructions retired measures that directly: it does not move with the CPU model the runner landed on or with whoever else is on the machine, so one run per side resolves a difference that no number of timed samples can. Hardware counters are not an option here. GitHub runners are Azure VMs whose hypervisor does not expose the PMU, so perf reports cycles, instructions and branches as unsupported, and reading a counter through ptrace is out for the same reason. Count under callgrind instead, which is pure user-space instrumentation and needs no counters at all. Counting first needs a workload that does not depend on the clock. CoreMark sizes itself by timing ten iterations, multiplying by ten until that takes at least a second, then settling on iterations * (1 + 10 / floor(seconds)). The divisor is an integer, so a calibration round of 1.9s and one of 2.1s differ by nearly 2x in the work that follows. Counting that without pinning it would be worse than timing it. COREMARK_FIXED_CLOCK feeds the module a fixed table of timestamps instead, which holds it at 110 iterations with an eleven second measured window: still a valid CoreMark run, and one that scores exactly 10.0 every time. The bench asserts that score, so a module that starts timing itself differently fails the job rather than producing two counts that describe different work. The score stays, at three samples per side rather than five. It is still worth reporting what the runner managed, but it is no longer the number a regression has to be read out of. This also increments CLOCK_CALL_COUNT, which nothing ever incremented before, since the fixed clock indexes its table by it. Co-authored-by: Cursor --- .github/workflows/ci.yml | 72 +++++++++++++++++++++++- .github/workflows/comment.yml | 32 +++++++++++ crates/spacewasm_std/benches/coremark.rs | 67 ++++++++++++++++++++-- 3 files changed, 163 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dbcc344..cc6cc06 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -206,9 +206,10 @@ jobs: permissions: contents: read env: - # How many times each side is measured. The reported figure is the - # median, so a single unlucky run no longer decides the result. - BENCH_SAMPLES: 5 + # 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 @@ -226,6 +227,15 @@ 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 @@ -267,6 +277,60 @@ jobs: fi fi + - name: Count instructions + id: instructions + run: | + set -euo pipefail + + # Instructions retired is the part of a benchmark a PR can actually + # change. It does not move with the runner's CPU model, its + # neighbours, or the hypervisor, so one run of each side resolves a + # difference the timed runs below cannot see at all. + # + # 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. + count_once() { + local side="$1" + local out="$RUNNER_TEMP/callgrind.$side" + local 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" >&2 + return 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. + local ir + ir=$(awk '/^(summary|totals):/ { print $2; exit }' "$out") + if ! [[ "$ir" =~ ^[0-9]+$ ]]; then + cat "$log" >&2 + echo "Could not read an instruction count for $side out of $out" >&2 + return 1 + fi + echo "$ir" + } + + instructions=$(count_once pr) + echo "instructions=$instructions" >> "$GITHUB_OUTPUT" + echo "Instructions: $instructions" + + if [ -x "$RUNNER_TEMP/base/coremark" ]; then + baseline_instructions=$(count_once base) + echo "baseline_instructions=$baseline_instructions" >> "$GITHUB_OUTPUT" + echo "Baseline instructions: $baseline_instructions" + else + echo "baseline_instructions=" >> "$GITHUB_OUTPUT" + fi + - name: Run CoreMark benchmark id: benchmark run: | @@ -338,6 +402,8 @@ jobs: cat > benchmark-results.json <= 0 ? '+' : ''}` + + `${diff.toLocaleString('en-US')} (${percentChange.toFixed(2)}%)\n\n`; + + if (percentChange >= 1.0) { + comment += '**Warning:** this PR executes more than 1% more instructions\n\n'; + } + } else { + comment += '\n_No baseline available for comparison_\n\n'; + } + + comment += '_Counted under callgrind on a workload pinned by ' + + '`COREMARK_FIXED_CLOCK`, so both sides do the same work and the count is ' + + 'exact: a difference here is work the PR added or removed, not runner ' + + 'noise. It ignores cache and branch behaviour, so it stands in for time ' + + 'rather than measuring it._\n\n'; + } + comment += `**Current Score:** ${describe(currentScore, currentRange)}\n`; if (baselineScore && !isNaN(baselineScore)) { diff --git a/crates/spacewasm_std/benches/coremark.rs b/crates/spacewasm_std/benches/coremark.rs index 0cac724..2b8e270 100644 --- a/crates/spacewasm_std/benches/coremark.rs +++ b/crates/spacewasm_std/benches/coremark.rs @@ -16,10 +16,43 @@ const MAX_CODE_PAGES: u32 = 32; const MAX_CONTROL_FRAMES: usize = 64; const MAX_STACK_DEPTH: usize = 256; +/// Timestamps handed to the wasm module, in order, when `COREMARK_FIXED_CLOCK=1`. +/// +/// CoreMark sizes its own workload from the clock: it times a run of ten +/// iterations, keeps multiplying by ten until that takes at least a second, +/// then settles on `iterations * (1 + 10 / floor(seconds))`. The divisor is an +/// integer, so a run that takes 1.9s and one that takes 2.1s end up doing +/// nearly twice as much work as each other. That is fine for a score, which +/// divides the work back out, but it makes the amount of code executed a +/// property of the machine rather than of the build, and so not worth counting. +/// +/// These four values are what the module reads instead: one timed calibration +/// round reporting exactly one second, which pins the workload at 110 +/// iterations, and a measured window of eleven seconds, which clears the ten +/// second minimum CoreMark requires for a valid result. The score is then a +/// constant 110 / 11, and [`FIXED_CLOCK_SCORE`] asserts it. +const FIXED_CLOCK_MS: [i64; 4] = [0, 1_000, 1_000, 12_000]; + +/// How far the fixed clock advances per call once [`FIXED_CLOCK_MS`] runs out, +/// so an unexpected extra timing round changes the score rather than seeing +/// time stand still. +const FIXED_CLOCK_STEP_MS: i64 = 12_000; + +/// The only score [`FIXED_CLOCK_MS`] can produce, if the module still times +/// itself the way it does today. +const FIXED_CLOCK_SCORE: f32 = 10.0; + fn main() { println!("\n=== CoreMark Benchmark ==="); println!("Reference: https://github.com/wasm3/wasm-coremark\n"); + // Fixed-clock runs are for counting instructions, not for timing: the + // workload is a fraction of a normal run and the score is a self-check. + let fixed_clock = std::env::var("COREMARK_FIXED_CLOCK").as_deref() == Ok("1"); + if fixed_clock { + println!("Fixed clock: workload pinned for instruction counting.\n"); + } + // According to the reference implementation, clock_ms should return current time in milliseconds // See: https://github.com/wasm3/wasm-coremark/blob/main/coremark-minimal.html // JavaScript: env: { clock_ms: () => BigInt(Date.now()) } @@ -35,11 +68,22 @@ fn main() { "clock_ms", "".into(), "I".into(), - |_, _| { - let ms = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_millis() as i64; + move |_, _| { + let call = CLOCK_CALL_COUNT.fetch_add(1, Ordering::Relaxed); + + let ms = if fixed_clock { + let past_end = (call + 1).saturating_sub(FIXED_CLOCK_MS.len()) as i64; + FIXED_CLOCK_MS + .get(call) + .copied() + .unwrap_or(FIXED_CLOCK_MS[FIXED_CLOCK_MS.len() - 1]) + + FIXED_CLOCK_STEP_MS * past_end + } else { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_millis() as i64 + }; ControlFlow::Continue(Some(Value::I64(ms))) }, @@ -148,6 +192,19 @@ fn main() { println!("Return value: {:.3}", coremark_score); println!(); + // CoreMark only returns a score at all once its own CRC checks + // pass, and under the fixed clock there is exactly one score it can + // return. Anything else means the workload is no longer the one + // FIXED_CLOCK_MS pins, so a count taken from it is not comparable + // to a count taken from another build. + if fixed_clock && (coremark_score - FIXED_CLOCK_SCORE).abs() > 0.001 { + eprintln!( + "Error: fixed-clock score is {coremark_score:.3}, expected {FIXED_CLOCK_SCORE:.3}" + ); + eprintln!("The module no longer times itself the way FIXED_CLOCK_MS assumes."); + std::process::exit(1); + } + if coremark_score > 1.0 { println!("=== CoreMark Results ==="); println!("CoreMark Score: {:.3}", coremark_score); From 07a2d16f471a36610e0b208b9c0daf4b3f88d3f5 Mon Sep 17 00:00:00 2001 From: suthat Date: Wed, 29 Jul 2026 22:15:04 +0700 Subject: [PATCH 3/6] Skip instruction counts when a side cannot pin its workload A bench binary built before COREMARK_FIXED_CLOCK existed ignores it and sizes itself from the clock, so counting it against one that pins its workload compares two different amounts of work. That is the situation for this PR's own baseline, and for any branch that predates it, so the step probes each side first: a pinned run scores exactly 10.000 and costs a fraction of a second, which is a cheaper way to find out than the callgrind run it would otherwise spoil. Skip the counts with a warning in that case rather than fail, leaving the timed comparison to stand on its own as it did before. Co-authored-by: Cursor --- .github/workflows/ci.yml | 60 +++++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cc6cc06..f1cc677 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -291,10 +291,29 @@ jobs: # 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. - count_once() { - local side="$1" - local out="$RUNNER_TEMP/callgrind.$side" - local log="$RUNNER_TEMP/$side.count.log" + sides=(pr) + if [ -x "$RUNNER_TEMP/base/coremark" ]; then + sides+=(base) + fi + + # A bench binary built before that variable existed ignores it and + # sizes itself from the clock, so it cannot be counted against one + # that pins its workload. A pinned run scores exactly 10.000 and takes + # a fraction of a second, which is a cheaper way to find that out than + # the callgrind run it would spoil. + 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" + echo "::warning::the $side benchmark 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" @@ -302,34 +321,29 @@ jobs: --cache-sim=no --branch-sim=no \ --callgrind-out-file="$out" ./coremark ) > "$log" 2>&1; then - cat "$log" >&2 - return 1 + 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. - local ir ir=$(awk '/^(summary|totals):/ { print $2; exit }' "$out") if ! [[ "$ir" =~ ^[0-9]+$ ]]; then - cat "$log" >&2 - echo "Could not read an instruction count for $side out of $out" >&2 - return 1 + cat "$log" + echo "No instruction count in $out" + exit 1 fi - echo "$ir" - } - - instructions=$(count_once pr) - echo "instructions=$instructions" >> "$GITHUB_OUTPUT" - echo "Instructions: $instructions" - if [ -x "$RUNNER_TEMP/base/coremark" ]; then - baseline_instructions=$(count_once base) - echo "baseline_instructions=$baseline_instructions" >> "$GITHUB_OUTPUT" - echo "Baseline instructions: $baseline_instructions" - else - echo "baseline_instructions=" >> "$GITHUB_OUTPUT" - fi + if [ "$side" = pr ]; then + echo "instructions=$ir" >> "$GITHUB_OUTPUT" + echo "Instructions: $ir" + else + echo "baseline_instructions=$ir" >> "$GITHUB_OUTPUT" + echo "Baseline instructions: $ir" + fi + done - name: Run CoreMark benchmark id: benchmark From 64e1852005f9b42c8cc4adcec6f129cbb9122fe7 Mon Sep 17 00:00:00 2001 From: arthurianresolve <268402532+arthurianresolve@users.noreply.github.com> Date: Thu, 30 Jul 2026 22:46:22 +0200 Subject: [PATCH 4/6] Fix CoreMark benchmark comparison validity --- .github/workflows/ci.yml | 51 ++++++++++++++++++++++------------- .github/workflows/comment.yml | 32 +++++----------------- 2 files changed, 39 insertions(+), 44 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f1cc677..2c1e1b9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -258,23 +258,38 @@ jobs: | 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/" } + # 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/" + stage "$RUNNER_TEMP/pr" - # The baseline is best effort: a PR should still get a score of its - # own if main cannot be built here. if [ "${{ github.event_name }}" == "pull_request" ]; then head=$(git rev-parse HEAD) - if git fetch --no-tags origin main && git checkout --detach FETCH_HEAD; then - stage "$RUNNER_TEMP/base" || echo "::warning::could not build the baseline benchmark" - git checkout --detach "$head" - else - echo "::warning::could not check out main to build a baseline" - fi + git fetch --no-tags origin main + git checkout --detach FETCH_HEAD + 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" + git restore --source=HEAD -- \ + crates/spacewasm_std/benches/coremark.rs \ + crates/spacewasm_std/benches/coremark-minimal.wasm + git checkout --detach "$head" fi - name: Count instructions @@ -282,10 +297,10 @@ jobs: run: | set -euo pipefail - # Instructions retired is the part of a benchmark a PR can actually - # change. It does not move with the runner's CPU model, its - # neighbours, or the hypervisor, so one run of each side resolves a - # difference the timed runs below cannot see at all. + # 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, @@ -296,18 +311,16 @@ jobs: sides+=(base) fi - # A bench binary built before that variable existed ignores it and - # sizes itself from the clock, so it cannot be counted against one - # that pins its workload. A pinned run scores exactly 10.000 and takes - # a fraction of a second, which is a cheaper way to find that out than - # the callgrind run it would spoil. + # A pinned run scores exactly 10.000 and takes a fraction of a second. + # Fail before Callgrind if either binary does not execute that known + # workload; comparing counts from different workloads would be invalid. 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" - echo "::warning::the $side benchmark did not produce a pinned run; skipping instruction counts" - exit 0 + echo "The $side benchmark did not produce the pinned workload" + exit 1 fi done diff --git a/.github/workflows/comment.yml b/.github/workflows/comment.yml index 71a539c..5851041 100644 --- a/.github/workflows/comment.yml +++ b/.github/workflows/comment.yml @@ -82,7 +82,7 @@ jobs: function spread(min, max) { const lo = parseFloat(min); const hi = parseFloat(max); - return isNaN(lo) || isNaN(hi) ? null : { lo, hi, width: hi - lo }; + return isNaN(lo) || isNaN(hi) ? null : { lo, hi }; } function describe(score, range) { @@ -114,19 +114,14 @@ jobs: `${baselineInsns.toLocaleString('en-US')}\n`; comment += `**Difference:** ${diff >= 0 ? '+' : ''}` + `${diff.toLocaleString('en-US')} (${percentChange.toFixed(2)}%)\n\n`; - - if (percentChange >= 1.0) { - comment += '**Warning:** this PR executes more than 1% more instructions\n\n'; - } } else { comment += '\n_No baseline available for comparison_\n\n'; } - comment += '_Counted under callgrind on a workload pinned by ' + - '`COREMARK_FIXED_CLOCK`, so both sides do the same work and the count is ' + - 'exact: a difference here is work the PR added or removed, not runner ' + - 'noise. It ignores cache and branch behaviour, so it stands in for time ' + - 'rather than measuring it._\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`; @@ -138,25 +133,12 @@ jobs: comment += `**Baseline Score (main):** ${describe(baselineScore, baselineRange)}\n`; comment += `**Difference:** ${diff >= 0 ? '+' : ''}${diff.toFixed(3)} ` + `(${percentChange.toFixed(2)}%)\n\n`; - - // The widest sample range seen in this job is the smallest - // difference this runner was able to resolve, so treat anything - // below it as noise rather than as a result. - const widest = Math.max( - currentRange ? currentRange.width : 0, - baselineRange ? baselineRange.width : 0 - ); - const noise = (widest / baselineScore) * 100; - - if (noise > 0) { - comment += Math.abs(percentChange) <= noise - ? `_Within the ${noise.toFixed(2)}% sample-to-sample noise of this run._\n` - : `_Larger than the ${noise.toFixed(2)}% sample-to-sample noise of this run._\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'; From f92c8713c17a7fa077b896abe5eb7c0b01c6261b Mon Sep 17 00:00:00 2001 From: suthat Date: Sun, 2 Aug 2026 22:46:54 +0700 Subject: [PATCH 5/6] Keep the baseline best effort under the shared harness Building main against this branch's harness is what makes the two instruction counts comparable, but it can fail on its own: a harness written against an interface main does not have yet will not compile there, and the first PR to change the interpreter and the bench together would fail a job it had nothing to do with. A baseline that cannot be fetched, built or pinned warns and skips the counts, leaving the timed comparison in place. A pr side that cannot pin is this branch's own doing and still fails. Also keeps the flag on an instruction difference over 1%, reworded. The count is exact, so the figure is a reading aid rather than a threshold, but a large difference is worth putting in front of a reviewer rather than leaving it to be picked out of the numbers. Co-authored-by: arthurianresolve <268402532+arthurianresolve@users.noreply.github.com> Co-authored-by: Cursor --- .github/workflows/ci.yml | 42 ++++++++++++++++++++++------------- .github/workflows/comment.yml | 8 +++++++ 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2c1e1b9..23ec157 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -278,18 +278,24 @@ jobs: 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 head=$(git rev-parse HEAD) - git fetch --no-tags origin main - git checkout --detach FETCH_HEAD - 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" - git restore --source=HEAD -- \ - crates/spacewasm_std/benches/coremark.rs \ - crates/spacewasm_std/benches/coremark-minimal.wasm - git checkout --detach "$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 @@ -311,16 +317,22 @@ jobs: sides+=(base) fi - # A pinned run scores exactly 10.000 and takes a fraction of a second. - # Fail before Callgrind if either binary does not execute that known - # workload; comparing counts from different workloads would be invalid. + # 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" - echo "The $side benchmark did not produce the pinned workload" - exit 1 + 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 diff --git a/.github/workflows/comment.yml b/.github/workflows/comment.yml index 5851041..712440d 100644 --- a/.github/workflows/comment.yml +++ b/.github/workflows/comment.yml @@ -114,6 +114,14 @@ jobs: `${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'; } From 0b37e40eb4fab54e645ed0e017837dd7df6f9473 Mon Sep 17 00:00:00 2001 From: suthat Date: Tue, 4 Aug 2026 10:45:54 +0700 Subject: [PATCH 6/6] Count instructions on the ARM runner's performance counter The x86 runners are Azure VMs whose hypervisor hides the PMU, so perf reports instructions, cycles and branches as unsupported and callgrind was the only way to count. It costs 29s per side, which is more than the measurement is worth. The ARM runners expose armv8_pmuv3_0, where the same count is read straight off the counter in 1.1s for both sides, and agrees with callgrind to within 0.0007%. With an exact count of the work done, timing each side repeatedly no longer buys anything, so BENCH_SAMPLES is gone along with the median, the sample range and the alternating order. The comment reports both scores without a percentage between them, since a single timed run on a shared runner is worth a few percent either way. WABT also comes out of this job, which builds and runs the bench fine without it. Co-authored-by: Cursor --- .github/workflows/ci.yml | 182 ++++++++++++---------------------- .github/workflows/comment.yml | 47 +++------ 2 files changed, 77 insertions(+), 152 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 37de2ab..ff22346 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -202,14 +202,13 @@ jobs: benchmark: name: Benchmark - runs-on: ubuntu-latest + # The ARM runners expose the CPU's performance counters; the x86 ones do + # not, being Azure VMs whose hypervisor hides the PMU. That is what decides + # the runner here, since it is the difference between reading the + # instruction count off the hardware and simulating the program to get it. + runs-on: ubuntu-24.04-arm 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 @@ -219,23 +218,6 @@ jobs: - name: Install Rust toolchain uses: dtolnay/rust-toolchain@stable - - name: Install WABT (WebAssembly Binary Toolkit) - run: | - WABT_VERSION=1.0.41 - WABT_PLATFORM="linux-x64" - wget https://github.com/WebAssembly/wabt/releases/download/${WABT_VERSION}/wabt-${WABT_VERSION}-${WABT_PLATFORM}.tar.gz - 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 @@ -303,29 +285,47 @@ jobs: 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. + # How many instructions each side executes, read off the CPU's own + # counter. Unlike the score below it does not move with the runner's + # CPU model or with whoever else is on the machine, so a difference in + # it is work this PR added or removed. # - # 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. + # The image ships perf_event_paranoid=4, which refuses even to count a + # process this job started itself. + sudo sysctl -w kernel.perf_event_paranoid=1 + + # If the counter ever goes away, say so and leave the timed + # comparison to stand rather than failing a PR over it. + probe=$(perf stat -e instructions:u true 2>&1 || true) + if ! grep -q 'instructions:u' <<< "$probe" || grep -q 'not supported' <<< "$probe"; then + echo "$probe" + echo "::warning::no hardware instruction counter on this runner; skipping instruction counts" + exit 0 + fi + sides=(pr) if [ -x "$RUNNER_TEMP/base/coremark" ]; then sides+=(base) fi - # 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 + stats="$RUNNER_TEMP/$side.perf" + log="$RUNNER_TEMP/$side.count.log" + + # 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. A pinned run scores exactly 10.000, so the + # score doubles as a check that the count came from that workload. + # + # 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. + if ! ( + cd "$RUNNER_TEMP/$side" + COREMARK_FIXED_CLOCK=1 perf stat -x, -e instructions:u \ + -o "$stats" ./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" @@ -334,30 +334,14 @@ jobs: 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") + # User space only, so kernel work done on the runner's behalf stays + # out of the figure. Everything the process itself does is in it, + # parsing and compiling the module as well as interpreting it. + ir=$(awk -F, '$3 == "instructions:u" { print $1 }' "$stats") if ! [[ "$ir" =~ ^[0-9]+$ ]]; then - cat "$log" - echo "No instruction count in $out" + cat "$stats" + echo "No instruction count for $side" exit 1 fi @@ -375,62 +359,29 @@ jobs: run: | set -euo pipefail + # One run each. The score is a figure in units people care about, but + # it is worth a few percent either way on a shared runner, so it is + # the count above that a regression is read out of. Timing it + # repeatedly would not change that. run_once() { - ( cd "$1" && ./coremark ) | awk '/CoreMark Score:/ { print $3 }' + local score + score=$( cd "$1" && ./coremark ) || return 1 + score=$(awk '/CoreMark Score:/ { print $3 }' <<< "$score") + if [ -z "$score" ]; then + echo "Failed to extract CoreMark score for $1" >&2 + return 1 + fi + echo "$score" } - 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] - }' - } + score=$(run_once "$RUNNER_TEMP/pr") + echo "score=$score" >> "$GITHUB_OUTPUT" + echo "Benchmark score: $score" - 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)" + baseline=$(run_once "$RUNNER_TEMP/base") + echo "baseline=$baseline" >> "$GITHUB_OUTPUT" + echo "Baseline score: $baseline" else echo "baseline=" >> "$GITHUB_OUTPUT" fi @@ -443,13 +394,8 @@ jobs: "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 }}", - "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 }}" + "baselineScore": "${{ steps.benchmark.outputs.baseline }}" } EOF diff --git a/.github/workflows/comment.yml b/.github/workflows/comment.yml index 712440d..42018f5 100644 --- a/.github/workflows/comment.yml +++ b/.github/workflows/comment.yml @@ -74,28 +74,9 @@ 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'; // Instruction counts, when the CI run that produced this artifact @@ -126,30 +107,28 @@ jobs: 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 += '_Read off the CPU performance counter, with the same benchmark ' + + 'harness on both sides and a workload pinned by `COREMARK_FIXED_CLOCK`. ' + + 'This is a repeatable count of the work done, not a timing measurement; ' + + 'it says nothing about cache or branch behaviour._\n\n'; } - comment += `**Current Score:** ${describe(currentScore, currentRange)}\n`; + comment += `**Current Score:** ${currentScore.toFixed(3)}\n`; if (baselineScore && !isNaN(baselineScore)) { - const diff = currentScore - baselineScore; - const percentChange = (diff / baselineScore) * 100; - - comment += `**Baseline Score (main):** ${describe(baselineScore, baselineRange)}\n`; - comment += `**Difference:** ${diff >= 0 ? '+' : ''}${diff.toFixed(3)} ` + - `(${percentChange.toFixed(2)}%)\n\n`; + comment += `**Baseline Score (main):** ${baselineScore.toFixed(3)}\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 += currentInsns + ? '\n_One timed run each. On a shared runner the score is worth a few percent ' + + 'either way, which is why no difference is quoted for it: read the ' + + 'instruction count above instead._\n' + : '\n_One timed run each, and worth a few percent either way on a shared ' + + 'runner, so no difference is quoted for it._\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'; + 'both sides on one runner._\n'; await upsertComment( parseInt(benchmark.prNumber, 10),