Skip to content

Commit

Permalink
fix: compilation.import_module use immutable ref (#6311)
Browse files Browse the repository at this point in the history
  • Loading branch information
jerrykingxyz authored Apr 22, 2024
1 parent 46a331b commit c9f4b54
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 44 deletions.
9 changes: 3 additions & 6 deletions crates/rspack_binding_values/src/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ impl JsCompilation {
#[allow(clippy::too_many_arguments)]
#[napi]
pub fn import_module(
&'static mut self,
&'static self,
env: Env,
request: String,
public_path: Option<String>,
Expand All @@ -452,7 +452,7 @@ impl JsCompilation {
let module_executor = self
.0
.module_executor
.as_mut()
.as_ref()
.expect("should have module executor");
let result = module_executor
.import_module(
Expand Down Expand Up @@ -491,12 +491,9 @@ impl JsCompilation {
.into_iter()
.map(|d| d.to_string_lossy().to_string())
.collect(),
assets: res.assets.keys().cloned().collect(),
assets: res.assets.into_iter().collect(),
id: res.id,
};
for (filename, asset) in res.assets {
self.0.emit_asset(filename, asset)
}
Ok(js_result)
}
Err(e) => Err(Error::new(napi::Status::GenericFailure, format!("{e}"))),
Expand Down
10 changes: 10 additions & 0 deletions crates/rspack_core/src/compiler/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1009,6 +1009,16 @@ impl Compilation {
self.create_chunk_assets(plugin_driver.clone()).await?;
logger.time_end(start);

// sync assets to compilation from module_executor
let assets = self
.module_executor
.as_mut()
.map(|module_executor| std::mem::take(&mut module_executor.assets))
.unwrap_or_default();
for (filename, asset) in assets {
self.emit_asset(filename, asset)
}

let start = logger.time("process assets");
plugin_driver
.compilation_hooks
Expand Down
4 changes: 2 additions & 2 deletions crates/rspack_core/src/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ where
let (plugin_driver, options) = PluginDriver::new(options, plugins, resolver_factory.clone());
let cache = Arc::new(Cache::new(options.clone()));
assert!(!(options.is_new_tree_shaking() && options.builtins.tree_shaking.enable()), "Can't enable builtins.tree_shaking and `experiments.rspack_future.new_treeshaking` at the same time");
let module_executor = ModuleExecutor::new(options.is_new_tree_shaking());
let module_executor = ModuleExecutor::default();
Self {
options: options.clone(),
compilation: Compilation::new(
Expand Down Expand Up @@ -121,7 +121,7 @@ where
// TODO: maybe it's better to use external entries.
self.plugin_driver.resolver_factory.clear_cache();

let module_executor = ModuleExecutor::new(self.options.is_new_tree_shaking());
let module_executor = ModuleExecutor::default();
fast_set(
&mut self.compilation,
Compilation::new(
Expand Down
58 changes: 22 additions & 36 deletions crates/rspack_core/src/compiler/module_executor.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::sync::{atomic::AtomicU32, Arc};
use std::{collections::hash_map, hash::BuildHasherDefault, iter::once};
use std::{hash::BuildHasherDefault, iter::once};

use dashmap::DashMap;
use rayon::prelude::*;
use rspack_error::Result;
use rspack_identifier::{Identifiable, IdentifierSet};
Expand All @@ -9,10 +10,10 @@ use rustc_hash::{FxHashMap as HashMap, FxHashSet as HashSet, FxHasher};
use crate::cache::Cache;
use crate::{
Chunk, ChunkGraph, ChunkKind, CodeGenerationDataAssetInfo, CodeGenerationDataFilename,
CodeGenerationResult, CompilationAssets, Dependency, DependencyType, EntryDependency,
EntryOptions, Entrypoint, ModuleFactory, ModuleGraphPartial, SourceType,
CodeGenerationResult, Dependency, DependencyType, EntryDependency, EntryOptions, Entrypoint,
ModuleFactory, SourceType,
};
use crate::{Compilation, CompilationAsset, DependencyId, MakeParam};
use crate::{Compilation, CompilationAsset, MakeParam};
use crate::{CompilerOptions, Context, ResolverFactory, SharedPluginDriver};

static EXECUTE_MODULE_ID: AtomicU32 = AtomicU32::new(0);
Expand All @@ -24,27 +25,19 @@ pub struct ExecuteModuleResult {
pub context_dependencies: HashSet<std::path::PathBuf>,
pub missing_dependencies: HashSet<std::path::PathBuf>,
pub build_dependencies: HashSet<std::path::PathBuf>,
pub assets: CompilationAssets,
pub assets: HashSet<String>,
pub id: ExecuteModuleId,
}

#[derive(Debug)]
#[derive(Debug, Default)]
pub struct ModuleExecutor {
pub make_module_graph: ModuleGraphPartial,
request_dep_map: HashMap<String, DependencyId>,
pub assets: DashMap<String, CompilationAsset>,
}

impl ModuleExecutor {
pub fn new(is_new_treeshaking: bool) -> Self {
Self {
make_module_graph: ModuleGraphPartial::new(is_new_treeshaking),
request_dep_map: HashMap::default(),
}
}

#[allow(clippy::too_many_arguments)]
pub async fn import_module(
&mut self,
&self,
options: Arc<CompilerOptions>,
plugin_driver: SharedPluginDriver,
resolver_factory: Arc<ResolverFactory>,
Expand All @@ -67,21 +60,16 @@ impl ModuleExecutor {
None,
);
compilation.dependency_factories = dependency_factories;
compilation.swap_make_module_graph(&mut self.make_module_graph);

let mut mg = compilation.get_module_graph_mut();
let dep_id = match self.request_dep_map.entry(request.clone()) {
hash_map::Entry::Vacant(v) => {
let dep = EntryDependency::new(
request,
original_module_context.unwrap_or(Context::from("")),
);
let dep_id = *dep.id();
mg.add_dependency(Box::new(dep));
v.insert(dep_id);
dep_id
}
hash_map::Entry::Occupied(v) => *v.get(),
let dep_id = {
let dep = EntryDependency::new(
request,
original_module_context.unwrap_or(Context::from("")),
);
let dep_id = *dep.id();
mg.add_dependency(Box::new(dep));
dep_id
};

compilation
Expand Down Expand Up @@ -278,18 +266,16 @@ impl ModuleExecutor {
};

if let Ok(ref mut result) = execute_result {
std::mem::swap(&mut result.assets, compilation.assets_mut());
let assets = std::mem::take(compilation.assets_mut());
for (key, value) in assets {
result.assets.insert(key.clone());
self.assets.insert(key, value);
}
}

let mut has_error = false;
for error in compilation.get_errors() {
has_error = true;
error.render_report(true)?;
}
if !has_error {
// save make module_graph for next import_module
compilation.swap_make_module_graph(&mut self.make_module_graph);
}

execute_result
}
Expand Down

2 comments on commit c9f4b54

@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 ❌ failure
_selftest, ubuntu-latest ✅ success
nx, ubuntu-latest ❌ failure
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-04-22 72638cb) Current Change
10000_development-mode + exec 2.67 s ± 31 ms 2.66 s ± 18 ms -0.20 %
10000_development-mode_hmr + exec 685 ms ± 7.4 ms 685 ms ± 9.5 ms +0.08 %
10000_production-mode + exec 2.46 s ± 29 ms 2.5 s ± 40 ms +1.55 %
arco-pro_development-mode + exec 2.47 s ± 80 ms 2.48 s ± 73 ms +0.42 %
arco-pro_development-mode_hmr + exec 429 ms ± 1.3 ms 428 ms ± 1.3 ms -0.29 %
arco-pro_development-mode_hmr_intercept-plugin + exec 440 ms ± 4.1 ms 441 ms ± 2.5 ms +0.12 %
arco-pro_development-mode_intercept-plugin + exec 3.23 s ± 71 ms 3.23 s ± 66 ms -0.12 %
arco-pro_production-mode + exec 3.93 s ± 107 ms 3.97 s ± 64 ms +0.87 %
arco-pro_production-mode_intercept-plugin + exec 4.73 s ± 74 ms 4.78 s ± 52 ms +1.08 %
threejs_development-mode_10x + exec 2.05 s ± 21 ms 2.05 s ± 18 ms -0.12 %
threejs_development-mode_10x_hmr + exec 746 ms ± 11 ms 763 ms ± 8.6 ms +2.29 %
threejs_production-mode_10x + exec 5.16 s ± 40 ms 5.19 s ± 34 ms +0.71 %

Please sign in to comment.