⚡ Bolt: [performance improvement] Reduce heap allocations in Tarjan's SCC algorithm#272
⚡ Bolt: [performance improvement] Reduce heap allocations in Tarjan's SCC algorithm#272bashandbone wants to merge 1 commit into
Conversation
… SCC algorithm 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 GuideRefactors Tarjan's SCC DFS implementation to reuse a single PathBuf per node visit and rely on borrowed map lookups, reducing redundant heap allocations and improving performance in large graph traversals. 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}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- You still create multiple clones of
v_buf(forindices,lowlinks,stack, andon_stack); if this path is truly hot it may be worth refactoringTarjanStateto key by something cheaper thanPathBuf(e.g., an interned ID or index) so you can avoid repeated allocations/clones entirely. - Now that
indices/lowlinkslookups use borrowed&Path, consider auditing the rest of the SCC-related code to consistently use borrowed lookups where possible so you don’t regress toto_path_buf()in other paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You still create multiple clones of `v_buf` (for `indices`, `lowlinks`, `stack`, and `on_stack`); if this path is truly hot it may be worth refactoring `TarjanState` to key by something cheaper than `PathBuf` (e.g., an interned ID or index) so you can avoid repeated allocations/clones entirely.
- Now that `indices`/`lowlinks` lookups use borrowed `&Path`, consider auditing the rest of the SCC-related code to consistently use borrowed lookups where possible so you don’t regress to `to_path_buf()` in other paths.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 incremental invalidation by reducing redundant PathBuf construction during Tarjan SCC traversal.
Changes:
- Reuses a local
PathBufduring node initialization intarjan_dfs. - Switches repeated map lookups from temporary
PathBufkeys to borrowed&Pathlookups.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
💡 What: Extracted multiple
v.to_path_buf()calls into a single allocatedPathBufand leveraged borrowed lookups (.get(v)) where possible intarjan_dfs.🎯 Why: To remove redundant O(E) heap allocations in performance-critical dependency graph traversals.
📊 Impact: Substantially reduces memory allocation churn, preventing degraded performance on very large graphs.
🔬 Measurement: Run the
thread-flowbenchmarks (cargo bench -p thread-flow) to observe graph traversal speedup.PR created automatically by Jules for task 13477554990645438749 started by @bashandbone
Summary by Sourcery
Enhancements: