-
Notifications
You must be signed in to change notification settings - Fork 26.1k
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
Eliminate unnecessary decode operations in node-web-streams-helpers.ts
#63427
Conversation
Failing test suitesCommit: 51875ea
Expand output● app dir - navigation › navigating to dynamic params & changing the casing › should load the page correctly
Read more about building and testing Next.js in contributing.md. |
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | vercel/next.js eliminate-unnecessary-decodes | Change | |
---|---|---|---|
buildDuration | 14s | 14s | N/A |
buildDurationCached | 7.6s | 6.4s | N/A |
nodeModulesSize | 198 MB | 198 MB | |
nextStartRea..uration (ms) | 436ms | 436ms | ✓ |
Client Bundles (main, webpack)
vercel/next.js canary | vercel/next.js eliminate-unnecessary-decodes | Change | |
---|---|---|---|
2453-HASH.js gzip | 30.5 kB | 30.5 kB | N/A |
3304.HASH.js gzip | 181 B | 181 B | ✓ |
3f784ff6-HASH.js gzip | 53.7 kB | 53.7 kB | N/A |
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 | 241 B | 242 B | N/A |
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 | 45.4 kB | 45.4 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | vercel/next.js eliminate-unnecessary-decodes | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | vercel/next.js eliminate-unnecessary-decodes | 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 | vercel/next.js eliminate-unnecessary-decodes | Change | |
---|---|---|---|
_buildManifest.js gzip | 481 B | 484 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | vercel/next.js eliminate-unnecessary-decodes | Change | |
---|---|---|---|
index.html gzip | 529 B | 529 B | ✓ |
link.html gzip | 541 B | 542 B | N/A |
withRouter.html gzip | 524 B | 523 B | N/A |
Overall change | 529 B | 529 B | ✓ |
Edge SSR bundle Size Overall increase ⚠️
vercel/next.js canary | vercel/next.js eliminate-unnecessary-decodes | Change | |
---|---|---|---|
edge-ssr.js gzip | 95.2 kB | 95.4 kB | |
page.js gzip | 3.04 kB | 3.04 kB | N/A |
Overall change | 95.2 kB | 95.4 kB |
Middleware size
vercel/next.js canary | vercel/next.js eliminate-unnecessary-decodes | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 625 B | 625 B | ✓ |
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 | 1.61 kB | 1.61 kB | ✓ |
Next Runtimes Overall increase ⚠️
vercel/next.js canary | vercel/next.js eliminate-unnecessary-decodes | Change | |
---|---|---|---|
app-page-exp...dev.js gzip | 171 kB | 171 kB | |
app-page-exp..prod.js gzip | 96.6 kB | 96.9 kB | |
app-page-tur..prod.js gzip | 98.3 kB | 98.6 kB | |
app-page-tur..prod.js gzip | 92.8 kB | 93.1 kB | |
app-page.run...dev.js gzip | 149 kB | 150 kB | |
app-page.run..prod.js gzip | 91.3 kB | 91.6 kB | |
app-route-ex...dev.js gzip | 21.3 kB | 21.3 kB | ✓ |
app-route-ex..prod.js gzip | 15 kB | 15 kB | ✓ |
app-route-tu..prod.js gzip | 15 kB | 15 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.52 kB | 9.52 kB | ✓ |
pages-api.ru...dev.js gzip | 9.8 kB | 9.8 kB | ✓ |
pages-api.ru..prod.js gzip | 9.52 kB | 9.52 kB | ✓ |
pages-turbo...prod.js gzip | 22.3 kB | 22.5 kB | |
pages.runtim...dev.js gzip | 22.9 kB | 23.1 kB | |
pages.runtim..prod.js gzip | 22.3 kB | 22.5 kB | |
server.runti..prod.js gzip | 50.6 kB | 50.7 kB | N/A |
Overall change | 897 kB | 900 kB |
build cache Overall increase ⚠️
vercel/next.js canary | vercel/next.js eliminate-unnecessary-decodes | Change | |
---|---|---|---|
0.pack gzip | 1.56 MB | 1.56 MB | |
index.pack gzip | 105 kB | 105 kB | N/A |
Overall change | 1.56 MB | 1.56 MB |
Diff details
Diff for middleware.js
Diff too large to display
Diff for edge-ssr.js
Diff too large to display
Diff for app-page-exp..ntime.dev.js
failed to diff
Diff for app-page-exp..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page.runtime.dev.js
failed to diff
Diff for app-page.runtime.prod.js
Diff too large to display
Diff for pages-turbo...time.prod.js
Diff too large to display
Diff for pages.runtime.dev.js
Diff too large to display
Diff for pages.runtime.prod.js
Diff too large to display
Diff for server.runtime.prod.js
Diff too large to display
packages/next/src/server/stream-utils/node-web-streams-helper.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things I'd recommend changing before landing. There are a few comments just for consideration that you don't need to address unless you want to
packages/next/src/server/stream-utils/node-web-streams-helper.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect there is a bug in here based on some of the failures I see on the tests but I can't see it in the code. Will need to get those tests to green once we figure out what the issue is but absent that this looks good to me
This PR is strictly a performance improvement. It should not change implementation behavior in anyway.
This PR replaces
decoder.decode()
operations by operating with the encodedUint8Array
instances directly. I added some utility functions to make things a bit easier to understand.Ideally, this change also maintains a fair amount of code readability.
Will measure estimate performance improvement shortly.
Closes NEXT-2848