Skip to content

Commit

Permalink
perf: Introduce RcStr (vercel/turborepo#8221)
Browse files Browse the repository at this point in the history
### Description

If we accept `: String` from a function, invoking the function needs one
owned `String` and it means allocation even if the value isn't changed.
But with this PR, functions now accept `RcStr`, which is shared, so
invoking the same function with the same value does not involve
allocation for `String` .

---

I tried using `Arc<String>` in
vercel/turborepo#7772, but a team member suggested
creating a new type so we can replace underlying implementation easily
in the future.

I used `ast-grep` with

```yml
id: convert-expr-into
language: rust
rule:
  kind: function_item
  all:
    - has:
        kind: parameters
        pattern: $PARAMS
    - any:
      - has:
          kind: visibility_modifier
          pattern: $VIS
      - not:
          has:
            kind: visibility_modifier

    - any:
      - has:
          kind: function_modifiers
          pattern: $MOD
      - not:
          has:
            kind: function_modifiers
    - has:
        field: return_type
        pattern: $RET
    - has:
        field: body
        pattern: $BODY
    - has:
        kind: identifier
        pattern: $NAME
    - follows:
        kind: attribute_item
        pattern: '#[turbo_tasks::function]'



transform:
  NEW_PARAMS:
    replace:
      replace: 'String'
      by: 'RcStr'
      source: $PARAMS

fix:
  $VIS $MOD fn $NAME $NEW_PARAMS -> $RET $BODY




```


### Performance difference:

#### `main`:

```
Benchmarking bench_startup/Next.js canary Turbo SSR/1000 modules: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 60.0s. You may wish to increase target time to 65.6s.
bench_startup/Next.js canary Turbo SSR/1000 modules
                        time:   [4.1476 s 4.1625 s 4.1752 s]
Benchmarking bench_startup/Next.js canary Turbo RSC/1000 modules: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 60.0s. You may wish to increase target time to 63.5s.
bench_startup/Next.js canary Turbo RSC/1000 modules
                        time:   [4.0396 s 4.0673 s 4.0956 s]
Benchmarking bench_startup/Next.js canary Turbo RCC/1000 modules: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 60.0s. You may wish to increase target time to 69.7s.
bench_startup/Next.js canary Turbo RCC/1000 modules
                        time:   [4.7222 s 4.7508 s 4.7790 s]
```

#### `pr`:

```
Benchmarking bench_startup/Next.js canary Turbo SSR/1000 modules: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 60.0s. You may wish to increase target time to 66.2s.
bench_startup/Next.js canary Turbo SSR/1000 modules
                        time:   [4.1175 s 4.1310 s 4.1496 s]
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) high severe
Benchmarking bench_startup/Next.js canary Turbo RSC/1000 modules: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 60.0s. You may wish to increase target time to 63.7s.
bench_startup/Next.js canary Turbo RSC/1000 modules
                        time:   [4.0401 s 4.0714 s 4.1105 s]
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) high mild
Benchmarking bench_startup/Next.js canary Turbo RCC/1000 modules: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 60.0s. You may wish to increase target time to 70.2s.
bench_startup/Next.js canary Turbo RCC/1000 modules
                        time:   [4.6696 s 4.6900 s 4.7107 s]
Benchmarking bench_startup/Next.js 14 Turbo SSR/1000 modules: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 60.0s. You may wish to increase target time to 63.9s.
```

---------

Co-authored-by: Benjamin Woodruff <benjamin.woodruff@vercel.com>
  • Loading branch information
kdy1 and bgw authored Jun 5, 2024
1 parent 3bb50cb commit 1caee85
Show file tree
Hide file tree
Showing 227 changed files with 2,606 additions and 2,364 deletions.
44 changes: 24 additions & 20 deletions crates/node-file-trace/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ use serde::Deserialize;
use serde::Serialize;
use tokio::sync::mpsc::channel;
use turbo_tasks::{
backend::Backend, util::FormatDuration, TaskId, TransientInstance, TransientValue, TurboTasks,
UpdateInfo, Value, Vc,
backend::Backend, util::FormatDuration, RcStr, TaskId, TransientInstance, TransientValue,
TurboTasks, UpdateInfo, Value, Vc,
};
use turbo_tasks_fs::{
glob::Glob, DirectoryEntry, DiskFileSystem, FileSystem, FileSystemPath, ReadGlobResult,
Expand Down Expand Up @@ -188,7 +188,7 @@ impl Args {
}

async fn create_fs(name: &str, root: &str, watch: bool) -> Result<Vc<Box<dyn FileSystem>>> {
let fs = DiskFileSystem::new(name.to_string(), root.to_string(), vec![]);
let fs = DiskFileSystem::new(name.into(), root.into(), vec![]);
if watch {
fs.await?.start_watching()?;
} else {
Expand Down Expand Up @@ -232,17 +232,17 @@ async fn add_glob_results(
#[turbo_tasks::function]
async fn input_to_modules(
fs: Vc<Box<dyn FileSystem>>,
input: Vec<String>,
input: Vec<RcStr>,
exact: bool,
process_cwd: Option<String>,
context_directory: String,
process_cwd: Option<RcStr>,
context_directory: RcStr,
module_options: TransientInstance<ModuleOptionsContext>,
resolve_options: TransientInstance<ResolveOptionsContext>,
) -> Result<Vc<Modules>> {
let root = fs.root();
let process_cwd = process_cwd
.clone()
.map(|p| format!("/ROOT{}", p.trim_start_matches(&context_directory)));
.map(|p| format!("/ROOT{}", p.trim_start_matches(&*context_directory)).into());

let asset_context: Vc<Box<dyn AssetContext>> = Vc::upcast(create_module_asset(
root,
Expand Down Expand Up @@ -283,7 +283,7 @@ fn process_context(dir: &Path, context_directory: Option<&String>) -> Result<Str
.to_string())
}

fn make_relative_path(dir: &Path, context_directory: &str, input: &str) -> Result<String> {
fn make_relative_path(dir: &Path, context_directory: &str, input: &str) -> Result<RcStr> {
let mut input = PathBuf::from(input);
if !input.is_absolute() {
input = dir.join(input);
Expand All @@ -299,10 +299,11 @@ fn make_relative_path(dir: &Path, context_directory: &str, input: &str) -> Resul
Ok(input
.to_str()
.ok_or_else(|| anyhow!("input contains invalid characters"))?
.replace('\\', "/"))
.replace('\\', "/")
.into())
}

fn process_input(dir: &Path, context_directory: &str, input: &[String]) -> Result<Vec<String>> {
fn process_input(dir: &Path, context_directory: &str, input: &[String]) -> Result<Vec<RcStr>> {
input
.iter()
.map(|input| make_relative_path(dir, context_directory, input))
Expand All @@ -314,7 +315,7 @@ pub async fn start(
turbo_tasks: Option<&Arc<TurboTasks<MemoryBackend>>>,
module_options: Option<ModuleOptionsContext>,
resolve_options: Option<ResolveOptionsContext>,
) -> Result<Vec<String>> {
) -> Result<Vec<RcStr>> {
register();
let &CommonArgs {
memory_limit,
Expand Down Expand Up @@ -394,7 +395,7 @@ async fn run<B: Backend + 'static, F: Future<Output = ()>>(
final_finish: impl FnOnce(Arc<TurboTasks<B>>, TaskId, Duration) -> F,
module_options: Option<ModuleOptionsContext>,
resolve_options: Option<ResolveOptionsContext>,
) -> Result<Vec<String>> {
) -> Result<Vec<RcStr>> {
let &CommonArgs {
watch,
show_all,
Expand Down Expand Up @@ -494,7 +495,7 @@ async fn run<B: Backend + 'static, F: Future<Output = ()>>(
if has_return_value {
let output_read_ref = output.await?;
let output_iter = output_read_ref.iter().cloned();
sender.send(output_iter.collect::<Vec<String>>()).await?;
sender.send(output_iter.collect::<Vec<RcStr>>()).await?;
drop(sender);
}
Ok::<Vc<()>, _>(Default::default())
Expand All @@ -515,7 +516,7 @@ async fn main_operation(
args: TransientInstance<Args>,
module_options: TransientInstance<ModuleOptionsContext>,
resolve_options: TransientInstance<ResolveOptionsContext>,
) -> Result<Vc<Vec<String>>> {
) -> Result<Vc<Vec<RcStr>>> {
let dir = current_dir.into_value();
let args = &*args;
let &CommonArgs {
Expand All @@ -526,8 +527,11 @@ async fn main_operation(
ref process_cwd,
..
} = args.common();
let context_directory = process_context(&dir, context_directory.as_ref()).unwrap();
let context_directory: RcStr = process_context(&dir, context_directory.as_ref())
.unwrap()
.into();
let fs = create_fs("context directory", &context_directory, watch).await?;
let process_cwd = process_cwd.clone().map(RcStr::from);

match *args {
Args::Print { common: _ } => {
Expand All @@ -549,7 +553,7 @@ async fn main_operation(
.await?;
for asset in set.await?.iter() {
let path = asset.ident().path().await?;
result.insert(path.path.to_string());
result.insert(RcStr::from(&*path.path));
}
}

Expand Down Expand Up @@ -620,7 +624,7 @@ async fn main_operation(
#[turbo_tasks::function]
async fn create_module_asset(
root: Vc<FileSystemPath>,
process_cwd: Option<String>,
process_cwd: Option<RcStr>,
module_options: TransientInstance<ModuleOptionsContext>,
resolve_options: TransientInstance<ResolveOptionsContext>,
) -> Result<Vc<ModuleAssetContext>> {
Expand All @@ -635,12 +639,12 @@ async fn create_module_asset(
let glob_mappings = vec![
(
root,
Glob::new("**/*/next/dist/server/next.js".to_string()),
Glob::new("**/*/next/dist/server/next.js".into()),
ImportMapping::Ignore.into(),
),
(
root,
Glob::new("**/*/next/dist/bin/next".to_string()),
Glob::new("**/*/next/dist/bin/next".into()),
ImportMapping::Ignore.into(),
),
];
Expand All @@ -662,7 +666,7 @@ async fn create_module_asset(
compile_time_info,
ModuleOptionsContext::clone(&*module_options).cell(),
resolve_options.cell(),
Vc::cell("node_file_trace".to_string()),
Vc::cell("node_file_trace".into()),
))
}

Expand Down
4 changes: 3 additions & 1 deletion crates/node-file-trace/src/nft_json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ impl OutputAsset for NftJsonAsset {
async fn ident(&self) -> Result<Vc<AssetIdent>> {
let path = self.entry.ident().path().await?;
Ok(AssetIdent::from_path(
path.fs.root().join(format!("{}.nft.json", path.path)),
path.fs
.root()
.join(format!("{}.nft.json", path.path).into()),
))
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/turbo-tasks-env/src/command_line.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use indexmap::IndexMap;
use turbo_tasks::Vc;
use turbo_tasks::{RcStr, Vc};

use crate::{sorted_env_vars, EnvMap, ProcessEnv, GLOBAL_ENV_LOCK};

Expand All @@ -16,7 +16,7 @@ impl CommandLineProcessEnv {
}

/// Clones the current env vars into a IndexMap.
fn env_snapshot() -> IndexMap<String, String> {
fn env_snapshot() -> IndexMap<RcStr, RcStr> {
let _lock = GLOBAL_ENV_LOCK.lock().unwrap();
sorted_env_vars()
}
Expand Down
4 changes: 2 additions & 2 deletions crates/turbo-tasks-env/src/custom.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use anyhow::Result;
use turbo_tasks::Vc;
use turbo_tasks::{RcStr, Vc};

use crate::{case_insensitive_read, EnvMap, ProcessEnv};

Expand Down Expand Up @@ -32,7 +32,7 @@ impl ProcessEnv for CustomProcessEnv {
}

#[turbo_tasks::function]
async fn read(&self, name: String) -> Result<Vc<Option<String>>> {
async fn read(&self, name: RcStr) -> Result<Vc<Option<RcStr>>> {
let custom = case_insensitive_read(self.custom, name.clone());
match &*custom.await? {
Some(_) => Ok(custom),
Expand Down
8 changes: 2 additions & 6 deletions crates/turbo-tasks-env/src/dotenv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::{env, sync::MutexGuard};

use anyhow::{anyhow, Context, Result};
use indexmap::IndexMap;
use turbo_tasks::{ValueToString, Vc};
use turbo_tasks::{RcStr, ValueToString, Vc};
use turbo_tasks_fs::{FileContent, FileSystemPath};

use crate::{sorted_env_vars, EnvMap, ProcessEnv, GLOBAL_ENV_LOCK};
Expand Down Expand Up @@ -84,11 +84,7 @@ impl ProcessEnv for DotenvProcessEnv {
}

/// Restores the global env variables to mirror `to`.
fn restore_env(
from: &IndexMap<String, String>,
to: &IndexMap<String, String>,
_lock: &MutexGuard<()>,
) {
fn restore_env(from: &IndexMap<RcStr, RcStr>, to: &IndexMap<RcStr, RcStr>, _lock: &MutexGuard<()>) {
for key in from.keys() {
if !to.contains_key(key) {
env::remove_var(key);
Expand Down
17 changes: 10 additions & 7 deletions crates/turbo-tasks-env/src/filter.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use anyhow::Result;
use indexmap::IndexMap;
use turbo_tasks::Vc;
use turbo_tasks::{RcStr, Vc};

use crate::{EnvMap, ProcessEnv};

Expand All @@ -9,16 +9,19 @@ use crate::{EnvMap, ProcessEnv};
#[turbo_tasks::value]
pub struct FilterProcessEnv {
prior: Vc<Box<dyn ProcessEnv>>,
filters: Vec<String>,
filters: Vec<RcStr>,
}

#[turbo_tasks::value_impl]
impl FilterProcessEnv {
#[turbo_tasks::function]
pub fn new(prior: Vc<Box<dyn ProcessEnv>>, filters: Vec<String>) -> Vc<Self> {
pub fn new(prior: Vc<Box<dyn ProcessEnv>>, filters: Vec<RcStr>) -> Vc<Self> {
FilterProcessEnv {
prior,
filters: filters.into_iter().map(|f| f.to_uppercase()).collect(),
filters: filters
.into_iter()
.map(|f| f.to_uppercase().into())
.collect(),
}
.cell()
}
Expand All @@ -33,7 +36,7 @@ impl ProcessEnv for FilterProcessEnv {
for (key, value) in &*prior {
let uppercase = key.to_uppercase();
for filter in &self.filters {
if uppercase.starts_with(filter) {
if uppercase.starts_with(&**filter) {
filtered.insert(key.clone(), value.clone());
break;
}
Expand All @@ -43,9 +46,9 @@ impl ProcessEnv for FilterProcessEnv {
}

#[turbo_tasks::function]
fn read(&self, name: String) -> Vc<Option<String>> {
fn read(&self, name: RcStr) -> Vc<Option<RcStr>> {
for filter in &self.filters {
if name.to_uppercase().starts_with(filter) {
if name.to_uppercase().starts_with(&**filter) {
return self.prior.read(name);
}
}
Expand Down
20 changes: 11 additions & 9 deletions crates/turbo-tasks-env/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@ use std::{env, sync::Mutex};

use anyhow::Result;
use indexmap::IndexMap;
use turbo_tasks::Vc;
use turbo_tasks::{RcStr, Vc};

pub use self::{
command_line::CommandLineProcessEnv, custom::CustomProcessEnv, dotenv::DotenvProcessEnv,
filter::FilterProcessEnv,
};

#[turbo_tasks::value(transparent)]
pub struct EnvMap(#[turbo_tasks(trace_ignore)] IndexMap<String, String>);
pub struct EnvMap(#[turbo_tasks(trace_ignore)] IndexMap<RcStr, RcStr>);

#[turbo_tasks::value_impl]
impl EnvMap {
Expand All @@ -35,7 +35,7 @@ impl ProcessEnv for EnvMap {
}

#[turbo_tasks::function]
async fn read(self: Vc<Self>, name: String) -> Vc<Option<String>> {
async fn read(self: Vc<Self>, name: RcStr) -> Vc<Option<RcStr>> {
case_insensitive_read(self, name)
}
}
Expand All @@ -51,23 +51,25 @@ pub trait ProcessEnv {
fn read_all(self: Vc<Self>) -> Vc<EnvMap>;

/// Reads a single env variable. Ignores casing.
fn read(self: Vc<Self>, name: String) -> Vc<Option<String>> {
fn read(self: Vc<Self>, name: RcStr) -> Vc<Option<RcStr>> {
case_insensitive_read(self.read_all(), name)
}
}

pub fn sorted_env_vars() -> IndexMap<String, String> {
let mut vars = env::vars().collect::<IndexMap<_, _>>();
pub fn sorted_env_vars() -> IndexMap<RcStr, RcStr> {
let mut vars = env::vars()
.map(|(k, v)| (k.into(), v.into()))
.collect::<IndexMap<_, _>>();
vars.sort_keys();
vars
}

#[turbo_tasks::function]
pub async fn case_insensitive_read(map: Vc<EnvMap>, name: String) -> Result<Vc<Option<String>>> {
pub async fn case_insensitive_read(map: Vc<EnvMap>, name: RcStr) -> Result<Vc<Option<RcStr>>> {
Ok(Vc::cell(
to_uppercase_map(map)
.await?
.get(&name.to_uppercase())
.get(&RcStr::from(name.to_uppercase()))
.cloned(),
))
}
Expand All @@ -77,7 +79,7 @@ async fn to_uppercase_map(map: Vc<EnvMap>) -> Result<Vc<EnvMap>> {
let map = &*map.await?;
let mut new = IndexMap::with_capacity(map.len());
for (k, v) in map {
new.insert(k.to_uppercase(), v.clone());
new.insert(k.to_uppercase().into(), v.clone());
}
Ok(Vc::cell(new))
}
Expand Down
Loading

0 comments on commit 1caee85

Please sign in to comment.