⚡ Bolt: [Performance: Defer Allocation during Traversal]#190
Open
bashandbone wants to merge 1 commit intomainfrom
Open
⚡ Bolt: [Performance: Defer Allocation during Traversal]#190bashandbone wants to merge 1 commit intomainfrom
bashandbone wants to merge 1 commit intomainfrom
Conversation
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 DAG traversal and dependency graph operations by reducing PathBuf allocations, updates rule-engine APIs to use simpler reference lifetimes, and includes minor formatting and Clippy cleanups across several modules. Class diagram for incremental graph and Tarjan traversal changesclassDiagram
class DependencyGraph {
+RapidMap~PathBuf, AnalysisDefFingerprint~ nodes
+fn ensure_node(file: &Path)
+fn get_dependencies(file: &Path) Vec~PathBuf~
}
class InvalidationDetector {
+DependencyGraph graph
+fn tarjan_dfs(v: &Path, state: &mut TarjanState, sccs: &mut Vec~Vec~PathBuf~~)
}
class TarjanState {
+RapidMap~PathBuf, usize~ indices
+RapidMap~PathBuf, usize~ lowlinks
+Vec~PathBuf~ stack
+RapidSet~PathBuf~ on_stack
+usize index_counter
}
DependencyGraph --> InvalidationDetector : used_by
InvalidationDetector --> TarjanState : mutates
InvalidationDetector --> DependencyGraph : queries_dependencies
%% Highlighted behavioral changes
Class diagram for rule engine variable checking and registration changesclassDiagram
class Rule {
+fn defined_vars() RapidSet~String~
}
class RuleRegistration {
+RapidMap~String, Rule~ rules
}
class Registration_R_ {
+Arc~RwLock~RapidMap~String, R~~~ inner
+fn read() Arc~RapidMap~String, R~~
+fn contains_key(key: &str) bool
}
class CheckHint_r_ {
<<enum>>
}
class CheckVarAPI {
+fn check_rule_with_hint_r_(rule: &Rule, utils: &RuleRegistration, constraints: &RapidMap~MetaVariableID, Rule~, transform: &Option~Transform~, fixer: &Vec~Fixer~, hint: CheckHint_r_) RResult~()~
+fn check_vars_in_rewriter_r_(rule: &Rule, utils: &RuleRegistration, constraints: &RapidMap~MetaVariableID, Rule~, transform: &Option~Transform~, fixer: &Vec~Fixer~, upper_var: &RapidSet~String~) RResult~()~
+fn check_vars_r_(rule: &Rule, utils: &RuleRegistration, constraints: &RapidMap~MetaVariableID, Rule~, transform: &Option~Transform~, fixer: &Vec~Fixer~) RResult~()~
+fn check_var_in_constraints(vars: RapidSet~String~, constraints: &RapidMap~MetaVariableID, Rule~) RResult~RapidSet~String~~
+fn check_var_in_transform(vars: RapidSet~String~, transform: &Option~Transform~) RResult~RapidSet~String~~
}
RuleRegistration --> Rule : contains
Registration_R_ --> Rule : value_type_R
CheckVarAPI --> Rule : analyzes
CheckVarAPI --> RuleRegistration : uses
CheckVarAPI --> Registration_R_ : uses_constraints
CheckVarAPI --> CheckHint_r_ : parameter
Rule --> CheckVarAPI : provides_defined_vars
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
DependencyGraph::ensure_node, the change fromentry(...).or_insert_with(...)to a separatecontains_keycheck followed byinsertintroduces an extra map lookup; you can keep usingentryto avoid the double lookup while still deferringPathBufallocation by only callingfile.to_path_buf()inside theor_insert_withclosure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DependencyGraph::ensure_node`, the change from `entry(...).or_insert_with(...)` to a separate `contains_key` check followed by `insert` introduces an extra map lookup; you can keep using `entry` to avoid the double lookup while still deferring `PathBuf` allocation by only calling `file.to_path_buf()` inside the `or_insert_with` closure.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 allocation overhead in performance-sensitive graph traversal and node creation paths by leveraging borrowed &Path lookups and minimizing repeated PathBuf conversions, alongside small refactors to address clippy lifetime warnings and formatting.
Changes:
- Update Tarjan SCC DFS to avoid
v.to_path_buf()allocations during map/set lookups by using borrowed&Path. - Optimize
ensure_nodeto skipPathBufallocation when the node already exists. - Fix lifetime/clippy issues in
check_var.rsand apply minor formatting cleanups.
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 | Minor refactor/formatting for RwLock read+clone path. |
| crates/rule-engine/src/rule/mod.rs | Formatting-only change to defined_vars() for Rule::Pattern. |
| crates/rule-engine/src/check_var.rs | Remove unnecessary explicit lifetimes on some parameters to satisfy clippy. |
| crates/flow/src/incremental/invalidation.rs | Use borrowed &Path for Tarjan state lookups to avoid repeated PathBuf allocations. |
| crates/flow/src/incremental/graph.rs | Avoid allocating PathBuf in ensure_node when a node is already present. |
| crates/ast-engine/src/tree_sitter/mod.rs | Formatting-only adjustments in UTF-8 recovery and a test assertion. |
💡 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: Optimized performance-critical DAG traversals and map lookups by avoiding unconditional `PathBuf` allocations. In `tarjan_dfs`, the `&Path` is now used to perform lookups against `RapidSet` and `RapidMap`, and multiple clones of `PathBuf` are minimized into single instances. In `ensure_node`, a simple `contains_key` avoids allocating when nodes already exist. Also fixed lifetime clippy errors in `check_var.rs`.
🎯 Why: Creating `PathBuf` triggers heap allocations which represent a major O(E) or O(V) overhead during traversal paths of the `thread-flow` execution.
📊 Impact: Reduces memory churn substantially during topological sorting and graph dependency building.
🔬 Measurement: Verify memory utilization drops when processing large dependency graphs. Tested via `cargo test -p thread-flow`.
PR created automatically by Jules for task 16640234953718659717 started by @bashandbone
Summary by Sourcery
Optimize graph traversal and dependency management to reduce unnecessary allocations and clean up rule-engine lifetimes and formatting.
Enhancements: