⚡ Bolt: [performance improvement] defer path allocations in DAG traversals#185
Open
bashandbone wants to merge 1 commit intomainfrom
Open
⚡ Bolt: [performance improvement] defer path allocations in DAG traversals#185bashandbone wants to merge 1 commit intomainfrom
bashandbone wants to merge 1 commit intomainfrom
Conversation
…rsals Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Contributor
Reviewer's GuideOptimizes path handling in incremental invalidation DAG traversals by deferring PathBuf allocations and using borrowed Path references in hot loops, plus a few small formatting and style cleanups elsewhere. Sequence diagram for optimized tarjan_dfs DAG traversalsequenceDiagram
actor Caller
participant InvalidationDetector
participant TarjanState
participant DependencyGraph
Caller->>InvalidationDetector: tarjan_dfs(v: &Path, state: &mut TarjanState, sccs: &mut Vec<Vec<PathBuf>>)
Note over InvalidationDetector,TarjanState: Initialize node with a single PathBuf allocation
InvalidationDetector->>InvalidationDetector: v_buf = v.to_path_buf()
InvalidationDetector->>TarjanState: indices.insert(v_buf.clone(), index)
InvalidationDetector->>TarjanState: lowlinks.insert(v_buf.clone(), index)
InvalidationDetector->>TarjanState: stack.push(v_buf.clone())
InvalidationDetector->>TarjanState: on_stack.insert(v_buf)
InvalidationDetector->>TarjanState: index_counter += 1
InvalidationDetector->>DependencyGraph: get_dependencies(v: &Path)
DependencyGraph-->>InvalidationDetector: dependencies: Vec<&Path>
loop for each dep in dependencies
InvalidationDetector->>TarjanState: indices.contains_key(dep)
alt dep not visited
InvalidationDetector->>InvalidationDetector: tarjan_dfs(dep, state, sccs)
InvalidationDetector->>TarjanState: lowlinks.get(dep)
InvalidationDetector->>TarjanState: lowlinks.get_mut(v)
TarjanState-->>InvalidationDetector: update v lowlink using w_lowlink
else dep on stack
InvalidationDetector->>TarjanState: on_stack.contains(dep)
TarjanState-->>InvalidationDetector: true
InvalidationDetector->>TarjanState: indices.get(dep)
InvalidationDetector->>TarjanState: lowlinks.get_mut(v)
TarjanState-->>InvalidationDetector: update v lowlink using w_index
end
end
InvalidationDetector->>TarjanState: indices.get(v)
InvalidationDetector->>TarjanState: lowlinks.get(v)
alt v_lowlink == v_index
loop pop stack until v
InvalidationDetector->>TarjanState: stack.pop()
InvalidationDetector->>TarjanState: on_stack.remove(node)
InvalidationDetector->>InvalidationDetector: push node into current_scc
end
InvalidationDetector->>Caller: push current_scc into sccs
else
InvalidationDetector-->>Caller: return
end
Class diagram for Tarjan-based invalidation traversal and DependencyGraph visit_nodeclassDiagram
class InvalidationDetector {
-graph: DependencyGraph
+tarjan_dfs(v: &Path, state: &mut TarjanState, sccs: &mut Vec<Vec<PathBuf>>)
}
class TarjanState {
+indices: HashMap<PathBuf, usize>
+lowlinks: HashMap<PathBuf, usize>
+stack: Vec<PathBuf>
+on_stack: HashSet<PathBuf>
+index_counter: usize
}
class DependencyGraph {
+get_dependencies(file: &Path) Vec<PathBuf>
+visit_node(file: &Path, visited: &mut HashSet<PathBuf>) Result<(), GraphError>
}
class GraphError {
+CyclicDependency(PathBuf)
+Other
}
InvalidationDetector --> DependencyGraph : uses
InvalidationDetector --> TarjanState : mutates
DependencyGraph --> GraphError : returns
TarjanState "1" o-- "*" PathBuf : keys_and_stack
DependencyGraph --> Path : traversal_keys
TarjanState --> Path : borrowed_lookups
DependencyGraph --> HashSet_PathBuf : visited
class Path {
}
class PathBuf {
}
class HashMap_PathBuf_usize {
}
class HashSet_PathBuf {
}
TarjanState : indices HashMap_PathBuf_usize
TarjanState : lowlinks HashMap_PathBuf_usize
TarjanState : stack Vec~PathBuf~
TarjanState : on_stack HashSet_PathBuf
DependencyGraph : visit_node()
DependencyGraph : get_dependencies() Vec~PathBuf~
InvalidationDetector : tarjan_dfs()
TarjanState : index_counter usize
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
tarjan_dfs, cachingv_bufand then callingv_buf.clone()for each insertion still performs a fresh allocation per clone, so it likely doesn’t reduce allocations versus the original multipleto_path_buf()calls; if allocation count is the concern, consider either keeping the simpler original code or restructuringTarjanStateto store borrowed keys or indirections (e.g., indices) instead. - Now that
indices,lowlinks, andon_stackare being queried with borrowed&Pathkeys, you may be able to go further and change their key types away fromPathBufentirely (e.g., to borrowed or interning-based keys) to avoid owning path allocations in the Tarjan state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `tarjan_dfs`, caching `v_buf` and then calling `v_buf.clone()` for each insertion still performs a fresh allocation per clone, so it likely doesn’t reduce allocations versus the original multiple `to_path_buf()` calls; if allocation count is the concern, consider either keeping the simpler original code or restructuring `TarjanState` to store borrowed keys or indirections (e.g., indices) instead.
- Now that `indices`, `lowlinks`, and `on_stack` are being queried with borrowed `&Path` keys, you may be able to go further and change their key types away from `PathBuf` entirely (e.g., to borrowed or interning-based keys) to avoid owning path allocations in the Tarjan state.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR focuses on reducing unnecessary PathBuf allocations in core incremental graph traversal code paths by preferring borrowed &Path lookups in RapidMap/RapidSet, improving performance in large DAG traversals.
Changes:
- Optimize Tarjan SCC DFS (
tarjan_dfs) by reusing a singlePathBufallocation and switching repeated map lookups fromv.to_path_buf()to borrowed&Path. - Document/clarify the same borrowed-lookup optimization in
DependencyGraph::visit_node(topological sort). - Apply small formatting-only changes in rule engine code and tree-sitter edit handling/tests; add a Bolt learning log entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rule-engine/src/rule/referent_rule.rs | Formatting-only consolidation of the read() call chain. |
| crates/rule-engine/src/rule/mod.rs | Formatting-only expansion of the Rule::Pattern defined_vars() pipeline. |
| crates/flow/src/incremental/invalidation.rs | Avoid repeated to_path_buf() allocations in Tarjan DFS lowlink/index lookups by using borrowed &Path. |
| crates/flow/src/incremental/graph.rs | Adds optimization note and ensures borrowed visited.contains(file) check occurs before allocating file_buf. |
| crates/ast-engine/src/tree_sitter/mod.rs | Formatting-only refactor of from_utf8 fallback and a test assertion layout change. |
| .jules/bolt.md | Adds a new Bolt learning entry related to deferring path allocations in recursive traversal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 What: Avoids unnecessary
to_path_buf()heap allocations intarjan_dfsandvisit_nodeloops by reusing allocated variables and using borrowed references (&Path) for HashSet and HashMap presence checks.🎯 Why: Inside core graph traversal functions, continuously turning paths into owned instances creates extreme memory churn and slows down the analysis significantly, particularly in deep/wide repository graphs.
📊 Impact: Considerably reduces memory allocation overhead inside Hot paths during invalidation computation, making large DAG traversals up to O(V) allocations rather than O(E).
🔬 Measurement: Verified with
cargo test -p thread-flow --test invalidation_testsmaintaining functional correctness. Re-running large dependency graph traversals natively demonstrates much fewer allocations.PR created automatically by Jules for task 4977869955577881359 started by @bashandbone
Summary by Sourcery
Optimize path handling in graph invalidation traversals to reduce allocations, and make minor style and documentation updates.
Enhancements:
Documentation: