Repository navigation
chore(bench): one command compares a one-file sync before and after a change - #530
Conversation
|
@coderabbitai full review |
|
cubic has no review quota until 2026-11-11, so it skips this pull request; Greptile and CodeRabbit review it. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 20 seconds. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
CodeRabbit skipped this pull request: "Review limit reached" (free OSS review quota used up). Per the no-quota rule it proceeds on Greptile; cubic also has no quota until 2026-11-11. |
[Medium impact] Adds benchmark tooling for one-file sync performance. Fix the unsafe directory deletion and workload checks before merging. Findings
Fix with agent prompt### Issue 1
scripts/benchmarks/runtime/sync-ab.mjs:119-120
`restore()` deletes `cgIndex` before checking whether it is safe to replace. Passing `--cgdir .` deletes the corpus itself. Pointing `--index` at the restore directory deletes the saved index before copying it. Validate `--cgdir` as a plain directory name and reject overlapping source and destination paths before deleting anything.
### Issue 2
scripts/benchmarks/runtime/sync-ab.mjs:110
`workCounters()` treats timing-only lines as work counters, but synthesis passes print those lines only when they exceed 250 ms with `CODEGRAPH_SYNTH_TIMINGS=1`. A pass taking 240 ms before and 260 ms after therefore looks like different work. The script stops with status 3 even when both builds did the same work. Compare only lines containing actual work counts, or exclude the threshold-gated pass timings.
### Issue 3
scripts/benchmarks/runtime/sync-ab.mjs:207-215
`--proxies` exits before the graph and work checks in the session loop. For example, `--proxies --no-relabel` reports instruction and allocation ratios even when one build rebuilds the skip list. The regular comparison rejects that mismatch. Keep the run records and apply the same graph, counter, and rescan checks before publishing these ratios.
### Issue 4
scripts/benchmarks/runtime/sync-ab.mjs:191
`noiseKey()` omits `BENCH_WRAP` and the saved index's contents. Reusing `--noise` after changing a memory cap or replacing the index skips all controls and judges the new run against variation measured under different conditions. Include the wrapper and a saved-index fingerprint in the key so those changes require fresh controls.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds a one-command benchmark for comparing one-file sync memory and time.
|
…heck proxies and key the noise floor on the wrapper and index
|
@coderabbitai full review |
|
One command now measures whether a change moves a one-file sync's peak memory, wall time or CPU time, and stops early when the comparison would mislead. On n8n it reproduces #529's result (peak 961 to 915.5 MiB, 4.7% lower) in 5 minutes. The run that measured #529 took a hand-written driver and a manual index fix.
scripts/benchmarks/runtime/sync-ab.mjstakes a corpus, the file to edit, a saved index and two builds or runtimes. Each run restores the saved index and checks it byte for byte, then runssyncunder/usr/bin/time -vand hashes the edges and near-duplicate tables. The two builds' order alternates within each session. Same-build control pairs give the noise floor. A metric qualifies when every session's median ratio is outside both the 3% band and that floor, in the same direction. The output is a Markdown table for a PR body, a chart spec and one JSON line per run.It stops early in three cases:
--no-relabelthe tool now stops after one pair and names the rebuilt list.--noisereuses the floor stored for the same host, kernel, runtime version, corpus revision and edit, and runs no controls; an upgrade changes the key.To avoid the rescan, each build runs once untimed. When that run rebuilt the skip list, the build gets an index copy labelled with its own fingerprint, and the run prints a note.
--proxiesruns each build once underperf stat -e instructions:u, and on Node once with a preload that totals the bytes allocated, as a four-run screen.Measured on n8n 7cc0450a, #528 (5ccc833) as the before build and #529 as the after build, Node 24.22.0:
--noisewith the floor stored by the run above--proxies--no-relabel#529 measured the same change by hand at 960 to 915 MiB (40 runs over Node and Bun).
What the proxies predict. Run on four landed changes (#493, #494, #528, #529, Node and Bun), the instruction ratio matched large time changes: #493's 0.676 and 0.658 against a sync 29% and 35% faster. A drop of about 5% in instructions did not reliably show in wall time, whose same-build variation on n8n is up to 7%. Bytes allocated did not predict peak memory: #528 allocated 5% less on Node with no peak change, and its 12% Bun drop had no proxy. The README records the table and says to use the proxies to tell whether a change does less work, and the full comparison for memory claims. Repeating two of the cases moved each ratio by 0.003 at most.
Also changed:
scripts/benchmarks/runtime/README.mddocuments the command, and thecodegraph-build-validationskill points to it and records its run times.Checks:
node --checkon the script,npm run check:agent-docs, and the four runs above at e2fecbe. At 3ef4c8b (review fixes):--cgdir .and an index inside the restore directory are refused before anything is deleted; the planted mismatch stops both the comparison and--proxieswith exit 3; normal proxy runs on #529 and #493 pass. Not covered: Bun arms of the proxy mode (allocation is Node-only; instructions work on both), and corpora other than n8n.README: no user-visible feature, command or number changes; the "About this fork" section is unaffected.