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

perf: bundle runtime dependencies #1554

Merged
merged 12 commits into from
Aug 15, 2023
Merged

perf: bundle runtime dependencies #1554

merged 12 commits into from
Aug 15, 2023

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Aug 9, 2023

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Nitro output uses file tracing for all node modules by default. It has many benefits such as supporting native modules and the best compatibility with minimized rollup involvement.

However, there are downsides with externals too at least for built-in and standard unjs packages inlining is better.

  • Issues with package hosting and multi-version handling
  • we can optimize tree-shaking output up to file level instead of function exports
  • Extra data such as package.json files are copied over the output directory
  • Extra startup overhead because of node_modules resolution
  • Possible wrong export condition resolutions
  • Inability to tree-shake with predictable platform injections (such as AsyncLocalStorage)

This PR uses a list of nitro built-in runtime dependencies and explicitly inlines them in the final output bundle.

TODO: This method however has a downside in that inlined sourcemaps add to the bundle overhead. Looking for a way to disable inline-source maps only for node_modules.

Benchmark Results

Measurement / Mode external deps inlined deps
number of files 120 13
disk size (without sourcemaps) 840 KB 476 KB
start 31.823 ms 20ms
fetch 14.18 ms 14ms
start-to-fetch 47.464ms 35ms

Config:

export default defineNitroConfig({
  sourceMap: false,
  analyze: {
    filename: "./analyze.html",
  },
});

Timing bench:

console.time("start");
console.time("start-to-fetch");

await import("./b/server/index.mjs").then(async () => {
  console.timeEnd("start");
  console.time("fetch");
  await fetch("http://localhost:3000/").then((r) => {
    console.timeEnd("fetch");
    console.timeEnd("start-to-fetch");
  });
});

Node.js version: v18.16.0 on Macbook Air M2

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@pi0 pi0 requested a review from danielroe August 9, 2023 00:06
@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #1554 (4aeda33) into main (6430ded) will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1554      +/-   ##
==========================================
+ Coverage   76.00%   76.20%   +0.19%     
==========================================
  Files          74       76       +2     
  Lines        7711     7778      +67     
  Branches      760      773      +13     
==========================================
+ Hits         5861     5927      +66     
- Misses       1848     1849       +1     
  Partials        2        2              
Files Changed Coverage Ξ”
src/deps.ts 100.00% <100.00%> (ΓΈ)
src/index.ts 100.00% <100.00%> (ΓΈ)
src/rollup/config.ts 91.48% <100.00%> (+0.26%) ⬆️
src/rollup/plugins/sourcemap-ignore.ts 100.00% <100.00%> (ΓΈ)
src/types/nitro.ts 100.00% <100.00%> (ΓΈ)

... and 1 file with indirect coverage changes

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

In future we might need an opt out for major versions somehow? (If user, or other library is using dependency.)

Or equally if one of these is a dependency of an external we might have an issue too.

src/deps.ts Outdated Show resolved Hide resolved
Co-authored-by: Daniel Roe <daniel@roe.dev>
@pi0
Copy link
Member Author

pi0 commented Aug 9, 2023

In future we might need an opt out for major versions somehow? (If user, or other library is using dependency.) Or equally if one of these is a dependency of an external we might have an issue too.

Yes, i was thinking to support externals.exclude (and filter-out default inlines when user/framework specifically requires opt-out for a feature) + a flag to completely disable this behavior.

Regarding major versions (or any custom version by user), we still use rollup's search path, resolution, and aliases so overriding is possible by all means, however even totday, we require them (defined list) to be in Nitro-Expected semver-major range as nitro internally uses all these deps. (if an external dependency in .output/server/node_modules/x requires defu@v1 for example and nitro itself rely on v2, we still just like today also have .output/server/node_modules/defu@1 and bundle the v2 for nitro runtime itself as external resolution is handled separately from rollup)

I want to make sure mainly it is good to you too for becoming the default behavior .

@danielroe
Copy link
Member

It is fine to me as default behaviour. I'm observing the potential issue that if a library in externals uses ufo/pathe then this will be included twice (once in node_modules and once bundled). If other major versions of libraries like ufo are included in externals that entirely addresses my first point.

@pi0
Copy link
Member Author

pi0 commented Aug 9, 2023

if a library in externals uses ufo/pathe then this will be included twice (once in node_modules and once bundled).

That's a valid point if both inline and external use same semver/major of ufo. And was initial reason that we made all externals by default. I guess we can think of some ways to automatically "deoptimize" inlining however it involves traced file dependency "pre-analysis" and sub-dependencny deptimization.

I guess for default behavior, it should benefit most of users both Nitro and Nuxt with both bundle size but more importantly the stability of resolutions with rollup comparing to current externals.

Copy link
Member

Yes, I see this as a good change, too.

@pi0
Copy link
Member Author

pi0 commented Aug 15, 2023

Updated: Sourcemap size issue is (kinda) resolved by using a sourcemap minify plugin that scans emited sourcemap chunks and dynamically disables the ones that include node modules (reducing basic build from 2.1 MB to 454 kB.

I think there is way more area for improving this plugin. We can tree-shake content only for example. Considering we use lazy chunks for user code, this should be relatively ok. This can be disabled with experimental.sourcemapMinify: false in case of any issues.

experimental.bundleRuntimeDependencies: false flag can also be used to disable the whole experiment.

Merging to main to test against edge about effect of this change and fine-tune until release.

@pi0 pi0 merged commit b8b834f into main Aug 15, 2023
5 checks passed
@pi0 pi0 deleted the perf/inline-known branch August 15, 2023 20:32
@pi0 pi0 mentioned this pull request Aug 21, 2023
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.

None yet

2 participants