bench-test262: fingerprint the contributing test set so test262.total is comparable - #24
Conversation
…comparable The macro emits test262.total as sum + count of the passing, non-timed-out tests, but that loses test identity: two captures with equal counts can cover different sets (a test flipping pass->timeout while another flips back keeps the count, changes the sum). The A/B comparator sidesteps this by diffing the per-test merged files, but the aggregate total advertised for the timeline cannot — so a composition change reads as an engine change. Each StreamRecord now carries a SetHash: an order-independent, truncated SHA-256 over the exact set of test paths it summed. It threads through bench-ratchet's aggregate onto the snapshot's BenchmarkEntry (disagreeing hashes across a key's -count samples blank out — no coherent identity), so a consumer can gate: a total whose SetHash differs from the reference isn't comparable. SetHash is omitempty and stays absent for ordinary micro benchmarks, where the op is fixed. Verified end-to-end: producer distinguishes equal-count/different-set runs and is order-independent (unit tests); the hash survives aggregate into the snapshot entry alongside ratio_to_anchor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nooga
left a comment
There was a problem hiding this comment.
Overview: Adds SetHash fingerprinting to cmd/bench-test262 and threads it through cmd/bench-ratchet's aggregation into pkg/perfdata's schema, so downstream consumers can distinguish "same count, different test set" from a real change.
Analysis: Hash construction (sorted paths, newline-delimited, truncated SHA-256) is correct and collision-safe for this use case. -filter/-subpath are handled correctly since the hash operates on the already-filtered result set. The aggregator's conflict-blanking logic (differing hashes across -count samples → blank, an honest "no coherent identity" signal) is sound, though untested directly.
Suggestions: Consider adding a focused unit test in cmd/bench-ratchet for the hash-conflict-blanking branch, since it's the one new piece of control flow without direct coverage — not blocking.
Risks: None material; change is additive, omitempty-guarded, doesn't require a schema version bump, and doesn't touch the interpreter/checker/compiler/VM. go build ./... and go vet ./... are clean, and cmd/bench-test262/main_test.go's new tests all pass.
The summed metric conflated per-test speed with how many tests pass, so a conformance win (more passing tests add their durations) read as a perf regression on the timeline (nooga#26). Fold the mean (summed_ns / passing_count) instead, which removes the count-inflation, and carry the contributing set's set_hash (from bench-test262, nooga#24) onto the snapshot entry so a residual composition shift the mean can't cancel stays visible to the page. Guards the empty passing set (skip the fold; mean is undefined). set_hash is soft: absent until nooga#24 lands, in which case the entry simply omits it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Heads-up: composes with #22, one small merge conflict. Both this PR and #22 edit NSPerOp: reduce(a.nsMin, a.nsSum, a.count),
BytesPerOp: int64(reduce(a.bytesMin, a.bytesSum, a.count)),
AllocsPerOp: int64(reduce(a.allocMin, a.allocSum, a.count)),
SetHash: a.setHash,The accumulator auto-merges (each PR adds distinct fields); only this block collides. Either order is fine — whichever lands second applies the above. (This PR should also precede #16, which consumes |
Adds direct coverage for aggregateFromFile's set_hash handling — the one new piece of control flow in this PR without a test (per review). Two cases: disagreeing -count repetitions blank the aggregated identity to "", agreeing ones preserve it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Folded in your suggestion ( |
Master tracker for the "are we fast yet?" benchmarking stack: merged foundation (#4/#5/#10), in-flight PRs, and the five review findings (F1–F5) from the #10/#15 reviews. Mirrors GitHub issue nooga#23; all five findings now fixed across PR #15 (F1+F2), nooga#24 (F3), and nooga#25 (F4+F5). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nooga
left a comment
There was a problem hiding this comment.
Thanks Matt — solid, well-tested fix (order-independent fingerprint, clean pass/fail/timeout exclusion coverage). Merging.
The summed metric conflated per-test speed with how many tests pass, so a conformance win (more passing tests add their durations) read as a perf regression on the timeline (#26). Fold the mean (summed_ns / passing_count) instead, which removes the count-inflation, and carry the contributing set's set_hash (from bench-test262, #24) onto the snapshot entry so a residual composition shift the mean can't cancel stays visible to the page. Guards the empty passing set (skip the fold; mean is undefined). set_hash is soft: absent until #24 lands, in which case the entry simply omits it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolves finding F3 from the retrospective review of #10 (tracked in #23).
The problem
The Test262 macro emits
test262.totalas sum + count of the passing, non-timed-out tests. That loses test identity: two captures with equal counts can cover different sets — a test flipping pass→timeout while another flips back keeps the count but changes both the set and the sum. The A/B comparator sidesteps this by diffing the per-test merged files, but the aggregate total advertised for the timeline (#16) can't, so a composition change reads as an engine change.The fix
Each
StreamRecordnow carries aset_hash: an order-independent, truncated SHA-256 over the exact set of test paths it summed.cmd/bench-test262): computes the hash per emitted record.pkg/perfdata):SetHashonStreamRecordandBenchmarkEntry,omitemptyso ordinary micro benchmarks stay unaffected.cmd/bench-ratchet): threads the hash onto the snapshot'sBenchmarkEntry. Disagreeing hashes across a key's-countsamples blank out — no coherent identity — which is the honest signal for a flaky set.A consumer can then gate: a
test262.totalpoint whoseset_hashdiffers from the reference isn't comparable.Verification
cmd/bench-test262/main_test.go): equal-count/different-set → different hash; order-independent; failed/timed-out excluded from both sum and set.set_hashon atest262.totalrecord survives aggregate into the snapshot entry alongsideratio_to_anchor, and correctly stays off per-sample rows.go build ./...,go vet ./...clean.Follow-on
#16 should gate its
test262.totalseries onset_hash— this PR gives it the tool; the page-side drop/flag is #16's job.🤖 Generated with Claude Code