Skip to content

⚡ Bolt: Optimize DependencyGraph traversal algorithms #152

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

Open
bashandbone wants to merge 1 commit into
base: main
Choose a base branch
from
Open

⚡ Bolt: Optimize DependencyGraph traversal algorithms #152

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
49 changes: 26 additions & 23 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,26 +267,28 @@ impl DependencyGraph {
/// assert!(affected.contains(&PathBuf::from("C")));
/// ```
pub fn find_affected_files(&self, changed_files: &RapidSet<PathBuf>) -> RapidSet<PathBuf> {
let mut affected = thread_utilities::get_set();
let mut queue: VecDeque<&PathBuf> = changed_files.iter().collect();
// Optimization: Use `&Path` during graph traversal to avoid `PathBuf` cloning.
// Allocations (`to_path_buf()`) only occur at the end for the affected paths.
let mut affected_refs = thread_utilities::get_set();
let mut queue: VecDeque<&Path> = changed_files.iter().map(|p| p.as_path()).collect();

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

// Follow reverse edges (files that depend on this file)
for edge in self.get_dependents(file) {
if edge.effective_strength() == DependencyStrength::Strong
&& !affected.contains(&edge.from)
&& !affected_refs.contains(edge.from.as_path())
{
queue.push_back(&edge.from);
}
}
}

affected
affected_refs.into_iter().map(|p| p.to_path_buf()).collect()
}

/// Performs topological sort on the given subset of files.
Expand Down Expand Up @@ -336,14 +338,16 @@ impl DependencyGraph {
/// assert!(pos_c < pos_b);
/// assert!(pos_b < pos_a);
/// ```
pub fn topological_sort(&self, files: &RapidSet<PathBuf>) -> Result<Vec<PathBuf>, GraphError> {
let mut sorted = Vec::new();
let mut visited = thread_utilities::get_set();
let mut temp_mark = thread_utilities::get_set();
pub fn topological_sort<'a>(&'a self, files: &'a RapidSet<PathBuf>) -> Result<Vec<PathBuf>, GraphError> {
// Optimization: Pre-allocate `sorted` capacity and use `&Path` sets
// for `visited` and `temp_mark` to avoid intermediate `PathBuf` allocations.
let mut sorted = Vec::with_capacity(files.len());
let mut visited: RapidSet<&'a Path> = thread_utilities::get_set();
let mut temp_mark: RapidSet<&'a Path> = thread_utilities::get_set();
Comment on lines +341 to +346
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

topological_sort now ties &self and files to the same lifetime (pub fn topological_sort<'a>(&'a self, files: &'a RapidSet<PathBuf>)). This makes the function type more restrictive than before (e.g., DependencyGraph::topological_sort can no longer be used where independent lifetimes are required), which is a potential breaking change for downstream callers. Consider keeping the original signature (&self, &RapidSet<PathBuf>) and restructuring the traversal so visited/temp_mark only store references originating from files (e.g., look up edge.to via subset.get(edge.to.as_path()) and recurse on that reference) to avoid needing a shared lifetime in the public API.

Suggested change
pub fn topological_sort<'a>(&'a self, files: &'a RapidSet<PathBuf>) -> Result<Vec<PathBuf>, GraphError> {
// Optimization: Pre-allocate `sorted` capacity and use `&Path` sets
// for `visited` and `temp_mark` to avoid intermediate `PathBuf` allocations.
let mut sorted = Vec::with_capacity(files.len());
let mut visited: RapidSet<&'a Path> = thread_utilities::get_set();
let mut temp_mark: RapidSet<&'a Path> = thread_utilities::get_set();
pub fn topological_sort(&self, files: &RapidSet<PathBuf>) -> Result<Vec<PathBuf>, GraphError> {
// Optimization: Pre-allocate `sorted` capacity and use `&Path` sets
// for `visited` and `temp_mark` to avoid intermediate `PathBuf` allocations.
let mut sorted = Vec::with_capacity(files.len());
let mut visited: RapidSet<&Path> = thread_utilities::get_set();
let mut temp_mark: RapidSet<&Path> = thread_utilities::get_set();

Copilot uses AI. Check for mistakes.

for file in files {
if !visited.contains(file) {
self.visit_node(file, files, &mut visited, &mut temp_mark, &mut sorted)?;
if !visited.contains(file.as_path()) {
self.visit_node(file.as_path(), files, &mut visited, &mut temp_mark, &mut sorted)?;
}
}

Expand Down Expand Up @@ -406,12 +410,12 @@ impl DependencyGraph {
}

/// DFS visit for topological sort with cycle detection.
fn visit_node(
&self,
file: &Path,
subset: &RapidSet<PathBuf>,
visited: &mut RapidSet<PathBuf>,
temp_mark: &mut RapidSet<PathBuf>,
fn visit_node<'a>(
&'a self,
file: &'a Path,
subset: &'a RapidSet<PathBuf>,
visited: &mut RapidSet<&'a Path>,
temp_mark: &mut RapidSet<&'a Path>,
Comment on lines +413 to +418
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

visit_node currently uses a single lifetime parameter for self, file, subset, and the visited/temp_mark sets. This couples traversal bookkeeping lifetimes to both the graph and the subset and is what forces the public topological_sort signature to share a lifetime. If you switch the traversal to always recurse using references obtained from subset (via subset.get(...)), visited/temp_mark can be keyed by subset references and this helper can drop the explicit lifetime plumbing, reducing complexity and avoiding accidental API restrictions.

Copilot uses AI. Check for mistakes.
sorted: &mut Vec<PathBuf>,
) -> Result<(), GraphError> {
if temp_mark.contains(file) {
Expand All @@ -422,19 +426,18 @@ impl DependencyGraph {
return Ok(());
}

let file_buf = file.to_path_buf();
temp_mark.insert(file_buf.clone());
temp_mark.insert(file);

// Visit dependencies (forward edges) that are in our subset
for edge in self.get_dependencies(file) {
if subset.contains(&edge.to) {
self.visit_node(&edge.to, subset, visited, temp_mark, sorted)?;
self.visit_node(edge.to.as_path(), subset, visited, temp_mark, sorted)?;
}
}

temp_mark.remove(file);
visited.insert(file_buf.clone());
sorted.push(file_buf);
visited.insert(file);
sorted.push(file.to_path_buf());

Ok(())
}
Expand Down
Loading