Skip to content

Commit c17af17

Browse files
perryqhclaude
andcommitted
Guard measure.sh against reporting numbers that mean nothing
Review points on #53, plus ideas borrowed from rubyatscale/codeowners-rs#121, which builds the same kind of harness and systematizes the failure modes. The theme is that a measurement tool's worst failure is a plausible number, not an error. Four guards: - Refuse to time a binary that does not work. `hyperfine --ignore-failure` is needed because `pks check` exits 1 on violations, but it also treats a panic (101) or an internal error (2) as a valid run -- so a change that broke the tool outright would report a fast, clean-looking mean. Now probes once first, accepts only 0 or 1, and greps for `panicked at`. Verified against tests/fixtures/app_with_monkey_patches, which panics: refused, panic printed. - Warn loudly under 1000 files. The phases this exists to compare scale with codebase size; on a fixture they are all startup cost. My own smoke test printed "19.3 ms +/- 3.0 ms" for a 9-file fixture, which looks like a measurement and is not one. - Report the noise floor next to the mean, so a delta can be judged against it rather than assumed real. Also states that this is *within-batch* spread and understates between-session drift -- an unchanged binary measured 5.1s and 8.1s on the same machine hours apart, which is larger than most effects worth hunting. The guidance is to A/B two builds in one hyperfine run. - Record provenance: corpus file count, pack count, commit, and whether the corpus is dirty, plus the pks commit and branch. A mean without the corpus it came from is not comparable to anything, and mixing two was previously silent. Also adds the `command -v hyperfine` check to run_benchmarks.sh, which measure.sh already had. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 7696a5f commit c17af17

2 files changed

Lines changed: 77 additions & 0 deletions

File tree

dev/measure.sh

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,59 @@ fi
6464
mkdir -p "$PKS_ROOT/target"
6565
cd "$PKS_APP"
6666

67+
# Record what was measured, not just the number. A mean is meaningless without
68+
# the corpus it came from, and two labels measured against different apps are not
69+
# comparable -- printing this makes mixing them obvious rather than silent.
70+
APP_FILES=$("$PKS_BIN" list-included-files 2>/dev/null | wc -l | tr -d ' ')
71+
APP_PACKS=$(find . -name package.yml -not -path './tmp/*' 2>/dev/null | wc -l | tr -d ' ')
72+
APP_COMMIT=$(git -C "$PKS_APP" rev-parse --short HEAD 2>/dev/null || echo "not-a-git-repo")
73+
APP_DIRTY=$(git -C "$PKS_APP" status --porcelain 2>/dev/null | wc -l | tr -d ' ')
74+
75+
echo "==> corpus: $APP_FILES files, $APP_PACKS packs, at $APP_COMMIT"
76+
echo " pks: $(git -C "$PKS_ROOT" rev-parse --short HEAD) on $(git -C "$PKS_ROOT" rev-parse --abbrev-ref HEAD)"
77+
78+
# A small corpus produces numbers that look real and mean nothing: the phases this
79+
# tool exists to compare scale with codebase size, and on a fixture they are all
80+
# rounding error. Refusing to be quiet about it is the point.
81+
if [ "$APP_FILES" -lt 1000 ]; then
82+
echo
83+
echo " !! WARNING: only $APP_FILES files. This is a smoke test, not a measurement." >&2
84+
echo " !! Phase timings will be dominated by process startup. Do not compare" >&2
85+
echo " !! these numbers against a real application, or publish them." >&2
86+
fi
87+
88+
if [ "$APP_DIRTY" -ne 0 ]; then
89+
echo
90+
echo " !! WARNING: corpus has $APP_DIRTY uncommitted change(s)." >&2
91+
echo " !! Results are not reproducible from $APP_COMMIT alone." >&2
92+
fi
93+
94+
echo
95+
echo "==> [$LABEL] verifying the binary works before timing it"
96+
97+
# `hyperfine --ignore-failure` treats *any* exit code as a valid run, so a binary
98+
# that panics on every invocation would be timed happily and report a fast,
99+
# clean-looking mean. Since this script exists to validate performance changes,
100+
# that is the worst possible failure: it does not look like a failure.
101+
#
102+
# `pks check` exits 0 (clean) or 1 (violations found); anything else -- 2 for an
103+
# internal error, 101 for a panic -- means we would be timing a broken binary.
104+
probe_out=$("$PKS_BIN" check 2>&1) && probe_code=0 || probe_code=$?
105+
case "$probe_code" in
106+
0|1) ;;
107+
*)
108+
echo "error: pks check exited $probe_code, so there is nothing meaningful to time" >&2
109+
echo "$probe_out" | tail -20 >&2
110+
exit 1
111+
;;
112+
esac
113+
if grep -q "panicked at" <<<"$probe_out"; then
114+
echo "error: pks check panicked; refusing to time it" >&2
115+
grep -m3 "panicked at" <<<"$probe_out" >&2
116+
exit 1
117+
fi
118+
echo " exit $probe_code (0 = no violations, 1 = violations found) -- ok to time"
119+
67120
echo
68121
echo "==> [$LABEL] hyperfine: pks check (warm cache, ${WARMUP} warmup / ${RUNS} runs)"
69122

@@ -85,6 +138,25 @@ if ! grep -E "Time|Range" <<<"$hyperfine_out"; then
85138
exit 1
86139
fi
87140

141+
# State the noise floor next to the mean, so a later delta can be judged against
142+
# it. A change smaller than this spread has not been shown to do anything -- the
143+
# same change measured on a busy and an idle machine can differ by more than the
144+
# effect being hunted.
145+
if [ -f "$EXPORT_JSON" ] && command -v python3 >/dev/null 2>&1; then
146+
python3 - "$EXPORT_JSON" <<'PY'
147+
import json, sys
148+
r = json.load(open(sys.argv[1]))["results"][0]
149+
mean, stddev = r["mean"], r.get("stddev") or 0.0
150+
spread = max(r["times"]) - min(r["times"])
151+
print(f" noise floor: +/-{stddev*1000:.0f}ms stddev, {spread*1000:.0f}ms spread "
152+
f"({spread/mean*100:.1f}% of mean)")
153+
print(f" -> treat any delta under ~{spread*1000:.0f}ms as within noise")
154+
print( " -> this is WITHIN-batch spread and understates drift BETWEEN sessions;")
155+
print( " machine load moved one unchanged binary 5.1s -> 8.1s across a day,")
156+
print( " so A/B two builds in one hyperfine run, not in two separate runs")
157+
PY
158+
fi
159+
88160
echo
89161
echo "==> [$LABEL] phase breakdown (single --debug run)"
90162

dev/run_benchmarks.sh

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@ set -euo pipefail
1515
PKS_ROOT="${PKS_ROOT:-../pks}"
1616
PKS_BIN="${PKS_BIN:-$PKS_ROOT/target/release/pks}"
1717

18+
if ! command -v hyperfine >/dev/null 2>&1; then
19+
echo "error: hyperfine not installed (brew install hyperfine)" >&2
20+
exit 1
21+
fi
22+
1823
if [ ! -x "$PKS_BIN" ]; then
1924
echo "error: no pks binary at $PKS_BIN" >&2
2025
echo " build it first: cargo build --release --manifest-path $PKS_ROOT/Cargo.toml" >&2

0 commit comments

Comments
 (0)