fix: tapable run in wrong closure (#8376)

web-infra-dev · Nov 8, 2024 · 75f9573 · 75f9573 · rspack-bot · Nov 8, 2024
1 parent b329f87
commit 75f9573
Show file tree

Hide file tree

Showing 9 changed files with 288 additions and 28 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/node_binding/Cargo.toml b/crates/node_binding/Cargo.toml
@@ -32,11 +32,9 @@ rspack_plugin_javascript = { version = "0.1.0", path = "../rspack_plugin_javascr
 rspack_util              = { version = "0.1.0", path = "../rspack_util" }
 
 rspack_tracing = { version = "0.1.0", path = "../rspack_tracing" }
-tokio          = { workspace = true, features = ["rt", "rt-multi-thread", "tracing"] }
 
 async-trait = { workspace = true }
 cow-utils   = { workspace = true }
-once_cell   = { workspace = true }
 tracing     = { workspace = true }
 
 napi        = { workspace = true }

diff --git a/crates/node_binding/src/plugins/interceptor.rs b/crates/node_binding/src/plugins/interceptor.rs
@@ -1,5 +1,4 @@
 use std::{
-  borrow::Cow,
   hash::Hash,
   sync::{Arc, RwLock},
 };
@@ -135,28 +134,22 @@ impl<T: 'static + JsValuesTupleIntoVec, R> Clone for RegisterJsTapsInner<T, R> {
 
 enum RegisterJsTapsCache<T: 'static + JsValuesTupleIntoVec, R> {
   NoCache,
-  Cache(Arc<tokio::sync::OnceCell<RegisterFunctionOutput<T, R>>>),
-  SyncCache(Arc<once_cell::sync::OnceCell<RegisterFunctionOutput<T, R>>>),
+  Cache(Arc<RwLock<Option<RegisterFunctionOutput<T, R>>>>),
 }
 
 impl<T: 'static + JsValuesTupleIntoVec, R> Clone for RegisterJsTapsCache<T, R> {
   fn clone(&self) -> Self {
     match self {
       Self::NoCache => Self::NoCache,
       Self::Cache(c) => Self::Cache(c.clone()),
-      Self::SyncCache(c) => Self::SyncCache(c.clone()),
     }
   }
 }
 
 impl<T: 'static + JsValuesTupleIntoVec, R> RegisterJsTapsCache<T, R> {
