perf: group chunks with right-first dfs #1554
Conversation
Walkthrough此次更改涉及多个文件的结构和逻辑调整，主要集中在依赖处理和代码优化方面。关键改动包括对 Changes
Sequence Diagram(s)sequenceDiagram
participant A as 用户
participant B as 模块处理器
participant C as 依赖图
A->>B: 调用 sync_dependencies_chunk
B->>C: 处理邻接块
C-->>B: 返回邻接块列表
B-->>A: 返回处理结果
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (25)
- crates/mako/src/generate/group_chunk.rs (4 hunks)
- e2e/fixtures/code-splitting.complex/expect.js (1 hunks)
- e2e/fixtures/css.css-merge-deep-level/expect.js (1 hunks)
- e2e/fixtures/css.css-merge-deep-level/mako.config.json (1 hunks)
- e2e/fixtures/css.css-merge-deep-level/src/a.css (1 hunks)
- e2e/fixtures/css.css-merge-deep-level/src/a1.css (1 hunks)
- e2e/fixtures/css.css-merge-deep-level/src/b.css (1 hunks)
- e2e/fixtures/css.css-merge-deep-level/src/b1.css (1 hunks)
- e2e/fixtures/css.css-merge-deep-level/src/c.css (1 hunks)
- e2e/fixtures/css.css-merge-deep-level/src/c1.css (1 hunks)
- e2e/fixtures/css.css-merge-deep-level/src/index.ts (1 hunks)
- e2e/fixtures/css.css-merge-in-css-multi-imports/expect.js (1 hunks)
- e2e/fixtures/css.css-merge-in-css-multi-imports/mako.config.json (1 hunks)
- e2e/fixtures/css.css-merge-in-css-multi-imports/src/a.css (1 hunks)
- e2e/fixtures/css.css-merge-in-css-multi-imports/src/b.css (1 hunks)
- e2e/fixtures/css.css-merge-in-css-multi-imports/src/c.css (1 hunks)
- e2e/fixtures/css.css-merge-in-css-multi-imports/src/index.css (1 hunks)
- e2e/fixtures/css.css-merge-in-css-multi-imports/src/index.ts (1 hunks)
- e2e/fixtures/css.css-merge-reverse-by-descent/expect.js (1 hunks)
- e2e/fixtures/css.css-merge-reverse-by-descent/mako.config.json (1 hunks)
- e2e/fixtures/css.css-merge-reverse-by-descent/src/a.css (1 hunks)
- e2e/fixtures/css.css-merge-reverse-by-descent/src/b.css (1 hunks)
- e2e/fixtures/css.css-merge-reverse-by-descent/src/c.css (1 hunks)
- e2e/fixtures/css.css-merge-reverse-by-descent/src/d.css (1 hunks)
- e2e/fixtures/css.css-merge-reverse-by-descent/src/index.ts (1 hunks)
Files skipped from review due to trivial changes (20)
- e2e/fixtures/css.css-merge-deep-level/mako.config.json
- e2e/fixtures/css.css-merge-deep-level/src/a.css
- e2e/fixtures/css.css-merge-deep-level/src/a1.css
- e2e/fixtures/css.css-merge-deep-level/src/b.css
- e2e/fixtures/css.css-merge-deep-level/src/b1.css
- e2e/fixtures/css.css-merge-deep-level/src/c.css
- e2e/fixtures/css.css-merge-deep-level/src/c1.css
- e2e/fixtures/css.css-merge-deep-level/src/index.ts
- e2e/fixtures/css.css-merge-in-css-multi-imports/mako.config.json
- e2e/fixtures/css.css-merge-in-css-multi-imports/src/a.css
- e2e/fixtures/css.css-merge-in-css-multi-imports/src/b.css
- e2e/fixtures/css.css-merge-in-css-multi-imports/src/c.css
- e2e/fixtures/css.css-merge-in-css-multi-imports/src/index.css
- e2e/fixtures/css.css-merge-in-css-multi-imports/src/index.ts
- e2e/fixtures/css.css-merge-reverse-by-descent/mako.config.json
- e2e/fixtures/css.css-merge-reverse-by-descent/src/a.css
- e2e/fixtures/css.css-merge-reverse-by-descent/src/b.css
- e2e/fixtures/css.css-merge-reverse-by-descent/src/c.css
- e2e/fixtures/css.css-merge-reverse-by-descent/src/d.css
- e2e/fixtures/css.css-merge-reverse-by-descent/src/index.ts
Additional comments not posted (5)
e2e/fixtures/css.css-merge-in-css-multi-imports/expect.js (1)
1-21: 测试逻辑和实现审核通过。此测试使用 Node.js 断言来验证
index.css文件中是否包含特定的 CSS 规则，并且顺序正确。这对于验证新的 DFS 方法是否正确处理 CSS 模块的依赖关系和顺序至关重要。e2e/fixtures/css.css-merge-reverse-by-descent/expect.js (1)
1-23: 测试逻辑和实现审核通过。此测试使用 Node.js 断言来验证
index.css文件中是否包含特定的 CSS 规则，并且顺序正确。这对于验证新的 DFS 方法是否正确处理 CSS 模块的依赖关系和顺序至关重要。e2e/fixtures/css.css-merge-deep-level/expect.js (1)
1-30: 测试逻辑和实现审核通过。此测试使用 Node.js 断言来验证
index.css文件中是否包含特定的 CSS 规则，并且顺序正确。这对于验证新的 DFS 方法是否正确处理 CSS 模块的依赖关系和顺序至关重要。e2e/fixtures/code-splitting.complex/expect.js (1)
48-48: 验证正则表达式中的字符串数组更改。此更改可能反映了块命名或组织方式的更新。请确保此更改与整个文件的上下文以及测试的准确性相符。
crates/mako/src/generate/group_chunk.rs (1)
485-506: 审查重构后的visit_modules函数。此函数现在使用泛型并返回
Vec<T>，这应提供更多灵活性并可能通过避免不必要的变更来提高性能。建议添加文档来解释泛型的使用及其对函数行为的影响。
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1554 +/- ##
==========================================
- Coverage 61.63% 61.63% -0.01%
==========================================
Files 127 127
Lines 15294 15294
==========================================
- Hits 9427 9426 -1
- Misses 5867 5868 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
crates/mako/src/generate/chunk_graph.rs (1)
114-121: 代码更改看起来不错，但有一个小建议。这些更改很好地解决了petgraph库中邻居排序的问题。通过引入中间变量
ret并显式反转顺序，代码变得更加清晰和易于理解。建议将变量
ret重命名为更具描述性的名称，例如neighbors或dependency_chunks，以更好地反映其内容。这样可以进一步提高代码的可读性。例如：
- let ret = self + let neighbors = self .graph .neighbors_directed(*idx, Direction::Outgoing) .filter(|idx| matches!(self.graph[*idx].chunk_type, ChunkType::Sync)) .map(|idx| self.graph[idx].id.clone()) .collect::<Vec<ChunkId>>(); // The neighbors ordering is reversed, see https://github.com/petgraph/petgraph/issues/116 - ret.into_iter().rev().collect() + neighbors.into_iter().rev().collect()
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- crates/mako/src/generate/chunk_graph.rs (1 hunks)
- e2e/fixtures/code-splitting.complex/expect.js (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/fixtures/code-splitting.complex/expect.js
xusd320
force-pushed
the
perf/group-chunks-with-post-order-dfs
branch
from
083e368
to
d97a8f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
crates/mako/src/generate/optimize_chunk.rs (1)
472-481: 优化了边的添加逻辑，提高了代码可读性和性能这个改动将原来的for循环替换为更加函数式的方法，使用了
flat_map和for_each。这种方式不仅提高了代码的可读性，还可能带来轻微的性能提升。值得注意的是，代码中反转了邻居的顺序（
tos.iter().rev()），这是为了解决petgraph库中的一个已知问题。建议修复注释中的拼写错误：
- // so need to add egde by reversed order + // so need to add edge by reversed orderTools
GitHub Check: Spell Check
[warning] 476-476:
"egde" should be "edge".
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- crates/mako/src/generate/optimize_chunk.rs (2 hunks)
- e2e/fixtures/code-splitting.complex/expect.js (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/fixtures/code-splitting.complex/expect.js
Additional context used
GitHub Check: Spell Check
crates/mako/src/generate/optimize_chunk.rs
[warning] 476-476:
"egde" should be "edge".
Additional comments not posted (1)
crates/mako/src/generate/optimize_chunk.rs (1)
513-515: 简化了边的添加逻辑这个改动将原来的for循环替换为
for_each方法，使代码更加简洁和易读。这种改变保持了原有的功能，同时采用了更现代的Rust编程风格。
xusd320
force-pushed
the
perf/group-chunks-with-post-order-dfs
branch
from
d97a8f8
to
a2ad316
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- crates/mako/src/generate/chunk_graph.rs (1 hunks)
- crates/mako/src/generate/optimize_chunk.rs (2 hunks)
- e2e/fixtures/code-splitting.complex/expect.js (2 hunks)
- e2e/fixtures/css.css-merge-deep-level/expect.js (1 hunks)
- e2e/fixtures/css.css-merge-in-css-multi-imports/expect.js (1 hunks)
- e2e/fixtures/css.css-merge-reverse-by-descent/expect.js (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- crates/mako/src/generate/chunk_graph.rs
- crates/mako/src/generate/optimize_chunk.rs
- e2e/fixtures/code-splitting.complex/expect.js
- e2e/fixtures/css.css-merge-deep-level/expect.js
- e2e/fixtures/css.css-merge-in-css-multi-imports/expect.js
- e2e/fixtures/css.css-merge-reverse-by-descent/expect.js
xusd320
force-pushed
the
perf/group-chunks-with-post-order-dfs
branch
from
a2ad316
to
26c5a2f
Compare
xusd320
force-pushed
the
perf/group-chunks-with-post-order-dfs
branch
from
26c5a2f
to
c99150c
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
crates/mako/src/generate/group_chunk.rs (2)
Line range hint
395-425: 依赖收集逻辑的改进新的实现使用后序深度优先搜索来收集依赖，这是一个很好的改进。然而，在将模块添加到chunk时，当前的实现可能会导致顺序问题。
考虑修改添加模块到chunk的逻辑，以保持正确的顺序：
- while let Some(dep) = chunk_deps.pop() { - chunk.add_module(dep); - } + for dep in chunk_deps.into_iter().rev() { + chunk.add_module(dep); + }这样可以确保模块按照后序遍历的顺序被添加到chunk中，保持了正确的依赖顺序。
485-509: 有关后序DFS的注释很有帮助这些注释清晰地解释了使用后序DFS的原因，并提供了一个很好的例子来说明依赖应该如何排序。这对于代码的可维护性来说是一个很大的改进。
为了使注释更加完整，可以考虑添加一行解释为什么c.css在最终顺序中只出现一次：
* ---------- * note that c.css, b.css, c.css before a.css will be deduplicated. +* c.css only appears once in the final order due to deduplication. */这可以帮助读者更好地理解去重过程。
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- crates/mako/src/generate/chunk_graph.rs (1 hunks)
- crates/mako/src/generate/group_chunk.rs (4 hunks)
- crates/mako/src/generate/optimize_chunk.rs (2 hunks)
- e2e/fixtures/css.css-merge-deep-level/expect.js (1 hunks)
- e2e/fixtures/css.css-merge-in-css-multi-imports/expect.js (1 hunks)
- e2e/fixtures/css.css-merge-reverse-by-descent/expect.js (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- crates/mako/src/generate/chunk_graph.rs
- crates/mako/src/generate/optimize_chunk.rs
- e2e/fixtures/css.css-merge-deep-level/expect.js
- e2e/fixtures/css.css-merge-in-css-multi-imports/expect.js
- e2e/fixtures/css.css-merge-reverse-by-descent/expect.js
Additional comments not posted (1)
crates/mako/src/generate/group_chunk.rs (1)
1-2: 导入更改看起来不错。新添加的导入是必要的，并且与后续的代码更改相符。
Comments failed to post (1)
crates/mako/src/generate/group_chunk.rs (1)
510-531:
visit_modules函数的泛化和优化函数的泛化是一个很好的改进，使其更加灵活和可重用。然而，当前的实现可能在某些情况下不够高效。
考虑使用
Vec的retain方法来优化去重逻辑：fn visit_modules<F, T>(mut queue: Vec<T>, visited: Option<HashSet<T>>, mut callback: F) -> Vec<T> where F: FnMut(&T) -> Vec<T>, T: Hash + Eq + Clone, { let mut post_order_dfs_ret: Vec<T> = Vec::new(); let mut visited = visited.unwrap_or_default(); while !queue.is_empty() { queue.retain(|id| { if !visited.contains(id) { visited.insert(id.clone()); post_order_dfs_ret.push(id.clone()); queue.extend(callback(id)); false } else { true } }); } post_order_dfs_ret }这个优化可以减少内存分配，并可能在某些情况下提高性能。
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- crates/mako/src/generate/chunk_graph.rs (1 hunks)
- crates/mako/src/generate/group_chunk.rs (4 hunks)
- crates/mako/src/generate/optimize_chunk.rs (2 hunks)
- e2e/fixtures/css.css-merge-deep-level/expect.js (1 hunks)
- e2e/fixtures/css.css-merge-in-css-multi-imports/expect.js (1 hunks)
- e2e/fixtures/css.css-merge-reverse-by-descent/expect.js (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- crates/mako/src/generate/chunk_graph.rs
- crates/mako/src/generate/group_chunk.rs
- e2e/fixtures/css.css-merge-deep-level/expect.js
- e2e/fixtures/css.css-merge-in-css-multi-imports/expect.js
- e2e/fixtures/css.css-merge-reverse-by-descent/expect.js
Additional context used
GitHub Check: codecov/patch
crates/mako/src/generate/optimize_chunk.rs
[warning] 472-472: crates/mako/src/generate/optimize_chunk.rs#L472
Added line #L472 was not covered by tests
[warning] 475-477: crates/mako/src/generate/optimize_chunk.rs#L475-L477
Added lines #L475 - L477 were not covered by tests
[warning] 509-509: crates/mako/src/generate/optimize_chunk.rs#L509
Added line #L509 was not covered by tests
[warning] 511-511: crates/mako/src/generate/optimize_chunk.rs#L511
Added line #L511 was not covered by tests
Additional comments not posted (2)
crates/mako/src/generate/optimize_chunk.rs (2)
472-477: 代码更改提高了可读性和性能这个更改使用了更加惯用的 Rust 风格，将传统的 for 循环替换为
flat_map和for_each的组合。这种方法可能会提高性能，并且代码更加简洁。为了提高可读性，建议添加一个简短的注释来解释这个转换：
+// 使用 flat_map 和 for_each 来高效地处理边缘关系 edges_map .iter() .flat_map(|(from, tos)| tos.iter().map(move |to| (from, to))) .for_each(|(from, to)| { chunk_graph.add_edge(from, to); });这些新添加的行目前没有被测试覆盖。建议添加相应的单元测试来确保这个逻辑的正确性。以下是一个验证测试覆盖率的脚本：
如果这些测试不存在，我可以协助您编写相应的单元测试。
Tools
GitHub Check: codecov/patch
[warning] 472-472: crates/mako/src/generate/optimize_chunk.rs#L472
Added line #L472 was not covered by tests
[warning] 475-477: crates/mako/src/generate/optimize_chunk.rs#L475-L477
Added lines #L475 - L477 were not covered by tests
509-511: 代码更改提高了一致性这个更改与
apply_optimize_infos方法的修改保持一致，使用for_each替换了传统的 for 循环。这种方法使代码更加简洁和惯用。代码变更看起来不错，保持了与之前更改的一致性。
这些新添加的行目前没有被测试覆盖。建议添加相应的单元测试来确保这个逻辑的正确性。以下是一个验证测试覆盖率的脚本：
如果这些测试不存在，我可以协助您编写相应的单元测试。
Tools
GitHub Check: codecov/patch
[warning] 509-509: crates/mako/src/generate/optimize_chunk.rs#L509
Added line #L509 was not covered by tests
[warning] 511-511: crates/mako/src/generate/optimize_chunk.rs#L511
Added line #L511 was not covered by tests
|
nice shot！ rightmost and deepest brings highest priority is so awesome！ |
|
after this pr is |
We need a hash_module check, and need iter by insertion order, at this point, there is no difference between LinkedHashSet and IndexSet. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- crates/mako/src/generate/chunk_graph.rs (1 hunks)
Additional comments not posted (1)
crates/mako/src/generate/chunk_graph.rs (1)
114-122: 改进了sync_dependencies_chunk方法的实现这个变更通过引入
ret变量并在返回前反转其顺序，解决了petgraph库中邻居节点顺序的问题。这个修改提高了代码的清晰度和正确性。建议添加一个简短的注释，解释为什么需要反转顺序，以便future维护者更容易理解。
例如：
// 由于petgraph库的特性，邻居节点的顺序是相反的，所以需要反转 ret.into_iter().rev().collect()
xusd320
changed the title
The previous impl of chunk dependencies by BFS is not correct, and in that way, must use Iter::position (compute as O(n)) api to insert depencies before parent. Just change the way by post order DFS, and remove calls of Iter::position.
Why before impl it not correct? With below files:
a.css
b.css
c.css
index.css
By BFS impl, the ordered modules will be
And after deduplicating, the modules order will be
the result css chunk is
Because in index.css, b.css is been imported after a.css, and b.css also import c.css, so c.css will overlap upon a.css again. So that this result is not correct.
By post order DFS, the ordered modules will be
That result is correct.
Coincidely, in parcel-bundler, these is a same case, https://github.com/parcel-bundler/lightningcss/blob/ad2ec9f10309ac27261a996210023b8e8ad3c793/src/bundler.rs#L1632, also see the same discussion parcel-bundler/parcel#5840.
When building big project, this impl is 8x faster than before.
Before:
After:
Summary by CodeRabbit
Summary by CodeRabbit
petgraph库相关的邻接模块顺序问题。