-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] fix: treeshaking & skipmodule should jump the async module #1661
Changes from all commits
cc6a69c
2156563
16e8927
193ccec
e1422ea
c581062
88bdaba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,4 +1,5 @@ | ||||||||||||||||||||||||||||||||||||||||||||
use std::collections::HashMap; | ||||||||||||||||||||||||||||||||||||||||||||
use std::ops::DerefMut; | ||||||||||||||||||||||||||||||||||||||||||||
use std::sync::mpsc::channel; | ||||||||||||||||||||||||||||||||||||||||||||
use std::sync::Arc; | ||||||||||||||||||||||||||||||||||||||||||||
use std::time::Instant; | ||||||||||||||||||||||||||||||||||||||||||||
|
@@ -34,7 +35,7 @@ | |||||||||||||||||||||||||||||||||||||||||||
use crate::visitors::optimize_define_utils::OptimizeDefineUtils; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
impl Compiler { | ||||||||||||||||||||||||||||||||||||||||||||
pub fn transform_all(&self, async_deps_map: HashMap<ModuleId, Vec<Dependency>>) -> Result<()> { | ||||||||||||||||||||||||||||||||||||||||||||
pub fn transform_all(&self) -> Result<()> { | ||||||||||||||||||||||||||||||||||||||||||||
let t = Instant::now(); | ||||||||||||||||||||||||||||||||||||||||||||
let context = &self.context; | ||||||||||||||||||||||||||||||||||||||||||||
let module_ids = { | ||||||||||||||||||||||||||||||||||||||||||||
|
@@ -47,39 +48,40 @@ | |||||||||||||||||||||||||||||||||||||||||||
.collect::<Vec<_>>() | ||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
transform_modules_in_thread(&module_ids, context, async_deps_map)?; | ||||||||||||||||||||||||||||||||||||||||||||
transform_modules_in_thread(&module_ids, context)?; | ||||||||||||||||||||||||||||||||||||||||||||
debug!(">> transform modules in {}ms", t.elapsed().as_millis()); | ||||||||||||||||||||||||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
pub fn transform_modules(module_ids: Vec<ModuleId>, context: &Arc<Context>) -> Result<()> { | ||||||||||||||||||||||||||||||||||||||||||||
let t = Instant::now(); | ||||||||||||||||||||||||||||||||||||||||||||
let async_deps_by_module_id = mark_async(&module_ids, context); | ||||||||||||||||||||||||||||||||||||||||||||
let mut module_graph = context.module_graph.write().unwrap(); | ||||||||||||||||||||||||||||||||||||||||||||
mark_async(module_graph.deref_mut(), &module_ids, context); | ||||||||||||||||||||||||||||||||||||||||||||
debug!(">> mark async in {}ms", t.elapsed().as_millis()); | ||||||||||||||||||||||||||||||||||||||||||||
let t = Instant::now(); | ||||||||||||||||||||||||||||||||||||||||||||
transform_modules_in_thread(&module_ids, context, async_deps_by_module_id)?; | ||||||||||||||||||||||||||||||||||||||||||||
transform_modules_in_thread(&module_ids, context)?; | ||||||||||||||||||||||||||||||||||||||||||||
debug!(">> transform modules in {}ms", t.elapsed().as_millis()); | ||||||||||||||||||||||||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
pub fn transform_modules_in_thread( | ||||||||||||||||||||||||||||||||||||||||||||
module_ids: &Vec<ModuleId>, | ||||||||||||||||||||||||||||||||||||||||||||
context: &Arc<Context>, | ||||||||||||||||||||||||||||||||||||||||||||
async_deps_by_module_id: HashMap<ModuleId, Vec<Dependency>>, | ||||||||||||||||||||||||||||||||||||||||||||
) -> Result<()> { | ||||||||||||||||||||||||||||||||||||||||||||
crate::mako_profile_function!(); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
let (rs, rr) = channel::<Result<(ModuleId, ModuleAst)>>(); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
for module_id in module_ids { | ||||||||||||||||||||||||||||||||||||||||||||
let async_deps_by_module_id = context.async_deps_map.read().unwrap(); | ||||||||||||||||||||||||||||||||||||||||||||
let context = context.clone(); | ||||||||||||||||||||||||||||||||||||||||||||
let rs = rs.clone(); | ||||||||||||||||||||||||||||||||||||||||||||
let module_id = module_id.clone(); | ||||||||||||||||||||||||||||||||||||||||||||
let async_deps = async_deps_by_module_id | ||||||||||||||||||||||||||||||||||||||||||||
.get(&module_id) | ||||||||||||||||||||||||||||||||||||||||||||
.expect(&module_id.id) | ||||||||||||||||||||||||||||||||||||||||||||
.clone(); | ||||||||||||||||||||||||||||||||||||||||||||
.cloned() | ||||||||||||||||||||||||||||||||||||||||||||
.unwrap_or(vec![]); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+76
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 建议优化异步依赖的获取逻辑 当前实现中有以下几个需要注意的地方:
建议改进实现: - let async_deps_by_module_id = context.async_deps_map.read().unwrap();
- let async_deps = async_deps_by_module_id
- .get(&module_id)
- .cloned()
- .unwrap_or(vec![]);
+ let async_deps = context.async_deps_map
+ .read()
+ .map_err(|e| anyhow::anyhow!("获取 async_deps_map 读锁失败: {}", e))?
+ .get(&module_id)
+ .map(|deps| deps.as_slice())
+ .unwrap_or(&[])
+ .to_vec(); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
thread_pool::spawn(move || { | ||||||||||||||||||||||||||||||||||||||||||||
let module_graph = context.module_graph.read().unwrap(); | ||||||||||||||||||||||||||||||||||||||||||||
let deps = module_graph.get_dependencies(&module_id); | ||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -491,7 +491,13 @@ | |||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
impl Debug for Module { | ||||||||||||||||||||||||||||||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||||||||||||||||||||||||||||||
write!(f, "id={}({:?})", self.id.id, self.get_module_type()) | ||||||||||||||||||||||||||||||
write!( | ||||||||||||||||||||||||||||||
f, | ||||||||||||||||||||||||||||||
"id={}({:?}) is_async={:?}", | ||||||||||||||||||||||||||||||
self.id.id, | ||||||||||||||||||||||||||||||
self.get_module_type(), | ||||||||||||||||||||||||||||||
self.info.as_ref().unwrap().is_async | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
Comment on lines
+494
to
+500
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 注意:Debug 实现中存在潜在的 panic 风险 当前的 Debug 实现中使用了 建议修改为以下实现: - write!(
- f,
- "id={}({:?}) is_async={:?}",
- self.id.id,
- self.get_module_type(),
- self.info.as_ref().unwrap().is_async
- )
+ write!(
+ f,
+ "id={}({:?}) is_async={:?}",
+ self.id.id,
+ self.get_module_type(),
+ self.info.as_ref().map_or(false, |info| info.is_async)
+ ) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -21,25 +21,33 @@ | |||||
use crate::plugins::tree_shaking::shake::module_concatenate::optimize_module_graph; | ||||||
use crate::plugins::tree_shaking::statement_graph::{ExportInfo, ExportSpecifierInfo, ImportInfo}; | ||||||
use crate::plugins::tree_shaking::{module, remove_useless_stmts, statement_graph}; | ||||||
use crate::visitors::async_module::mark_async; | ||||||
use crate::{mako_profile_function, mako_profile_scope}; | ||||||
|
||||||
type TreeShakingModuleMap = HashMap<ModuleId, RefCell<TreeShakeModule>>; | ||||||
|
||||||
pub fn optimize_modules(module_graph: &mut ModuleGraph, context: &Arc<Context>) -> Result<()> { | ||||||
let module_ids = { | ||||||
let (mut module_ids, _) = module_graph.toposort(); | ||||||
// start from the leaf nodes, so reverser the sort | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 修正注释中的拼写错误 注释中“reverser”应为“reverse”,请修正拼写错误。 建议修改为: - // start from the leaf nodes, so reverser the sort
+ // start from the leaf nodes, so reverse the sort 📝 Committable suggestion
Suggested change
|
||||||
module_ids.reverse(); | ||||||
module_ids | ||||||
}; | ||||||
mark_async(module_graph, &module_ids, context); | ||||||
let (topo_sorted_modules, _cyclic_modules) = { | ||||||
mako_profile_scope!("tree shake topo-sort"); | ||||||
module_graph.toposort() | ||||||
}; | ||||||
|
||||||
let (_skipped, tree_shake_modules_ids): (Vec<ModuleId>, Vec<ModuleId>) = | ||||||
topo_sorted_modules.into_iter().partition(|module_id| { | ||||||
let module = module_graph.get_module(module_id).unwrap(); | ||||||
|
||||||
let module_type = module.get_module_type(); | ||||||
|
||||||
// skip non script modules and external modules | ||||||
if module_type != ModuleType::Script || module.is_external() { | ||||||
if module_type != ModuleType::Script && !module.is_external() { | ||||||
let is_async = &module.info.as_ref().unwrap().is_async; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 避免使用可能导致崩溃的 使用 可以考虑修改为: -let is_async = &module.info.as_ref().unwrap().is_async;
+let is_async = module.info.as_ref().map_or(&false, |info| &info.is_async); 📝 Committable suggestion
Suggested change
|
||||||
if module_type != ModuleType::Script || module.is_external() || *is_async { | ||||||
if module_type != ModuleType::Script && !module.is_external() || *is_async { | ||||||
// mark all non script modules' script dependencies as side_effects | ||||||
for dep_id in module_graph.dependence_module_ids(module_id) { | ||||||
let dep_module = module_graph.get_module_mut(&dep_id).unwrap(); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
use std::collections::HashMap; | ||
use std::collections::{HashSet, VecDeque}; | ||
use std::sync::Arc; | ||
|
||
use swc_core::common::util::take::Take; | ||
|
@@ -14,6 +14,7 @@ | |
use crate::ast::DUMMY_CTXT; | ||
use crate::compiler::Context; | ||
use crate::module::{Dependency, ModuleId}; | ||
use crate::module_graph::ModuleGraph; | ||
|
||
const ASYNC_IMPORTED_MODULE: &str = "_async__mako_imported_module_"; | ||
|
||
|
@@ -205,30 +206,61 @@ | |
} | ||
} | ||
|
||
pub fn mark_async( | ||
module_ids: &[ModuleId], | ||
context: &Arc<Context>, | ||
) -> HashMap<ModuleId, Vec<Dependency>> { | ||
let mut async_deps_by_module_id = HashMap::new(); | ||
let mut module_graph = context.module_graph.write().unwrap(); | ||
// TODO: 考虑成环的场景 | ||
pub fn mark_async(module_graph: &mut ModuleGraph, module_ids: &[ModuleId], context: &Arc<Context>) { | ||
let mut to_visit_queue = module_graph | ||
.modules() | ||
.iter() | ||
.filter_map(|m| { | ||
m.info | ||
.as_ref() | ||
.and_then(|i| if i.is_async { Some(m.id.clone()) } else { None }) | ||
}) | ||
.collect::<VecDeque<_>>(); | ||
let mut visited = HashSet::new(); | ||
|
||
// polluted async to dependants | ||
while let Some(module_id) = to_visit_queue.pop_front() { | ||
if visited.contains(&module_id) { | ||
continue; | ||
} | ||
|
||
module_graph | ||
.get_dependents(&module_id) | ||
.iter() | ||
.filter_map(|(dependant, dependency)| { | ||
if !dependency.resolve_type.is_sync_esm() { | ||
return None; | ||
} | ||
if !visited.contains(*dependant) { | ||
Some((*dependant).clone()) | ||
} else { | ||
None | ||
} | ||
}) | ||
.collect::<Vec<_>>() | ||
.iter() | ||
.for_each(|module_id| { | ||
let m = module_graph.get_module_mut(module_id).unwrap(); | ||
m.info.as_mut().unwrap().is_async = true; | ||
Comment on lines
+243
to
+244
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 建议避免在生产代码中使用 在第243行, 您可以将代码修改为: -let m = module_graph.get_module_mut(module_id).unwrap();
+if let Some(m) = module_graph.get_module_mut(module_id) {
+ m.info.as_mut().unwrap().is_async = true;
+ // 继续处理
+} else {
+ // 处理未找到模块的情况,例如记录日志或跳过
+ continue;
+}
|
||
|
||
to_visit_queue.push_back(module_id.clone()); | ||
}); | ||
|
||
visited.insert(module_id.clone()); | ||
} | ||
|
||
Comment on lines
+209
to
+251
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification 发现未更新的 在代码库中发现一处调用尚未更新:
🔗 Analysis chain确认
运行以下脚本以查找未更新的 该脚本将帮助您定位可能遗漏更新的函数调用,以确保新签名在整个代码库中一致使用。 Let me analyze the actual usage and definition of the mark_async function to verify if all calls have been properly updated. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# 描述:查找未传入`module_graph`参数的`mark_async`函数调用
# 期望结果:所有调用都包含`module_graph`参数
rg 'mark_async\s*\(\s*module_ids' -A1
Length of output: 39 Script: #!/bin/bash
# 搜索所有mark_async的调用
rg "mark_async\(" -A 2
# 搜索mark_async函数的定义
ast-grep --pattern 'fn mark_async($$$)'
Length of output: 919 |
||
module_ids.iter().for_each(|module_id| { | ||
let deps = module_graph.get_dependencies_info(module_id); | ||
let mut async_deps_map = context.async_deps_map.write().unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 处理写锁获取失败的情况 在第254行,使用了 您可以将代码修改为: -let mut async_deps_map = context.async_deps_map.write().unwrap();
+let mut async_deps_map = match context.async_deps_map.write() {
+ Ok(lock) => lock,
+ Err(e) => {
+ // 处理获取锁失败的情况,例如记录错误并返回
+ eprintln!("Failed to acquire write lock: {}", e);
+ return;
+ }
+};
|
||
let async_deps: Vec<Dependency> = deps | ||
.into_iter() | ||
.filter(|(_, dep, is_async)| dep.resolve_type.is_sync_esm() && *is_async) | ||
.map(|(_, dep, _)| dep.clone()) | ||
.collect(); | ||
let module = module_graph.get_module_mut(module_id).unwrap(); | ||
if let Some(info) = module.info.as_mut() { | ||
// a module with async deps need to be polluted into async module | ||
if !info.is_async && !async_deps.is_empty() { | ||
info.is_async = true; | ||
} | ||
async_deps_by_module_id.insert(module_id.clone(), async_deps); | ||
if !async_deps.is_empty() { | ||
async_deps_map.insert(module_id.clone(), async_deps); | ||
} | ||
}); | ||
async_deps_by_module_id | ||
} | ||
|
||
#[cfg(test)] | ||
|
@@ -255,8 +287,8 @@ | |
"# | ||
.trim()); | ||
assert_eq!( | ||
code, | ||
r#" | ||
code, | ||
r#" | ||
__mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{ | ||
"use strict"; | ||
Object.defineProperty(exports, "__esModule", { | ||
|
@@ -273,7 +305,7 @@ | |
asyncResult(); | ||
}, true); | ||
"#.trim() | ||
); | ||
); | ||
} | ||
|
||
#[test] | ||
|
@@ -286,8 +318,8 @@ | |
"# | ||
.trim()); | ||
assert_eq!( | ||
code, | ||
r#" | ||
code, | ||
r#" | ||
__mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{ | ||
"use strict"; | ||
Object.defineProperty(exports, "__esModule", { | ||
|
@@ -308,8 +340,8 @@ | |
asyncResult(); | ||
}, true); | ||
"# | ||
.trim() | ||
); | ||
.trim() | ||
); | ||
} | ||
|
||
#[test] | ||
|
@@ -321,8 +353,8 @@ | |
"# | ||
.trim()); | ||
assert_eq!( | ||
code, | ||
r#" | ||
code, | ||
r#" | ||
__mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{ | ||
"use strict"; | ||
Object.defineProperty(exports, "__esModule", { | ||
|
@@ -340,8 +372,8 @@ | |
asyncResult(); | ||
}, true); | ||
"# | ||
.trim() | ||
); | ||
.trim() | ||
); | ||
} | ||
|
||
#[test] | ||
|
@@ -374,8 +406,8 @@ | |
"# | ||
.trim()); | ||
assert_eq!( | ||
code, | ||
r#" | ||
code, | ||
r#" | ||
__mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{ | ||
"use strict"; | ||
Object.defineProperty(exports, "__esModule", { | ||
|
@@ -393,7 +425,7 @@ | |
asyncResult(); | ||
}, true); | ||
"#.trim() | ||
); | ||
); | ||
} | ||
|
||
fn run(js_code: &str) -> String { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建议改进 mark_async 的错误处理
直接使用
write().unwrap()
获取锁可能在锁竞争或毒锁情况下导致程序崩溃。建议添加适当的错误处理: