From 731dc68c92d9feb278c1ed6aead166e43cbc9652 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EA=B0=95=EB=8F=99=EC=9C=A4?= Date: Mon, 26 Apr 2021 18:18:57 +0900 Subject: [PATCH] fix(bundler): Use proper algorithm for dependency analysis (#1610) swc_bundler: - Optimize detection of circular imports. --- bundler/Cargo.toml | 2 +- bundler/src/bundler/chunk/cjs.rs | 2 +- bundler/src/bundler/chunk/merge.rs | 3 +- bundler/src/bundler/chunk/mod.rs | 3 +- bundler/src/bundler/chunk/plan/mod.rs | 43 +++++++++----- bundler/src/modules/sort/chunk.rs | 59 +++++++------------ bundler/src/modules/sort/mod.rs | 10 +++- .../deno-8211-3/output/entry.inlined.ts | 24 ++++---- .../tests/fixture/deno-8211-3/output/entry.ts | 24 ++++---- .../circular/top-level-idents/output/entry.js | 2 +- 10 files changed, 90 insertions(+), 82 deletions(-) diff --git a/bundler/Cargo.toml b/bundler/Cargo.toml index 39cfa5452ce..51c75adb711 100644 --- a/bundler/Cargo.toml +++ b/bundler/Cargo.toml @@ -9,7 +9,7 @@ include = ["Cargo.toml", "build.rs", "src/**/*.rs", "src/**/*.js"] license = "Apache-2.0/MIT" name = "swc_bundler" repository = "https://github.com/swc-project/swc.git" -version = "0.32.5" +version = "0.32.7" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [features] diff --git a/bundler/src/bundler/chunk/cjs.rs b/bundler/src/bundler/chunk/cjs.rs index 3e66d9a9ec0..4ef661189c0 100644 --- a/bundler/src/bundler/chunk/cjs.rs +++ b/bundler/src/bundler/chunk/cjs.rs @@ -63,7 +63,7 @@ where module.visit_mut_with(&mut DefaultHandler { local_ctxt: info.local_ctxt(), }); - module.sort(info.id, &ctx.graph, &self.cm); + module.sort(info.id, &ctx.graph, &ctx.cycles, &self.cm); let stmt = ModuleItem::Stmt(wrap_module( SyntaxContext::empty(), diff --git a/bundler/src/bundler/chunk/merge.rs b/bundler/src/bundler/chunk/merge.rs index 6d324ef9f04..45868e7ad8c 100644 --- a/bundler/src/bundler/chunk/merge.rs +++ b/bundler/src/bundler/chunk/merge.rs @@ -29,6 +29,7 @@ use EdgeDirection::Outgoing; pub(super) struct Ctx { /// Full dependency graph. pub graph: ModuleGraph, + pub cycles: Vec>, pub merged: CHashSet, pub transitive_remap: CloneMap, pub export_stars_in_wrapped: Lock>>, @@ -405,7 +406,7 @@ where inline(self.injected_ctxt, entry); - entry.sort(id, &ctx.graph, &self.cm); + entry.sort(id, &ctx.graph, &ctx.cycles, &self.cm); // crate::debug::print_hygiene("done", &self.cm, &entry.clone().into()); diff --git a/bundler/src/bundler/chunk/mod.rs b/bundler/src/bundler/chunk/mod.rs index 3fb9c5f2d0d..e99848b9d6a 100644 --- a/bundler/src/bundler/chunk/mod.rs +++ b/bundler/src/bundler/chunk/mod.rs @@ -46,7 +46,7 @@ where entries: AHashMap, ) -> Result, Error> { let start = Instant::now(); - let (plan, graph) = self.determine_entries(entries).context("failed to plan")?; + let (plan, graph, cycles) = self.determine_entries(entries).context("failed to plan")?; let dur = Instant::now() - start; log::debug!("Dependency analysis took {:?}", dur); @@ -68,6 +68,7 @@ where let ctx = Ctx { graph, + cycles, merged: Default::default(), transitive_remap: Default::default(), export_stars_in_wrapped: Default::default(), diff --git a/bundler/src/bundler/chunk/plan/mod.rs b/bundler/src/bundler/chunk/plan/mod.rs index 53d6b4dec31..6bf35df3ec0 100644 --- a/bundler/src/bundler/chunk/plan/mod.rs +++ b/bundler/src/bundler/chunk/plan/mod.rs @@ -10,6 +10,7 @@ struct PlanBuilder { tracked: FxHashSet<(ModuleId, ModuleId)>, graph: ModuleGraph, + cycles: Vec>, all: Vec, kinds: FxHashMap, @@ -31,16 +32,7 @@ where pub(super) fn determine_entries( &self, entries: AHashMap, - ) -> Result<(Plan, ModuleGraph), Error> { - let plan = self.calculate_plan(entries)?; - - Ok(plan) - } - - fn calculate_plan( - &self, - entries: AHashMap, - ) -> Result<(Plan, ModuleGraph), Error> { + ) -> Result<(Plan, ModuleGraph, Vec>), Error> { let mut builder = PlanBuilder::default(); for (name, module) in entries { @@ -49,7 +41,7 @@ where None => {} } - self.add_to_graph(&mut builder, module.id); + self.add_to_graph(&mut builder, module.id, &mut vec![]); } Ok(( @@ -58,11 +50,31 @@ where all: builder.all, }, builder.graph, + builder.cycles, )) } - fn add_to_graph(&self, builder: &mut PlanBuilder, module_id: ModuleId) { - if !builder.all.contains(&module_id) { + fn add_to_graph( + &self, + builder: &mut PlanBuilder, + module_id: ModuleId, + path: &mut Vec, + ) { + let visited = builder.all.contains(&module_id); + let cycle_rpos = if visited { + path.iter().rposition(|v| *v == module_id) + } else { + None + }; + + if let Some(rpos) = cycle_rpos { + let cycle = path[rpos..].to_vec(); + builder.cycles.push(cycle); + } + + path.push(module_id); + + if !visited { builder.all.push(module_id); } builder.graph.add_node(module_id); @@ -84,8 +96,11 @@ where // Prevent infinite loops. if builder.tracked.insert((module_id, src.module_id)) { - self.add_to_graph(builder, src.module_id); + self.add_to_graph(builder, src.module_id, path); } } + + let res = path.pop(); + debug_assert_eq!(res, Some(module_id)); } } diff --git a/bundler/src/modules/sort/chunk.rs b/bundler/src/modules/sort/chunk.rs index 1a97d4c6295..38bcbbe25c9 100644 --- a/bundler/src/modules/sort/chunk.rs +++ b/bundler/src/modules/sort/chunk.rs @@ -2,10 +2,8 @@ use super::stmt::sort_stmts; use crate::dep_graph::ModuleGraph; use crate::modules::Modules; use crate::ModuleId; -use fxhash::FxBuildHasher; use fxhash::FxHashSet; use indexmap::IndexSet; -use petgraph::algo::all_simple_paths; use petgraph::EdgeDirection::Outgoing; use std::collections::VecDeque; use std::iter::from_fn; @@ -29,6 +27,7 @@ impl Modules { &mut self, entry_id: ModuleId, graph: &ModuleGraph, + cycle: &Vec>, cm: &Lrc, ) -> Vec { let injected_ctxt = self.injected_ctxt; @@ -51,6 +50,7 @@ impl Modules { modules, entry_id, graph, + cycle, cm, )); @@ -64,6 +64,7 @@ fn toposort_real_modules<'a>( mut modules: Vec<(ModuleId, Module)>, entry: ModuleId, graph: &'a ModuleGraph, + cycles: &'a Vec>, cm: &Lrc, ) -> Vec { let mut queue = modules.iter().map(|v| v.0).collect::>(); @@ -72,7 +73,7 @@ fn toposort_real_modules<'a>( let mut chunks = vec![]; let start = Instant::now(); - let sorted_ids = toposort_real_module_ids(queue, graph).collect::>(); + let sorted_ids = toposort_real_module_ids(queue, graph, &cycles).collect::>(); let end = Instant::now(); log::debug!("Toposort of module ids took {:?}", end - start); for ids in sorted_ids { @@ -128,50 +129,35 @@ fn toposort_real_modules<'a>( chunks } -/// Get all modules in a cycle. -fn all_modules_in_circle( +fn cycles_for( + cycles: &Vec>, id: ModuleId, - done: &FxHashSet, - already_in_index: &mut IndexSet, - graph: &ModuleGraph, -) -> IndexSet { - let deps = graph - .neighbors_directed(id, Outgoing) - .filter(|dep| !done.contains(&dep) && !already_in_index.contains(dep)) - .collect::>(); - - let mut ids = deps + checked: &mut Vec, +) -> IndexSet { + checked.push(id); + let mut v = cycles .iter() - .copied() - .flat_map(|dep| { - let mut paths = - all_simple_paths::, _>(&graph, dep, id, 0, None).collect::>(); - - for path in paths.iter_mut() { - path.reverse(); - } - - paths - }) + .filter(|v| v.contains(&id)) .flatten() - .filter(|module_id| !done.contains(&module_id) && !already_in_index.contains(module_id)) - .collect::>(); + .copied() + .collect::>(); - already_in_index.extend(ids.iter().copied()); - let mut new_ids = IndexSet::<_, FxBuildHasher>::default(); + let ids = v.clone(); - for &dep_id in &ids { - let others = all_modules_in_circle(dep_id, done, already_in_index, graph); - new_ids.extend(others) + for added in ids { + if checked.contains(&added) { + continue; + } + v.extend(cycles_for(cycles, added, checked)); } - ids.extend(new_ids); - ids + v } fn toposort_real_module_ids<'a>( mut queue: VecDeque, graph: &'a ModuleGraph, + cycles: &'a Vec>, ) -> impl 'a + Iterator> { let mut done = FxHashSet::::default(); @@ -197,8 +183,7 @@ fn toposort_real_module_ids<'a>( // dbg!(&deps); - let mut all_modules_in_circle = - all_modules_in_circle(id, &done, &mut Default::default(), graph); + let mut all_modules_in_circle = cycles_for(cycles, id, &mut Default::default()); all_modules_in_circle.reverse(); if all_modules_in_circle.is_empty() { diff --git a/bundler/src/modules/sort/mod.rs b/bundler/src/modules/sort/mod.rs index c3b3808ef39..099f6cd8038 100644 --- a/bundler/src/modules/sort/mod.rs +++ b/bundler/src/modules/sort/mod.rs @@ -19,12 +19,18 @@ impl Modules { /// dependency between statements. /// /// TODO: Change this to return [Module]. - pub fn sort(&mut self, entry_id: ModuleId, module_graph: &ModuleGraph, cm: &Lrc) { + pub fn sort( + &mut self, + entry_id: ModuleId, + module_graph: &ModuleGraph, + cycles: &Vec>, + cm: &Lrc, + ) { log::debug!("Sorting {:?}", entry_id); let injected_ctxt = self.injected_ctxt; let start = Instant::now(); - let chunks = self.take_chunks(entry_id, module_graph, cm); + let chunks = self.take_chunks(entry_id, module_graph, cycles, cm); let dur = Instant::now() - start; log::debug!("Sorting took {:?}", dur); diff --git a/bundler/tests/fixture/deno-8211-3/output/entry.inlined.ts b/bundler/tests/fixture/deno-8211-3/output/entry.inlined.ts index f0a9b60a1f3..1d9d88cb81a 100644 --- a/bundler/tests/fixture/deno-8211-3/output/entry.inlined.ts +++ b/bundler/tests/fixture/deno-8211-3/output/entry.inlined.ts @@ -1837,9 +1837,9 @@ function hasInvalidTimeData(obj) { } else return false; } const INVALID = "Invalid DateTime"; +const INVALID1 = "Invalid Duration"; let intlDTCache = { }; -const INVALID1 = "Invalid Duration"; let now = ()=>Date.now() , defaultZone = null, defaultLocale = null, defaultNumberingSystem = null, defaultOutputCalendar = null, throwOnInvalid = false; const INVALID2 = "Invalid Interval"; @@ -2575,6 +2575,17 @@ function diffRelative(start, end, opts) { } return format(0, opts.units[opts.units.length - 1]); } +function friendlyDuration(durationish) { + if (isNumber(durationish)) { + return Duration.fromMillis(durationish); + } else if (Duration.isDuration(durationish)) { + return durationish; + } else if (typeof durationish === "object") { + return Duration.fromObject(durationish); + } else { + throw new InvalidArgumentError(`Unknown duration argument ${durationish} of type ${typeof durationish}`); + } +} class DateTime { constructor(config1){ const zone1 = config1.zone || Settings.defaultZone; @@ -3709,17 +3720,6 @@ class Locale { return this.locale === other.locale && this.numberingSystem === other.numberingSystem && this.outputCalendar === other.outputCalendar; } } -function friendlyDuration(durationish) { - if (isNumber(durationish)) { - return Duration.fromMillis(durationish); - } else if (Duration.isDuration(durationish)) { - return durationish; - } else if (typeof durationish === "object") { - return Duration.fromObject(durationish); - } else { - throw new InvalidArgumentError(`Unknown duration argument ${durationish} of type ${typeof durationish}`); - } -} class Settings { static get now() { return now; diff --git a/bundler/tests/fixture/deno-8211-3/output/entry.ts b/bundler/tests/fixture/deno-8211-3/output/entry.ts index a5487dbabfd..df31b266908 100644 --- a/bundler/tests/fixture/deno-8211-3/output/entry.ts +++ b/bundler/tests/fixture/deno-8211-3/output/entry.ts @@ -1837,9 +1837,9 @@ function hasInvalidTimeData(obj) { } else return false; } const INVALID = "Invalid DateTime"; +const INVALID1 = "Invalid Duration"; let intlDTCache = { }; -const INVALID1 = "Invalid Duration"; let now = ()=>Date.now() , defaultZone = null, defaultLocale = null, defaultNumberingSystem = null, defaultOutputCalendar = null, throwOnInvalid = false; const INVALID2 = "Invalid Interval"; @@ -2576,6 +2576,17 @@ function diffRelative(start, end, opts) { } return format(0, opts.units[opts.units.length - 1]); } +function friendlyDuration(durationish) { + if (isNumber(durationish)) { + return Duration.fromMillis(durationish); + } else if (Duration.isDuration(durationish)) { + return durationish; + } else if (typeof durationish === "object") { + return Duration.fromObject(durationish); + } else { + throw new InvalidArgumentError(`Unknown duration argument ${durationish} of type ${typeof durationish}`); + } +} class DateTime { constructor(config1){ const zone1 = config1.zone || Settings.defaultZone; @@ -3710,17 +3721,6 @@ class Locale { return this.locale === other.locale && this.numberingSystem === other.numberingSystem && this.outputCalendar === other.outputCalendar; } } -function friendlyDuration(durationish) { - if (isNumber(durationish)) { - return Duration.fromMillis(durationish); - } else if (Duration.isDuration(durationish)) { - return durationish; - } else if (typeof durationish === "object") { - return Duration.fromObject(durationish); - } else { - throw new InvalidArgumentError(`Unknown duration argument ${durationish} of type ${typeof durationish}`); - } -} class Settings { static get now() { return now; diff --git a/spack/tests/pass/circular/top-level-idents/output/entry.js b/spack/tests/pass/circular/top-level-idents/output/entry.js index 6f2d74b6140..3330e73d33d 100644 --- a/spack/tests/pass/circular/top-level-idents/output/entry.js +++ b/spack/tests/pass/circular/top-level-idents/output/entry.js @@ -1,4 +1,4 @@ console.log('a'); -console.log('c'); console.log('b'); +console.log('c'); console.log('entry');