feat(lazy-barrel): support incremental build mode#8112
Conversation
How to use the Graphite Merge QueueAdd the label graphite: merge-when-ready to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for incremental build mode in the lazy-barrel optimization feature. The implementation enables barrel modules to be processed correctly when building from cached snapshots during incremental/watch mode rebuilds.
Changes:
- Added
resolved_barrel_modulesfield toBarrelStateto track import resolutions that need to be deferred when working with cached snapshots - Modified barrel module processing in
module_loader.rsto handle both live modules and cached snapshot modules - Added merge logic in
scan_stage_cache.rsto apply deferred import resolutions to cached modules
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/rolldown_common/src/types/lazy_barrel.rs | Added resolved_barrel_modules field to track deferred import resolutions for incremental builds |
| crates/rolldown/src/types/scan_stage_cache.rs | Added logic to apply deferred barrel module import resolutions during cache merge |
| crates/rolldown/src/module_loader/module_loader.rs | Refactored barrel module processing to use raw pointers for handling both live and cached modules, with conditional logic to defer updates when working with cached snapshots |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Benchmarks Rust |
| if !needed_records.contains_key(&rec_idx) { | ||
| return true; | ||
| } | ||
| let barrel_normal_module = unsafe { &mut *barrel_module_ptr }; |
There was a problem hiding this comment.
Yes, dereference of raw pointer is unsafe and requires an unsafe function or block
There was a problem hiding this comment.
I think I knew that. What I'm asking is does the logic have to be implemented with using unsafe?
There was a problem hiding this comment.
Yes, this is necessary. As shown below, barrel_normal_module and self.try_spawn_new_task conflict due to a &mut vs & borrow. In addition, barrel_normal_module is immutably borrowed both before and after the call to self.try_spawn_new_task and also mutably borrowed after in full build.
let new_idx = self.try_spawn_new_task(
resolved_id.clone(),
Some(ModuleTaskOwnerRef::new(barrel_normal_module, import_record_state.span)),
false,
import_record_state.asserted_module_type.as_ref(),
user_defined_entries,
)There was a problem hiding this comment.
Using a raw pointer is the simplest solution. Otherwise, we would need to refactor and add a significant amount of extra code to work around this. And they ultimately achieve the same result.
There was a problem hiding this comment.
Don't feel so. I looked into the code and I think the current design has issues.
No description provided.