Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor optimizations to the codegen of TaskFnInputFunction #8304

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

bgw
Copy link
Member

@bgw bgw commented Jun 5, 2024

What?

I noticed a number of small optimizations that could be applied to these hot and heavily-monomorphized functions:

  • Using with_context() instead of .context(), to avoid evaluating the error message in the common case that it's unused. I also tried concat!() since this can be a static string, but the resulting binary is slightly larger, and we don't need to optimize for the unlikely error case.
  • Extracted the parts of the monomorphized functions that didn't require the type parameters into separate non-generic functions. While the goal here is mostly to reduce binary size and compilation time, this optimization on it's own seems to help with the runtime benchmarks too (though I didn't test it rigorously in isolation).

Here's a section from Rust for Rustaceans explaining this "non-generic function" trick:

Screenshot from 2024-06-04 20-26-20.png

Binary Size?

Slightly negative, at least for stripped debug builds:

time pnpm pack-next
-rw-r--r-- 1 bgw bgw 167895040 Jun  4 17:20 next-swc.after.tar
-rw-r--r-- 1 bgw bgw 168622080 Jun  4 15:37 next-swc.before.tar

Runtime Performance?

Using https://github.com/bgw/benchmark-scripts/

Microbenchmark (turbo_tasks_memory_stress/fibonacci/200)

$ TURBOPACK_BENCH_STRESS=yes cargo bench -p turbo-tasks-memory -- fibonacci/200
   Compiling turbo-tasks v0.1.0 (/home/bgw/turbo/crates/turbo-tasks)
   Compiling turbo-tasks-memory v0.1.0 (/home/bgw/turbo/crates/turbo-tasks-memory)
   Compiling turbo-tasks-testing v0.1.0 (/home/bgw/turbo/crates/turbo-tasks-testing)
    Finished `bench` profile [optimized] target(s) in 10.79s
     Running benches/mod.rs (target/release/deps/mod-8c0f970371f8713d)
turbo_tasks_memory_stress/fibonacci/200
                        time:   [64.420 ms 64.683 ms 64.941 ms]
                        thrpt:  [309.53 Kelem/s 310.76 Kelem/s 312.03 Kelem/s]
                 change:
                        time:   [-2.2828% -1.7587% -1.2206%] (p = 0.00 < 0.05)
                        thrpt:  [+1.2357% +1.7902% +2.3361%]
                        Performance has improved.
Found 1 outliers among 20 measurements (5.00%)
  1 (5.00%) low mild

"Realistic" Benchmark (bench_startup/Turbopack CSR/1000 modules)

The difference is small. I patched the benchmark to increase the number of iterations so I could get something statistically significant.

diff --git a/crates/turbopack-bench/src/lib.rs b/crates/turbopack-bench/src/lib.rs
index 4e3df12db0..d950d76071 100644
--- a/crates/turbopack-bench/src/lib.rs
+++ b/crates/turbopack-bench/src/lib.rs
@@ -35,8 +35,8 @@ pub mod util;

 pub fn bench_startup(c: &mut Criterion, bundlers: &[Box<dyn Bundler>]) {
     let mut g = c.benchmark_group("bench_startup");
-    g.sample_size(10);
-    g.measurement_time(Duration::from_secs(60));
+    g.sample_size(100);
+    g.measurement_time(Duration::from_secs(600));

     bench_startup_internal(g, false, bundlers);
 }
cargo bench -p turbopack-cli -- bench_startup
    Finished `bench` profile [optimized] target(s) in 1.30s
     Running benches/mod.rs (target/release/deps/mod-2681e324dfd90da1)
bench_startup/Turbopack CSR/1000 modules
                        time:   [2.2684 s 2.2717 s 2.2750 s]
                        change: [-1.9365% -1.7387% -1.5602%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild

Build Speed?

Not enough of a difference to measure.

rm -rf target/ && time cargo build -p turbopack-cli

Before:

real    10m42.174s

After:

real    10m40.735s

Copy link

vercel bot commented Jun 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-basic-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback Jun 5, 2024 0:24am
examples-gatsby-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback Jun 5, 2024 0:24am
examples-kitchensink-blog 🔄 Building (Inspect) Visit Preview 💬 Add feedback Jun 5, 2024 0:24am
examples-native-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback Jun 5, 2024 0:24am
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 5, 2024 0:24am
rust-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 5, 2024 0:24am
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Jun 5, 2024 0:24am
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview Jun 5, 2024 0:24am
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview Jun 5, 2024 0:24am
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Jun 5, 2024 0:24am

Copy link
Member Author

bgw commented Jun 5, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bgw and the rest of your teammates on Graphite Graphite

Copy link
Contributor

github-actions bot commented Jun 5, 2024

🟢 Turbopack Benchmark CI successful 🟢

Thanks

Copy link
Contributor

github-actions bot commented Jun 5, 2024

✅ This change can build next-swc

Copy link
Contributor

github-actions bot commented Jun 5, 2024

⚠️ CI failed ⚠️

The following steps have failed in CI:

  • Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

@bgw bgw changed the title Use static strings for .context() calls in TaskFnInputFunction impls Minor optimizations to the codegen of TaskFnInputFunction Jun 5, 2024
@bgw bgw marked this pull request as ready for review June 5, 2024 03:29
@bgw bgw requested a review from a team as a code owner June 5, 2024 03:29
@bgw bgw merged commit e65d1e7 into main Jun 5, 2024
56 of 58 checks passed
@bgw bgw deleted the bgw/task-fn-errors branch June 5, 2024 05:02
kdy1 added a commit to vercel/next.js that referenced this pull request Jun 5, 2024
# Turbopack

* vercel/turbo#8272 <!-- Donny/강동윤 - feat:
Update `swc_core` to `v0.92.8` -->
* vercel/turbo#8262 <!-- Alexander Lyon - add
crate to calculate prehashes -->
* vercel/turbo#8174 <!-- Tobias Koppers - use
prehash to avoid rehashing the key in the task cache -->
* vercel/turbo#7674 <!-- Alexander Lyon - [turbo
trace] add ability to filter by value and occurences -->
* vercel/turbo#8287 <!-- Donny/강동윤 - feat:
Update `swc_core` to `v0.92.10` -->
* vercel/turbo#8037 <!-- Alexander Lyon - create
turbo-static for compile time graph analysis -->
* vercel/turbo#8293 <!-- Will Binns-Smith - Sync
Cargo.lock with Next.js -->
* vercel/turbo#8239 <!-- Benjamin Woodruff -
Reduce amount of code generated by ValueDebugFormat -->
* vercel/turbo#8304 <!-- Benjamin Woodruff -
Minor optimizations to the codegen of TaskFnInputFunction -->
* vercel/turbo#8221 <!-- Donny/강동윤 - perf:
Introduce `RcStr` -->


### What?

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

### Why?

To reduce memory usage.

### How?

Closes PACK-2776
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants