Skip to content

Commit 6d4c0f7

Browse files
zackeesclaude
andauthored
perf(library-select): #236 parallel BFS walker + Pass1/Pass2 scan memoization + tracing spans (#237)
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>
1 parent dd134d2 commit 6d4c0f7

9 files changed

Lines changed: 358 additions & 39 deletions

File tree

Cargo.lock

Lines changed: 24 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ rusqlite = { version = "0.31", features = ["bundled"] }
6666
shell-words = "1"
6767
bincode = "1"
6868
zccache-artifact = "1.4.0"
69+
rayon = "1"
70+
tracing-test = "0.2"
6971

7072
# Process containment: all subprocess spawns the daemon performs (compilers,
7173
# esptool, qemu, simavr, node, npm, …) and any grandchildren they fork must

crates/fbuild-header-scan/Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ rust-version.workspace = true
77
license.workspace = true
88

99
[dependencies]
10+
rayon = { workspace = true }
11+
tracing = { workspace = true }
1012

1113
[dev-dependencies]
1214
criterion = { workspace = true }

crates/fbuild-header-scan/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ mod scanner;
1010
mod walker;
1111

1212
pub use scanner::{scan, IncludeKind, IncludeRef, Span};
13-
pub use walker::{walk, WalkResult};
13+
pub use walker::{walk, walk_with_state, WalkResult, WalkState};
1414

1515
/// Bumped whenever the scanner output shape changes. Mixed into cache keys so a
1616
/// scanner change invalidates memoized library-selection results.

crates/fbuild-header-scan/src/walker.rs

Lines changed: 133 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,56 +5,169 @@
55
//! plus the set of include strings that could not be resolved. The walker is
66
//! BFS over a visited set so cycles, diamonds, and arbitrary depth all
77
//! terminate correctly.
8+
//!
9+
//! Two public entry points:
10+
//! * [`walk`] -- one-shot convenience wrapper that allocates a fresh
11+
//! [`WalkState`] internally. `WalkResult::reached` is the full set of files
12+
//! reached from `seeds`.
13+
//! * [`walk_with_state`] -- accepts a caller-owned [`WalkState`] so multiple
14+
//! walks can share a scan cache and a `visited` set across calls (used by
15+
//! `fbuild-library-select` to avoid re-reading files between LDF passes).
16+
//! `WalkResult::reached` is the *delta* of canonical paths newly discovered
17+
//! in this call; the union of deltas across calls equals the full set.
818
9-
use std::collections::{BTreeSet, HashSet, VecDeque};
19+
use std::collections::{BTreeSet, HashMap, HashSet, VecDeque};
1020
use std::path::{Path, PathBuf};
1121

22+
use rayon::prelude::*;
23+
1224
use crate::scanner::{scan, IncludeKind, IncludeRef};
1325

1426
/// Result of a walk. `reached` and `unresolved` are sorted for deterministic
1527
/// cache keys.
28+
///
29+
/// For [`walk`] (fresh-state wrapper) `reached` is the full set of files
30+
/// transitively reached from the seeds. For [`walk_with_state`] the same
31+
/// fields contain only the *delta* added in this call -- files already
32+
/// present in the shared `WalkState::visited` set are not re-emitted.
1633
#[derive(Debug, Clone, Default, PartialEq, Eq)]
1734
pub struct WalkResult {
1835
pub reached: Vec<PathBuf>,
1936
pub unresolved: Vec<String>,
2037
}
2138

