Skip to content

Commit

Permalink
perf: bump string_wizard for better performance (#292)
Browse files Browse the repository at this point in the history
<!-- Thank you for contributing! -->

### Description

rolldown/string_wizard#9

The problem is that bundling cjs module requre calling `MagicString#indent`, which was incredibly slow due to lacking of optimization: rolldown/string_wizard@4515818.


```
┌─────────┬────────────────────┬─────────┬───────────────────┬───────────┬─────────┐
│ (index) │     Task Name      │ ops/sec │ Average Time (ns) │  Margin   │ Samples │
├─────────┼────────────────────┼─────────┼───────────────────┼───────────┼─────────┤
│    0    │ 'rolldown-threejs' │  '109'  │ 9110844.725912267 │ '±10.05%' │   11    │
│    1    │ 'esbuild-threejs'  │  '52'   │ 19223512.59738207 │ '±13.03%' │   10    │
└─────────┴────────────────────┴─────────┴───────────────────┴───────────┴─────────┘
┌─────────┬───────────────────────┬─────────┬────────────────────┬───────────┬─────────┐
│ (index) │       Task Name       │ ops/sec │ Average Time (ns)  │  Margin   │ Samples │
├─────────┼───────────────────────┼─────────┼────────────────────┼───────────┼─────────┤
│    0    │ 'rolldown-threejs10x' │   '9'   │ 102347845.79873085 │ '±10.12%' │   10    │
│    1    │ 'esbuild-threejs10x'  │   '5'   │ 166986120.80067396 │ '±3.19%'  │   10    │
└─────────┴───────────────────────┴─────────┴────────────────────┴───────────┴─────────┘
┌─────────┬────────────────────────────────┬─────────┬────────────────────┬──────────┬─────────┐
│ (index) │           Task Name            │ ops/sec │ Average Time (ns)  │  Margin  │ Samples │
├─────────┼────────────────────────────────┼─────────┼────────────────────┼──────────┼─────────┤
│    0    │ 'rolldown-react_and_react_dom' │  '49'   │ 20094408.302009106 │ '±5.22%' │   10    │
│    1    │ 'esbuild-react_and_react_dom'  │  '45'   │ 21785495.999455452 │ '±3.87%' │   10    │
└─────────┴────────────────────────────────┴─────────┴────────────────────┴──────────┴─────────┘
```

<!-- Please insert your description here and provide especially info about the "what" this PR is solving -->

### Test Plan

<!-- e.g. is there anything you'd like reviewers to focus on? -->

---
  • Loading branch information
hyf0 authored Nov 17, 2023
1 parent 8310a17 commit 10a884a
Show file tree
Hide file tree
Showing 21 changed files with 101 additions and 38 deletions.
6 changes: 4 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 Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ serde_json = "1.0.108"
insta = "1.34.0"
testing_macros = "0.2.11"
scoped-tls = "1.0.1"
string_wizard = { version = "0.0.14" }
string_wizard = { version = "0.0.17" }
async-trait = "0.1.74"
futures = "0.3.29"
thiserror = "1.0.50"
Expand Down
3 changes: 2 additions & 1 deletion crates/rolldown/examples/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@ use sugar_path::SugarPathBuf;

#[tokio::main]
async fn main() -> Result<()> {
let _guard = rolldown_tracing::try_init_tracing_with_chrome_layer();
let root = PathBuf::from(&std::env::var("CARGO_MANIFEST_DIR").unwrap());
let cwd = root.join("./examples").into_normalize();
let mut bundler = Bundler::new(InputOptions {
input: vec![InputItem { name: Some("basic".to_string()), import: "./index.js".to_string() }],
input: vec![InputItem { name: Some("basic".to_string()), import: "react-dom".to_string() }],
cwd,
..Default::default()
});
Expand Down
9 changes: 8 additions & 1 deletion crates/rolldown/src/bundler/chunk/render_chunk_exports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,14 @@ impl Chunk {
if matches!(linking_info.wrap_kind, WrapKind::Cjs) {
match output_options.format {
OutputFormat::Esm => {
let wrap_ref_name = &self.canonical_names[&linking_info.wrap_ref.unwrap()];
let wrap_ref_name =
&self.canonical_names.get(&linking_info.wrap_ref.unwrap()).unwrap_or_else(|| {
panic!(
"Cannot find canonical name for wrap ref {:?} of {:?}",
linking_info.wrap_ref.unwrap(),
graph.modules[entry].resource_id()
)
});
return Some(MagicString::new(format!("export default {wrap_ref_name}();\n")));
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/rolldown/src/bundler/module/external_module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,14 @@ impl ExternalModule {
Specifier::Star => {
self.namespace_ref = Some(symbols.create_symbol(
self.id,
Atom::from(format!("{}_ns", self.resource_id.expect_file().generate_unique_name())),
Atom::from(format!("{}_ns", self.resource_id.expect_file().representative_name())),
));
self.namespace_ref.unwrap()
}
Specifier::Literal(exported) => {
*self.exported_name_to_binding_ref.entry(exported.clone()).or_insert_with_key(|exported| {
let declared_name = if exported.as_ref() == "default" {
Atom::from(format!("{}_default", self.resource_id.expect_file().generate_unique_name()))
Atom::from(format!("{}_default", self.resource_id.expect_file().representative_name()))
} else {
exported.clone()
};
Expand Down
1 change: 1 addition & 0 deletions crates/rolldown/src/bundler/module/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ impl Module {
}
}

#[tracing::instrument(skip_all)]
pub fn render(&self, ctx: ModuleRenderContext) -> Option<MagicString<'_>> {
match self {
Self::Normal(m) => m.render(ctx),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ impl<'task, T: FileSystem + Default + 'static> NormalModuleTask<'task, T> {
}
}

#[tracing::instrument(skip_all)]
pub async fn run(mut self) -> BatchedResult<()> {
let mut builder = NormalModuleBuilder::default();
tracing::trace!("process {:?}", self.path);
Expand Down Expand Up @@ -158,12 +159,12 @@ impl<'task, T: FileSystem + Default + 'static> NormalModuleTask<'task, T> {
let (mut symbol_table, scope) = semantic.into_symbol_table_and_scope_tree();
let ast_scope = AstScope::new(scope, std::mem::take(&mut symbol_table.references));
let mut symbol_for_module = AstSymbol::from_symbol_table(symbol_table);
let unique_name = self.path.generate_unique_name();
let unique_name = self.path.representative_name();
let mut scanner = scanner::Scanner::new(
self.module_id,
&ast_scope,
&mut symbol_for_module,
unique_name,
unique_name.into_owned(),
self.module_type,
);
scanner.visit_program(program.program());
Expand Down Expand Up @@ -204,6 +205,7 @@ impl<'task, T: FileSystem + Default + 'static> NormalModuleTask<'task, T> {
Ok(info)
}

#[tracing::instrument(skip_all)]
async fn resolve_dependencies(
&mut self,
dependencies: &IndexVec<ImportRecordId, RawImportRecord>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ impl RuntimeNormalModuleTask {
Self { module_id: id, tx, warnings: Vec::default() }
}

#[tracing::instrument(skip_all)]
pub fn run(self) {
let mut builder = NormalModuleBuilder::default();

Expand Down
8 changes: 8 additions & 0 deletions crates/rolldown/src/bundler/renderer/impl_visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@ use crate::bundler::{module::Module, renderer::RenderControl};
use super::AstRenderer;

impl<'ast, 'r> Visit<'ast> for AstRenderer<'r> {
#[tracing::instrument]
fn visit_program(&mut self, program: &ast::Program<'ast>) {
for directive in &program.directives {
self.visit_directive(directive);
}
self.visit_statements(&program.body);
}

fn visit_binding_identifier(&mut self, ident: &oxc::ast::ast::BindingIdentifier) {
self.render_binding_identifier(ident);
}
Expand Down
6 changes: 3 additions & 3 deletions crates/rolldown/src/bundler/renderer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,9 @@ impl<'r> AstRenderer<'r> {
self.wrapped_esm_ctx.hoisted_functions.iter().for_each(|f| {
self.ctx.source.relocate(f.start, f.end, 0);
self.ctx.source.append_left(f.end, "\n");
indent_excludes.push([f.start, f.end]);
indent_excludes.push((f.start, f.end));
});
self.ctx.source.indent2(&self.indentor, indent_excludes);
self.ctx.source.indent2(&self.indentor, &indent_excludes);
if !self.wrapped_esm_ctx.hoisted_vars.is_empty() {
self
.ctx
Expand All @@ -146,7 +146,7 @@ impl<'r> AstRenderer<'r> {
RenderKind::Cjs => {
let wrap_ref_name = self.ctx.wrap_ref_name.unwrap();
let prettify_id = &self.ctx.module.pretty_path;
self.ctx.source.indent2(&self.indentor, Vec::default());
self.ctx.source.indent2(&self.indentor, &[]);
let commonjs_ref_name = self.ctx.canonical_name_for_runtime("__commonJS");
self.ctx.source.prepend(format!(
"var {wrap_ref_name} = {commonjs_ref_name}({{\n{}'{prettify_id}'(exports, module) {{\n",
Expand Down
1 change: 1 addition & 0 deletions crates/rolldown/src/bundler/stages/scan_stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ impl<Fs: FileSystem + Default + 'static> ScanStage<Fs> {
}

/// Resolve `InputOptions.input`
#[tracing::instrument(skip_all)]
fn resolve_user_defined_entries(
&self,
) -> BatchedResult<Vec<(Option<String>, ResolvedRequestInfo)>> {
Expand Down
1 change: 1 addition & 0 deletions crates/rolldown/src/bundler/visitors/scanner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ impl<'a> Scanner<'a> {
}

impl<'ast> Visit<'ast> for Scanner<'ast> {
#[tracing::instrument(skip_all)]
fn visit_program(&mut self, program: &oxc::ast::ast::Program<'ast>) {
for (idx, stmt) in program.body.iter().enumerate() {
self.current_stmt_info.stmt_idx = Some(idx);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
source: crates/rolldown/tests/common/case.rs
expression: content
input_file: crates/rolldown/tests/fixtures/misc/generate_valid_name_for_kebab_case_files
---
# Assets

## main.mjs

```js
import { __commonJS } from "./_rolldown_runtime.mjs";
// react-dom.js
var require_react_dom = __commonJS({
'react-dom.js'(exports, module) {
module.exports = 'react-dom'
}
});
export default require_react_dom();
```
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = 'react-dom'
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"input": {
"input": [
{
"name": "main",
"import": "./react-dom.js"
}
]
}
}
4 changes: 3 additions & 1 deletion crates/rolldown_common/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ repository.workspace = true


[dependencies]
oxc = { workspace = true }
oxc = { workspace = true, features = ["semantic"] }
rustc-hash = { workspace = true }
index_vec = { workspace = true }
sugar_path = { workspace = true }
regex = { workspace = true }
once_cell = { workspace = true }
45 changes: 24 additions & 21 deletions crates/rolldown_common/src/file_path.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use std::{
borrow::Cow,
ffi::OsStr,
path::{Component, Path},
sync::Arc,
};

use regex::Regex;
use sugar_path::{AsPath, SugarPath};

#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone)]
Expand Down Expand Up @@ -61,39 +63,40 @@ impl FilePath {
name
}

pub fn generate_unique_name(&self) -> String {
// This doesn't ensure uniqueness, but should be valid as a JS identifier.
pub fn representative_name(&self) -> Cow<str> {
let path = Path::new(self.0.as_ref());
let unique_name =
path.file_stem().expect("should have file_stem").to_str().expect("should be valid utf8");
let mut unique_name = Cow::Borrowed(
path.file_stem().expect("should have file_stem").to_str().expect("should be valid utf8"),
);

if unique_name == "index" {
if let Some(unique_name_of_parent_dir) =
path.parent().and_then(Path::file_stem).and_then(OsStr::to_str)
{
return [unique_name_of_parent_dir, "_index"].concat();
unique_name = Cow::Owned([unique_name_of_parent_dir, "_index"].concat());
}
}

ensure_valid_identifier(unique_name)
}
}

fn ensure_valid_identifier(s: &str) -> String {
let mut ident = String::new();
let mut need_gap = false;
for i in s.chars() {
if i.is_ascii_alphabetic() || (i.is_ascii_digit() && !ident.is_empty()) {
if need_gap {
ident.push('_');
need_gap = false;
}
ident.push(i);
} else if !ident.is_empty() {
need_gap = true;
}
}
if ident.is_empty() {
ident.push('_');
static VALID_RE: once_cell::sync::Lazy<Regex> =
once_cell::sync::Lazy::new(|| Regex::new(r"[^a-zA-Z0-9_$]").unwrap());

fn ensure_valid_identifier(s: Cow<str>) -> Cow<str> {
match s {
Cow::Borrowed(str) => VALID_RE.replace_all(str, "_"),
Cow::Owned(owned_str) => VALID_RE.replace_all(&owned_str, "_").into_owned().into(),
}
ident
}

#[test]
fn test_ensure_valid_identifier() {
assert_eq!(ensure_valid_identifier("foo".into()), "foo");
assert_eq!(ensure_valid_identifier("$foo$".into()), "$foo$");
assert_eq!(ensure_valid_identifier("react-dom".into()), "react_dom");
}

#[test]
Expand Down
4 changes: 2 additions & 2 deletions crates/rolldown_utils/src/magic_string_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ use string_wizard::{IndentOptions, MagicString, UpdateOptions};

pub trait MagicStringExt {
fn overwrite(&mut self, start: u32, end: u32, content: String) -> &mut Self;
fn indent2(&mut self, indentor: &str, exclude: Vec<[u32; 2]>) -> &mut Self;
fn indent2(&mut self, indentor: &str, exclude: &[(u32, u32)]) -> &mut Self;
}
impl<'text> MagicStringExt for MagicString<'text> {
fn overwrite(&mut self, start: u32, end: u32, content: String) -> &mut Self {
self.update_with(start, end, content, UpdateOptions { overwrite: true, ..Default::default() })
}

fn indent2(&mut self, indentor: &str, exclude: Vec<[u32; 2]>) -> &mut Self {
fn indent2(&mut self, indentor: &str, exclude: &[(u32, u32)]) -> &mut Self {
self.indent_with(IndentOptions { indentor: Some(&indentor.repeat(2)), exclude })
}
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"test:update": "yarn workspaces foreach --all run test:update",
"prettier": "prettier . '!**/*.{js||ts|json|md|yml|yaml}' -w",
"prettier:ci": "prettier . '!**/*.{js||ts|json|md|yml|yaml}' -c",
"bench": "yarn workspace bench run bench",
"TODO(hyf0): #need to investigate following commands": "_",
"build:ci:release": "run-s build:packages:ci build:ts build:binding:release",
"build:ts": "tsc -b tsconfig.project.json"
Expand Down
1 change: 1 addition & 0 deletions packages/bench/package.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{
"name": "bench",
"private": true,
"type": "module",
"scripts": {
Expand Down
4 changes: 2 additions & 2 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2915,9 +2915,9 @@ __metadata:
languageName: node
linkType: hard

"bench-f64382@workspace:packages/bench":
"bench@workspace:packages/bench":
version: 0.0.0-use.local
resolution: "bench-f64382@workspace:packages/bench"
resolution: "bench@workspace:packages/bench"
dependencies:
"@rolldown/node": "workspace:*"
esbuild: "npm:^0.19.5"
Expand Down

0 comments on commit 10a884a

Please sign in to comment.