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

implement basic side effects optimization #6590

Merged
merged 30 commits into from
Dec 6, 2023
Merged

Conversation

sokra
Copy link
Member

@sokra sokra commented Nov 27, 2023

Description

  • optimizes reexports for modules that are inside of folders with a package.json that contains "sideEffects": false
  • For named reexports
    • skips resolving, reading, parsing, transforms, code generation and bundling for unrelated reexports
    • skips code generation and bundling for the module containing the followed reexport
  • For star reexports
    • skips code generation and bundling for unrelated reexports before the followed reexport
    • skips resolving, reading, parsing, transforms, code generation and bundling for unrelated reexports after the followed reexport
    • skips code generation and bundling for the module containing the followed reexport

Follow-up work:

  • sideEffects in package.json also supports a glob. We need to support that too.
  • renaming exports is not yet supported. We need to create a naming module for that to avoid effects on the importer side.

Closes PACK-2042

Copy link

vercel bot commented Nov 27, 2023

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 Dec 6, 2023 7:41am
examples-tailwind-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback Dec 6, 2023 7:41am
rust-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 6, 2023 7:41am
8 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2023 7:41am
examples-gatsby-web ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2023 7:41am
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2023 7:41am
examples-native-web ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2023 7:41am
examples-nonmonorepo ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2023 7:41am
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2023 7:41am
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2023 7:41am
turbo-site ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2023 7:41am

Copy link
Contributor

github-actions bot commented Nov 27, 2023

🟢 Turbopack Benchmark CI successful 🟢

Thanks

Copy link
Contributor

github-actions bot commented Nov 27, 2023

✅ This change can build next-swc

Copy link
Contributor

github-actions bot commented Nov 27, 2023

🟢 CI successful 🟢

Thanks

Copy link
Contributor

Linux Benchmark for 24c112b

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 21.39ms ± 0.83ms 21.33ms ± 0.84ms -0.26%
bench_hmr_to_eval/Turbopack CSR/1000 modules 20.88ms ± 0.81ms 20.90ms ± 0.79ms +0.07%
bench_startup/Turbopack CSR/1000 modules 1120.34ms ± 5.70ms 1117.24ms ± 9.87ms -0.28%

Comment on lines 64 to 68
self.inner()
.await?
.into_iter()
.map(|&inner| Vc::upcast(InternalCssAssetReference::new(inner)))
.collect(),
Copy link
Contributor

@ForsakenHarmony ForsakenHarmony Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

into_iter and collect here is kinda confusing

makes it look like inner returns multiple items instead of an option

same thing in other places in this PR

Copy link
Contributor

Linux Benchmark for 7d2bb95

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 21.24ms ± 0.83ms 21.20ms ± 0.82ms -0.18%
bench_hmr_to_eval/Turbopack CSR/1000 modules 20.86ms ± 0.80ms 21.05ms ± 0.89ms +0.88%
bench_startup/Turbopack CSR/1000 modules 1137.93ms ± 10.81ms 1115.80ms ± 14.55ms -1.94%

Copy link
Contributor

@ForsakenHarmony ForsakenHarmony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems good apart from some missing documentation and convoluted code 🙃

sorry for the individual comments, I often forget to submit reviews if I don't post the comments right away

@sokra
Copy link
Member Author

sokra commented Nov 27, 2023

sorry for the individual comments, I often forget to submit reviews if I don't post the comments right away

same

Copy link
Contributor

Linux Benchmark for eb330ba

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 22.08ms ± 0.88ms 22.09ms ± 0.87ms +0.05%
bench_hmr_to_eval/Turbopack CSR/1000 modules 21.70ms ± 0.82ms 21.79ms ± 0.81ms +0.42%
bench_startup/Turbopack CSR/1000 modules 1127.11ms ± 13.41ms 1098.80ms ± 10.59ms -2.51%

let inner = self
.inner()
.await?
.context("inner asset should be CSS processable")?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not sure about this, I guess right now this can't happen

but if it does it would just throw a random error not telling you what the inner asset was and why it's not processable

Copy link
Contributor

Linux Benchmark for 9212a80

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 21.55ms ± 0.87ms 21.54ms ± 0.83ms -0.03%
bench_hmr_to_eval/Turbopack CSR/1000 modules 21.11ms ± 0.84ms 21.02ms ± 0.84ms -0.44%
bench_startup/Turbopack CSR/1000 modules 1114.35ms ± 2.82ms 1121.91ms ± 7.59ms +0.68%

