Skip to content

Commit 02287c4

Browse files
sokrasokra
authored
Turbopack: fix bug in handling of module batches (#77638)
### What? Before that change pre batch boundaries where discovered while collecting the pre batch items. But in some edge cases this is too late. A module should be a boundary when any edge fulfills the boundary condition. So we need to check all incoming edges before traversing the module for pre batch items. This adds a step before pre batches, that checks all edges and discovers all module boundaries.
1 parent 534bfd7 commit 02287c4

File tree

42 files changed

+421
-34
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

42 files changed

+421
-34
lines changed

‎turbopack/crates/turbopack-core/src/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#![feature(impl_trait_in_assoc_type)]
77
#![feature(iter_intersperse)]
88
#![feature(map_try_insert)]
9+
#![feature(hash_set_entry)]
910

1011
pub mod asset;
1112
pub mod changed;

‎turbopack/crates/turbopack-core/src/module_graph/module_batches.rs

+74-23
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,7 @@ struct TraversalState<'l> {
241241
}
242242

243243
struct PreBatches {
244+
boundary_modules: FxHashSet<ResolvedVc<Box<dyn Module>>>,
244245
batches: Vec<PreBatch>,
245246
entries: FxHashMap<ResolvedVc<Box<dyn Module>>, PreBatchIndex>,
246247
single_module_entries: FxIndexSet<ResolvedVc<Box<dyn Module>>>,
@@ -249,6 +250,7 @@ struct PreBatches {
249250
impl PreBatches {
250251
fn new() -> Self {
251252
Self {
253+
boundary_modules: FxHashSet::default(),
252254
batches: Vec::new(),
253255
entries: FxHashMap::default(),
254256
single_module_entries: FxIndexSet::default(),
@@ -258,20 +260,24 @@ impl PreBatches {
258260
fn ensure_pre_batch_for_module(
259261
&mut self,
260262
module: ResolvedVc<Box<dyn Module>>,
261-
chunk_groups: &RoaringBitmapWrapper,
263+
chunk_group_info: &ChunkGroupInfo,
262264
queue: &mut VecDeque<(ResolvedVc<Box<dyn Module>>, PreBatchIndex)>,
263-
) -> PreBatchIndex {
264-
match self.entries.entry(module) {
265+
) -> Result<PreBatchIndex> {
266+
Ok(match self.entries.entry(module) {
265267
Entry::Vacant(e) => {
266268
let index = self.batches.len();
267269
queue.push_back((module, index));
270+
let chunk_groups = chunk_group_info
271+
.module_chunk_groups
272+
.get(&module)
273+
.context("all modules need to have chunk group info")?;
268274
let batch = PreBatch::new(chunk_groups.clone());
269275
self.batches.push(batch);
270276
e.insert(index);
271277
index
272278
}
273279
Entry::Occupied(e) => *e.get(),
274-
}
280+
})
275281
}
276282

277283
async fn get_pre_batch_items(
@@ -281,10 +287,6 @@ impl PreBatches {
281287
module_graph: &ModuleGraph,
282288
queue: &mut VecDeque<(ResolvedVc<Box<dyn Module>>, PreBatchIndex)>,
283289
) -> Result<Vec<PreBatchItem>> {
284-
let entry_chunk_groups = chunk_group_info
285-
.module_chunk_groups
286-
.get(&ResolvedVc::upcast(entry))
287-
.context("all modules need to have chunk group info")?;
288290
let mut state = TraversalState {
289291
items: Vec::new(),
290292
this: self,
@@ -305,15 +307,12 @@ impl PreBatches {
305307
return Ok(GraphTraversalAction::Exclude);
306308
}
307309
if visited.insert(module) {
308-
let chunk_groups = chunk_group_info
309-
.module_chunk_groups
310-
.get(&module)
311-
.context("all modules need to have chunk group info")?;
312-
if chunk_groups != entry_chunk_groups {
313-
let idx =
314-
state
315-
.this
316-
.ensure_pre_batch_for_module(module, chunk_groups, queue);
310+
if parent_info.is_some() && state.this.boundary_modules.contains(&module) {
311+
let idx = state.this.ensure_pre_batch_for_module(
312+
module,
313+
chunk_group_info,
314+
queue,
315+
)?;
317316
state.items.push(PreBatchItem::ParallelReference(idx));
318317
return Ok(GraphTraversalAction::Exclude);
319318
}
@@ -351,6 +350,62 @@ pub async fn compute_module_batches(
351350
let module_graph = module_graph.await?;
352351

353352
let mut pre_batches = PreBatches::new();
353+
354+
// Walk the module graph and mark all modules that are boundary modules (referenced from a
355+
// different chunk group bitmap)
356+
module_graph
357+
.traverse_all_edges_unordered(|(parent, ty), node| {
358+
let std::collections::hash_set::Entry::Vacant(entry) =
359+
pre_batches.boundary_modules.entry(node.module)
360+
else {
361+
// Already a boundary module, can skip check
362+
return Ok(());
363+
};
364+
if ty.is_parallel() {
365+
let parent_chunk_groups = chunk_group_info
366+
.module_chunk_groups
367+
.get(&parent.module)
368+
.context("all modules need to have chunk group info")?;
369+
let chunk_groups = chunk_group_info
370+
.module_chunk_groups
371+
.get(&node.module)
372+
.context("all modules need to have chunk group info")?;
373+
if parent_chunk_groups != chunk_groups {
374+
// This is a boundary module
375+
entry.insert();
376+
}
377+
} else {
378+
entry.insert();
379+
}
380+
Ok(())
381+
})
382+
.await?;
383+
384+
// All entries are boundary modules too
385+
for chunk_group in &chunk_group_info.chunk_groups {
386+
for entry in chunk_group.entries() {
387+
pre_batches.boundary_modules.insert(entry);
388+
}
389+
}
390+
391+
// Pre batches would be incorrect with cycles, so we need to opt-out of pre batches for
392+
// cycles that include boundary modules
393+
module_graph
394+
.traverse_cycles(
395+
|ty| ty.is_parallel(),
396+
|cycle| {
397+
if cycle
398+
.iter()
399+
.any(|node| pre_batches.boundary_modules.contains(&node.module))
400+
{
401+
pre_batches
402+
.boundary_modules
403+
.extend(cycle.iter().map(|node| node.module));
404+
}
405+
},
406+
)
407+
.await?;
408+
354409
let mut queue: VecDeque<(ResolvedVc<Box<dyn Module>>, PreBatchIndex)> = VecDeque::new();
355410

356411
let mut chunk_group_indicies_with_merged_children = FxHashSet::default();
@@ -359,15 +414,11 @@ pub async fn compute_module_batches(
359414
for chunk_group in &chunk_group_info.chunk_groups {
360415
for entry in chunk_group.entries() {
361416
if let Some(chunkable_module) = ResolvedVc::try_downcast(entry) {
362-
let chunk_groups = chunk_group_info
363-
.module_chunk_groups
364-
.get(&entry)
365-
.context("all modules need to have chunk group info")?;
366417
pre_batches.ensure_pre_batch_for_module(
367418
chunkable_module,
368-
chunk_groups,
419+
&chunk_group_info,
369420
&mut queue,
370-
);
421+
)?;
371422
} else {
372423
pre_batches.single_module_entries.insert(entry);
373424
}

‎turbopack/crates/turbopack-test-utils/src/snapshot.rs

+4-10
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
1-
use std::{env, fs, path::PathBuf};
1+
use std::{env, path::PathBuf};
22

3-
use anyhow::{anyhow, bail, Context, Result};
3+
use anyhow::{bail, Context, Result};
44
use once_cell::sync::Lazy;
55
use regex::Regex;
66
use rustc_hash::{FxHashMap, FxHashSet};
77
use similar::TextDiff;
88
use turbo_rcstr::RcStr;
99
use turbo_tasks::{ReadRef, TryJoinIterExt, ValueToString, Vc};
1010
use turbo_tasks_fs::{
11-
DirectoryContent, DirectoryEntry, DiskFileSystem, File, FileContent, FileSystemEntryType,
12-
FileSystemPath,
11+
DirectoryContent, DirectoryEntry, File, FileContent, FileSystemEntryType, FileSystemPath,
1312
};
1413
use turbo_tasks_hash::{encode_hex, hash_xxh3_hash64};
1514
use turbopack_cli_utils::issue::{format_issue, LogOptions};
@@ -187,12 +186,7 @@ async fn get_contents(file: Vc<AssetContent>, path: Vc<FileSystemPath>) -> Resul
187186
}
188187

189188
async fn remove_file(path: Vc<FileSystemPath>) -> Result<()> {
190-
let fs = Vc::try_resolve_downcast_type::<DiskFileSystem>(path.fs())
191-
.await?
192-
.context(anyhow!("unexpected fs type"))?
193-
.await?;
194-
let sys_path = fs.to_sys_path(path).await?;
195-
fs::remove_file(&sys_path).context(format!("remove file {} error", sys_path.display()))?;
189+
path.write(FileContent::NotFound.cell()).await?;
196190
Ok(())
197191
}
198192

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/1.css

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
@import "a.css";
2+
@import "x.css";
3+
@import "y.css";

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/2.css

+2
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
@import "y.css";
2+
@import "x.css";

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/a.css

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
@import "b.css";
2+
3+
.a {
4+
content: "5 2";
5+
}

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/b.css

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
@import "c.css";
2+
3+
.b {
4+
content: "4 1";
5+
}

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/c.css

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
@import "d.css";
2+
3+
.c {
4+
content: "3 5";
5+
}

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/d.css

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
@import "e.css";
2+
3+
.d {
4+
content: "2 4";
5+
}

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/e.css

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
@import "a.css";
2+
3+
.e {
4+
content: "1 3";
5+
}

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/index.js

+2
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
import("./1.css");
2+
import("./2.css");

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/x.css

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
@import "a.css";
2+
3+
.x {
4+
content: "6 7";
5+
}

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/y.css

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
@import "c.css";
2+
3+
.y {
4+
content: "7 6";
5+
}

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_1_css_b7298ed4._.single.css

+3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_1_css_b7298ed4._.single.css.map

+6
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_23ea52ac._.js

+86
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
(globalThis.TURBOPACK = globalThis.TURBOPACK || []).push(["output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_23ea52ac._.js", {
2+
3+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/1.css [test] (css, async loader)": ((__turbopack_context__) => {
4+
5+
var { g: global, __dirname } = __turbopack_context__;
6+
{
7+
__turbopack_context__.v((parentImport) => {
8+
return Promise.all([
9+
{
10+
"path": "output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_ea25098c._.css",
11+
"included": [
12+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/e.css [test] (css)",
13+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/d.css [test] (css)",
14+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/c.css [test] (css)",
15+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/b.css [test] (css)",
16+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/a.css [test] (css)",
17+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/x.css [test] (css)",
18+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/y.css [test] (css)",
19+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/1.css [test] (css)"
20+
],
21+
"moduleChunks": [
22+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_e_css_b7298ed4._.single.css",
23+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_d_css_b7298ed4._.single.css",
24+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_c_css_b7298ed4._.single.css",
25+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_b_css_b7298ed4._.single.css",
26+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_a_css_b7298ed4._.single.css",
27+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_x_css_b7298ed4._.single.css",
28+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_y_css_b7298ed4._.single.css",
29+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_1_css_b7298ed4._.single.css",
30+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_a_css_b7298ed4._.single.css",
31+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_e_css_b7298ed4._.single.css",
32+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_d_css_b7298ed4._.single.css",
33+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_c_css_b7298ed4._.single.css",
34+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_b_css_b7298ed4._.single.css",
35+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_a_css_b7298ed4._.single.css",
36+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_c_css_b7298ed4._.single.css",
37+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_a_css_b7298ed4._.single.css",
38+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_x_css_b7298ed4._.single.css",
39+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_y_css_b7298ed4._.single.css"
40+
]
41+
}
42+
].map((chunk) => __turbopack_context__.l(chunk))).then(() => {});
43+
});
44+
}}),
45+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/2.css [test] (css, async loader)": ((__turbopack_context__) => {
46+
47+
var { g: global, __dirname } = __turbopack_context__;
48+
{
49+
__turbopack_context__.v((parentImport) => {
50+
return Promise.all([
51+
{
52+
"path": "output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_2bc55788._.css",
53+
"included": [
54+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/b.css [test] (css)",
55+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/a.css [test] (css)",
56+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/e.css [test] (css)",
57+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/d.css [test] (css)",
58+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/c.css [test] (css)",
59+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/y.css [test] (css)",
60+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/x.css [test] (css)",
61+
"[project]/turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/input/2.css [test] (css)"
62+
],
63+
"moduleChunks": [
64+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_b_css_b7298ed4._.single.css",
65+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_a_css_b7298ed4._.single.css",
66+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_e_css_b7298ed4._.single.css",
67+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_d_css_b7298ed4._.single.css",
68+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_c_css_b7298ed4._.single.css",
69+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_y_css_b7298ed4._.single.css",
70+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_x_css_b7298ed4._.single.css",
71+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_2_css_b7298ed4._.single.css",
72+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_c_css_b7298ed4._.single.css",
73+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_b_css_b7298ed4._.single.css",
74+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_a_css_b7298ed4._.single.css",
75+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_e_css_b7298ed4._.single.css",
76+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_d_css_b7298ed4._.single.css",
77+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_c_css_b7298ed4._.single.css",
78+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_a_css_b7298ed4._.single.css",
79+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_y_css_b7298ed4._.single.css",
80+
"output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_x_css_b7298ed4._.single.css"
81+
]
82+
}
83+
].map((chunk) => __turbopack_context__.l(chunk))).then(() => {});
84+
});
85+
}}),
86+
}]);

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_23ea52ac._.js.map

+5
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_2_css_b7298ed4._.single.css

+3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_2_css_b7298ed4._.single.css.map

+6
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎turbopack/crates/turbopack-tests/tests/snapshot/css/cycle2/output/turbopack_crates_turbopack-tests_tests_snapshot_css_cycle2_input_2bc55788._.css

+25
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)