Skip to content

⚡ Optimize PathBuf clones in graph traversal #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bashandbone merged 4 commits into from
Mar 26, 2026
Merged

⚡ Optimize PathBuf clones in graph traversal #133

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4960a4d
perf(flow): optimize PathBuf clones in graph traversal
google-labs-jules[bot] Mar 23, 2026
aaae491
Update crates/flow/src/incremental/graph.rs
bashandbone Mar 26, 2026
a91cd43
perf(flow): skip enqueuing already-visited dependents to reduce queue…
Copilot Mar 26, 2026
1d814e8
Merge branch 'main' into optimize-graph-traversal-clones-141103825911…
bashandbone Mar 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions crates/flow/src/incremental/graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,15 +267,19 @@ impl DependencyGraph {
/// assert!(affected.contains(&PathBuf::from("C")));
/// ```
pub fn find_affected_files(&self, changed_files: &RapidSet<PathBuf>) -> RapidSet<PathBuf> {
let mut affected: RapidSet<PathBuf> = set_with_capacity(changed_files.len());
affected.extend(changed_files.iter().cloned());
let mut affected = thread_utilities::get_set();
let mut queue: VecDeque<&PathBuf> = changed_files.iter().collect();

while let Some(file) = queue.pop_front() {
if affected.contains(file) {
continue;
}
affected.insert(file.clone());

// Follow reverse edges (files that depend on this file)
for edge in self.get_dependents(file) {
if edge.effective_strength() == DependencyStrength::Strong
&& affected.insert(edge.from.clone())
&& !affected.contains(&edge.from)
{
queue.push_back(&edge.from);
}
Comment on lines +280 to 285
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependents are enqueued unconditionally (queue.push_back(&edge.from)), so the same path can be pushed many times in fan-in/diamond graphs, causing extra queue churn and (due to the clone-on-pop) extra allocations. To keep the traversal closer to “one clone per unique affected node”, skip enqueuing when affected already contains edge.from (or maintain a separate lightweight visited check before pushing).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@bashandbone bashandbone Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot apply changes based on this feedback

Copy link
Copy Markdown
Contributor

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a91cd43. Dependents are now only enqueued when affected doesn't already contain them, eliminating duplicate queue entries in fan-in/diamond graphs. The guard is applied consistently alongside the existing contains check at dequeue time.

Expand Down
Loading