ci: retain historical benchmark rows on Pages - #17
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
The change is well-scoped and correct in its goal: deriving state.benchmarkNames from the union of all runs (benchmarkNamesFromDocuments) so historical benchmarks stay visible. loadRun memoizes into state.runs, so the upfront load also warms the cache reused by later render passes, and the README update accurately describes the new union semantics. Output encoding at every sink (the <select> options, matrix labels) correctly uses escapeHtml, so no injection concern.
Two findings worth a conscious decision are noted inline. Minor optional follow-ups:
state.activeBenchmarknow defaults to the alphabetically-first name across all runs (app.js:576) rather than the first benchmark of the latest run — a subtle change to the initially-selected trend benchmark, flowing from the ordering finding below.benchmarkNamesFromDocumentsaddsbenchmark.nameunconditionally (app.js:137); a malformed entry lackingnamewould addundefined. This matches existingbenchmarkMapbehavior and is low risk, butif (benchmark && benchmark.name)would harden it.
| documents.forEach(function (document) { | ||
| (document && document.benchmarks || []).forEach(function (benchmark) { names.add(benchmark.name); }); | ||
| }); | ||
| return Array.from(names).sort(function (a, b) { return a.localeCompare(b, undefined, { sensitivity: "base" }); }); |
There was a problem hiding this comment.
Ordering diverges from the server-generated TSV/summary.
This sorts names case-insensitively (localeCompare(..., { sensitivity: "base" })), but the server produces each run's benchmarks array via sort -u in report.sh:60 — a byte/locale sort that is case-sensitive under C/POSIX. For the mixed-case names this project uses (XGo, iXGo, Toml, Aws_restjson, ...), these orderings differ: e.g. iXGo sorts after all uppercase-initial names under sort -u but adjacent to XGo here. So the dashboard matrix rows (app.js:316) and the benchmark dropdown (app.js:578) will be ordered differently from the published total-bytes.tsv / summary.md, and differently from the previous code (which used the latest run's array order matching the TSV).
Consider matching sort -u (plain code-point comparison, or localeCompare without sensitivity: "base") if cross-referencing the dashboard against the raw TSV matters, or documenting the case-insensitive display order as intentional.
| if (!state.index.runs || !state.index.runs.length) throw new Error("No benchmark runs are available"); | ||
| const latest = await loadRun(state.index.runs[0]); | ||
| state.benchmarkNames = (latest.benchmarks || []).map(function (benchmark) { return benchmark.name; }); | ||
| const documents = await Promise.all(state.index.runs.map(loadRun)); |
There was a problem hiding this comment.
Eager fetch of every historical run at startup blocks first paint.
This replaces the previous single-run fetch with a fan-out over runs.length documents, each doing fetch(..., { cache: "no-store" }) (plus a possible second legacy build-times fetch per run in loadLegacyBuildTimes). Cost is now O(N runs) and grows unbounded as history accumulates, and no-store defeats the HTTP cache so the full history re-downloads on every reload. This await sits on the critical path before attachEvents()/refreshAll(), so the UI stays non-interactive until all documents download and parse.
Deriving benchmark names doesn't require loading every run. Options, best first: (1) publish the benchmark-name list in data/index.json for O(1) startup; (2) seed names from the latest run as before and let renderTrend/chartRuns lazily load the rest (they already loadRun per meta); (3) at minimum drop cache: "no-store" for the immutable per-run documents so reloads hit cache.
1 participant
Summary
IXGovisible—Validation
go test ./cmd/bentbash -n ci/llgo-size/report.shgit diff --checkIXGoappears among 9 benchmark groups, its latest four missing commits display—, older size/time values remain visible, both IXGo trend charts render 108 points, and the console has no warnings or errors