perf: optimize benchmark dashboard bundle and collapsed DOM - #115
Conversation
TheHiddenObserver
left a comment
There was a problem hiding this comment.
Final optimization review on f845f8a36f56f8e06685b465f89c6c8e23eb1e9c found no blocking issue.
Reviewed the bounded source diff rather than the generated bundle alone:
- the shared
echarts/coreregistration includes every component actually used by Timing/Speedup (Bar, Grid, Tooltip, Legend, DataZoom, MarkLine, Canvas renderer) and preserves the existing ECharts lifecycle/API; - lazy chart-data tables keep the disclosure element and row count present initially, materialize exactly once on first expansion, and are rebuilt from the current selected runs whenever normal dashboard state causes the main view to rerender;
- generation/data/statistical identities are unchanged;
- concurrent metadata requests preserve the previous failure semantics and generation-ID validation while eliminating the serial metadata start;
- deployed assets are deterministic and staleness-clean;
- no temporary measurement or patch workflow remains in the final nine-file diff.
Measurement evidence is material rather than marginal: JS gzip -46.8%, total gzip payload -37.4%, cold-ready median -42.7%, Full-matrix median -32.0%, Focused return -28.3%, Focused DOM -65.8%, and Full-matrix pre-expansion DOM -86.5%. The measurements also show no current justification for virtualization or data partitioning.
Permanent validation on the exact final head is green: Tests #1421, Maintenance compatibility #434, Benchmark Frontend CI #492, including Python 3.9/3.11 data gates, TypeScript/build, deterministic staleness, Chromium regression, and committed-deployment production QA in Chromium/Firefox/WebKit. The production test explicitly verifies lazy table materialization and exact table availability after disclosure expansion.
No correctness, provenance, accessibility, deployment, maintainability, or performance-regression blocker remains in this optimization scope. Do not merge automatically; merge remains a separate maintainer decision.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
TheHiddenObserver
left a comment
There was a problem hiding this comment.
Final clean-head optimization review.
Reviewed the exact nine-file diff on d10d49a51bd70b3f524e78ad74fac3d14885d1c2 after all temporary audit/measurement workflows were removed.
Correctness and architecture
frontend/src/echarts.tsuses the supportedecharts/coreregistration surface and registers every chart/component used by Timing/Speedup; existing chart lifecycle and option contracts remain intact.- Lazy chart-data fallback preserves immediate disclosure/row-count semantics, materializes the exact table once on first expansion, and is recreated from current selected runs on normal dashboard rerender. Permanent production E2E independently checks collapsed absence and expanded exact-table availability.
- Concurrent metadata fetch removes a serial request start without weakening benchmark-data-required semantics or generation-ID consistency validation.
- No benchmark schema, statistical value, provenance identity, filter identity, or canonical data changes are present.
Measurement audit
Deterministic improvements are material:
- JavaScript raw/gzip: -46.8%;
- total gzip deployment payload: -37.4%;
- Focused pre-expansion DOM: 6,934 -> 2,370 (-65.8%);
- Full-matrix pre-expansion DOM: 17,608 -> 2,370 (-86.5%).
The final latency claim uses a same-runner paired A/B design (one Chromium process, fresh contexts, ten alternating-order pairs) instead of cross-runner wall-clock comparisons:
- cold-ready paired improvement median: 17.7%, all 10 pairs faster;
- Full-matrix paired improvement median: 16.7%, all 10 pairs faster;
- Focused return: mixed per-pair, paired median +11.3%, not treated as guaranteed;
- CV cascade: mixed per-pair, paired median +3.1%, treated as effectively unchanged/no regression.
This evidence does not justify virtualization, benchmark-data partitioning, or an ECharts lifecycle rewrite at the current 1,796-run scale.
Final gates
Exact clean head:
- Tests #1424 — success;
- Maintenance compatibility #437 — success;
- Benchmark Frontend CI #495 — success (6/6: Python 3.9/3.11, frontend build/typecheck, staleness, Chromium E2E, Chromium/Firefox/WebKit committed-deployment production QA).
No unresolved review thread or temporary workflow remains. No blocking correctness, accessibility, provenance, deployment, maintainability, or performance-regression issue remains in the scoped optimization. Merge is intentionally left as a separate explicit maintainer decision.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
TheHiddenObserver
left a comment
There was a problem hiding this comment.
Review/fix loop convergence review on exact head 3d141c6688e7c5110da4fb28e0e9ec55ff3d440a.
This is a fresh review after closing the prior findings; previously reported items were treated as known/fixed and were not counted again.
Fresh review scope
- tree-shaken ECharts registration and both Timing/Speedup option surfaces;
- asynchronous chart lifecycle (
requestAnimationFrame, render epoch, dispose/recreate, CanvasRenderer initialization); - concurrent benchmark/metadata fetch semantics and generation-ID validation;
- lazy accessible table lifecycle, exact row identity, rerender/reset behavior, and disclosure accessibility;
- committed production E2E strength and possible false-positive gaps;
- generated deployment/staleness consistency;
- current PR description/validation provenance;
- repository benchmark-development guidance and final changed-file scope.
Prior findings now closed
- P2: production error assertions could run before deferred ECharts initialization. Fixed with a committed-deployment regression that waits for both real CanvasRenderer surfaces, crosses the asynchronous render boundary, and then asserts no page/console errors.
- P2: lazy fallback regression only required a non-empty table. Fixed with exact announced-row-count assertions plus Full-matrix rerender/reset coverage.
- P3: PR validation metadata became stale after those fixes. Updated to the actual current head, CI runs, and ten-file diff.
Clean-head gates
- Tests #1426 — success;
- Maintenance compatibility #439 — success;
- Benchmark Frontend CI #497 — success, 6/6 jobs including Python 3.9/3.11, frontend typecheck/build, deterministic staleness, Chromium E2E, and committed-deployment production QA in Chromium/Firefox/WebKit.
There are no unresolved inline review threads and no temporary audit/patch workflow in the final diff.
Result
No new actionable issue found in this fresh review. The review/fix loop has therefore converged under the requested stopping rule. PR #115 remains open and Ready; this review does not merge it.
Summary
Measurement-driven optimization pass for the benchmark dashboard after PR #76.
Tracks #114.
This PR makes three bounded product changes:
echarts/coreregistration containing only the chart/components actually used by the dashboard;<details>summaries present at first render but materializes their exact HTML tables only on first expansion instead of eagerly building hundreds or thousands of hidden rows;It also contains production regression coverage added during the review/fix loop for asynchronous ECharts initialization and exact lazy-table materialization/rerender behavior.
No benchmark schema, statistical metric, source data, filter identity, or backend contract changes.
Baseline
Baseline commit:
Five-context production audit against the exact committed
/docs/assets/benchmarks/deployment found:Baseline payload:
Baseline DOM:
This baseline did not establish a need for table virtualization or data partitioning at the current scale: Full matrix remained below ~226 ms p95 on the hosted runner.
Deterministic structural improvements
Optimized deployment payload:
Robust, deterministic reductions:
Full matrix still exposes all 146 timing chart groups and all 1,252 speedup chart rows. Exact accessible table rows are not removed: they are generated on first disclosure expansion with their full labels and values.
Latency evidence
Cross-runner wall-clock timings were noisy, so this PR does not use the earlier single-runner
-42.7% / -32.0% / -28.3%figures as causal claims.The final latency check used one
ubuntu-latestrunner, one Chromium process, fresh browser contexts, ten paired baseline/optimized rounds, and alternating execution order. The baseline was served from a detached worktree while the optimized committed deployment was served simultaneously.Paired results:
An independent same-protocol sensitivity run also showed smaller cold/Full/Focused improvements and an essentially unchanged CV cascade, confirming that absolute hosted-runner milliseconds are noisy while the bundle/DOM reductions are deterministic.
Implementation details
Tree-shakeable ECharts
frontend/src/echarts.tsregisters only:Timing and Speedup keep the same chart identities, axes, data zoom, tooltip, and mark-line behavior.
Lazy accessible chart tables
The timing/speedup
<details>controls and row counts remain available immediately. The full exact table is built once, on first expansion. A normal dashboard rerender recreates the disclosure from the current selected runs, so chart/table identity remains filter-synchronized.Permanent production E2E now asserts:
setOption/CanvasRenderer path before error checks;Concurrent production metadata requests
benchmark_data.json,parse_report.json, andsource_inventory.jsonnow begin fetching together. Benchmark data remains required; metadata retains the previous non-blocking failure semantics and generation-ID consistency checks.Review/fix loop
The dedicated review/fix pass found and closed the following new issues:
pageerror/console state after the header appeared, while chart initialization is deferred withrequestAnimationFrame. Added a committed-deployment regression that waits for both real CanvasRenderer surfaces, crosses the asynchronous render boundary, and only then asserts no runtime error.Final validation
Authoritative clean-head validation on:
completed successfully:
Benchmark Frontend CI #497 has 6/6 successful jobs:
The final PR diff contains exactly ten formal files and no temporary audit/measurement/patch workflow. There are no unresolved inline review threads.
Explicit non-goals
The measurements do not justify additional complexity at the current 1,796-run scale, so this PR deliberately does not add:
These should be reconsidered only if a future larger canonical bundle produces a measured regression.
Status
Implementation, paired measurement, deterministic payload/DOM audit, review/fix loop, permanent cross-browser QA, and clean-head CI are complete. Merge remains a separate explicit maintainer decision.