⚡ Bolt: [performance improvement] optimize pathbuf allocations in tarjan SCC algorithm#188
Conversation
Eliminates O(E) unnecessary heap allocations during graph traversal by utilizing borrowed `&Path` references for `HashMap` / `HashSet` lookups (`get`, `get_mut`, `contains`) instead of explicitly creating owned `PathBuf` via `v.to_path_buf()`. Also resolves a few clippy explicit lifetime warnings in `check_var.rs`. 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 GuideOptimizes Tarjan SCC traversal by eliminating repeated PathBuf allocations in map/set lookups and simplifies some rule-engine APIs by removing unnecessary lifetimes, plus updates the Bolt playbook documentation with the new performance lesson. Class diagram for optimized TarjanState SCC path lookupsclassDiagram
class InvalidationDetector {
+tarjan_dfs(v: &Path, state: &mut TarjanState)
}
class TarjanState {
+indices: HashMap~PathBuf, usize~
+lowlinks: HashMap~PathBuf, usize~
+on_stack: HashSet~PathBuf~
+stack: Vec~PathBuf~
+get_index(v: &Path) usize
+get_lowlink(v: &Path) usize
+get_lowlink_mut(v: &Path) &mut usize
+is_on_stack(v: &Path) bool
}
InvalidationDetector --> TarjanState : uses
class Path {
}
class PathBuf {
}
TarjanState ..> Path : borrowed_lookup_keys
TarjanState ..> PathBuf : owned_storage_keys
Class diagram for simplified check_var helper APIsclassDiagram
class RuleEngineCheckVarModule {
+get_vars_from_rules(rule: &Rule, utils: &RuleRegistration) RapidSetString
+check_var_in_constraints(vars: RapidSetString, constraints: &RapidMapMetaVarRule) RResultRapidSetString
+check_var_in_transform(vars: RapidSetString, transform: &OptionTransform) RResultRapidSetString
}
class Rule {
+defined_vars() RapidSetString
+used_vars() RapidSetString
}
class RuleRegistration {
}
class RapidSetString {
}
class RapidMapMetaVarRule {
}
class OptionTransform {
}
class Transform {
}
class RResultRapidSetString {
}
RuleEngineCheckVarModule --> Rule : reads
RuleEngineCheckVarModule --> RuleRegistration : reads
RuleEngineCheckVarModule --> RapidSetString : moves_ownership
RuleEngineCheckVarModule --> RapidMapMetaVarRule : borrows_constraints
RuleEngineCheckVarModule --> OptionTransform : borrows_transform
OptionTransform --> Transform : wraps
Rule --> RapidSetString : returns_sets
RuleEngineCheckVarModule --> RResultRapidSetString : returns
Flow diagram for optimized PathBuf allocation in Tarjan SCC lookupsflowchart TD
A["Start tarjan_dfs with v: &Path"] --> B["Lookup v in indices using &Path"]
B --> C{"Index for v exists?"}
C -->|"Yes"| D["Lookup v in lowlinks using &Path"]
C -->|"No"| E["Allocate PathBuf from v and insert into indices and lowlinks"]
E --> F["Push owned PathBuf onto stack and mark on_stack"]
D --> G["Process dependencies using borrowed dep: &Path"]
G --> H["Use dep directly in indices, lowlinks, on_stack lookups"]
H --> I{"SCC root detected?"}
I -->|"Yes"| J["Pop PathBuf nodes from stack to form SCC"]
I -->|"No"| K["Continue DFS traversal"]
J --> L["End traversal for v"]
K --> L
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull request overview
Optimizes parts of the invalidation cycle-detection path (Tarjan SCC) by removing unnecessary PathBuf allocations during map/set lookups, and cleans up an unrelated clippy-driven lifetime signature issue.
Changes:
- Use borrowed
&Pathkeys directly forRapidMap<PathBuf, _>lookups insidetarjan_dfs, eliminating repeatedto_path_buf()heap allocations on hot paths. - Remove redundant explicit lifetimes from helper functions in
check_var.rsto satisfy clippy and simplify signatures. - Document the performance learning in
.jules/bolt.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/rule-engine/src/check_var.rs | Simplifies helper signatures by removing unnecessary lifetime parameters. |
| crates/flow/src/incremental/invalidation.rs | Avoids to_path_buf() allocations in Tarjan SCC lookups by using borrowed &Path keys. |
| .jules/bolt.md | Adds a note describing the performance rationale and recommended practice. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
💡 What: Optimized
to_path_buf()allocations in thetarjan_dfsgraph traversal function for strongly connected components insidethread-flow's invalidation system.🎯 Why: In
TarjanStateduring graph traversal for finding cycles, calling.to_path_buf()on borrowed&Pathvalues repeatedly within loops to query HashSets and HashMaps led to unnecessary O(E) heap allocations.📊 Impact: Reduces memory allocation churn significantly during large invalidation graph cycle detections by changing lookup complexity from allocating to pure read.
🔬 Measurement: Verify by running
cargo test -p thread-flow --test invalidation_testsensuring no regressions, and note the avoidance of O(Edges) string allocations. Also clippy no longer emits explicit lifetime warnings incheck_var.rs.PR created automatically by Jules for task 3281374725584587497 started by @bashandbone
Summary by Sourcery
Optimize path-based lookups in the Tarjan SCC invalidation traversal to avoid redundant allocations and clean up related rule-engine lifetimes.
Enhancements:
Documentation: