perf(library-select): #236 parallel BFS walker + scan memoization + tracing spans#237
Conversation
…oization + tracing spans The LDF resolver shipped under #205 was correct but left real perf on the table: the walker was single-threaded and Pass 2 re-read every file Pass 1 had already touched. This PR closes both gaps. Changes: - fbuild-header-scan: add `WalkState` (visited + scan cache + files_read counter) and `walk_with_state()`. BFS now reads each wave's files in parallel via rayon, and the scan cache persists across calls so callers that walk multiple seed sets only pay for each file once. `walk()` stays a thin wrapper for one-shot callers. `walk_with_state` is wrapped in an `ldf_walk` tracing span. - fbuild-library-select: add `ResolveStats { files_read, passes }` and `resolve_with_stats()`. `resolve()` now delegates. A single `WalkState` is threaded through Pass 1 and the reconciliation loop, and each pass runs inside an `ldf_pass` span. Library-attribution against the per-pass delta is equivalent to the old full-set check because a lib can only become newly-selected via a path reached for the first time in this pass. TDD gates (crates/fbuild-library-select/tests/perf_tdd.rs): - `pass2_reuses_pass1_scan_results_no_re_reads` -- asserts `files_read == included_files.len()` over a 2-pass scenario where Wire is only reachable through SPI.cpp. - `resolve_emits_ldf_pass_and_ldf_walk_spans` -- asserts both spans are visible via tracing-test. Measured perf (crates/fbuild-library-select/benches/, --quick): - resolve_cold: -35% time (3.27 ms vs ~5.1 ms baseline). - resolve_warm: -27% time (1.17 ms vs ~1.6 ms baseline). Behavior unchanged: all 8 walker tests, all 10 resolver tests, all 7 cache tests, and the full fbuild-build 499-test suite stay green. Closes #236 (proposals A, B, C). Proposals D (header-name precompute) and E (CI bench gates) tracked as separate follow-ups. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR implements issue ChangesParallel and Memoized Header Scanner with Multi-Pass Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
… release v2.2.5 (#265) Closes #263. ## The regression fbuild 2.2.4 broke ALL teensy41 examples on FastLED's CI: every example link-fails with multiple-definition errors on every FastLED symbol. Root cause: the LDF resolver (introduced as `perf(library-select)` in PR #237) selects the framework's bundled FastLED library at `cores/teensy4/.../libraries/FastLED/` even when the user's project ships its own FastLED. The bundled library's source files get appended to `core_sources` (teensy orchestrator.rs:207), get compiled into the build, and produce duplicate symbols at link time. The path-prefix attribution in `fbuild-library-select::resolve` can mis-attribute a `#include <FastLED.h>` when the user's transitive includes resolve into the bundled library — even though the user's project owns `FastLED.h` directly. ## The fix New `filter_framework_libs_shadowed_by_project(libraries, roots)` in `framework_libs.rs` drops any framework library whose primary header (`<lib_name>.h`) is shadowed by a same-basename header anywhere under the project's include roots. Applied at the start of both the cached and non-cached resolver paths. Conservative: only drops a library when the project itself ships a header matching the library's canonical name. Other framework libraries (SPI, Wire, etc.) are unaffected. ## Tests - `project_is_the_library_does_not_pull_in_bundled_copy` — the simpler case (project src/FastLED.h, framework libraries/FastLED/); passed before the fix too (the resolver handled this case via path-prefix attribution) but stays as a regression gate. - `example_only_root_does_not_pull_in_bundled_fastled_when_user_owns_fastled` — the failing case (per-example walker root doesn't see the repo's src/, but the user owns FastLED at a higher level). Demonstrates the filter dropping the bundled library before the resolver runs. Full workspace cargo check / clippy / fmt / test all green. ## Release v2.2.5 Patch release rolling up: - THIS fix (#263 regression repair) - The LTO-tmpdir fix from #261 / PR #262 (Windows MSYS `mv` path collapse) Cargo.toml + pyproject.toml bumped to 2.2.5. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes #236 (proposals A, B, C).
The LDF resolver shipped under #205 was correct, but the cold scan left real performance on the table:
std::fs::read_to_stringat a time.visitedset was local to eachwalk()invocation.tracingspans, so per-pass regressions were invisible without external profiling.This PR closes all three.
TDD process (RED → GREEN)
Two failing tests went in first (
crates/fbuild-library-select/tests/perf_tdd.rs):pass2_reuses_pass1_scan_results_no_re_reads— builds a scenario where Wire is only reachable through SPI.cpp (forces a 2-pass reconciliation), then assertsfiles_read == included_files.len(). Without memoization this assertion fails by a factor of ~2×.resolve_emits_ldf_pass_and_ldf_walk_spans— usestracing-test::traced_testto confirm both span names appear in the captured log.Both tests failed to compile against
main(noresolve_with_stats, noWalkState) — that's the RED gate. The implementation that follows makes them pass.Implementation
fbuild-header-scanWalkState { visited, scan_cache, files_read }andwalk_with_state(seeds, search_paths, &mut state).rayon::par_iter().filter_map(...).collect(), merges them serially into the scan cache + bumpsfiles_read, then resolves every include and queues the next wave.walk()stays a thin wrapper around a fresh state for one-shot callers — all 8 existing walker tests pass unchanged.walk_with_statecarries a#[tracing::instrument(name = \"ldf_walk\", ...)]attribute.fbuild-library-selectResolveStats { files_read, passes }andresolve_with_stats().resolve()now delegates toresolve_with_stats(...).0.WalkStateis threaded through Pass 1 and the reconciliation loop. Each pass runs inside aninfo_span!(\"ldf_pass\", pass = N).Measured performance
uv run soldr cargo bench -p fbuild-library-select -- --quick:resolve/cold_30_libs_chain_5resolve/warm_30_libs_chain_5Cold-resolve gain is dominated by parallel file reads + the deduplicated Pass 1/Pass 2 scans; warm-resolve gain comes from the faster cache-key construction path that benefits from the same memoization.
Test plan
uv run soldr cargo test -p fbuild-library-select --test perf_tdd(2 passed — the new TDD gates)uv run soldr cargo test -p fbuild-header-scan -p fbuild-library-select(51 + 19 passed — full existing coverage)uv run soldr cargo test -p fbuild-build --lib(499 passed — orchestrator consumers)uv run soldr cargo clippy --workspace --all-targets -- -D warnings(clean)uv run soldr cargo fmt --all(clean)uv run soldr cargo bench -p fbuild-library-select -- --quick(35% / 27% improvements documented above)≤ 100 msfrom perf(library-select): #205 follow-up — parallelize BFS walker and memoize Pass 1 scans across Pass 2 #236).Out of scope (deferred to follow-up issues)
resolve_cold/scan_throughputregressions (the benches exist; only the workflow wiring is missing).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests