feat: optimize dynamic entry facade chunks by merging with common chunks when they are captured by common chunks#7486
Conversation
How to use the Graphite Merge QueueAdd the label graphite: merge 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. |
1eb6763 to
47bc505
Compare
✅ Deploy Preview for rolldown-rs canceled.
|
✅ Deploy Preview for rolldown-rs canceled.
|
CodSpeed Performance ReportMerging #7486 will not alter performanceComparing Summary
Footnotes |
196ea01 to
0588142
Compare
Benchmarks Rust |
00404a0 to
a9c63e9
Compare
a9c63e9 to
92c4920
Compare
92c4920 to
0fed809
Compare
related to #7486, we need to update the symbol usage after linking stage when merge dynamic chunks to common chunk
related to #7486, we need to update the symbol usage after linking stage when merge dynamic chunks to common chunk
bcda2c1 to
dbde4a3
Compare
0fed809 to
4520460
Compare
8649da9 to
4883c75
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements an optimization for code splitting that addresses issue #5276. The optimization handles cases where dynamic entry chunks become empty after all their modules are moved to common chunks during advanced chunking. Instead of keeping empty entry chunks, the PR rewrites dynamic imports to extract the correct namespace from the common chunk and removes the empty chunks.
Key Changes
- Introduces AST manipulation utilities to transform dynamic imports into
.then(n => n.namespace)chains - Adds a
SimulatedFacadeChunktracking mechanism to properly handle namespace symbols without marking modules as included - Implements chunk merging optimization in
optimize_facade_dynamic_entry_chunksthat removes empty dynamic entry chunks and updates import references
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rolldown_ecmascript_utils/src/ast_snippet.rs | Adds helper methods to generate arrow functions and call expressions for extracting properties from dynamic import results |
| crates/rolldown_common/src/types/module_namespace_included_reason.rs | Adds SimulateFacadeChunk flag to track namespace inclusion for simulated facade chunks |
| crates/rolldown_common/src/ecmascript/symbol_id_ext.rs | Adds module_namespace_symbol_ref helper method to construct namespace symbol references |
| crates/rolldown/src/types/linking_metadata.rs | Adds utilities to extract and restore inclusion information from linking metadata for temporary modifications |
| crates/rolldown/src/stages/link_stage/tree_shaking/include_statements.rs | Adds SimulatedFacadeChunk include reason and logic to skip module inclusion when only tracking namespace symbols |
| crates/rolldown/src/stages/link_stage/tree_shaking/mod.rs | Exports tree-shaking types and functions needed by the generate stage |
| crates/rolldown/src/stages/link_stage/mod.rs | Re-exports tree-shaking utilities and adds normal_symbol_exports_chain_map to link output |
| crates/rolldown/src/stages/generate_stage/chunk_optimizer.rs | Implements the main optimization logic to merge empty dynamic entry chunks with their target common chunks |
| crates/rolldown/src/stages/generate_stage/code_splitting.rs | Refactors entry initialization and integrates the chunk optimization into the splitting flow |
| crates/rolldown/src/stages/generate_stage/chunk_ext.rs | Moves chunk reason type assignment to always execute, not just in debug mode |
| crates/rolldown/src/stages/generate_stage/compute_cross_chunk_links.rs | Adds logic to export namespace symbols for merged facade chunks in common chunks |
| crates/rolldown/src/stages/generate_stage/render_chunk_to_assets.rs | Filters out removed chunks during asset rendering |
| crates/rolldown/src/stages/generate_stage/mod.rs | Passes used_symbol_refs to finalizer context for export filtering |
| crates/rolldown/src/module_finalizers/mod.rs | Implements dynamic import transformation and filters namespace exports based on actually used symbols |
| crates/rolldown/src/module_finalizers/finalizer_context.rs | Adds used_symbol_refs field to finalizer context |
| crates/rolldown/src/chunk_graph.rs | Adds tracking for removed chunks and common chunks that export facade namespace symbols |
| crates/rolldown/tests/rolldown/code_splitting/issue_5276/* | Adds comprehensive test case validating the optimization with multiple dynamic imports |
| crates/rolldown/tests/snapshots/integration_rolldown__filename_with_hash.snap | Updates snapshot with new test output hashes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4457889 to
c22bbed
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
crates/rolldown/src/stages/generate_stage/code_splitting.rs:715
- The method signature changed from
&selfto&mut selfto accommodate the call tooptimize_facade_dynamic_entry_chunkswhich requires mutable access toself.link_output. While this works, consider whetheroptimize_facade_dynamic_entry_chunkscould be refactored to take only the necessary mutable parts as parameters (similar to howinit_entry_pointtakes parameters) rather than requiring&mut self. This would make the mutability requirements more explicit and the code easier to reason about.
async fn split_chunks(
&mut self,
index_splitting_info: &mut IndexSplittingInfo,
chunk_graph: &mut ChunkGraph,
bits_to_chunk: &mut FxHashMap<BitSet, ChunkIdx>,
input_base: &ArcStr,
) -> BuildResult<()> {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/rolldown/src/stages/link_stage/tree_shaking/include_statements.rs
Show resolved
Hide resolved
|
@IWANABETHATGUY I've opened a new pull request, #7614, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/rolldown/tests/rolldown/code_splitting/issue_5276/main.js
Outdated
Show resolved
Hide resolved
crates/rolldown/src/stages/link_stage/tree_shaking/include_statements.rs
Show resolved
Hide resolved
7a4db2b to
73b5a6e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I wonder if we can reuse the optimization for |
The first focused on merging common chunks into a dynamic entry chunk, while the second focused on merging dynamic entry chunks into common chunks. The first focused on module assignment; it reduced chunks by pushing modules that should belong to a new common chunk into existing entry chunks, whereas the second focused on chunk merging (the module is already assigned). Also, the first optimization did not involve any tree-shake status update. I split them up to improve code maintenance and make it easier to review. I will try to merge the two-pass optimization into a single pass in the future. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@sapphi-red I have addressed all the issues you raised. I am going to merge it to avoid further conflicts. Feel free to point out any other issues. I will fix them in future pr. |
Merge activity
|
related to #5726, #4905