ci: load compact full-history benchmark trends - #20
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: faster pages loading
Solid performance refactor. Pre-computing data/trends.json collapses N per-run results.json fetches into a single request for the trend chart, and the versioned-URL scheme (runAssetUrl) lets the browser cache immutable per-run assets instead of forcing no-store on every load. Atomic temp-file + os.replace writes, boolean-safe numeric filtering in build_trends, and the new build_trends test are all good. A few points worth addressing, plus some non-blocking notes below.
Non-blocking notes
- Degraded-mode fallback is the slow path. If
trends.jsonfails to load,loadTrendsreturnsnullandrenderTrendfalls back toPromise.all(metas.map(loadRun))(app.js:535-537). With the history-range default now changed to "All commits" (index.html:70), that fallback fetches every run document on first paint — the exact behavior this PR is trying to avoid. Consider capping the range in the trends-unavailable case. - Trends payload is wall-time only.
build_trendsemits{"wallNs": ...}inbuildTimesand dropscpuNs/userNs/sysNs(enrich_pull_requests.py:148-155). The chart only plots size + wall so this is currently harmless, but any future reader of the"cpu"measure from a trend document (measureValue, app.js:263) would getNaN. Worth a comment noting trends is wall-only, or emitcpuNsfor symmetry. - Benchmark-name sort collation differs between producer and consumer. Python sorts with
str.casefold(enrich_pull_requests.py:163) while JSsortedBenchmarkNamesuseslocaleCompare(..., {sensitivity:"base"})(app.js:171-173). They agree for ASCII but can diverge for accented/locale-sensitive names, and since app.js re-sortsstate.index.benchmarkNames, the row order can differ from the persisted index. A shared collation (or a comment documenting the ASCII-only assumption) would remove the ambiguity. chartBandrebuildsbenchmarkMapper document per series (app.js:478) — O(configs × runs) Map allocations per re-render.renderMatrixalready hoistsdocuments.map(benchmarkMap)once; doing the same here would help now that "All commits" routes more documents through this path.- Docs are now stale.
ci/llgo-size/README.md("Publishing and history") anddocs/llgo-binary-size-handoff.md(CI Structure, step 5) describe the published data schema/pipeline but don't mention the newdata/trends.jsonartifact or thebenchmarkNamesfield now injected intoindex.json.
| function runAssetUrl(meta, path) { | ||
| const version = [meta.id || meta.key, meta.attempt || 1, meta.createdAt || ""] | ||
| .map(String).join("-"); | ||
| return "data/" + path + "?v=" + encodeURIComponent(version); |
There was a problem hiding this comment.
encodeURIComponent is applied only to version, not to path, which is concatenated raw. In loadLegacyBuildTimes (app.js:218-219) path includes nativePath = document.native.buildTimes, a value from fetched run JSON. An unexpected nativePath such as foo.tsv?x=1, foo.tsv#x, or one containing ../ could alter the query/fragment or perform same-origin relative traversal of the fetched URL. It's same-origin only (no DOM/XSS), but encoding each segment — e.g. path.split("/").map(encodeURIComponent).join("/") — keeps untrusted content from changing the URL structure. (Minor: the version key falls back to -1- if id/key/createdAt are all missing; meta.key is always present today, so low risk.)
| timing_path.relative_to(data_dir.resolve()) | ||
| with timing_path.open(newline="", encoding="utf-8") as timing_file: | ||
| return { | ||
| (row["benchmark"], row["configuration"]): int(row["real-ns"]) |
There was a problem hiding this comment.
int(row["real-ns"]) runs eagerly inside the dict comprehension, so a single missing key or non-integer value raises and the surrounding except returns {} — discarding all legacy timings for that run. The JS counterpart parseBuildTimes (app.js:188-199) skips individual bad rows and keeps the good ones. Consider per-row skipping here so one malformed line doesn't drop the whole file's timings.
1 participant
Summary
data/trends.jsonwhenever benchmark results or the site are publishedValidation
PYTHONDONTWRITEBYTECODE=1 python3 -B -m unittest ci/llgo-size/test_enrich_pull_requests.pygit diff --checkpagesdata preview: 84 commits, 9 benchmark names including IXGo, 928 full-history chart points, working A/B comparisonindex.json, eight fullresults.jsondocuments, and one compacttrends.json; history-range switches made no additional requestsThe real 84-run trend payload is 300,452 bytes uncompressed and 44,863 bytes with gzip.