perf: optimize destination template expansion and improve cache consistency#343
Conversation
…stency - Refactor `expand_destination_template` in `src/linker.rs` to use a single-pass expansion with `String::with_capacity` and `Cow<str>`, significantly reducing heap allocations during `NestedGlob` discovery. - Improve cache invalidation in `src/linker.rs` by clearing `path_cache` when other discovery caches are dropped. - Ensure `ensure_safe_destination_path` continues to use uncached canonicalization for critical security checks. - Clean up temporary test files.
|
👋 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. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe linker optimization expands cache invalidation to include path caches alongside existing globe and root caches, and refactors destination template expansion to use copy-on-write strings and single-pass placeholder loop processing instead of eager allocations and chained string replacements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
This PR is ready to merge but needs two things before it can land:
The |
…y-3552465723482046686
|
yacosta738
deleted the
perf-template-expansion-and-cache-consistency-3552465723482046686
branch
1 participant
Performance Optimization Summary
1. Zero-Allocation (Heuristic) Template Expansion
The$O(N)$ heap allocations per discovered file in
expand_destination_templatefunction insrc/linker.rswas previously using a chain of four.replace()calls. In Rust, each.replace()call allocates a newString, leading toNestedGloboperations.I've refactored this to use:
{placeholder}markers.String::with_capacityis used to allocate the final string buffer once, based on a safe heuristic of the template and component lengths.Cow<str>fromto_string_lossy()to avoid cloning UTF-8 path segments.2. Enhanced Cache Consistency
Updated
invalidate_discovery_cachesinsrc/linker.rsto clear thepath_cache. This ensures that any filesystem mutations (like symlink updates or directory creation) that trigger a cache invalidation also refresh the canonical path information used for relative path calculations.3. Security-First Caching
While exploring canonicalization caching in
ensure_safe_destination_path, I verified that the existing use ofcanonicalize_uncachedis necessary for defense-in-depth against "time-of-check to time-of-use" (TOCTOU) style attacks where an external process might swap a symlink during a sync run. I preserved this uncached behavior for the security check while improving performance in the template expansion logic.Verification Results
cargo test test_expand_destination_template_root_file: PASScargo test test_expand_destination_template_nested_file: PASScargo test test_ensure_safe_destination_uses_fresh_canonicalization: PASScargo test --all-features: PASScargo clippy --all-targets --all-features -- -D warnings: PASScargo fmt --all -- --check: PASSPR created automatically by Jules for task 3552465723482046686 started by @yacosta738