-  pub fn new(cache: bool, sync: bool) -> Self {
+  pub fn new(cache: bool, _sync: bool) -> Self {
     if cache {
-      if sync {
-        Self::SyncCache(Default::default())
-      } else {
-        Self::Cache(Default::default())
-      }
+      Self::Cache(Default::default())
     } else {
       Self::NoCache
     }
@@ -180,15 +173,27 @@ impl<T: 'static + ToNapiValue, R: 'static + FromNapiValue> RegisterJsTapsInner<T
   pub async fn call_register(
     &self,
     hook: &impl Hook,
-  ) -> rspack_error::Result<Cow<RegisterFunctionOutput<T, R>>> {
-    if let RegisterJsTapsCache::Cache(cache) = &self.cache {
-      let js_taps = cache
-        .get_or_try_init(|| self.call_register_impl(hook))
-        .await?;
-      Ok(Cow::Borrowed(js_taps))
+  ) -> rspack_error::Result<RegisterFunctionOutput<T, R>> {
+    if let RegisterJsTapsCache::Cache(rw) = &self.cache {
+      let cache = {
+        #[allow(clippy::unwrap_used)]
+        rw.read().unwrap().clone()
+      };
+      Ok(match cache {
+        Some(js_taps) => js_taps,
+        None => {
+          let js_taps = self.call_register_impl(hook).await?;
+          {
+            #[allow(clippy::unwrap_used)]
+            let mut cache = rw.write().unwrap();
+            *cache = Some(js_taps.clone());
+          }
+          js_taps
+        }
+      })
     } else {
       let js_taps = self.call_register_impl(hook).await?;
-      Ok(Cow::Owned(js_taps))
+      Ok(js_taps)
     }
   }
 
@@ -204,13 +209,27 @@ impl<T: 'static + ToNapiValue, R: 'static + FromNapiValue> RegisterJsTapsInner<T
   pub fn call_register_blocking(
     &self,
     hook: &impl Hook,
-  ) -> rspack_error::Result<Cow<RegisterFunctionOutput<T, R>>> {
-    if let RegisterJsTapsCache::SyncCache(cache) = &self.cache {
-      let js_taps = cache.get_or_try_init(|| self.call_register_blocking_impl(hook))?;
-      Ok(Cow::Borrowed(js_taps))
+  ) -> rspack_error::Result<RegisterFunctionOutput<T, R>> {
+    if let RegisterJsTapsCache::Cache(rw) = &self.cache {
+      let cache = {
+        #[allow(clippy::unwrap_used)]
+        rw.read().unwrap().clone()
+      };
+      Ok(match cache {
+        Some(js_taps) => js_taps,
+        None => {
+          let js_taps = self.call_register_blocking_impl(hook)?;
+          {
+            #[allow(clippy::unwrap_used)]
+            let mut cache = rw.write().unwrap();
+            *cache = Some(js_taps.clone());
+          }
+          js_taps
+        }
+      })
     } else {
       let js_taps = self.call_register_blocking_impl(hook)?;
-      Ok(Cow::Owned(js_taps))
+      Ok(js_taps)
     }
   }
 
@@ -222,6 +241,17 @@ impl<T: 'static + ToNapiValue, R: 'static + FromNapiValue> RegisterJsTapsInner<T
     used_stages.sort();
     self.register.blocking_call_with_sync(used_stages)
   }
+
+  fn clear_cache(&self) {
+    match &self.cache {
+      RegisterJsTapsCache::NoCache => {}
+      RegisterJsTapsCache::Cache(cache) => {
+        #[allow(clippy::unwrap_used)]
+        let mut cache = cache.write().unwrap();
+        *cache = None;
+      }
+    }
+  }
 }
 
 /// define js taps register
@@ -243,6 +273,12 @@ macro_rules! define_register {
       inner: RegisterJsTapsInner<$arg, $ret>,
     }
 
+    impl $name {
+      pub fn clear_cache(&self) {
+        self.inner.clear_cache();
+      }
+    }
+
     #[derive(Clone)]
     struct $tap_name {
       function: ThreadsafeFunction<$arg, $ret>,

diff --git a/crates/node_binding/src/plugins/mod.rs b/crates/node_binding/src/plugins/mod.rs
@@ -312,6 +312,90 @@ impl rspack_core::Plugin for JsHooksAdapterPlugin {
 
     Ok(())
   }
+
+  fn clear_cache(&self) {
+    self.register_compiler_this_compilation_taps.clear_cache();
+    self.register_compiler_compilation_taps.clear_cache();
+    self.register_compiler_make_taps.clear_cache();
+    self.register_compiler_finish_make_taps.clear_cache();
+    self.register_compiler_should_emit_taps.clear_cache();
+    self.register_compiler_emit_taps.clear_cache();
+    self.register_compiler_after_emit_taps.clear_cache();
+    self.register_compiler_asset_emitted_taps.clear_cache();
+    self.register_compilation_build_module_taps.clear_cache();
+    self
+      .register_compilation_still_valid_module_taps
+      .clear_cache();
+    self.register_compilation_succeed_module_taps.clear_cache();
+    self.register_compilation_execute_module_taps.clear_cache();
+    self.register_compilation_finish_modules_taps.clear_cache();
+    self
+      .register_compilation_optimize_modules_taps
+      .clear_cache();
+    self
+      .register_compilation_after_optimize_modules_taps
+      .clear_cache();
+    self.register_compilation_optimize_tree_taps.clear_cache();
+    self
+      .register_compilation_optimize_chunk_modules_taps
+      .clear_cache();
+    self
+      .register_compilation_additional_tree_runtime_requirements
+      .clear_cache();
+    self
+      .register_compilation_runtime_requirement_in_tree
+      .clear_cache();
+    self.register_compilation_runtime_module_taps.clear_cache();
+    self.register_compilation_chunk_hash_taps.clear_cache();
+    self.register_compilation_chunk_asset_taps.clear_cache();
+    self.register_compilation_process_assets_taps.clear_cache();
+    self
+      .register_compilation_after_process_assets_taps
+      .clear_cache();
+    self.register_compilation_seal_taps.clear_cache();
+    self.register_compilation_after_seal_taps.clear_cache();
+    self
+      .register_normal_module_factory_before_resolve_taps
+      .clear_cache();
+    self
+      .register_normal_module_factory_factorize_taps
+      .clear_cache();
+    self
+      .register_normal_module_factory_resolve_taps
+      .clear_cache();
+    self
+      .register_normal_module_factory_resolve_for_scheme_taps
+      .clear_cache();
+    self
+      .register_normal_module_factory_after_resolve_taps
+      .clear_cache();
+    self
+      .register_normal_module_factory_create_module_taps
+      .clear_cache();
+    self
+      .register_context_module_factory_before_resolve_taps
+      .clear_cache();
+    self
+      .register_context_module_factory_after_resolve_taps
+      .clear_cache();
+    self
+      .register_javascript_modules_chunk_hash_taps
+      .clear_cache();
+    self
+      .register_html_plugin_before_asset_tag_generation_taps
+      .clear_cache();
+    self
+      .register_html_plugin_alter_asset_tags_taps
+      .clear_cache();
+    self
+      .register_html_plugin_alter_asset_tag_groups_taps
+      .clear_cache();
+    self
+      .register_html_plugin_after_template_execution_taps
+      .clear_cache();
+    self.register_html_plugin_before_emit_taps.clear_cache();
+    self.register_html_plugin_after_emit_taps.clear_cache();
+  }
 }
 
 #[plugin_hook(CompilerCompilation for JsHooksAdapterPlugin)]

diff --git a/crates/rspack_core/src/compiler/hmr.rs b/crates/rspack_core/src/compiler/hmr.rs
@@ -61,7 +61,8 @@ impl Compiler {
       self
         .old_cache
         .set_modified_files(all_files.into_iter().collect());
-      self.plugin_driver.resolver_factory.clear_cache();
+
+      self.plugin_driver.clear_cache();
 
       let mut new_compilation = Compilation::new(
         self.options.clone(),

diff --git a/crates/rspack_core/src/compiler/mod.rs b/crates/rspack_core/src/compiler/mod.rs
@@ -143,7 +143,7 @@ impl Compiler {
     self.old_cache.end_idle();
     // TODO: clear the outdated cache entries in resolver,
     // TODO: maybe it's better to use external entries.
-    self.plugin_driver.resolver_factory.clear_cache();
+    self.plugin_driver.clear_cache();
 
     let module_executor = ModuleExecutor::default();
     fast_set(

diff --git a/crates/rspack_core/src/plugin/mod.rs b/crates/rspack_core/src/plugin/mod.rs
@@ -22,6 +22,8 @@ pub trait Plugin: fmt::Debug + Send + Sync {
   ) -> Result<()> {
     Ok(())
   }
+
+  fn clear_cache(&self) {}
 }
 
 pub type BoxPlugin = Box<dyn Plugin>;

diff --git a/crates/rspack_core/src/plugin/plugin_driver.rs b/crates/rspack_core/src/plugin/plugin_driver.rs
@@ -76,4 +76,11 @@ impl PluginDriver {
     let mut diagnostic = self.diagnostics.lock().expect("TODO:");
     std::mem::take(&mut diagnostic)
   }
+
+  pub fn clear_cache(&self) {
+    self.resolver_factory.clear_cache();
+    for plugin in &self.plugins {
+      plugin.clear_cache();
+    }
+  }
 }
Name	Base (2024-11-08 dfbe05f)	Current	Change
10000_big_production-mode + exec	44.5 s ± 1.64 s	44.3 s ± 1.29 s	-0.54 %
10000_development-mode + exec	1.84 s ± 19 ms	1.85 s ± 18 ms	+0.69 %
10000_development-mode_hmr + exec	644 ms ± 4.5 ms	644 ms ± 2.6 ms	-0.12 %
10000_production-mode + exec	2.43 s ± 34 ms	2.44 s ± 31 ms	+0.68 %
arco-pro_development-mode + exec	1.79 s ± 60 ms	1.78 s ± 68 ms	-0.89 %
arco-pro_development-mode_hmr + exec	430 ms ± 2.3 ms	431 ms ± 1.5 ms	+0.14 %
arco-pro_production-mode + exec	3.2 s ± 61 ms	3.2 s ± 99 ms	-0.02 %
arco-pro_production-mode_generate-package-json-webpack-plugin + exec	3.25 s ± 63 ms	3.27 s ± 105 ms	+0.48 %
threejs_development-mode_10x + exec	1.59 s ± 15 ms	1.59 s ± 16 ms	-0.45 %
threejs_development-mode_10x_hmr + exec	777 ms ± 7.8 ms	772 ms ± 8.8 ms	-0.70 %
threejs_production-mode_10x + exec	4.99 s ± 40 ms	4.96 s ± 28 ms	-0.45 %
suite	result
modernjs	✅ success
_selftest	✅ success
rspress	✅ success
rslib	✅ success
rsbuild	✅ success
examples	✅ success
devserver	✅ success