From ecad6bc03a4dcaf114151fe93854aae8682829eb Mon Sep 17 00:00:00 2001 From: ahabhgk Date: Wed, 28 Aug 2024 19:57:52 +0800 Subject: [PATCH] fix: topological order when optimize incoming connections --- .../rspack_core/src/compiler/compilation.rs | 27 +- crates/rspack_core/src/exports_info.rs | 2 +- crates/rspack_core/src/module_graph/mod.rs | 9 +- .../src/plugin/side_effects_flag_plugin.rs | 347 +++++++++++------- .../side-effects-unsorted-modules/index.js | 9 + .../node_modules/dep/a.js | 3 + .../node_modules/dep/b.js | 3 + .../node_modules/dep/c.js | 3 + .../node_modules/dep/index.js | 3 + .../node_modules/dep/package.json | 6 + .../node_modules/dep/trackModules.js | 4 + .../webpack.config.js | 24 ++ 12 files changed, 286 insertions(+), 154 deletions(-) create mode 100644 tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/index.js create mode 100644 tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/a.js create mode 100644 tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/b.js create mode 100644 tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/c.js create mode 100644 tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/index.js create mode 100644 tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/package.json create mode 100644 tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/trackModules.js create mode 100644 tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/webpack.config.js diff --git a/crates/rspack_core/src/compiler/compilation.rs b/crates/rspack_core/src/compiler/compilation.rs index 8d7ab9850a9..3515132b658 100644 --- a/crates/rspack_core/src/compiler/compilation.rs +++ b/crates/rspack_core/src/compiler/compilation.rs @@ -1077,20 +1077,11 @@ impl Compilation { .finish_modules .call(self) .await?; + logger.time_end(start); // Collect dependencies diagnostics at here to make sure: // 1. after finish_modules: has provide exports info // 2. before optimize dependencies: side effects free module hasn't been skipped (move_target) - let module_graph = self.get_module_graph(); - let diagnostics: Vec<_> = module_graph - .module_graph_modules() - .par_iter() - .flat_map(|(_, mgm)| &mgm.all_dependencies) - .filter_map(|dependency_id| module_graph.dependency_by_id(dependency_id)) - .filter_map(|dependency| dependency.get_diagnostics(&module_graph)) - .flat_map(|ds| ds) - .collect(); - self.extend_diagnostics(diagnostics); - logger.time_end(start); + self.collect_dependencies_diagnostics(); // take make diagnostics let diagnostics = self.make_artifact.take_diagnostics(); @@ -1110,6 +1101,20 @@ impl Compilation { Ok(()) } + #[tracing::instrument(skip_all)] + fn collect_dependencies_diagnostics(&mut self) { + let module_graph = self.get_module_graph(); + let diagnostics: Vec<_> = module_graph + .module_graph_modules() + .par_iter() + .flat_map(|(_, mgm)| &mgm.all_dependencies) + .filter_map(|dependency_id| module_graph.dependency_by_id(dependency_id)) + .filter_map(|dependency| dependency.get_diagnostics(&module_graph)) + .flat_map(|ds| ds) + .collect(); + self.extend_diagnostics(diagnostics); + } + #[instrument(name = "compilation:seal", skip_all)] pub async fn seal(&mut self, plugin_driver: SharedPluginDriver) -> Result<()> { self.other_module_graph = Some(ModuleGraphPartial::default()); diff --git a/crates/rspack_core/src/exports_info.rs b/crates/rspack_core/src/exports_info.rs index 6082108d602..376898992a1 100644 --- a/crates/rspack_core/src/exports_info.rs +++ b/crates/rspack_core/src/exports_info.rs @@ -1590,7 +1590,7 @@ pub struct ResolvedExportInfoTarget { pub module: ModuleIdentifier, pub export: Option>, /// using dependency id to retrieve Connection - connection: DependencyId, + pub connection: DependencyId, } #[derive(Clone, Debug)] diff --git a/crates/rspack_core/src/module_graph/mod.rs b/crates/rspack_core/src/module_graph/mod.rs index 2afa7fc3658..51d3d456b00 100644 --- a/crates/rspack_core/src/module_graph/mod.rs +++ b/crates/rspack_core/src/module_graph/mod.rs @@ -1034,7 +1034,11 @@ impl<'a> ModuleGraph<'a> { .insert(dep_id, DependencyExtraMeta { ids }); } - pub fn update_module(&mut self, dep_id: &DependencyId, module_id: &ModuleIdentifier) { + pub fn update_module( + &mut self, + dep_id: &DependencyId, + module_id: &ModuleIdentifier, + ) -> Option { let connection_id = *self .connection_id_by_dependency_id(dep_id) .expect("should have connection id"); @@ -1042,7 +1046,7 @@ impl<'a> ModuleGraph<'a> { .connection_by_connection_id_mut(&connection_id) .expect("should have connection"); if connection.module_identifier() == module_id { - return; + return None; } // clone connection @@ -1097,6 +1101,7 @@ impl<'a> ModuleGraph<'a> { mgm.add_incoming_connection(new_connection_id); mgm.remove_incoming_connection(&connection_id); } + Some(new_connection_id) } pub fn get_exports_info(&self, module_identifier: &ModuleIdentifier) -> ExportsInfo { diff --git a/crates/rspack_plugin_javascript/src/plugin/side_effects_flag_plugin.rs b/crates/rspack_plugin_javascript/src/plugin/side_effects_flag_plugin.rs index 47d4fb4ce62..c9e374ccb49 100644 --- a/crates/rspack_plugin_javascript/src/plugin/side_effects_flag_plugin.rs +++ b/crates/rspack_plugin_javascript/src/plugin/side_effects_flag_plugin.rs @@ -1,9 +1,10 @@ -use std::collections::VecDeque; use std::fmt::Debug; use std::sync::Arc; use std::sync::LazyLock; +use rspack_collections::IdentifierMap; use rspack_collections::IdentifierSet; +use rspack_core::ConnectionId; use rspack_core::{ BoxModule, Compilation, CompilationOptimizeDependencies, ConnectionState, FactoryMeta, ModuleFactoryCreateData, ModuleGraph, ModuleIdentifier, NormalModuleCreateData, @@ -13,10 +14,9 @@ use rspack_error::Result; use rspack_hook::{plugin, plugin_hook}; use rspack_paths::AssertUtf8; use rspack_paths::Utf8Path; +use rustc_hash::FxHashSet; use sugar_path::SugarPath; use swc_core::common::comments::Comments; -// use rspack_core::Plugin; -// use rspack_error::Result; use swc_core::common::{comments, Span, Spanned, SyntaxContext, GLOBALS}; use swc_core::ecma::ast::*; use swc_core::ecma::utils::{ExprCtx, ExprExt}; @@ -656,121 +656,26 @@ async fn nmf_module( #[plugin_hook(CompilationOptimizeDependencies for SideEffectsFlagPlugin)] fn optimize_dependencies(&self, compilation: &mut Compilation) -> Result> { - let entries = compilation.entry_modules(); - let level_order_module_identifier = - get_level_order_module_ids(&compilation.get_module_graph(), entries); - for module_identifier in level_order_module_identifier { - let module_graph = compilation.get_module_graph(); - let mut module_chain = IdentifierSet::default(); - // dbg!(&module_identifier); - let Some(module) = module_graph.module_by_identifier(&module_identifier) else { - continue; - }; - let side_effects_state = - module.get_side_effects_connection_state(&module_graph, &mut module_chain); - if side_effects_state != rspack_core::ConnectionState::Bool(false) { - continue; - } - let cur_exports_info = module_graph.get_exports_info(&module_identifier); - - let incoming_connections = module_graph - .module_graph_module_by_identifier(&module_identifier) - .map(|mgm| mgm.incoming_connections().clone()) - .unwrap_or_default(); - for con_id in incoming_connections { - let mut module_graph = compilation.get_module_graph_mut(); - let con = module_graph - .connection_by_connection_id(&con_id) - .expect("should have connection"); - let Some(dep) = module_graph.dependency_by_id(&con.dependency_id) else { - continue; - }; - let dep_id = *dep.id(); - let is_reexport = dep - .downcast_ref::() - .is_some(); - let is_valid_import_specifier_dep = dep - .downcast_ref::() - .map(|import_specifier_dep| !import_specifier_dep.namespace_object_as_context) - .unwrap_or_default(); - if !is_reexport && !is_valid_import_specifier_dep { - continue; - } - if let Some(name) = dep - .downcast_ref::() - .and_then(|dep| dep.name.clone()) - { - let export_info = module_graph.get_export_info( - con - .original_module_identifier - .expect("should have original_module_identifier"), - &name, - ); - export_info.move_target( - &mut module_graph, - Arc::new(|target: &ResolvedExportInfoTarget, mg: &ModuleGraph| { - mg.module_by_identifier(&target.module) - .expect("should have module") - .get_side_effects_connection_state(mg, &mut IdentifierSet::default()) - == ConnectionState::Bool(false) - }), - Arc::new( - move |target: &ResolvedExportInfoTarget, mg: &mut ModuleGraph| { - mg.update_module(&dep_id, &target.module); - // TODO: Explain https://github.com/webpack/webpack/blob/ac7e531436b0d47cd88451f497cdfd0dad41535d/lib/optimize/SideEffectsFlagPlugin.js#L303-L306 - let ids = dep_id.get_ids(mg); - let processed_ids = target - .export - .as_ref() - .map(|item| { - let mut ret = Vec::from_iter(item.iter().cloned()); - ret.extend_from_slice(ids.get(1..).unwrap_or_default()); - ret - }) - .unwrap_or_else(|| ids.get(1..).unwrap_or_default().to_vec()); - dep_id.set_ids(processed_ids, mg); - mg.connection_by_dependency(&dep_id).map(|_| dep_id) - }, - ), - ); - continue; - } - - let ids = dep_id.get_ids(&module_graph); - - if !ids.is_empty() { - let export_info = cur_exports_info.get_export_info(&mut module_graph, &ids[0]); - - let target = export_info.get_target( - &module_graph, - Some(Arc::new( - |target: &ResolvedExportInfoTarget, mg: &ModuleGraph| { - mg.module_by_identifier(&target.module) - .expect("should have module graph") - .get_side_effects_connection_state(mg, &mut IdentifierSet::default()) - == ConnectionState::Bool(false) - }, - )), - ); - let Some(target) = target else { - continue; - }; - - // dbg!(&mg.connection_by_dependency(&dep_id)); - module_graph.update_module(&dep_id, &target.module); - // TODO: Explain https://github.com/webpack/webpack/blob/ac7e531436b0d47cd88451f497cdfd0dad41535d/lib/optimize/SideEffectsFlagPlugin.js#L303-L306 - let processed_ids = target - .export - .map(|mut item| { - item.extend_from_slice(&ids[1..]); - item - }) - .unwrap_or_else(|| ids[1..].to_vec()); - - // dbg!(&mg.connection_by_dependency(&dep_id)); - dep_id.set_ids(processed_ids, &mut module_graph); - } - } + let mut modules: IdentifierSet = if compilation.options.new_incremental_enabled() { + compilation + .unaffected_modules_cache + .get_affected_modules_with_module_graph() + .lock() + .expect("should lock") + .iter() + .copied() + .collect() + } else { + compilation + .get_module_graph() + .modules() + .keys() + .copied() + .collect() + }; + let mut new_connections = Default::default(); + for module in modules.clone() { + optimize_incoming_connections(module, &mut modules, &mut new_connections, compilation); } Ok(None) } @@ -799,31 +704,193 @@ impl Plugin for SideEffectsFlagPlugin { } } -fn get_level_order_module_ids(mg: &ModuleGraph, entries: IdentifierSet) -> Vec { - let mut res = vec![]; - let mut visited = IdentifierSet::default(); - for entry in entries { - let mut q = VecDeque::from_iter([entry]); - while let Some(mi) = q.pop_front() { - if visited.contains(&mi) { - continue; - } else { - visited.insert(mi); - res.push(mi); - } - for con in mg.get_outgoing_connections(&mi) { - let mi = *con.module_identifier(); - q.push_back(mi); - } +#[tracing::instrument(skip_all, fields(module = ?module_identifier))] +fn optimize_incoming_connections( + module_identifier: ModuleIdentifier, + to_be_optimized: &mut IdentifierSet, + new_connections: &mut IdentifierMap>, + compilation: &mut Compilation, +) { + if !to_be_optimized.remove(&module_identifier) { + return; + } + let module_graph = compilation.get_module_graph(); + let Some(module) = module_graph.module_by_identifier(&module_identifier) else { + return; + }; + let side_effects_state = + module.get_side_effects_connection_state(&module_graph, &mut IdentifierSet::default()); + if side_effects_state != rspack_core::ConnectionState::Bool(false) { + return; + } + let incoming_connections = module_graph + .module_graph_module_by_identifier(&module_identifier) + .map(|mgm| mgm.incoming_connections().clone()) + .unwrap_or_default(); + for &connection_id in &incoming_connections { + optimize_incoming_connection( + connection_id, + module_identifier, + to_be_optimized, + new_connections, + compilation, + ); + } + // It is possiable to add additional new connections when optimizing module's incoming connections + while let Some(connections) = new_connections.remove(&module_identifier) { + for new_connection_id in connections { + optimize_incoming_connection( + new_connection_id, + module_identifier, + to_be_optimized, + new_connections, + compilation, + ); } } +} + +fn optimize_incoming_connection( + connection_id: ConnectionId, + module_identifier: ModuleIdentifier, + to_be_optimized: &mut IdentifierSet, + new_connections: &mut IdentifierMap>, + compilation: &mut Compilation, +) { + let module_graph = compilation.get_module_graph(); + let connection = module_graph + .connection_by_connection_id(&connection_id) + .expect("should have connection"); + let Some(dep) = module_graph.dependency_by_id(&connection.dependency_id) else { + return; + }; + let is_reexport = dep + .downcast_ref::() + .is_some(); + let is_valid_import_specifier_dep = dep + .downcast_ref::() + .map(|import_specifier_dep| !import_specifier_dep.namespace_object_as_context) + .unwrap_or_default(); + if !is_reexport && !is_valid_import_specifier_dep { + return; + } + let Some(origin_module) = connection.original_module_identifier else { + return; + }; + // For the best optimization results, connection.origin_module must optimize before connection.module + // See: https://github.com/webpack/webpack/pull/17595 + optimize_incoming_connections(origin_module, to_be_optimized, new_connections, compilation); + do_optimize_incoming_connection( + connection_id, + module_identifier, + origin_module, + new_connections, + compilation, + ); +} + +#[tracing::instrument(skip_all, fields(origin = ?origin_module, module = ?module_identifier))] +fn do_optimize_incoming_connection( + connection_id: ConnectionId, + module_identifier: ModuleIdentifier, + origin_module: ModuleIdentifier, + new_connections: &mut IdentifierMap>, + compilation: &mut Compilation, +) { + if let Some(connections) = new_connections.get_mut(&module_identifier) { + connections.remove(&connection_id); + } + let mut module_graph = compilation.get_module_graph_mut(); + let connection = module_graph + .connection_by_connection_id(&connection_id) + .expect("should have connection"); + let dep_id = connection.dependency_id; + let dep = module_graph + .dependency_by_id(&dep_id) + .expect("should have dep"); + if let Some(name) = dep + .downcast_ref::() + .and_then(|dep| dep.name.clone()) + { + let export_info = module_graph.get_export_info(origin_module, &name); + let target = export_info.move_target( + &mut module_graph, + Arc::new(|target: &ResolvedExportInfoTarget, mg: &ModuleGraph| { + mg.module_by_identifier(&target.module) + .expect("should have module") + .get_side_effects_connection_state(mg, &mut IdentifierSet::default()) + == ConnectionState::Bool(false) + }), + Arc::new( + move |target: &ResolvedExportInfoTarget, mg: &mut ModuleGraph| { + mg.update_module(&dep_id, &target.module)?; + // TODO: Explain https://github.com/webpack/webpack/blob/ac7e531436b0d47cd88451f497cdfd0dad41535d/lib/optimize/SideEffectsFlagPlugin.js#L303-L306 + let ids = dep_id.get_ids(mg); + let processed_ids = target + .export + .as_ref() + .map(|item| { + let mut ret = Vec::from_iter(item.iter().cloned()); + ret.extend_from_slice(ids.get(1..).unwrap_or_default()); + ret + }) + .unwrap_or_else(|| ids.get(1..).unwrap_or_default().to_vec()); + dep_id.set_ids(processed_ids, mg); + Some(dep_id) + }, + ), + ); + if let Some(ResolvedExportInfoTarget { + connection, module, .. + }) = target + && let Some(&connection_id) = compilation + .get_module_graph() + .connection_id_by_dependency_id(&connection) + { + new_connections + .entry(module) + .or_default() + .insert(connection_id); + }; + return; + } - res.sort_by(|a, b| { - let ad = mg.get_depth(a); - let bd = mg.get_depth(b); - ad.cmp(&bd) - }); - res + let ids = dep_id.get_ids(&module_graph); + if !ids.is_empty() { + let cur_exports_info = module_graph.get_exports_info(&module_identifier); + let export_info = cur_exports_info.get_export_info(&mut module_graph, &ids[0]); + + let target = export_info.get_target( + &module_graph, + Some(Arc::new( + |target: &ResolvedExportInfoTarget, mg: &ModuleGraph| { + mg.module_by_identifier(&target.module) + .expect("should have module graph") + .get_side_effects_connection_state(mg, &mut IdentifierSet::default()) + == ConnectionState::Bool(false) + }, + )), + ); + let Some(target) = target else { + return; + }; + let Some(connection_id) = module_graph.update_module(&dep_id, &target.module) else { + return; + }; + new_connections + .entry(target.module) + .or_default() + .insert(connection_id); + // TODO: Explain https://github.com/webpack/webpack/blob/ac7e531436b0d47cd88451f497cdfd0dad41535d/lib/optimize/SideEffectsFlagPlugin.js#L303-L306 + let processed_ids = target + .export + .map(|mut item| { + item.extend_from_slice(&ids[1..]); + item + }) + .unwrap_or_else(|| ids[1..].to_vec()); + dep_id.set_ids(processed_ids, &mut module_graph); + } } #[cfg(test)] diff --git a/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/index.js b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/index.js new file mode 100644 index 00000000000..b1a42ca2737 --- /dev/null +++ b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/index.js @@ -0,0 +1,9 @@ +import { b } from "dep"; + +b.c(); + +import { modules } from "dep/trackModules.js"; + +it("should not contain side-effect-free modules", () => { + expect(modules).toEqual(["c"]); +}); diff --git a/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/a.js b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/a.js new file mode 100644 index 00000000000..dbcb9480348 --- /dev/null +++ b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/a.js @@ -0,0 +1,3 @@ +import { track } from "./trackModules.js"; +track("a"); +export * as b from "./b.js"; diff --git a/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/b.js b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/b.js new file mode 100644 index 00000000000..4f79dddc406 --- /dev/null +++ b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/b.js @@ -0,0 +1,3 @@ +import { track } from "./trackModules.js"; +track("b"); +export * from "./c.js"; diff --git a/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/c.js b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/c.js new file mode 100644 index 00000000000..48e53b3520f --- /dev/null +++ b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/c.js @@ -0,0 +1,3 @@ +import { track } from "./trackModules.js"; +track("c"); +export function c() {} diff --git a/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/index.js b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/index.js new file mode 100644 index 00000000000..bec05fc5011 --- /dev/null +++ b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/index.js @@ -0,0 +1,3 @@ +import { track } from "./trackModules.js"; +track("index"); +export * from "./a.js" diff --git a/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/package.json b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/package.json new file mode 100644 index 00000000000..644d902d8e0 --- /dev/null +++ b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/package.json @@ -0,0 +1,6 @@ +{ + "name": "dep", + "version": "1.0.0", + "type": "module", + "sideEffects": false +} diff --git a/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/trackModules.js b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/trackModules.js new file mode 100644 index 00000000000..99f5f10b0ca --- /dev/null +++ b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/node_modules/dep/trackModules.js @@ -0,0 +1,4 @@ +export const modules = []; +export function track(name) { + modules.push(name); +} diff --git a/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/webpack.config.js b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/webpack.config.js new file mode 100644 index 00000000000..f8541b27311 --- /dev/null +++ b/tests/webpack-test/configCases/side-effects/side-effects-unsorted-modules/webpack.config.js @@ -0,0 +1,24 @@ +/** @type {import("../../../../").Configuration} */ + +class ReorderModulesPlugin { + constructor() {} + + apply(compiler) { + compiler.hooks.compilation.tap("ReorderModulesPlugin", compilation => { + compilation.hooks.seal.tap("ReorderModulesPlugin", () => { + const sortedModules = Array.from(compilation.modules).sort((a, _b) => + a.request.includes("b.js") ? -1 : 1 + ); + compilation.modules = new Set(sortedModules); + }); + }); + } +} + +module.exports = { + // We don't support modify the order of compilation.modules, but it's always unsorted + // plugins: [new ReorderModulesPlugin()], + optimization: { + sideEffects: true + } +};