Skip to content

Commit

Permalink
fix: stablize chunk id (#6021)
Browse files Browse the repository at this point in the history
  • Loading branch information
JSerFeng authored Mar 25, 2024
1 parent 41e4e21 commit ffd46f0
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 35 deletions.
68 changes: 36 additions & 32 deletions crates/rspack_core/src/utils/find_graph_roots.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ impl<T: Hash + PartialEq + Eq> DatabaseItem for Cycle<T> {
struct Node<T> {
pub ukey: Ukey<Node<T>>,
pub item: T,
pub dependencies: FxHashSet<Ukey<Node<T>>>,
pub dependencies: Vec<Ukey<Node<T>>>,
pub marker: Marker,
pub cycle: Option<Ukey<Cycle<Ukey<Node<T>>>>>,
pub incoming: usize,
Expand Down Expand Up @@ -67,7 +67,9 @@ struct StackEntry<T> {
open_edges: Vec<T>,
}

pub fn find_graph_roots<Item: Clone + PartialEq + Eq + Hash + Send + Sync + Ord + 'static>(
pub fn find_graph_roots<
Item: Clone + std::fmt::Debug + PartialEq + Eq + Hash + Send + Sync + Ord + 'static,
>(
items: Vec<Item>,
get_dependencies: impl Sync + Fn(Item) -> Vec<Item>,
) -> Vec<Item> {
Expand Down Expand Up @@ -98,7 +100,7 @@ pub fn find_graph_roots<Item: Clone + PartialEq + Eq + Hash + Send + Sync + Ord
.into_iter()
.filter_map(|item| item_to_node_ukey.get(&item))
.cloned()
.collect();
.collect::<Vec<_>>();
});

// Set of current root modules
Expand All @@ -110,8 +112,11 @@ pub fn find_graph_roots<Item: Clone + PartialEq + Eq + Hash + Send + Sync + Ord
// that is not part of the cycle
let mut root_cycles: FxHashSet<Ukey<Cycle<Ukey<Node<Item>>>>> = FxHashSet::default();

let mut keys = db.keys().cloned().collect::<Vec<_>>();
keys.sort_by(|a, b| a.as_ref(&db).item.cmp(&b.as_ref(&db).item));

// For all non-marked nodes
for select_node in db.keys().cloned().collect::<Vec<_>>() {
for select_node in keys {
if matches!(select_node.as_ref(&db).marker, Marker::NoMarker) {
// deep-walk all referenced modules
// in a non-recursive way
Expand All @@ -122,12 +127,11 @@ pub fn find_graph_roots<Item: Clone + PartialEq + Eq + Hash + Send + Sync + Ord
// keep a stack to avoid recursive walk
let mut stack = vec![StackEntry {
node: select_node,
open_edges: select_node
.as_ref(&db)
.dependencies
.iter()
.cloned()
.collect(),
open_edges: {
let mut v: Vec<_> = select_node.as_ref(&db).dependencies.to_vec();
v.sort_by(|a, b| a.as_ref(&db).item.cmp(&b.as_ref(&db).item));
v
},
}];

// process the top item until stack is empty
Expand All @@ -136,6 +140,14 @@ pub fn find_graph_roots<Item: Clone + PartialEq + Eq + Hash + Send + Sync + Ord

// Are there still edges unprocessed in the current node?
if !stack[top_of_stack_idx].open_edges.is_empty() {
let mut edges = stack[top_of_stack_idx]
.open_edges
.iter()
.map(|edge| edge.as_ref(&db))
.collect::<Vec<_>>();

edges.sort_by(|a, b| a.item.cmp(&b.item));

// Process one dependency
let dependency = stack[top_of_stack_idx]
.open_edges
Expand All @@ -147,12 +159,11 @@ pub fn find_graph_roots<Item: Clone + PartialEq + Eq + Hash + Send + Sync + Ord
// mark it as in-progress and recurse
stack.push(StackEntry {
node: dependency,
open_edges: dependency
.as_ref(&db)
.dependencies
.iter()
.cloned()
.collect(),
open_edges: {
let mut v: Vec<_> = dependency.as_ref(&db).dependencies.to_vec();
v.sort_unstable();
v
},
});
dependency.as_mut(&mut db).marker = Marker::InProgressMarker;
}
Expand All @@ -171,7 +182,7 @@ pub fn find_graph_roots<Item: Clone + PartialEq + Eq + Hash + Send + Sync + Ord
// we merge the cycles to a shared cycle
{
let mut i = stack.len() - 1;
while stack[i].node != dependency {
while stack[i].node.as_ref(&db).item != dependency.as_ref(&db).item {
let node = stack[i].node;
if let Some(node_cycle) = node.as_ref(&db).cycle {
if node_cycle != cycle {
Expand All @@ -180,13 +191,6 @@ pub fn find_graph_roots<Item: Clone + PartialEq + Eq + Hash + Send + Sync + Ord
cycle.as_mut(&mut cycle_db).nodes.insert(cycle_node);
}
}
node_cycle
.as_ref(&cycle_db)
.nodes
.iter()
.for_each(|cycle_node| {
cycle_node.as_mut(&mut db).cycle = Some(cycle);
});
} else {
node.as_mut(&mut db).cycle = Some(cycle);
cycle.as_mut(&mut cycle_db).nodes.insert(node);
Expand Down Expand Up @@ -244,15 +248,15 @@ pub fn find_graph_roots<Item: Clone + PartialEq + Eq + Hash + Send + Sync + Ord
for dep in node.as_ref(&db).dependencies.clone().into_iter() {
if nodes.contains(&dep) {
dep.as_mut(&mut db).incoming += 1;
if dep.as_ref(&db).incoming < max {
continue;
}
if dep.as_ref(&db).incoming > max {
cycle_roots.clear();
max = dep.as_ref(&db).incoming;
}
cycle_roots.insert(dep);
}
if dep.as_ref(&db).incoming < max {
continue;
}
if dep.as_ref(&db).incoming > max {
cycle_roots.clear();
max = dep.as_ref(&db).incoming;
}
cycle_roots.insert(dep);
}
}
for cycle_root in cycle_roots {
Expand Down
15 changes: 12 additions & 3 deletions crates/rspack_ids/src/id_helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,10 @@ pub fn assign_names_par<T: Copy + Send>(
items.sort_unstable_by(&comparator);
let mut i = 0;
for item in items {
let formatted_name = format!("{name}{i}");
let mut formatted_name = format!("{name}{i}");
while name_to_items_keys.contains(&formatted_name) && used_ids.contains(&formatted_name) {
i += 1;
formatted_name = format!("{name}{i}");
}
assign_name(item, formatted_name.clone());
used_ids.insert(formatted_name);
Expand Down Expand Up @@ -494,7 +495,7 @@ fn compare_chunks_by_modules(
let a_modules = chunk_graph.get_ordered_chunk_modules(&a.ukey, module_graph);
let b_modules = chunk_graph.get_ordered_chunk_modules(&b.ukey, module_graph);

a_modules
let eq = a_modules
.into_iter()
.zip_longest(b_modules)
.find_map(|pair| match pair {
Expand All @@ -513,7 +514,15 @@ fn compare_chunks_by_modules(
Left(_) => Some(Ordering::Greater),
Right(_) => Some(Ordering::Less),
})
.unwrap_or(Ordering::Equal)
.unwrap_or(Ordering::Equal);

// 2 chunks are exactly the same, we have to compare
// the ukey to get stable results
if matches!(eq, Ordering::Equal) {
return a.ukey.cmp(&b.ukey);
}

eq
}

pub fn compare_chunks_natural(
Expand Down

2 comments on commit ffd46f0

@rspack-bot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Ran ecosystem CI: Open

suite result
modernjs, self-hosted, Linux, ci, ec2-linux ✅ success
_selftest, ubuntu-latest ✅ success
nx, ubuntu-latest ✅ success
rspress, ubuntu-latest ✅ success
rsbuild, ubuntu-latest ✅ success
compat, ubuntu-latest ✅ success
examples, ubuntu-latest ✅ success

@rspack-bot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Benchmark detail: Open

Name Base (2024-03-25 ca8afe6) Current Change
10000_development-mode + exec 1.92 s ± 15 ms 1.93 s ± 33 ms +0.61 %
10000_development-mode_hmr + exec 738 ms ± 10 ms 745 ms ± 14 ms +1.05 %
10000_production-mode + exec 2.96 s ± 41 ms 2.92 s ± 88 ms -1.19 %
arco-pro_development-mode + exec 2.52 s ± 17 ms 2.51 s ± 29 ms -0.40 %
arco-pro_development-mode_hmr + exec 513 ms ± 4.1 ms 488 ms ± 4.5 ms -4.95 %
arco-pro_development-mode_hmr_intercept-plugin + exec 531 ms ± 2 ms 503 ms ± 4.7 ms -5.22 %
arco-pro_development-mode_intercept-plugin + exec 3.39 s ± 33 ms 3.37 s ± 72 ms -0.57 %
arco-pro_production-mode + exec 4.19 s ± 61 ms 4.08 s ± 25 ms -2.56 %
arco-pro_production-mode_intercept-plugin + exec 5.07 s ± 97 ms 5.04 s ± 95 ms -0.57 %
threejs_development-mode_10x + exec 1.88 s ± 21 ms 1.91 s ± 36 ms +1.58 %
threejs_development-mode_10x_hmr + exec 738 ms ± 3.5 ms 740 ms ± 6.6 ms +0.33 %
threejs_production-mode_10x + exec 5.65 s ± 40 ms 5.8 s ± 89 ms +2.74 %

Please sign in to comment.