perf: conditionally import Telemetry #63574
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grajen3
requested review from
timneutkens,
ijjk,
shuding,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
ijjk
approved these changes
Mar 21, 2024
Failing test suitesCommit: 01ea276
Expand output● AMP SSG Support › production mode › should load an amp first page correctly ● AMP SSG Support › production mode › should load a hybrid amp page without query correctly ● AMP SSG Support › production mode › should load dynamic hybrid SSG/AMP page ● AMP SSG Support › production mode › should load dynamic hybrid SSG/AMP page with trailing slash ● AMP SSG Support › production mode › should load dynamic hybrid SSG/AMP page with query ● AMP SSG Support › production mode › should load a hybrid amp page with query correctly ● AMP SSG Support › production mode › should output prerendered files correctly during build ● AMP SSG Support › export mode › production mode › should have copied SSG files correctly Read more about building and testing Next.js in contributing.md.
Expand output● Prerender › should not error when rewriting to fallback dynamic SSG page Read more about building and testing Next.js in contributing.md. |
Stats from current PRDefault Build (Increase detected
|
| vercel/next.js canary | grajen3/next.js perf/lazy-import-telemetry-in-standalone | Change | |
|---|---|---|---|
| buildDuration | 14.4s | 14.5s | |
| buildDurationCached | 7.9s | 6.7s | N/A |
| nodeModulesSize | 198 MB | 198 MB | |
| nextStartRea..uration (ms) | 445ms | 406ms | N/A |
Client Bundles (main, webpack)
| vercel/next.js canary | grajen3/next.js perf/lazy-import-telemetry-in-standalone | Change | |
|---|---|---|---|
| 2453-HASH.js gzip | 30.8 kB | 30.8 kB | N/A |
| 3304.HASH.js gzip | 181 B | 181 B | ✓ |
| 3f784ff6-HASH.js gzip | 53.7 kB | 53.7 kB | ✓ |
| 8299-HASH.js gzip | 5.04 kB | 5.04 kB | N/A |
| framework-HASH.js gzip | 45.2 kB | 45.2 kB | ✓ |
| main-app-HASH.js gzip | 242 B | 242 B | ✓ |
| main-HASH.js gzip | 32.2 kB | 32.2 kB | N/A |
| webpack-HASH.js gzip | 1.68 kB | 1.68 kB | N/A |
| Overall change | 99.3 kB | 99.3 kB | ✓ |
Legacy Client Bundles (polyfills)
| vercel/next.js canary | grajen3/next.js perf/lazy-import-telemetry-in-standalone | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | grajen3/next.js perf/lazy-import-telemetry-in-standalone | Change | |
|---|---|---|---|
| _app-HASH.js gzip | 196 B | 197 B | N/A |
| _error-HASH.js gzip | 184 B | 184 B | ✓ |
| amp-HASH.js gzip | 505 B | 505 B | ✓ |
| css-HASH.js gzip | 324 B | 325 B | N/A |
| dynamic-HASH.js gzip | 2.5 kB | 2.5 kB | N/A |
| edge-ssr-HASH.js gzip | 258 B | 258 B | ✓ |
| head-HASH.js gzip | 352 B | 352 B | ✓ |
| hooks-HASH.js gzip | 370 B | 371 B | N/A |
| image-HASH.js gzip | 4.21 kB | 4.21 kB | ✓ |
| index-HASH.js gzip | 259 B | 259 B | ✓ |
| link-HASH.js gzip | 2.67 kB | 2.67 kB | N/A |
| routerDirect..HASH.js gzip | 314 B | 312 B | N/A |
| script-HASH.js gzip | 386 B | 386 B | ✓ |
| withRouter-HASH.js gzip | 309 B | 309 B | ✓ |
| 1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
| Overall change | 6.57 kB | 6.57 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | grajen3/next.js perf/lazy-import-telemetry-in-standalone | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 481 B | 484 B | N/A |
| Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
| vercel/next.js canary | grajen3/next.js perf/lazy-import-telemetry-in-standalone | Change | |
|---|---|---|---|
| index.html gzip | 529 B | 529 B | ✓ |
| link.html gzip | 541 B | 541 B | ✓ |
| withRouter.html gzip | 523 B | 523 B | ✓ |
| Overall change | 1.59 kB | 1.59 kB | ✓ |
Edge SSR bundle Size
| vercel/next.js canary | grajen3/next.js perf/lazy-import-telemetry-in-standalone | Change | |
|---|---|---|---|
| edge-ssr.js gzip | 95.3 kB | 95.3 kB | N/A |
| page.js gzip | 3.04 kB | 3.04 kB | N/A |
| Overall change | 0 B | 0 B | ✓ |
Middleware size
| vercel/next.js canary | grajen3/next.js perf/lazy-import-telemetry-in-standalone | Change | |
|---|---|---|---|
| middleware-b..fest.js gzip | 625 B | 626 B | N/A |
| middleware-r..fest.js gzip | 151 B | 151 B | ✓ |
| middleware.js gzip | 25.5 kB | 25.5 kB | N/A |
| edge-runtime..pack.js gzip | 839 B | 839 B | ✓ |
| Overall change | 990 B | 990 B | ✓ |
Next Runtimes
| vercel/next.js canary | grajen3/next.js perf/lazy-import-telemetry-in-standalone | Change | |
|---|---|---|---|
| app-page-exp...dev.js gzip | 170 kB | 170 kB | ✓ |
| app-page-exp..prod.js gzip | 96.8 kB | 96.8 kB | ✓ |
| app-page-tur..prod.js gzip | 98.5 kB | 98.5 kB | ✓ |
| app-page-tur..prod.js gzip | 92.8 kB | 92.8 kB | ✓ |
| app-page.run...dev.js gzip | 144 kB | 144 kB | ✓ |
| app-page.run..prod.js gzip | 91.3 kB | 91.3 kB | ✓ |
| app-route-ex...dev.js gzip | 21.4 kB | 21.4 kB | ✓ |
| app-route-ex..prod.js gzip | 15.1 kB | 15.1 kB | ✓ |
| app-route-tu..prod.js gzip | 15.1 kB | 15.1 kB | ✓ |
| app-route-tu..prod.js gzip | 14.8 kB | 14.8 kB | ✓ |
| app-route.ru...dev.js gzip | 21 kB | 21 kB | ✓ |
| app-route.ru..prod.js gzip | 14.8 kB | 14.8 kB | ✓ |
| pages-api-tu..prod.js gzip | 9.55 kB | 9.55 kB | ✓ |
| pages-api.ru...dev.js gzip | 9.82 kB | 9.82 kB | ✓ |
| pages-api.ru..prod.js gzip | 9.55 kB | 9.55 kB | ✓ |
| pages-turbo...prod.js gzip | 22.5 kB | 22.5 kB | ✓ |
| pages.runtim...dev.js gzip | 23.1 kB | 23.1 kB | ✓ |
| pages.runtim..prod.js gzip | 22.4 kB | 22.4 kB | ✓ |
| server.runti..prod.js gzip | 50.9 kB | 50.9 kB | ✓ |
| Overall change | 943 kB | 943 kB | ✓ |
build cache Overall increase ⚠️
| vercel/next.js canary | grajen3/next.js perf/lazy-import-telemetry-in-standalone | Change | |
|---|---|---|---|
| 0.pack gzip | 1.56 MB | 1.57 MB | |
| index.pack gzip | 106 kB | 105 kB | N/A |
| Overall change | 1.56 MB | 1.57 MB |
Diff details
Diff for middleware.js
Diff too large to display
|
Those tests that are reported as failing are passing at least locally - are those perhaps flaky? |
ijjk
approved these changes
Mar 25, 2024
|
Awesome! I wonder if there are more opportunities like this to improve cold start performance |
From time spent on importing also caught my attention as something that doesn't seem like it would be needed to run production server (this seems more like something for dev server to watch source files and recompile them when users make edits), but this one had much smaller cost so I didn't yet look into it as I do have other opportunities to look into first (more on my side of things than Next.js) - but maybe this is something someone else can look into as well? This changed just seemed like real low hanging fruit Of course there is also whole other part which is actual initialization of the server (after it is imported) that could be profiled? I did see open-telemetry instrumentation across Next.js codebase as well, so potentially that could yield some insights that would be easier to read than something like CPU profile |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Make Telemetry import in
packages/next/src/server/lib/router-server.tslazy and conditionalWhy?
Telemetryin that module is only used whenopts.devis truthy, so in production mode the module is imported and used.Cost of importing that module is quite significant chunk of starting
standaloneserver. Those are example logs I got from https://www.npmjs.com/package/require-times (with updates to make it work as that is pretty old package that wasn't updated and doesn't work as is) on the server:Before the change:
And after the change:
I would not pay much attention to absolute numbers as there will be variance and those are single runs - but from first "require time dump" you can estimate importing Telemetry (that might be unused) is costing 149ms / 658ms ~= 22.5% of entire time spent on importing modules - that's pretty significant
How?
By moving static import/require from top level to conditional code path that actually uses it. This code path already have some modules lazy/conditionally loaded.
packages/next/src/telemetry/storage.tsdoesn't seem to have import side effects so at least on my first glance it doesn't seem like moving import should cause problems?