refactor(perf): canonicalize the benchmark / wt-perf system#3403
Merged
Conversation
Design proposal (review-only, per design/ convention). Inventories the five perf-tooling layers and proposes seven consolidations: delete the cache-check subcommand (subsumed by logs profile --format=json), move the timeline renderer into worktrunk::trace, add by_context to Profile and retire the trace_processor SQL section, share one warm/cold bench runner, prune the bench matrix (~15-20 min/day of CI), move the wt-perf CLI tests in-package, and unify the setup config parser. Emission, chrome.rs, the separate wt-perf package, and fixture shapes stay as is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implements design/simplify-bench-perf.md (items 1-7): 1. Delete wt-perf cache-check — its output is the .cache field of wt config state logs profile --format=json. 2. Move the timeline renderer into worktrunk::trace, sharing command_label/render_table/fmt_dur with the profile so the two views can't drift (drops wt-perf's tabwriter and insta deps; timeline durations now render in the profile's fixed-point format). 3. Add by_context to Profile (BY CONTEXT table + JSON array): per-worktree subprocess totals, the one analysis that previously required trace_processor SQL. benches/CLAUDE.md's SQL section becomes a pointer to logs profile + Perfetto; the running-tend statusline recipe now uses logs profile too. 4. One warm/cold bench runner (wt_perf::bench_wt) owns the BatchSize::PerIteration rationale once; five inline copies deleted. All runs now assert child exit status (list.rs previously ignored it). 5. Prune the matrix: real_repo keeps only the 8-worktree variants, cow_copy is deleted (shadow serial impl validating a settled choice), and the remove_e2e/first_output arm (duplicate of first_output/remove) is deleted. Ends three gist series; saves ~15-20 min/day of bench CI. 6. Move the wt-perf CLI tests in-package (native CARGO_BIN_EXE_wt-perf), deleting the dummy builds.rs and dropping wt-perf from the nextest setup script. 7. parse_config returns a SetupConfig enum covering mixed-W-B; shared init_bench_repo plumbing between the two fixture builders. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
From two review passes over the branch: - CacheReport shrinks to its same-context fields. The six dropped fields (total_commands, total_time, contexts, duplicated_commands, extra_calls, unique_commands) duplicated Profile fields or tallied cross-context repeats — expected fan-out noise the text report never showed. Their only external reader was wt-perf cache-check, deleted earlier on this branch. - src/trace/mod.rs docstring cut over to the module map + logs profile pointer; the trace_processor SQL examples it carried were retired from benches/CLAUDE.md earlier on this branch for the same reason. - Chrome args drop duration_ms (redundant with the native dur field). - render_table pads by display width (unicode-width, already a direct dep), replacing the ASCII-only invariant that the new BY CONTEXT table violated; CJK alignment test added. - Timeline instant-event row gains coverage (the one render path no test exercised); stale /main suffix fixed in benches/CLAUDE.md's invalidate example. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Main's diagnostic-section addition calls the per-render cache analysis 'a separate tool', pointing at the wt-perf cache-check recipe — which this branch already cut over to wt config state logs profile. Reword: it's the same report against a statusline capture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per the design/ convention, proposals guide implementation PRs but don't land on main. The doc remains in this branch's history (b22aced) and its content is implemented by the commits above. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # tests/helpers/wt-perf/src/lib.rs # tests/helpers/wt-perf/src/main.rs
worktrunk-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
The refactor is clean and internally consistent — the by_context aggregation, the moved traced/wall timeline computation, the SetupConfig enum, and the bench_wt warm/cold strategy all preserve the prior behavior, and I found no correctness issues. Two documentation-accuracy nits:
benches/CLAUDE.md(expected-numbers table, theremove_e2e/first_outputrow) — this row (~86 ms, "single-target validation up to first output") documents the bench variant this PR deletes frombenches/remove.rs. It's outside this diff's hunks so I can't attach an inline suggestion; either drop the row or repoint it at the survivingfirst_output/removevariant intime_to_first_output.rs.
Inline suggestion below for the other one.
The profile module spec referenced a nonexistent `wt diagnose` command (the consumer is the -vv diagnostic report), and the expected-numbers table still documented the deleted remove_e2e/first_output variant — repointed at the surviving first_output/remove. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
worktrunk-bot
approved these changes
Jul 10, 2026
Merged
max-sixty
added a commit
that referenced
this pull request
Jul 10, 2026
Release v0.67.0: version bump and changelog. Highlights since v0.66.0: experimental `wt remove --reap` (#3396), `wt switch -x` opening the picker (#3394), the termimad panic containment in the picker's PR-comments preview (#3408), the `wt config show` SIGTTOU fix (#3327), the `GIT_*` discovery-var scrub for `wt step for-each` and the `--execute` fallback (#3400), and the `wt list` prompt-reserve fix (#3409). Seven commits landed on `main` after the initial cut; `origin/main` is merged back in and the two user-facing ones (#3394, #3411) plus the BY CONTEXT profile table (#3403) are folded into the changelog. Pre-release validation: local pre-merge gate green on the merged tree (4389 tests); nightly cross-platform suite green on the initial cut ([run 29086956095](https://github.com/max-sixty/worktrunk/actions/runs/29086956095)), with the seven post-cut commits each validated by their own PR CI; `cargo semver-checks` reports no breaking changes; data-loss surface review of the cumulative diff (including the drift commits) found one new destructive capability (`--reap`, explicit opt-in, adjudicated acceptable). > _This was written by Claude Code on behalf of max_
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the design proposal from this branch's first commit (b22aced,
design/simplify-bench-perf.md— reviewable there; dropped from the merge candidate per thedesign/convention). The perf tooling's emission side was already canonical; this consolidates the consumption side around one split: capture/harness work inwt-perf, analysis inworktrunk::tracewithwt config state logs profileas its one CLI surface.The seven design items:
wt-perf cache-checkdeleted — its output was thecachefield oflogs profile --format=json. The tend statusline recipe now useslogs profile(its old pipe also fed human-format stderr into a JSON parser, so it's now correct as well as canonical).worktrunk::trace(src/trace/timeline.rs), sharingcommand_label/render_table/fmt_durwith the profile so the two views can't drift (they already had); drops wt-perf'stabwriterandinstadeps. Durations now render in the profile's fixed-point format.Profile.by_context(BY CONTEXT table + JSON array): per-worktree subprocess totals, the one analysis that previously required trace_processor SQL. The SQL sections inbenches/CLAUDE.mdand thetracemodule docs are replaced by alogs profilepointer; Perfetto stays for visual critical-path work.wt_perf::bench_wt) owns theBatchSize::PerIterationrationale once, replacing five inline copies. All runs now assert child exit status (benches/list.rspreviously ignored it).real_repokeeps only the 8-worktree variants (scaling shape stays on syntheticworktree_scaling),cow_copydeleted (benchmarked a hand-written serial copy against production rayon — a settled choice),remove_e2e/first_outputdeleted (duplicate offirst_output/remove). Ends three gist series with this PR as the single discontinuity point; saves roughly 15–20 min of the ~80-min daily bench run.CARGO_BIN_EXE_wt-perf): the dummybuilds.rsis gone and the nextest setup script builds onlymock-stub. Notecargo test --test integrationno longer runs these four tests; full-workspace runs still do.parse_configreturns aSetupConfigenum coveringmixed-W-B; the two fixture builders shareinit_bench_repo.Merge with main (#3401): the new prune benches and fixtures landed mid-flight in exactly these files; the resolution routes them through the consolidated API —
prune-M-Uis aSetupConfig::Prunevariant, the pair parsing is one sharedparse_pair, andprune-realkeeps its own cache-managed path (no--path, self-repairing).benches/prune.rskeeps its customiter_batchedarms deliberately: every variant asserts the candidate count from stdout, whichbench_wt's shape doesn't express — the exact carve-out the cache-handling docs describe.Two review passes (correctness + subtraction) then folded in:
CacheReporttrimmed to its same-context fields (the dropped six duplicatedProfilefields or counted cross-context noise the text report never showed),render_tablepads by display width (replacing an ASCII-only invariant the new BY CONTEXT table violated), the timeline's instant-event row gained test coverage, and Chrome args dropped the redundantduration_ms.Decisions surfaced, not taken (from the review passes): (a)
wt-perf setupcould become--persist-only, dropping the interactive Enter-to-cleanup path — a UX preference; (b) whether the daily benchmark cron's gist time series justifies its remaining ~60 min/day is a cost/value call. Also deferred: measuring whetherskeleton/coldadds signal overskeleton/warmbefore cutting it.Gate green throughout (4374 tests);
clippy --features shell-integration-testsclean;setup/timeline(warm, cold, chrome)/logs profile(text + JSON)/traceall driven end-to-end on a scratchmixed-4-8repo.