Skip to content

Fix CoreMark benchmark comparison validity #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
arthurianresolve wants to merge 1 commit into from
Closed

Fix CoreMark benchmark comparison validity #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 32 additions & 19 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,34 +258,49 @@ 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
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.
# 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,
Expand All @@ -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

Expand Down
32 changes: 7 additions & 25 deletions .github/workflows/comment.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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`;
Expand All @@ -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';
Expand Down