fix(dashboard): render the EnumMap and EnumSet cards, and gate blank cards in CI - #327
Merged
Merged
Conversation
…cards in CI
The dashboard required an `(ItemCount: N)` suffix on every BenchmarkDotNet result
name. `EnumMapBenchmark` and `EnumSetBenchmark` deliberately declare no `[Params]`
sweep — their workload is the bounded enum universe — so their measurements were
published to gh-pages and then dropped by `parseName`, and both cards had rendered
empty since they shipped.
The `ItemCount` group is now optional in both `index.html` and `detail.html`. An
unparameterized class parses with `itemCount === NO_SWEEP` and charts as a single
bucket: the grid card omits the "@ N items" detail, the detail page drops its
item-count toggle, and such benchmarks are excluded from the headline speedup
stats rather than bucketed at zero items. The suffix is still matched strictly as
`ItemCount`, so a params property named anything else is rejected rather than
charted under an "items" label it does not mean.
That failure mode was silent, and it had bitten twice: `DisjointSet` also blanked
for five runs when its params property was briefly named `ElementCount`. So
`scripts/check_dashboard_coverage.js` now fails CI on it. It lifts the
`COLLECTIONS` tables and both name parsers straight out of the dashboard HTML
rather than reimplementing them, so it validates the parser that actually ships.
Structural checks (the two `COLLECTIONS` arrays agree; every charted collection
has a `{Key}Benchmark` in `CoreBenchmarks`) run on every PR in `ci.yml`, since
`benchmarks.yml` only fires on `src/**` and would never see a dashboard-only
change. The full check runs against the merged report in the aggregate job, last,
so a wiring mistake reddens the job without costing us the measurement.
Closes #301
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the benchmark dashboard’s collection-name parser so unparameterized benchmarks (notably EnumMap/EnumSet) render correctly, and adds CI guards to prevent future “blank card” regressions by validating the dashboard wiring and report coverage.
Changes:
- Make the
(ItemCount: N)suffix optional in both dashboard pages and treat missing sweeps as a dedicated “NO_SWEEP” bucket. - Add
scripts/check_dashboard_coverage.jsand wire it into CI (ci.yml) and the benchmark aggregate workflow (benchmarks.yml). - Document the dashboard naming contract and guard script usage; record the work in roadmap/changelog.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/dev/bench/index.html | Makes ItemCount optional, introduces NO_SWEEP, and adjusts indexing/headline logic to support unparameterized benchmarks. |
| web/dev/bench/detail.html | Mirrors parser change, removes item-count toggle UI for single-bucket benchmarks, and makes labels/notice text null-safe. |
| scripts/check_dashboard_coverage.js | New dashboard coverage guard script (structural + report-backed checks) extracted from the shipped dashboard logic. |
| .github/workflows/ci.yml | Adds a fast structural dashboard-wiring check on every PR. |
| .github/workflows/benchmarks.yml | Adds a final, report-backed dashboard coverage check gated on complete shard reporting. |
| CONTRIBUTING.md | Documents dashboard naming rules and how/when to run the guard script; updates CI description. |
| CHANGELOG.md | Adds [Unreleased] fixed entries for the dashboard render bug and the new CI guard. |
| ROADMAP.md | Marks the dashboard guard work as done under pipeline integrity. |
The report-backed check accepted a BCL+Celerity pair at any of a collection's item counts, but a grid card's chart is always `seriesFor(..., primaryN)` — the ratio text falls back to the smaller count, the sparkline does not. So a collection with data only at 1k rendered "awaiting data" while the guard passed. Check the primary count alone, which is what decides the card. Also condense the two new CHANGELOG entries to the documented "a few sentences" budget, since release.yml lifts the whole section into the GitHub Release body. Both raised in review on #327. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
detail.html built its "no measurements recorded" notice by concatenating `params.op` — read straight from the query string — into innerHTML, so `?op=<img src=x onerror=...>` executed on the published site. The file already carries an escapeHtml helper for commit messages; route `op` through it too. `params.n` needs no change: it is parseInt'd on the way in, so it is a number or NaN by the time it is interpolated. Raised in review on #327. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closed
4 tasks
Benchmarks5 regressions Highlights
Collections (472)
Hashers (111)
Same-runner A/B (sharded 8-way): main ( |
8 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the two benchmark dashboard cards that have rendered empty since they shipped, and turns that class of silent failure into a red CI check.
The bug
The dashboard parses BenchmarkDotNet result names with a regex that hard-required an
(ItemCount: N)suffix:EnumMapBenchmarkandEnumSetBenchmarkdeliberately declare no[Params]— their workload is the bounded enum universe, and a synthetic item-count sweep would chart a dimension that does not exist. So BenchmarkDotNet emitsEnumMapBenchmark.EnumMap_Lookupwith no suffix,parseNamereturnednull, and the measurements were published togh-pagescorrectly and then discarded at render time.The fix
The parameter group is now optional in both
web/dev/bench/index.htmlandweb/dev/bench/detail.html. An unparameterized class parses withitemCount === NO_SWEEPand charts as a single bucket that can never collide with a real item count:@ N itemsdetail line;no item-count sweep, and the grid links to it without annquery param;ItemCount: 0and compared against the 100k sweep.The suffix is still matched strictly as
ItemCount. A[Params]property named anything else is rejected rather than charted under an "items" label it does not mean — the guard below is what surfaces it.The guard
The audit in #301 said
EnumMap/EnumSetwere the only two offenders. They were the only two current ones: the published history showsDisjointSetBenchmarkemitting(ElementCount: …)for five runs, so that card was blank then too, and nothing anywhere went red.scripts/check_dashboard_coverage.jscloses that.It lifts the
COLLECTIONStables and both name parsers out of the dashboard HTML and evaluates them, rather than reimplementing the regexes — so it validates the parser that actually ships and cannot drift from it. Extraction failing is itself a hard error with a pointer to the script.index.htmlanddetail.htmlagree on collection keys and theiritemsci.yml, every PR{Key}BenchmarkinProgram.cs'sCoreBenchmarksci.yml, every PRbenchmarks.ymlaggregate(collection, op)card resolves to both a BCL and a Celerity measurementbenchmarks.ymlaggregateTwo placements because
benchmarks.ymlonly fires onsrc/**— a dashboard-only change (like this PR) would never reach the report-backed run. The full check is deliberately the last step of the aggregate job, after the PR comment and the gh-pages publish, so a wiring mistake reddens the job without costing us the measurement; it is skipped unless every shard reported, since a partial merge is legitimately missing whole classes.Parity
web/dev/bench/index.html— optionalItemCountgroup,idxKeyhelper,NO_SWEEPmarker onEnumMap/EnumSet, headline exclusion, card detail line and click-through query string.web/dev/bench/detail.html— same parser change,NO_SWEEPentries, no toggle for a single bucket (.toggle:emptyhidden), null-safe labels and notice.web/index.html— no change needed; both types already have ship cards.scripts/check_dashboard_coverage.js— new..github/workflows/ci.yml/benchmarks.yml— wiring, plus ahead_shardsoutput on the merge step.CONTRIBUTING.md— new The dashboard subsection documenting the naming contract, the three lists a new collection must be added to, and how to run the script. Also corrects the neighbouring CI paragraph, which still described the pre-sharding single-job setup (wrong workflow file, wrong duration, a 200% fail-the-job threshold that no longer exists).CHANGELOG.md— two### Fixedentries under[Unreleased](the render fix, the guard).ROADMAP.md— recorded under Build- and release-pipeline integrity on milestone 2.4.0, statusdone..csfiles this PR touches: none.Test plan
dotnet buildclean (0 errors; pre-existing analyzer warnings only). No C# changed, so the xUnit suite is untouched — CI runs it across theubuntu/windows/macos×net8.0/net9.0/net10.0matrix regardless.gh-pagesdata.js(583 benchmarks, latest run):EnumMapcharts Add 5.71× / Lookup 1.92× / Remove 2.39× / Enumerate 1.13×,EnumSetcharts Add 4.71× / Contains 3.11× / Remove 1.18× / Union 15.23×. Zero.chart-cell.no-dataon the whole page, no console errors.IntDictionary(100k),SmallSet(8/64),BitSet(1024/1M),DisjointSet; headline stats still resolve at 100k items (SparseSet · Contains,HyperLogLog · Add,SwissSet · Remove), i.e. the 15.23×EnumSetfigure correctly did not leak into them.?c=EnumSet&op=Unionrenders headline + chart + 10 measurement rows with no toggle;?c=SmallSet&op=Contains&n=8unchanged with both toggle buttons;?c=EnumSet&op=Union&n=100000still errors with Unknown item count; click-through from anEnumSetgrid card lands ondetail.html?c=EnumSet&op=Addand renders.ElementCountrun with "12 benchmark name(s) match no dashboard parser … The usual cause is a [Params] property not named ItemCount"; and mutation-tested — droppingtypeof(EnumSetBenchmark)fromCoreBenchmarksand deletingSparseSetfromdetail.htmlboth produce named failures.mainpush confirms the cards render live.Closes #301
🤖 Generated with Claude Code