39+
/// State that can be shared across multiple [`walk_with_state`] calls so the
40+
/// include-scan results are memoized and each on-disk file is read at most
41+
/// once for the lifetime of the state.
42+
///
43+
/// Used by `fbuild-library-select::resolve_with_stats` to share scan results
44+
/// across LDF passes -- pass 1 reads every file once, pass 2 re-seeds with
45+
/// library `.cpp` files but reuses the cached scans for everything already
46+
/// reached.
47+
#[derive(Debug, Default)]
48+
pub struct WalkState {
49+
/// Canonical paths the walker has already enqueued/visited.
50+
visited: HashSet<PathBuf>,
51+
/// Canonical path -> parsed include list. Populated lazily on first read.
52+
/// Missing entries mean either "not yet read" or "read failed" -- they are
53+
/// indistinguishable here, matching the existing `let Ok(...) else
54+
/// { continue }` semantics of the original walker.
55+
scan_cache: HashMap<PathBuf, Vec<IncludeRef>>,
56+
/// Number of successful `std::fs::read_to_string` invocations across the
57+
/// lifetime of this state. Each unique file is counted exactly once
58+
/// because subsequent walks hit `scan_cache` instead.
59+
files_read: usize,
60+
}
61+
62+
impl WalkState {
63+
/// Create an empty state. No files have been scanned, nothing is visited.
64+
pub fn new() -> Self {
65+
Self::default()
66+
}
67+
68+
/// Number of files physically read from disk so far. Used by
69+
/// `resolve_with_stats` to assert the no-re-read contract in tests.
70+
pub fn files_read(&self) -> usize {
71+
self.files_read
72+
}
73+
}
74+
2275
/// Walk the include graph starting from `seeds` over `search_paths`.
2376
///
2477
/// `search_paths` is consulted in order for `<...>` includes and as a
2578
/// secondary lookup for `"..."` includes (after the same-directory check).
2679
/// A file is added to `reached` exactly once. Files outside `search_paths`
2780
/// are still reached if they are seeds or `"..."`-resolved relative to a
2881
/// seed/visited file.
82+
///
83+
/// Allocates a fresh [`WalkState`] internally, so `WalkResult::reached`
84+
/// contains every file transitively reached from `seeds`.
2985
pub fn walk(seeds: &[PathBuf], search_paths: &[PathBuf]) -> WalkResult {
86+
let mut state = WalkState::new();
87+
walk_with_state(seeds, search_paths, &mut state)
88+
}
89+
90+
/// Walk the include graph using a caller-owned [`WalkState`] so the scan cache
91+
/// and visited set persist across calls.
92+
///
93+
/// `WalkResult::reached` contains only the *delta* of canonical paths newly
94+
/// reached in this call. Files already in `state.visited` from a previous
95+
/// call are not re-emitted (and not re-read).
96+
///
97+
/// The BFS proceeds in waves: each wave reads all not-yet-cached files in
98+
/// parallel via rayon, then resolves every `#include` in every cached scan
99+
/// result to enqueue the next wave.
100+
#[tracing::instrument(
101+
name = "ldf_walk",
102+
skip_all,
103+
fields(seeds = seeds.len(), search_paths = search_paths.len())
104+
)]
105+
pub fn walk_with_state(
106+
seeds: &[PathBuf],
107+
search_paths: &[PathBuf],
108+
state: &mut WalkState,
109+
) -> WalkResult {
110+
tracing::debug!(
111+
seeds = seeds.len(),
112+
search_paths = search_paths.len(),
113+
"ldf_walk"
114+
);
30115
let mut reached: BTreeSet<PathBuf> = BTreeSet::new();
31116
let mut unresolved: BTreeSet<String> = BTreeSet::new();
32-
let mut visited: HashSet<PathBuf> = HashSet::new();
33-
let mut queue: VecDeque<PathBuf> = VecDeque::new();
117+
let mut frontier: VecDeque<PathBuf> = VecDeque::new();
34118

35119
for seed in seeds {
36120
let canon = canon(seed);
37-
if visited.insert(canon.clone()) {
38-
queue.push_back(canon.clone());
121+
if state.visited.insert(canon.clone()) {
122+
frontier.push_back(canon.clone());
39123
reached.insert(canon);
40124
}
41125
}
42126

43-
while let Some(file) = queue.pop_front() {
44-
let Ok(text) = std::fs::read_to_string(&file) else {
45-
continue;
46-
};
47-
for inc in scan(&text) {
48-
match resolve(&inc, &file, search_paths) {
49-
Some(resolved) => {
50-
let canon = canon(&resolved);
51-
if visited.insert(canon.clone()) {
52-
reached.insert(canon.clone());
53-
queue.push_back(canon);
127+
while !frontier.is_empty() {
128+
// Read all not-yet-cached files in the current wave in parallel.
129+
let to_read: Vec<PathBuf> = frontier
130+
.iter()
131+
.filter(|p| !state.scan_cache.contains_key(*p))
132+
.cloned()
133+
.collect();
134+
135+
if !to_read.is_empty() {
136+
let scanned: Vec<(PathBuf, Vec<IncludeRef>)> = to_read
137+
.par_iter()
138+
.filter_map(|p| {
139+
let text = std::fs::read_to_string(p).ok()?;
140+
Some((p.clone(), scan(&text)))
141+
})
142+
.collect();
143+
144+
for (path, includes) in scanned {
145+
state.scan_cache.insert(path, includes);
146+
state.files_read += 1;
147+
}
148+
}
149+
150+
// Resolve includes for every file in the frontier and build the next
151+
// wave from any newly discovered canonical paths.
152+
let current: Vec<PathBuf> = frontier.drain(..).collect();
153+
for file in &current {
154+
let Some(includes) = state.scan_cache.get(file).cloned() else {
155+
// Read failed (file is a directory, permission denied, etc.).
156+
// Match the existing behavior: silently skip.
157+
continue;
158+
};
159+
for inc in &includes {
160+
match resolve_include(inc, file, search_paths) {
161+
Some(resolved) => {
162+
let canon = canon(&resolved);
163+
if state.visited.insert(canon.clone()) {
164+
reached.insert(canon.clone());
165+
frontier.push_back(canon);
166+
}
167+
}
168+
None => {
169+
unresolved.insert(inc.path.clone());
54170
}
55-
}
56-
None => {
57-
unresolved.insert(inc.path.clone());
58171
}
59172
}
60173
}
@@ -66,7 +179,7 @@ pub fn walk(seeds: &[PathBuf], search_paths: &[PathBuf]) -> WalkResult {
66179
}
67180
}
68181

69-
fn resolve(inc: &IncludeRef, from: &Path, search_paths: &[PathBuf]) -> Option<PathBuf> {
182+
fn resolve_include(inc: &IncludeRef, from: &Path, search_paths: &[PathBuf]) -> Option<PathBuf> {
70183
if inc.kind == IncludeKind::Quoted {
71184
if let Some(parent) = from.parent() {
72185
let candidate = parent.join(&inc.path);

crates/fbuild-library-select/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ zccache-artifact = { workspace = true }
2020
tempfile = { workspace = true }
2121
criterion = { workspace = true }
2222
fbuild-test-support = { path = "../fbuild-test-support" }
23+
tracing-test = { workspace = true, features = ["no-env-filter"] }
2324

2425
[[bench]]
2526
name = "resolve_cold"

0 commit comments

Comments
 (0)