Copy link
Contributor

Linux Benchmark for 9e7bbcd

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 21.40ms ± 0.83ms 21.34ms ± 0.83ms -0.31%
bench_hmr_to_eval/Turbopack CSR/1000 modules 21.11ms ± 0.76ms 21.02ms ± 0.82ms -0.42%
bench_startup/Turbopack CSR/1000 modules 1096.33ms ± 7.22ms 1076.10ms ± 8.10ms -1.85%

@sokra sokra force-pushed the sokra/side-effects-optimization branch from 13d5147 to d0f389d Compare December 6, 2023 07:31
@sokra sokra enabled auto-merge (squash) December 6, 2023 07:32
@sokra sokra merged commit 16bbf81 into main Dec 6, 2023
39 checks passed
@sokra sokra deleted the sokra/side-effects-optimization branch December 6, 2023 07:42
Copy link
Contributor

github-actions bot commented Dec 6, 2023

Linux Benchmark for ac33c8a

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 22.02ms ± 0.88ms 22.24ms ± 0.96ms +1.00%
bench_hmr_to_eval/Turbopack CSR/1000 modules 21.60ms ± 0.89ms 21.63ms ± 0.87ms +0.16%
bench_startup/Turbopack CSR/1000 modules 1102.32ms ± 7.53ms 1115.03ms ± 12.86ms +1.15%

sokra added a commit to vercel/next.js that referenced this pull request Dec 6, 2023
### What?

Code update for refactoring in vercel/turborepo#6590


Closes PACK-2043
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
### Description

* optimizes reexports for modules that are inside of folders with a
package.json that contains `"sideEffects": false`
* For named reexports
* skips resolving, reading, parsing, transforms, code generation and
bundling for unrelated reexports
* skips code generation and bundling for the module containing the
followed reexport
* For star reexports
* skips code generation and bundling for unrelated reexports before the
followed reexport
* skips resolving, reading, parsing, transforms, code generation and
bundling for unrelated reexports after the followed reexport
* skips code generation and bundling for the module containing the
followed reexport


Follow-up work:
* `sideEffects` in package.json also supports a glob. We need to support
that too.
* renaming exports is not yet supported. We need to create a naming
module for that to avoid effects on the importer side.

Closes PACK-2042
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
### Description

* optimizes reexports for modules that are inside of folders with a
package.json that contains `"sideEffects": false`
* For named reexports
* skips resolving, reading, parsing, transforms, code generation and
bundling for unrelated reexports
* skips code generation and bundling for the module containing the
followed reexport
* For star reexports
* skips code generation and bundling for unrelated reexports before the
followed reexport
* skips resolving, reading, parsing, transforms, code generation and
bundling for unrelated reexports after the followed reexport
* skips code generation and bundling for the module containing the
followed reexport


Follow-up work:
* `sideEffects` in package.json also supports a glob. We need to support
that too.
* renaming exports is not yet supported. We need to create a naming
module for that to avoid effects on the importer side.

Closes PACK-2042
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
### Description

* optimizes reexports for modules that are inside of folders with a
package.json that contains `"sideEffects": false`
* For named reexports
* skips resolving, reading, parsing, transforms, code generation and
bundling for unrelated reexports
* skips code generation and bundling for the module containing the
followed reexport
* For star reexports
* skips code generation and bundling for unrelated reexports before the
followed reexport
* skips resolving, reading, parsing, transforms, code generation and
bundling for unrelated reexports after the followed reexport
* skips code generation and bundling for the module containing the
followed reexport


Follow-up work:
* `sideEffects` in package.json also supports a glob. We need to support
that too.
* renaming exports is not yet supported. We need to create a naming
module for that to avoid effects on the importer side.

Closes PACK-2042
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
### Description

* optimizes reexports for modules that are inside of folders with a
package.json that contains `"sideEffects": false`
* For named reexports
* skips resolving, reading, parsing, transforms, code generation and
bundling for unrelated reexports
* skips code generation and bundling for the module containing the
followed reexport
* For star reexports
* skips code generation and bundling for unrelated reexports before the
followed reexport
* skips resolving, reading, parsing, transforms, code generation and
bundling for unrelated reexports after the followed reexport
* skips code generation and bundling for the module containing the
followed reexport


Follow-up work:
* `sideEffects` in package.json also supports a glob. We need to support
that too.
* renaming exports is not yet supported. We need to create a naming
module for that to avoid effects on the importer side.

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

Successfully merging this pull request may close these issues.

2 participants