perf(cli): four walk and gate performance fixes (#1113, #1119, #1116, #1114) by dekobon · Pull Request #1156 · dekobon/big-code-analysis

dekobon · 2026-08-01T03:31:59Z

Four CLI performance issues, plus the follow-up work each one turned up.

Fixes #1113
Fixes #1119
Fixes #1116
Fixes #1114

Results

All measured with release binaries over tests/repositories/DeepSpeech
(12,732 analyzable files), median of three to five runs on a 16-core host.

Change	Result
#1113 `check` computes only thresholded metric families	1.24–1.28× less user CPU on a one- or two-metric gate
#1119 result channels lose their `Mutex`	No throughput change; −48 lines of unreachable error handling
#1116 `diff --since` builds its sets in memory	wall 9.42 s → 7.62 s, sys 3.59 s → 2.34 s, output byte-identical
#1114 parallel directory walk, no idle pool slot	walk 100.8 ms → 33.6 ms (3.0×); full `check` ~1.12×

Where the issues were wrong

Each issue's plan was checked rather than followed, and three premises did not
survive.

#1113's proposed approach would have introduced a bug. It suggested
deriving the metric selection from threshold_metric_for_name. That helper
answers a suppression question and returns None for tokens — but tokens
is a real configurable threshold, so selecting by it leaves m.tokens at zero
and 0 > limit silently never fires. The family is instead declared per
MetricExtractor entry, making the registry the single source of truth.
check_tokens_threshold_fires_under_narrowed_metric_selection is the
regression test.

#1113's other two claims are also wrong. Action::Exemptions already
computed no metrics (dispatch_exemptions reads suppression markers off the
parse), so there was nothing to narrow. And the projected 5–7× came from
bca metrics, which serializes a document per file; check emits nothing per
file, so its ~22 s parse-and-walk floor bounds the saving. This repo's own
bca.toml thresholds nine of thirteen families, so make self-scan gains
nothing — contrary to the issue.

#1114 is not the bottleneck it describes. Warm, the serial walk was 0.09 s
of a 3.6 s check — 2.5%. The 3.0× speedup is real but it is 3× of a small
number. The cold-cache monorepo claim may well hold; it is not measurable
without root to drop the page cache.

#1119's stated rationale is stale. std::sync::mpsc::Sender has been
Sync since Rust 1.72 (MSRV is 1.94), so the mutex had been unnecessary for
years and crossbeam was never required — only consistent with the library's
existing channel. Corrected in the code comments and on the issue.

Deliberately not done

The streaming half of #1114. It asks to feed the pool directly from the
walker with no intermediate Vec. Three reasons: it contradicts the same
issue's determinism requirement (you cannot sort a stream); FilesData is
published API, not #[non_exhaustive], and constructed with struct literals,
so changing run's parameter is a SemVer break needing an additive entry
point; and with the walk itself now 3× faster the remaining overlap is 33.6 ms
of a ~2 s run. Happy to file a follow-up against the next major.

A regression this PR introduced and then fixed

Reviewing #1116 showed it had traded memory for time without saying so: peak
RSS went 437 MB → 831 MB, because the unbounded aggregate channel held a
FuncSpace for every file. A collector thread started before the walk, over a
per-job bounded channel, reduces each tree and drops it as it arrives:

	sys	wall (min)	peak RSS
pre-#1116 (temp JSON)	3.59 s	9.42 s	437 MB
#1116 as first written	3.36 s	12.42 s	831 MB
final	2.34 s	7.62 s	599 MB

Memory falls 28% and wall time improves rather than paying for it. The residual
599 vs 437 MB is inherent — a side's MetricSet now accumulates during its
walk instead of in a pass afterwards, and that overlap is what buys the speed.
Called out in the CHANGELOG for anyone sizing a CI container.

Public API

Additive: ConcurrentRunner::without_path_verification.

Behavioural, signature unchanged: ConcurrentRunner::new's num_jobs is now
the consumer count rather than a budget shared with a producer thread, so a
caller passing n gets n consumers instead of n - 1.
ConcurrentErrors::Producer is no longer constructed and is retained until
3.0 so a downstream match still compiles. Both recorded in STABILITY.md
and CHANGELOG.md.

Two user-visible CLI changes, both in the CHANGELOG: per-file output order for
a directory walk is now sorted rather than readdir order (previously
unspecified and filesystem-dependent; the parallel walker would otherwise vary
it run to run), and a file that vanishes between the walk and its analysis is
now a tool error rather than a silent skip that exited 0.

Tests

Twelve new tests, each verified to fail against the exact production line it
names. audit-tests caught three of them passing for the wrong reason and
they were fixed:

check_mi_value_is_identical… widened with tokens/abc/nom, none of
which are MI's dependencies, so deleting Metric::Mi's dependency list drove
both sides to 0 and they still matched.
report_body_… and exemptions_audit_… guarded only against emptiness, so a
deterministic drop removed the same record from both sides and still
compared equal. They now assert the absolute counts their output carries.

One test is deliberately a unit test rather than end-to-end:
walk_directory_seed_returns_sorted_paths. The end-to-end version — five
bca metrics runs compared for identical order — stayed green with the sort
deleted, because the ordering bug is a race.

Validation

make pre-commit cannot complete in a nested worktree (#1145 — cargo fmt --all and enums-check resolve the workspace-excluded crates against the main
checkout's manifest). Every other stage was run individually and passes:
scoped fmt, clippy, test, test-doc, cargo doc with -D warnings, udeps,
manpages-check, both self-scan tiers, all lint families, and all four Python
stages. Baseline refreshed: 178 → 175 entries, four dropping out because
removing the unreachable poisoned-lock branches took nexits back under its
limit.

The three enums-* stages are unverified here and are untouched by this
branch; worth one make pre-commit from the main checkout before merge. I
added the findings to #1145, including that its planned five-manifest fix needs
to be six — enums is affected too.

`bca check` ran the full metric suite regardless of what the resolved `ThresholdSet` actually reads, then discarded the rest. Derive the selection from the set and hand it to `MetricsOptions::with_only`. The family each threshold reads is declared per entry on `MetricExtractor` rather than derived from the name via `threshold_metric_for_name`. That helper answers a *suppression* question and returns `None` for `tokens` — a marker may never silence it — but `tokens` is a real configurable threshold, so selecting by that mapping would leave `m.tokens` at its zero default and silently disarm the gate. Measured over `tests/repositories/DeepSpeech` (12.7k files), median user CPU of five runs: a one- or two-family gate drops from 28.6-29.2s to 22.4-23.3s (1.24-1.28x). A gate naming nine families — this repo's own `bca.toml` — is unchanged at 1.00x, since it already selects nearly everything. The issue's projected 5-7x came from `bca metrics`, which also serializes a document per file; `bca check` emits nothing per file, so the ~22s parse-and-walk floor bounds the saving. `Action::Exemptions` needed no change: `dispatch_exemptions` reads suppression markers off the parse and never computed metrics. Fixes #1113

`crossbeam::channel::Sender` is `Sync`, so the five result channels on `Config` no longer need the `Mutex` that `std::sync::mpsc::Sender` required, and every worker stops locking once per file. The four poisoned-lock degradation branches go with it: nothing could poison those mutexes (no code panics while holding one) and no test covered them, so they were unreachable error handling. No measurable throughput change on a 16-core host — `check` and `report` over `tests/repositories/DeepSpeech` (12.7k files) at `--jobs 32` and `--jobs 64` land within 0.975-1.003x of the previous binary. The lock was taken once per file, not per record, so it was never hot at this core count. The change stands on removing a global serialization point that scales the wrong way and 48 lines of dead error handling, which is what the issue itself proposed. `tests/walk_channel_completeness.rs` pins the rewired send path of all four channels: each command's output must be identical at `--jobs 1` and `--jobs 16`. Verified falsifiable by dropping one file's violations, which fails the check case on the record count. Fixes #1119

`walk_metric_set` ran its walk with `output_dir` pointing at a TempDir, writing one JSON document per source file (each behind a `create_dir_all`), then immediately re-walked that tree and re-parsed every document back into a `serde_json::Value`. Both sides of every `bca diff --since` paid it, while the `FuncSpace` trees were already in the workers' hands. Stream the spaces over the existing aggregate channel instead and build the `MetricSet` with `serde_json::to_value` — the same `Serialize` impl the writer used, so the resulting document, and therefore the pairing key and the `metrics` value, are the ones that used to be written out. `load_dir_set` stays for the explicit two-directory `bca diff <old> <new>` form, whose input genuinely is a directory of documents. Output is byte-identical. Verified against the previous binary on this repo (`--since HEAD~1/~2/~3`, 14,954 lines) and on `tests/repositories/DeepSpeech` (`--since HEAD~5`, 12,469 lines), plus the 23 existing `diff_since` integration tests unchanged. Float values survive because the workspace enables serde_json's `float_roundtrip`, so the parse the old route depended on was already bit-exact. Measured on DeepSpeech (12.7k files), median of three: wall 11.73s -> 8.74s (1.34x), system time 2.75s -> 1.57s (-43%), user 37.08s -> 35.62s. On a small tree the saving is inside the noise; the filesystem round-trip only becomes visible at scale. `set_from_spaces_matches_the_file_round_trip` pins the in-memory set against a real `load_dir_set` round-trip of the same space. Verified falsifiable on both halves: storing the whole document instead of its `metrics` object, and keying off the streamed path instead of the document `name`. Fixes #1116

Three changes to the walk prologue, which ran entirely before any analysis started. The CLI's directory walk drove `ignore`'s single-threaded iterator on the main thread while every worker sat idle. It now uses `build_parallel()` with the same thread budget the pool gets — the two never run at the same time, so there is nothing to split it with. Walking `tests/repositories/DeepSpeech` (12.7k files) in isolation falls from 100.8ms to 33.6ms (3.0x, 15 runs); a full `check` over the same tree improves ~1.12x. Total CPU rises slightly (69.8ms -> 102.1ms on the walk) — the trade is threads for wall time. `ConcurrentRunner::run` spawned a producer thread and reserved a pool slot for it, so `--jobs auto` on 16 cores ran 15 consumers for work that finished almost immediately. Dispatch moves to the calling thread; consumers are spawned first, so they still start draining as the first paths land. `num_jobs` now means the consumer count. The producer also re-`stat`ed every path that the CLI walk had already classified from its `dirent`. `without_path_verification` opts out; the default is unchanged for library callers, whose `FilesData` may hold arbitrary paths. The resolved file list is now sorted. A parallel walk yields entries in thread-completion order, so without it per-file output order would vary run to run at `--jobs 1` — output that used to be reproducible. Sorting also makes that order independent of readdir order, so it no longer varies by filesystem or machine, which the single-threaded walk never guaranteed. Consumers pinning the old order see a one-time reshuffle; the file *set* is unchanged, verified byte-for-byte against the previous binary over DeepSpeech. Deliberately not done: the issue also proposed streaming paths into the pool with no intermediate `Vec`. That trades away the determinism the same issue asks to preserve — you cannot sort a stream — and `FilesData`'s materialized shape is published API. With the walk itself now 3x faster the remaining overlap is small; measured warm, the whole walk is 33.6ms of a ~2s run. `num_jobs_is_the_consumer_count_not_a_budget_shared_with_a_producer`, `run_verifies_paths_by_default`, `without_path_verification_dispatches_every_path`, `walk_directory_seed_returns_sorted_paths`, and `parallel_walk_finds_every_file_at_every_job_count` cover the four behaviours; each was verified to fail against the pre-change code. The sortedness assertion is a unit test on purpose — an end-to-end five-run comparison through `bca metrics` stayed green with the sort deleted, because the ordering bug is a race. Fixes #1114

`load_dir_set` and `set_from_spaces` derived the pairing key and pulled the `metrics` object with the same four lines. `bca diff --since` builds one side in memory and `bca diff <old> <new>` reads the other off disk, so the two agreeing is load-bearing rather than incidental — a shared `insert_document` makes it structural instead of conventional. Also drops an intermediate binding in `ConcurrentRunner::run` that only renamed the dispatch result.

`thresholds.rs` gains a `metric:` line per extractor entry plus `selected_metrics`, which puts the file at 526 loc.ploc — inside the 95-100% soft band (522.5) though still under the 550 hard limit. Net the refresh removes more than it adds: four entries drop out because deleting the unreachable poisoned-lock branches took `nexits` back under its limit in `compute_since_diff`, `dispatch_exemptions` and `dispatch.rs`, and `ConcurrentRunner::run` shed enough to clear its Halstead entry. 178 entries -> 175.

#1116 moved `diff --since` off its temp-JSON round-trip but drained the aggregate channel only after the walk had joined every worker, so the unbounded channel held a `FuncSpace` tree for every file at peak. Over `tests/repositories/DeepSpeech` (12,732 files) that took peak RSS from 437 MB to 831 MB — a regression the original commit measured for time and not for memory. Run the collector on its own thread, started before the walk, over a channel bounded per job. Each tree is reduced to its metrics value and dropped as it arrives instead of queueing, and the reduction overlaps the walk rather than following it. Median of three on that tree: route sys wall(min) peak RSS temp JSON (pre-#1116) 3.59s 9.42s 437 MB #1116 as shipped 3.36s 12.42s 831 MB this commit 2.34s 7.62s 599 MB So memory falls 28% and wall time improves rather than paying for it. The residual 599 vs 437 MB is inherent to the in-memory design: a side's `MetricSet` is now accumulated during its walk instead of in a separate pass afterwards, and that overlap is what buys the speed. It is called out in the CHANGELOG so anyone sizing a CI container knows. A tighter bound does not help — at 4x per job the peak is 600 MB and wall time rises to 6.55s, because what remains at peak is the accumulated set, not the backlog. The bound's job is to stop unbounded growth, not to tune the peak. Also from review: - `walk_metric_set`'s doc still described the deleted `json_out_dir` and claimed the pairing key came from a path relative to it. The key comes from the document's `name`, which is the invariant `set_from_spaces_matches_the_file_round_trip` exists to pin. - `tests/common/mod.rs` still added 1 to `available_parallelism` to offset the producer slot #1114 removed, so every corpus runner was oversubscribed by one. - `set_from_spaces` skipped a non-metrics aggregate item with `continue`. Silently shortening a side's set is the wrong-answer failure mode #1098 guards against, so it is refused instead. - `Metric::suppressible`'s doc asserted `tokens` has no configurable threshold. It has one; only suppression excludes it. That wrong premise is exactly the trap #1113 had to work around. - `report_hotspot_tx`, the fifth channel #1119 rewired and the only one whose call site changed shape, had no serial-vs-parallel coverage. Added, asserting every change-history row carries a joined hotspot score; verified falsifiable by dropping one file's record.

Audit of the tests this branch added, by perturbing the production line each one names. Three passed against a bug they claimed to cover. `check_mi_value_is_identical_whether_or_not_the_walk_is_narrowed` widened its comparison run with `tokens`/`abc`/`nom` — none of which are MI's dependencies. Deleting `Metric::Mi`'s dependency list drove *both* sides to `mi.original = 0`, so they still matched and the test passed, while claiming to pin exactly that closure. The comparison run now names `loc.sloc`, `cyclomatic` and `halstead.volume`, selecting those families independently of the closure under test. `report_body_is_identical_serially_and_in_parallel` and `exemptions_audit_is_identical_serially_and_in_parallel` guarded only against emptiness. Serial-vs-parallel equality cannot see a deterministic drop — it removes the same record from both sides — so skipping one file's records left both runs equal and both non-empty. They now assert the absolute counts their outputs already carry (`| Functions/methods | 120 |`, `# In-source markers (40)`), matching the guards the check and aggregate tests already had. All three now fail under the perturbation and pass without it. The remaining nine tests added on this branch were already verified the same way; `parallel_walk_finds_every_file_at_every_job_count` needed a dropped-file perturbation rather than a duplicated one, since the caller's cross-seed dedupe absorbs duplicates by design.

codecov · 2026-08-01T03:36:36Z

Codecov Report

❌ Patch coverage is 62.50000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.16%. Comparing base (0b17b02) to head (62d12c9).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/concurrent_files.rs	62.50%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1156      +/-   ##
==========================================
- Coverage   98.16%   98.16%   -0.01%     
==========================================
  Files         276      276              
  Lines       69312    69308       -4     
  Branches    68882    68878       -4     
==========================================
- Hits        68038    68034       -4     
+ Misses        873      871       -2     
- Partials      401      403       +2

Flag	Coverage Δ
python	`100.00% <ø> (ø)`
rust	`98.15% <62.50%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/metric_set.rs	`99.02% <ø> (ø)`
src/concurrent_files.rs	`90.07% <62.50%> (-0.30%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

`parallel_walk_finds_every_file_at_every_job_count` reduced each emitted `name` to a basename by splitting on '/'. The name carries the platform separator, so on Windows the split found nothing and every "basename" stayed a full `C:\…\a0-impl.rs` path, failing the set comparison. `Path::file_name` handles both separators. Windows-only, and the test was new on this branch, so nothing shipped was affected. Caught by `test (windows-latest)` on PR #1156; the other 80 checks passed.

dekobon added 8 commits July 31, 2026 17:37

dekobon merged commit 1bbd4b5 into main Aug 1, 2026
49 checks passed

dekobon deleted the fix/batch-2026-07-31 branch August 1, 2026 04:12

Navigation Menu

perf(cli): four walk and gate performance fixes (#1113, #1119, #1116, #1114) - #1156

perf(cli): four walk and gate performance fixes (#1113, #1119, #1116, #1114)#1156
dekobon merged 9 commits into
mainfrom
fix/batch-2026-07-31

dekobon commented Aug 1, 2026

Uh oh!

codecov Bot commented Aug 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

dekobon commented Aug 1, 2026

Results

Where the issues were wrong

Deliberately not done

A regression this PR introduced and then fixed

Public API

Tests

Validation

Uh oh!

codecov Bot commented Aug 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codecov Bot commented Aug 1, 2026 •

edited

Loading