Skip to content

bench-test262: fingerprint the contributing test set so test262.total is comparable - #24

Merged
nooga merged 2 commits into
nooga:mainfrom
mparrett:fix/bench-test262-set-fingerprint
Jul 25, 2026
Merged

bench-test262: fingerprint the contributing test set so test262.total is comparable#24
nooga merged 2 commits into
nooga:mainfrom
mparrett:fix/bench-test262-set-fingerprint

Conversation

@mparrett

Copy link
Copy Markdown
Contributor

Resolves finding F3 from the retrospective review of #10 (tracked in #23).

The problem

The Test262 macro emits test262.total as 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 StreamRecord now carries a set_hash: an order-independent, truncated SHA-256 over the exact set of test paths it summed.

  • Producer (cmd/bench-test262): computes the hash per emitted record.
  • Schema (pkg/perfdata): SetHash on StreamRecord and BenchmarkEntry, omitempty so ordinary micro benchmarks stay unaffected.
  • Aggregate (cmd/bench-ratchet): threads the hash onto the snapshot's BenchmarkEntry. Disagreeing hashes across a key's -count samples blank out — no coherent identity — which is the honest signal for a flaky set.

A consumer can then gate: a test262.total point whose set_hash differs from the reference isn't comparable.

Verification

  • Unit tests (cmd/bench-test262/main_test.go): equal-count/different-set → different hash; order-independent; failed/timed-out excluded from both sum and set.
  • End-to-end: a set_hash on a test262.total record survives aggregate into the snapshot entry alongside ratio_to_anchor, and correctly stays off per-sample rows.
  • go build ./..., go vet ./... clean.

Follow-on

#16 should gate its test262.total series on set_hash — this PR gives it the tool; the page-side drop/flag is #16's job.

🤖 Generated with Claude Code

…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 nooga left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mparrett added a commit to mparrett/paserati that referenced this pull request Jul 13, 2026
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>
@mparrett

Copy link
Copy Markdown
Contributor Author

Heads-up: composes with #22, one small merge conflict. Both this PR and #22 edit cmd/bench-ratchet/main.go's aggregateFromFile. Verified by composing all open PRs on a clean nooga/main base — this is the only conflict across the set, and it's a 3-line resolution in the Result{...} build: take #22's reducer form and keep this PR's SetHash:

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 set_hash.)

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>
@mparrett

Copy link
Copy Markdown
Contributor Author

Folded in your suggestion (fa8ccf1): a focused cmd/bench-ratchet test for the set_hash conflict-blanking branch — disagreeing -count repetitions blank the aggregated identity to "", agreeing ones preserve it. Only touches the new test file, so the #22 conflict region is unchanged.

@mparrett
mparrett requested a review from nooga July 16, 2026 19:52
mparrett added a commit to mparrett/paserati that referenced this pull request Jul 18, 2026
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 nooga left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Matt — solid, well-tested fix (order-independent fingerprint, clean pass/fail/timeout exclusion coverage). Merging.

@nooga
nooga merged commit 0eb6239 into nooga:main Jul 25, 2026
8 checks passed
nooga pushed a commit that referenced this pull request Jul 25, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants