⚡ Bolt: [performance improvement] Defer PathBuf allocations during Tarjan's SCC DFS#265
⚡ Bolt: [performance improvement] Defer PathBuf allocations during Tarjan's SCC DFS#265bashandbone wants to merge 1 commit into
Conversation
By passing borrowed `&Path` values to `.get()`, `.get_mut()`, and `.contains()` queries on `RapidMap` and `RapidSet`, we avoid triggering `v.to_path_buf()` multiple times for each node dependency during Tarjan's strongly connected components algorithm. This saves O(E) heap allocations during invalidation graph traversals. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 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. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideDefers repeated PathBuf allocations in Tarjan's SCC DFS by reusing a single PathBuf per node and switching map lookups from owned PathBuf keys to borrowed Path keys, reducing heap allocations during graph traversal. Sequence diagram for tarjan_dfs PathBuf allocation reusesequenceDiagram
participant Caller
participant InvalidationDetector
participant TarjanState
Caller->>InvalidationDetector: tarjan_dfs(v, state, sccs)
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)
loop for each dependency
InvalidationDetector->>TarjanState: lowlinks.get(v)
InvalidationDetector->>TarjanState: indices.get(v)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- You can avoid one of the
PathBufclones during initialization by reusing the ownedv_bufwhen inserting intolowlinks(e.g.,indices.insert(v_buf.clone(), index); lowlinks.insert(v_buf, index);) and only cloning again when pushing tostack/on_stackif needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You can avoid one of the `PathBuf` clones during initialization by reusing the owned `v_buf` when inserting into `lowlinks` (e.g., `indices.insert(v_buf.clone(), index); lowlinks.insert(v_buf, index);`) and only cloning again when pushing to `stack`/`on_stack` if needed.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR improves performance in the incremental invalidation path by reducing repeated PathBuf heap allocations inside Tarjan’s strongly connected components DFS (tarjan_dfs) while traversing the dependency graph.
Changes:
- Allocate
PathBuffor the current node (v) once per DFS call and reuse it for inserts/pushes. - Replace repeated
&v.to_path_buf()lookups with borrowed&Pathlookups intoRapidMap<PathBuf, _>(and similar structures), avoiding per-lookup allocations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
💡 What: Defer
PathBufheap allocations during Tarjan's strongly connected components algorithm (tarjan_dfs).🎯 Why:
v.to_path_buf()was called multiple times per node and during cycle checking for dependencies, causing O(E) unnecessary heap allocations during graph traversal.📊 Impact: Significantly reduces memory allocations and churn in the invalidation path, making dependency graph processing much faster for large projects.
🔬 Measurement: Observe reduction in memory allocation during tests like
cargo test -p thread-flow --test invalidation_tests.PR created automatically by Jules for task 8749202397797120216 started by @bashandbone
Summary by Sourcery
Enhancements: