⚡ Bolt: [performance improvement] optimize path allocations in incremental invalidation#218
⚡ Bolt: [performance improvement] optimize path allocations in incremental invalidation#218bashandbone wants to merge 1 commit intomainfrom
Conversation
…ath` instead of allocating `PathBuf` for map lookups Co-authored-by: bashandbone <[email protected]>
|
👋 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 GuideOptimizes incremental invalidation SCC traversal by using borrowed path references instead of heap-allocating PathBufs on every map access, and cleans up unnecessary explicit lifetimes in rule-engine variable checking helpers. Sequence diagram for optimized SCC traversal map lookups in incremental invalidationsequenceDiagram
participant InvalidationDetector
participant TarjanState
participant LowlinksMap as RapidMap_lowlinks
participant IndicesMap as RapidMap_indices
participant OnStackSet as RapidSet_on_stack
participant V as Path_v
participant Dep as Path_dep
InvalidationDetector->>TarjanState: tarjan_visit(V)
loop for_each_dependency_of_v
InvalidationDetector->>TarjanState: next dependency -> Dep
alt dep_not_visited
InvalidationDetector->>TarjanState: recurse tarjan_visit(Dep)
TarjanState->>LowlinksMap: get(Dep)
LowlinksMap-->>TarjanState: w_lowlink
Note over TarjanState,LowlinksMap: Before: get_mut(&v.to_path_buf())
TarjanState->>LowlinksMap: get_mut(V)
LowlinksMap-->>TarjanState: v_lowlink_ref
TarjanState->>TarjanState: *v_lowlink_ref = min(*v_lowlink_ref, w_lowlink)
else dep_on_stack
TarjanState->>OnStackSet: contains(Dep)
OnStackSet-->>TarjanState: true
TarjanState->>IndicesMap: get(Dep)
IndicesMap-->>TarjanState: w_index
Note over TarjanState,LowlinksMap: Before: get_mut(&v.to_path_buf())
TarjanState->>LowlinksMap: get_mut(V)
LowlinksMap-->>TarjanState: v_lowlink_ref
TarjanState->>TarjanState: *v_lowlink_ref = min(*v_lowlink_ref, w_index)
end
end
Note over TarjanState,IndicesMap: Before: get(&v.to_path_buf())
TarjanState->>IndicesMap: get(V)
IndicesMap-->>TarjanState: v_index
Note over TarjanState,LowlinksMap: Before: get(&v.to_path_buf())
TarjanState->>LowlinksMap: get(V)
LowlinksMap-->>TarjanState: v_lowlink
TarjanState->>TarjanState: if v_lowlink == v_index then form_SCC()
TarjanState-->>InvalidationDetector: SCC for V
Class diagram for updated invalidation and rule-engine helper APIsclassDiagram
class InvalidationDetector {
+tarjan_visit(v: &Path, state: &mut TarjanState)
}
class TarjanState {
+indices: RapidMap~Path, usize~
+lowlinks: RapidMap~Path, usize~
+on_stack: RapidSet~Path~
+stack: Vec~Path~
}
InvalidationDetector --> TarjanState: uses
class CheckVarModule {
+get_vars_from_rules(rule: &Rule, utils: &RuleRegistration) RapidSet~String~
+check_var_in_constraints(vars: RapidSet~String~, constraints: &RapidMap~MetaVariableID, Rule~) RResult~RapidSet~String~~
+check_var_in_transform(vars: RapidSet~String~, transform: &Option~Transform~) RResult~RapidSet~String~~
}
class Rule {
+defined_vars() Iterator~String~
+meta_vars() Iterator~String~
}
class RuleRegistration {
+constraints: RapidMap~MetaVariableID, Rule~
+transform: Option~Transform~
}
class RapidMap~K, V~ {
}
class RapidSet~T~ {
}
class Transform {
}
class MetaVariableID {
}
class RResult~T~ {
}
CheckVarModule --> Rule: reads
CheckVarModule --> RuleRegistration: reads
CheckVarModule --> RapidMap: uses
CheckVarModule --> RapidSet: uses
CheckVarModule --> Transform: uses
CheckVarModule --> MetaVariableID: uses
CheckVarModule --> RResult: returns
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 the Tarjan loop you repeatedly call
state.lowlinks.get_mut(v)in both branches; consider factoring this lookup sov_lowlinkis obtained once per iteration (or once perv) to reduce RapidMap lookups in this hot path. - The new
// Bolt: ...comments are quite tool-specific; consider rephrasing them in neutral terms (e.g., just explaining the allocation avoidance) to keep the codebase free of vendor- or tool-branded annotations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the Tarjan loop you repeatedly call `state.lowlinks.get_mut(v)` in both branches; consider factoring this lookup so `v_lowlink` is obtained once per iteration (or once per `v`) to reduce RapidMap lookups in this hot path.
- The new `// Bolt: ...` comments are quite tool-specific; consider rephrasing them in neutral terms (e.g., just explaining the allocation avoidance) to keep the codebase free of vendor- or tool-branded annotations.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 reduces heap allocations during incremental invalidation’s Tarjan SCC traversal by switching RapidMap lookups from &v.to_path_buf() (allocating) to borrowed &Path lookups, leveraging PathBuf: Borrow<Path> support in the underlying hash map. It also simplifies a couple of helper function signatures in the rule-engine to remove unnecessary explicit lifetimes.
Changes:
- Avoids per-edge
PathBufallocations in Tarjan’s algorithm by using borrowed&PathforRapidMap::get(_mut)queries. - Removes explicit lifetimes from two internal helper functions in
check_var.rs(constraints/transform checks).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/rule-engine/src/check_var.rs | Removes unnecessary explicit lifetimes in helper function signatures. |
| crates/flow/src/incremental/invalidation.rs | Replaces allocating to_path_buf() map lookups with borrowed &Path lookups in Tarjan SCC traversal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
💡 What: Removed
&v.to_path_buf()calls incrates/flow/src/incremental/invalidation.rsmap queries (get_mutandget), using borrowedvinstead. Cleaned up explicit lifetime lint warnings incheck_var.rsalong the way.🎯 Why:
&v.to_path_buf()allocates a new string-backed Path on the heap. Doing this inside Tarjan's strongly-connected-components inner loop (graph edges) resulted inO(E)memory churn per graph connected component calculation. Borrowing allows us to look up references instantly in RapidMap without allocations.📊 Impact: Faster invalidation propagation algorithms with lower peak memory footprint on deeply-connected component graphs.
🔬 Measurement: Verified via memory correctness traces (
test_get.rschecks) andcargo test -p thread-flow --test invalidation_tests. All SCC, topological, and tree validation checks passed successfully.PR created automatically by Jules for task 968163309968730354 started by @bashandbone
Summary by Sourcery
Optimize incremental invalidation graph traversal to reduce path allocation overhead and clean up lifetime usage in variable checking utilities.
Enhancements: