⚡ Bolt: [performance improvement] optimize file path map lookups in invalidation detector#154
Conversation
…nvalidation detector 💡 What: Used borrowed `&Path` reference instead of owned `PathBuf` for HashMap existence and mutability checks in `tarjan_dfs`. 🎯 Why: Avoid O(E) unnecessary heap memory allocations of `PathBuf` within tight nested loops traversing nodes and edges of the invalidation graph. 📊 Impact: Considerably reduces memory pressure and allocation overhead during calculation of the increment invalidation sets. 🔬 Measurement: Verify using `cargo test -p thread-flow --test invalidation_tests`. 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 GuideOptimizes Tarjan DFS invalidation detection by switching HashMap lookups from owned PathBuf to borrowed &Path, and performs a few minor API and formatting cleanups across rule- and AST-related modules. 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:
- In
tarjan_dfs, you still perform multipleindices/lowlinkslookups for the samev(e.g., when computingv_indexandv_lowlinkat the end); consider caching these values or holding a single mutable reference per iteration to further cut down hashmap accesses and potential contention.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `tarjan_dfs`, you still perform multiple `indices`/`lowlinks` lookups for the same `v` (e.g., when computing `v_index` and `v_lowlink` at the end); consider caching these values or holding a single mutable reference per iteration to further cut down hashmap accesses and potential contention.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
Optimizes Tarjan SCC traversal in the incremental invalidation detector by avoiding repeated PathBuf allocations during map lookups, alongside a few small refactors/formatting cleanups.
Changes:
- Use borrowed
&Pathforindices/lowlinksget/get_mutlookups intarjan_dfsto avoid per-edgePathBufallocations. - Remove unnecessary explicit lifetimes in internal variable-check helper functions.
- Minor formatting-only refactors in a few Rust modules/tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/flow/src/incremental/invalidation.rs | Avoids v.to_path_buf() allocations on tight-loop map lookups in Tarjan DFS. |
| crates/rule-engine/src/check_var.rs | Simplifies helper fn signatures by removing redundant lifetimes. |
| crates/rule-engine/src/rule/referent_rule.rs | Collapses a read/clone chain into a single line (no behavior change). |
| crates/rule-engine/src/rule/mod.rs | Reformats defined_vars mapping for readability (no behavior change). |
| crates/ast-engine/src/tree_sitter/mod.rs | Minor formatting refactor and rewraps an assertion for readability (no behavior change). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
💡 What: Used borrowed
&Pathreference instead of ownedPathBuffor HashMap existence and mutability checks intarjan_dfs.🎯 Why: Avoid O(E) unnecessary heap memory allocations of
PathBufwithin tight nested loops traversing nodes and edges of the invalidation graph.📊 Impact: Considerably reduces memory pressure and allocation overhead during calculation of the increment invalidation sets.
🔬 Measurement: Verify using
cargo test -p thread-flow --test invalidation_tests.PR created automatically by Jules for task 16106423400975348129 started by @bashandbone
Summary by Sourcery
Optimize invalidation graph traversal to reduce allocation overhead and perform minor API and formatting cleanups.
Enhancements: