Skip to content

Commit

Permalink
fix: avoid invoking toJSON method of additionalData (#5904)
Browse files Browse the repository at this point in the history
  • Loading branch information
NotEvenANeko authored Apr 18, 2024
1 parent c3c83ed commit b194017
Show file tree
Hide file tree
Showing 28 changed files with 385 additions and 84 deletions.
2 changes: 0 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/node_binding/binding.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ export interface JsExecuteModuleResult {
export interface JsLoaderContext {
/** Content maybe empty in pitching stage */
content: null | Buffer
additionalData?: Buffer
additionalData?: any
sourceMap?: Buffer
resource: string
resourcePath: string
Expand Down
2 changes: 1 addition & 1 deletion crates/node_binding/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Rspack {
let js_plugin = JsHooksAdapterPlugin::from_js_hooks(env, register_js_taps)?;
plugins.push(js_plugin.clone().boxed());
for bp in builtin_plugins {
bp.append_to(&mut plugins)
bp.append_to(env, &mut plugins)
.map_err(|e| Error::from_reason(format!("{e}")))?;
}

Expand Down
1 change: 1 addition & 0 deletions crates/rspack_binding_options/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![feature(try_blocks)]
#![feature(let_chains)]
mod options;
mod plugins;
pub use options::*;
13 changes: 8 additions & 5 deletions crates/rspack_binding_options/src/options/raw_builtins/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ mod raw_progress;
mod raw_swc_js_minimizer;
mod raw_to_be_deprecated;

use napi::{bindgen_prelude::FromNapiValue, JsUnknown};
use napi::{bindgen_prelude::FromNapiValue, Env, JsUnknown};
use napi_derive::napi;
use rspack_core::{BoxPlugin, Define, DefinePlugin, PluginExt, Provide, ProvidePlugin};
use rspack_error::Result;
Expand Down Expand Up @@ -77,9 +77,10 @@ use self::{
raw_mf::{RawConsumeSharedPluginOptions, RawContainerReferencePluginOptions, RawProvideOptions},
};
use crate::{
plugins::JsLoaderResolverPlugin, JsLoaderRunner, RawEntryPluginOptions,
RawEvalDevToolModulePluginOptions, RawExternalItemWrapper, RawExternalsPluginOptions,
RawHttpExternalsRspackPluginOptions, RawSourceMapDevToolPluginOptions, RawSplitChunksOptions,
plugins::{CssExtractRspackAdditionalDataPlugin, JsLoaderResolverPlugin},
JsLoaderRunner, RawEntryPluginOptions, RawEvalDevToolModulePluginOptions, RawExternalItemWrapper,
RawExternalsPluginOptions, RawHttpExternalsRspackPluginOptions, RawSourceMapDevToolPluginOptions,
RawSplitChunksOptions,
};

#[napi(string_enum)]
Expand Down Expand Up @@ -163,7 +164,7 @@ pub struct BuiltinPlugin {
}

impl BuiltinPlugin {
pub fn append_to(self, plugins: &mut Vec<BoxPlugin>) -> rspack_error::Result<()> {
pub fn append_to(self, env: Env, plugins: &mut Vec<BoxPlugin>) -> rspack_error::Result<()> {
match self.name {
// webpack also have these plugins
BuiltinPluginName::DefinePlugin => {
Expand Down Expand Up @@ -420,6 +421,8 @@ impl BuiltinPlugin {
)
}
BuiltinPluginName::CssExtractRspackPlugin => {
let additional_data_plugin = CssExtractRspackAdditionalDataPlugin::new(env)?.boxed();
plugins.push(additional_data_plugin);
let plugin = rspack_plugin_extract_css::plugin::PluginCssExtract::new(
downcast_into::<RawCssExtractPluginOption>(self.options)?.into(),
)
Expand Down
25 changes: 13 additions & 12 deletions crates/rspack_binding_options/src/options/raw_module/js_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use {
rspack_error::error,
rspack_identifier::{Identifiable, Identifier},
rspack_napi::threadsafe_function::ThreadsafeFunction,
rspack_napi::threadsafe_js_value_ref::ThreadsafeJsValueRef,
};

use crate::get_builtin_loader;
Expand Down Expand Up @@ -138,11 +139,11 @@ fn sync_loader_context(
.map_err(|e| error!(e.to_string()))?;
loader_context.additional_data = loader_result.additional_data_external.clone();
if let Some(data) = loader_result.additional_data {
loader_context.additional_data.insert(data);
} else {
loader_context
.additional_data
.insert(String::from_utf8_lossy(&data).to_string());
} else {
loader_context.additional_data.remove::<String>();
.remove::<ThreadsafeJsValueRef<Unknown>>();
}
loader_context.asset_filenames = loader_result.asset_filenames.into_iter().collect();

Expand All @@ -153,7 +154,8 @@ fn sync_loader_context(
pub struct JsLoaderContext {
/// Content maybe empty in pitching stage
pub content: Either<Null, Buffer>,
pub additional_data: Option<Buffer>,
#[napi(ts_type = "any")]
pub additional_data: Option<ThreadsafeJsValueRef<Unknown>>,
pub source_map: Option<Buffer>,
pub resource: String,
pub resource_path: String,
Expand Down Expand Up @@ -208,8 +210,8 @@ impl TryFrom<&mut rspack_core::LoaderContext<'_, rspack_core::LoaderRunnerContex
},
additional_data: cx
.additional_data
.get::<String>()
.map(|v| v.clone().into_bytes().into()),
.get::<ThreadsafeJsValueRef<Unknown>>()
.cloned(),
source_map: cx
.source_map
.clone()
Expand Down Expand Up @@ -269,10 +271,7 @@ pub async fn run_builtin_loader(
let list = &[loader_item];
let additional_data = {
let mut additional_data = loader_context.additional_data_external.clone();
if let Some(data) = loader_context
.additional_data
.map(|b| String::from_utf8_lossy(b.as_ref()).to_string())
{
if let Some(data) = loader_context.additional_data {
additional_data.insert(data);
}
additional_data
Expand Down Expand Up @@ -358,7 +357,7 @@ pub struct JsLoaderResult {
pub build_dependencies: Vec<String>,
pub asset_filenames: Vec<String>,
pub source_map: Option<Buffer>,
pub additional_data: Option<Buffer>,
pub additional_data: Option<ThreadsafeJsValueRef<Unknown>>,
pub additional_data_external: External<AdditionalData>,
pub cacheable: bool,
/// Used to instruct how rust loaders should execute
Expand Down Expand Up @@ -432,7 +431,9 @@ impl napi::bindgen_prelude::FromNapiValue for JsLoaderResult {
)
})?;
let source_map_: Option<Buffer> = obj.get("sourceMap")?;
let additional_data_: Option<Buffer> = obj.get("additionalData")?;
let additional_data_: Option<ThreadsafeJsValueRef<Unknown>> =
obj.get::<_, ThreadsafeJsValueRef<Unknown>>("additionalData")?;

// change: eagerly clone this field since `External<T>` might be dropped.
let additional_data_external_: External<AdditionalData> = obj
.get("additionalDataExternal")?
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
use std::fmt::Debug;

use napi::{bindgen_prelude::Unknown, Env};
use rspack_core::{
ApplyContext, CompilerOptions, NormalModuleAdditionalData, Plugin, PluginContext,
};
use rspack_error::Result;
use rspack_hook::{plugin, plugin_hook};
use rspack_loader_runner::AdditionalData;
use rspack_napi::{threadsafe_js_value_ref::ThreadsafeJsValueRef, JsCallback, NapiResultExt};
use rspack_plugin_extract_css::{CssExtractJsonData, CssExtractJsonDataList};
use tokio::sync::oneshot;

#[plugin]
pub(crate) struct CssExtractRspackAdditionalDataPlugin {
js_callback: JsCallback<Box<dyn FnOnce(Env) + Sync>>,
}

impl CssExtractRspackAdditionalDataPlugin {
pub fn new(env: Env) -> Result<Self> {
Ok(Self::new_inner(
unsafe { JsCallback::new(env.raw()) }.into_rspack_result()?,
))
}
}

impl Debug for CssExtractRspackAdditionalDataPlugin {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "CssExtractRspackAdditionalDataPlugin(..)")
}
}

#[plugin_hook(NormalModuleAdditionalData for CssExtractRspackAdditionalDataPlugin)]
async fn additional_data(&self, additional_data: &mut AdditionalData) -> Result<()> {
if !additional_data.contains::<ThreadsafeJsValueRef<Unknown>>() {
return Ok(());
}
let (tx, rx) = oneshot::channel::<AdditionalData>();
let mut old_data = std::mem::take(additional_data);
self.js_callback.call(Box::new(move |env| {
if let Some(data) = old_data.get::<ThreadsafeJsValueRef<Unknown>>()
&& let Ok(data) = data.get(env)
&& let Ok(data) = data.coerce_to_object()
&& let Ok(Some(data)) = data.get::<_, String>("css-extract-rspack-plugin")
{
let mut list = data.split("__RSPACK_CSS_EXTRACT_SEP__");
let mut data_list = vec![];
while let Some(identifier) = list.next() {
#[allow(clippy::unwrap_used)]
{
// parse the css data from js loader
// data:
// [identifier]__RSPACK_CSS_EXTRACT_SEP__
// [content]__RSPACK_CSS_EXTRACT_SEP__
// [context]__RSPACK_CSS_EXTRACT_SEP__
// [media]__RSPACK_CSS_EXTRACT_SEP__
// [supports]__RSPACK_CSS_EXTRACT_SEP__
// [sourceMap]__RSPACK_CSS_EXTRACT_SEP__
// [identifier]__RSPACK_CSS_EXTRACT_SEP__ ... repeated
// [content]__RSPACK_CSS_EXTRACT_SEP__
data_list.push(CssExtractJsonData {
identifier: identifier.into(),
content: list.next().unwrap().into(),
context: list.next().unwrap().into(),
media: list.next().unwrap().into(),
supports: list.next().unwrap().into(),
source_map: list.next().unwrap().into(),
identifier_index: list
.next()
.unwrap()
.parse()
.expect("Cannot parse identifier_index, this should never happen"),
filepath: list.next().unwrap().into(),
});
}
}
old_data.insert(CssExtractJsonDataList(data_list));
};
tx.send(old_data)
.expect("should send `additional_data` for `CssExtractRspackAdditionalDataPlugin`");
}));
let new_data = rx
.await
.expect("should receive `additional_data` for `CssExtractRspackAdditionalDataPlugin`");
// ignore the default value here
let _ = std::mem::replace(additional_data, new_data);
Ok(())
}

#[async_trait::async_trait]
impl Plugin for CssExtractRspackAdditionalDataPlugin {
fn name(&self) -> &'static str {
"CssExtractRspackAdditionalDataPlugin"
}

fn apply(
&self,
ctx: PluginContext<&mut ApplyContext>,
_options: &mut CompilerOptions,
) -> Result<()> {
ctx
.context
.normal_module_hooks
.additional_data
.tap(additional_data::new(self));
Ok(())
}
}
2 changes: 2 additions & 0 deletions crates/rspack_binding_options/src/plugins/mod.rs
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
mod css_extract_additional_data;
mod js_loader_resolver;
pub(super) use css_extract_additional_data::CssExtractRspackAdditionalDataPlugin;
pub(super) use js_loader_resolver::JsLoaderResolverPlugin;
10 changes: 9 additions & 1 deletion crates/rspack_core/src/normal_module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,14 @@ impl ModuleIssuer {
define_hook!(NormalModuleReadResource: AsyncSeriesBail(resource_data: &mut ResourceData) -> Content);
define_hook!(NormalModuleLoader: SyncSeries(loader_context: &mut LoaderContext<CompilerContext>));
define_hook!(NormalModuleBeforeLoaders: SyncSeries(module: &mut NormalModule));
define_hook!(NormalModuleAdditionalData: AsyncSeries(additional_data: &mut AdditionalData));

#[derive(Debug, Default)]
pub struct NormalModuleHooks {
pub read_resource: NormalModuleReadResourceHook,
pub loader: NormalModuleLoaderHook,
pub before_loaders: NormalModuleBeforeLoadersHook,
pub additional_data: NormalModuleAdditionalDataHook,
}

#[impl_source_map_config]
Expand Down Expand Up @@ -411,7 +413,7 @@ impl Module for NormalModule {
additional_data,
)
.await;
let (loader_result, ds) = match loader_result {
let (mut loader_result, ds) = match loader_result {
Ok(r) => r.split_into_parts(),
Err(e) => {
let mut e = ModuleBuildError(e).boxed();
Expand Down Expand Up @@ -441,6 +443,12 @@ impl Module for NormalModule {
});
}
};
build_context
.plugin_driver
.normal_module_hooks
.additional_data
.call(&mut loader_result.additional_data)
.await?;
self.add_diagnostics(ds);

let content = if self.module_type().is_binary() {
Expand Down
2 changes: 1 addition & 1 deletion crates/rspack_loader_runner/src/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ impl<C> TryFrom<LoaderContext<'_, C>> for TWithDiagnosticArray<LoaderResult> {
}
}

pub async fn run_loaders<C: Send>(
pub async fn run_loaders<C: 'static + Send>(
loaders: &[Arc<dyn Loader<C>>],
resource_data: &mut ResourceData,
plugins: &[&dyn LoaderRunnerPlugin<Context = C>],
Expand Down
4 changes: 3 additions & 1 deletion crates/rspack_napi/src/callback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ pub struct JsCallback<Resolver: FnOnce(Env)> {
unsafe impl<Resolver: FnOnce(Env)> Send for JsCallback<Resolver> {}

impl<Resolver: FnOnce(Env)> JsCallback<Resolver> {
pub(crate) fn new(env: sys::napi_env) -> Result<Self> {
/// # Safety
/// The provided `env` must be a valid `napi_env`.
pub unsafe fn new(env: sys::napi_env) -> Result<Self> {
let mut async_resource_name = ptr::null_mut();
let s = unsafe { CStr::from_bytes_with_nul_unchecked(b"napi_js_callback\0") };
check_status!(
Expand Down
44 changes: 44 additions & 0 deletions crates/rspack_napi/src/js_values/js_value_ref.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
use std::marker::PhantomData;

use napi::bindgen_prelude::*;
use napi::{Env, NapiValue, Ref};

pub struct JsValueRef<T: NapiValue> {
ref_: Ref<()>,
_phantom: PhantomData<T>,
}

impl<T: NapiValue> JsValueRef<T> {
pub fn new(env: Env, value: T) -> Result<Self> {
let ref_ = env.create_reference(value)?;

Ok(Self {
ref_,
_phantom: PhantomData,
})
}

pub fn get(&self, env: Env) -> Result<T> {
env.get_reference_value(&self.ref_)
}

pub fn unref(&mut self, env: Env) -> Result<u32> {
self.ref_.unref(env)
}
}

impl<T: NapiValue> ToNapiValue for JsValueRef<T> {
unsafe fn to_napi_value(env: sys::napi_env, val: Self) -> Result<sys::napi_value> {
val
.get(Env::from(env))
.and_then(|v| unsafe { T::to_napi_value(env, v) })
}
}

impl<T: NapiValue> FromNapiValue for JsValueRef<T> {
unsafe fn from_napi_value(env: sys::napi_env, napi_val: sys::napi_value) -> Result<Self> {
JsValueRef::<T>::new(Env::from(env), unsafe {
T::from_napi_value(env, napi_val)
}?)
}
}
1 change: 1 addition & 0 deletions crates/rspack_napi/src/js_values/mod.rs
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
pub mod js_reg_exp;
pub mod js_value_ref;
3 changes: 2 additions & 1 deletion crates/rspack_napi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ mod errors;
pub use errors::{NapiErrorExt, NapiResultExt};

mod callback;
pub(crate) use callback::JsCallback;
pub use callback::JsCallback;

pub mod threadsafe_function;
pub mod threadsafe_js_value_ref;

pub mod regexp {
pub use crate::ext::js_reg_exp_ext::JsRegExpExt;
Expand Down
2 changes: 1 addition & 1 deletion crates/rspack_napi/src/threadsafe_function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ impl<T: 'static + JsValuesTupleIntoVec, R> FromNapiValue for ThreadsafeFunction<
Ok(Self {
inner,
env,
resolver: JsCallback::new(env)?,
resolver: unsafe { JsCallback::new(env) }?,
_data: PhantomData,
})
}
Expand Down
Loading

2 comments on commit b194017

@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

@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-18 0a88b52) Current Change
10000_development-mode + exec 2.69 s ± 28 ms 2.68 s ± 31 ms -0.60 %
10000_development-mode_hmr + exec 699 ms ± 7.2 ms 688 ms ± 29 ms -1.50 %
10000_production-mode + exec 2.57 s ± 16 ms 2.47 s ± 24 ms -4.10 %
arco-pro_development-mode + exec 2.51 s ± 78 ms 2.49 s ± 54 ms -0.72 %
arco-pro_development-mode_hmr + exec 430 ms ± 2 ms 430 ms ± 3.9 ms +0.12 %
arco-pro_development-mode_hmr_intercept-plugin + exec 441 ms ± 1.9 ms 442 ms ± 3.2 ms +0.24 %
arco-pro_development-mode_intercept-plugin + exec 3.3 s ± 63 ms 3.23 s ± 88 ms -1.92 %
arco-pro_production-mode + exec 4.04 s ± 88 ms 3.94 s ± 97 ms -2.50 %
arco-pro_production-mode_intercept-plugin + exec 4.84 s ± 84 ms 4.72 s ± 101 ms -2.44 %
threejs_development-mode_10x + exec 2.06 s ± 21 ms 2.04 s ± 23 ms -0.88 %
threejs_development-mode_10x_hmr + exec 749 ms ± 5.4 ms 763 ms ± 5.9 ms +1.87 %
threejs_production-mode_10x + exec 5.25 s ± 62 ms 5.21 s ± 40 ms -0.70 %

Please sign in to comment.