Skip to content

test(perf): add reproducible plan load benchmark - #1198

Open
corrius wants to merge 2 commits into
backnotprop:mainfrom
corrius:perf/ac/browser-load-benchmark
Open

test(perf): add reproducible plan load benchmark#1198
corrius wants to merge 2 commits into
backnotprop:mainfrom
corrius:perf/ac/browser-load-benchmark

Conversation

@corrius

@corrius corrius commented Aug 4, 2026

Copy link
Copy Markdown

Summary

  • add a reproducible headless Chrome/CDP benchmark for plan UI cold loads
  • serve the built plan HTML with a representative Pi /api/plan response
  • report usable-render, navigation, JavaScript/task, long-task, document-transfer, and heap metrics
  • include browser/CDP provenance and the measured HTML SHA-256
  • support custom Chrome/HTML paths and machine-readable JSON output

Motivation

This provides a repeatable baseline for investigating the per-open delay reported in #926 and for comparing future bundle-splitting or server-delivery experiments against the same artifact and environment.

Each run creates a fresh page target, clears its HTTP cache, waits until both the plan content and approval control are rendered, and tears down its isolated Chrome profile.

Validation

  • bun run build:review
  • bun run build:hook
  • bun run typecheck
  • node tests/bench-plan-load.mjs --runs 3 --chrome /usr/bin/chromium
  • bun test: 2863 passed; two existing environment/dependency errors remain because the installed @pierre/diffs@1.3.2 build does not export EditProvider

Closes #926 only if maintainers consider the benchmark sufficient; this PR does not attempt the performance fix itself.

@backnotprop

Copy link
Copy Markdown
Owner

Review (at 796edc14)

TLDR: needs one small change, then merge. The harness hangs for the full 30s timeout on macOS, the platform #926 was filed from, because the readiness probe waits for the "Approve" label, which is hidden below the 768px breakpoint, and Chrome's default macOS window is 756px wide. Pinning --window-size=1280,900 fixes it (verified) and is worth having anyway since an unpinned viewport makes numbers incomparable across machines. Everything else is minor, and the safety, cleanup, and provenance discipline here are genuinely good for a first contribution. Welcome.

Detail (AI review findings, skim as needed):

The blocker mechanics: waitUntilUsable requires innerText.includes("Approve"), but below md the approve control renders as "OK" (ApproveDropdown.tsx, hidden md:flex). With the window pinned, macOS produces plausible numbers matching the shape of your Linux table scaled by hardware (~420-460ms usable, transfer bytes matching the file plus headers, and the reported sha256 matching an independent hash of the served file, so the provenance is genuine). Consider also switching the probe to a stable DOM hook rather than a responsive label so the readiness definition is viewport-independent.

Worth taking in the same pass: the measured window is not hermetic. The app fires a live request to api.github.com (the update check) inside every run, which makes a reproducible benchmark depend on network conditions and the anonymous rate limit. Blocking non-loopback resolution (--host-resolver-rules="MAP * ~NOTFOUND, EXCLUDE 127.0.0.1") and documenting it would tighten the claim. And since the issue thread names gzip as the first optimization this harness will judge, a --gzip serving flag would let it A/B the exact experiment it was built for.

Noted as fine or follow-up: the script needs Node 22+ for global WebSocket (an engines field or README line would save someone a bare ReferenceError); the fixture omits a few fields of the real /api/plan response (renders fine, slightly lighter than production); usableMs is quantized by the 25ms poll and the heap number swings with GC timing, both worth a caveat line; the free-port probe has a harmless race a --remote-debugging-port=0 plus DevToolsActivePort read would remove; parseArgs sits outside the try so unknown flags print a stack trace instead of the friendly error.

Verified clean, for the record: both embedded servers bind loopback only; no external URLs in the script itself; Chrome flags contain no sandbox or web-security weakening; the throwaway profile is mkdtemp'd and removed in a finally with zero leftovers after success and failure paths both; teardown escalates TERM then KILL; zero new dependencies (CDP over Node's built-in WebSocket, exactly the right call this week); not referenced by any CI workflow and invisible to bun test, so it is manual-only as it should be; the package.json delta is one script line and the lockfile is untouched; README accurate; no em dashes; single commit on current main, mergeable.

@corrius

corrius commented Aug 4, 2026

Copy link
Copy Markdown
Author

Addressed in f189c345:

  • pinned Chrome to a 1280x900 viewport
  • made the usable-state probe accept the responsive approval control instead of requiring the desktop-only Approve label
  • blocked external host resolution while preserving loopback traffic
  • added --gzip for identity/gzip A/B runs and included encoding, served bytes, viewport, and network policy in the report
  • documented Node 22+, the fixed viewport/network policy, 25ms readiness sampling, and heap variability

Validated with:

  • bun run build:hook
  • bun run typecheck
  • node tests/bench-plan-load.mjs --runs 3 --chrome /usr/bin/chromium
  • node tests/bench-plan-load.mjs --runs 3 --gzip --chrome /usr/bin/chromium

Both benchmark modes completed successfully with external traffic blocked. The gzip run reported 6.98 MiB served versus 21.97 MiB identity for the same HTML SHA-256.

@backnotprop

Copy link
Copy Markdown
Owner

Delta re-review (at f189c345)

TLDR: merge as-is. All findings fixed and verified empirically, the turnaround fixed root causes rather than symptoms, and your own harness already produced its first real result: gzip makes the loopback load about 46ms slower, not faster.

Detail (AI review findings, skim as needed):

The macOS failure is fixed properly: the viewport is pinned for comparability AND the probe is viewport-independent on its own merits (verified by forcing a 700px window, where it correctly matches the mobile OK control; the regex also correctly rejects the "Approving..." loading state and accepts the split-button variant). The network block works with the right semantics: the update-check still executes and fails locally at DNS with no packet leaving the machine, so the app's real code path is preserved while the network variable is removed; all seven loopback requests still flow. The gzip A/B byte accounting is correct (31.77 percent, agreeing with Vite's own gzip report for the same artifact to within rounding), the encoding is labeled in both output modes, and the sha256 correctly stays the hash of the uncompressed artifact so provenance identifies the build rather than the transport.

The substantive result: identity averaged 426ms usable, gzip 473ms. Over loopback the transfer saving buys nothing while decompression costs real time, which is direct evidence against the gzip experiment as a perceived-load improvement for the local server case. One methodological caveat if this number gets cited: the harness pre-compresses at startup, so server-side compression CPU is excluded; the A/B measures transfer plus decompression only. Remote sessions (where transfer is real) remain the case where compression could still pay.

Still clean: zero dependencies (node:zlib is a builtin), loopback-only with the resolver rule tightening posture, CI-invisible, zero leftover profiles or processes after six more launches, README accurate including the Node 22 floor and measurement caveats, no em dashes, mergeable on current main.

Cosmetic nits, fine as-is or follow-up: the provenance viewport string reports the window size rather than measured innerWidth/innerHeight; parseArgs sits outside the try so unknown flags stack-trace; the free-port probe has a benign race; the fixture omits a few production response fields.

Approving the workflow runs now so CI has real signal. Quality first contribution, twice over.

@corrius
corrius marked this pull request as ready for review August 5, 2026 06:45
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.

Plan HTML page takes around 5 seconds to load and shows a long loading state on macOS

2 participants