perf(benchmark): stop seeding a 10k-event fixture on every run - #903
perf(benchmark): stop seeding a 10k-event fixture on every run#903BunsDev wants to merge 4 commits into
Conversation
The CLI performance baseline spends almost all of its wall time seeding fixtures rather than measuring: a local run took 398s, of which ~325s was seeding session input events. Events are seeded one PTY round trip at a time, ~29ms each, because that is the only shape that works. Measured while looking for a faster one: - Issuing the input requests concurrently does not speed them up, it wedges - the daemon serializes PTY writes and the seed hangs until the socket timeout, at concurrency 8 and 32 alike. - Batching newline separated lines into a single request records ONE event, not one per line. - Letting the fixture harness print its own output records events chunked by PTY read size - 1000 printed lines produced 12 events. `prepareEventTail` needs exactly `count` events, so none of those work. The actual problem is that one flag drove two axes with very different unit costs: `eventCounts` was just `options.sessionCounts`. Nobody chose to seed 10000 events; it came along with 10000 sessions, and at ~29ms each that single tier costs ~5 minutes - roughly seven times the entire 11100-session fixture, which parallelizes at ~4ms each. Split the two. Sessions keep [100, 1000, 10000]; events default to [100, 1000]. The deep tail stays available with `--event-counts 100,1000,10000`, and the report now records which tiers it collected so baselines stay comparable. Also raise session seeding concurrency 8 -> 32, measured per 1000 sessions: 28.9s serial, 3.9s at 8, 2.5s at 32, 2.5s at 64. Verified this does not perturb the read latency the scenario reports (interleaved runs: 30.5/29.7/37.5/44.2ms at 8/32/8/32 - noise, no systematic shift). End to end on the same binary: 398s -> 87s, a 4.6x cut. Scenario coverage is unchanged except for the intentionally dropped `event_tail_10000`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new --event-counts parsing (and adjacent --session-counts) uses parseInt, which can accept invalid tokens with trailing junk, so malformed CLI input may be treated as valid fixture sizes instead of being rejected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reduces benchmark wall-clock time by decoupling the number of seeded input events from the session fixture sizes, so routine baseline runs spend their time measuring scenarios rather than seeding a 10k-event tail.
Changes:
- Add
--event-countsCLI option and split default tiers: sessions remain[100, 1000, 10000]while events default to[100, 1000]. - Record
eventCountsin the benchmark report options and preserve backward compatibility for older callers by falling back tosessionCountswheneventCountsis unset. - Increase default session fixture seeding concurrency (8 → 32) and add comments documenting why event seeding must remain serial.
File summaries
| File | Description |
|---|---|
| scripts/benchmark-cli.mjs | Adds --event-counts, changes defaults, threads event tiers through collection/reporting, and raises session seeding concurrency. |
| scripts/benchmark-cli.test.mjs | Extends option parsing tests to cover --event-counts behavior and validation. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const values = raw.split(',').map((value) => Number.parseInt(value, 10)); | ||
| if (values.some((value) => !Number.isSafeInteger(value) || value <= 0)) { | ||
| throw new Error('--event-counts must contain positive integers'); | ||
| } | ||
| eventCounts = values; |
There was a problem hiding this comment.
Fixed in 2436a2e — count-list tokens now go through a strict /^\d+$/ check before conversion, so trailing-junk tokens like 100abc are rejected for both --session-counts and --event-counts, with regression tests added.
Co-authored-by: BunsDev <68980965+BunsDev@users.noreply.github.com>
Addresses the root cause behind #897, #899 and #902: the perf baseline spends its wall time seeding fixtures, not measuring. Those three bought headroom and visibility; this cuts the cost.
Measured end to end on the same binary: 398s -> 87s (4.6x).
Where the time went
~325s of the 398s was seeding session input events. They cost ~29ms each because each one is a PTY round trip, versus ~4ms for a session.
Three faster seeding strategies, all measured and all rejected
I tried to make seeding cheaper before cutting anything:
prepareEventTailasserts exactlycountevents exist, so none are viable. The serial loop is the only correct shape, and it now says so in a comment so nobody "optimizes" it into a CI hang.The actual bug: one flag, two very differently-priced axes
eventCountswas literallyoptions.sessionCounts. Nobody chose to seed 10,000 events - it came along for free with 10,000 sessions. At ~29ms each that one tier costs ~5 minutes, roughly 7x the entire 11,100-session fixture.So the two are split:
[100, 1000, 10000](cheap, parallelizes)[100, 1000]--event-counts 100,1000,10000Also raised session seeding concurrency 8 -> 32. Measured per 1000 sessions: 28.9s serial, 3.9s at 8, 2.5s at 32, 2.5s at 64 - 32 is where the curve flattens.
Coverage: exactly one scenario dropped
Diffed the reports rather than asserting it:
11 of 12 scenarios retained, including all three session tiers.
Verifying the fix does not distort the measurement
A harness change must not move the numbers it reports.
sessions_1000looked like it moved (29 -> 76ms), so I isolated it with interleaved runs at 5 iterations instead of trusting one sample:Noise, no systematic shift. (
helpswung 888ms -> 9.9ms between the same two runs, which shows the noise floor.)Gates
benchmark-cli.test.mjs61 pass / 0 fail (5 new tests for--event-counts),benchmark-chaos.test.mjs,check-secrets.py,check-ci-workflow-test.py,check-coven-privacy.py --staged- all pass.Both workflows invoke the script with no count flags, so they pick up the new default automatically; no workflow edit needed. Backward compatible: callers passing only
sessionCountsstill get their previous event coverage.Note on the tradeoff
This does drop deep-cursor read coverage at 10k events from routine runs. That is a real reduction, made deliberately and kept one flag away - flagging it explicitly rather than burying it.