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

refactor: collectAppPageSegments #73908

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Conversation

ztanner
Copy link
Member

@ztanner ztanner commented Dec 13, 2024

This refactors collectAppPageSegments to be more efficient especially when dealing with multiple parallel routes, where the number of combinations of segments can drastically increase how many segments are returned per route. Previously this function only considered the children parallel route. When it was updated to consider all possible parallel routes in #73358, this had the unintended side effect of increasing the amount of duplicate segments this function would have to process.

We only need to return unique segments discovered from a particular loader tree (representing a page), as opposed to the same segment as many times as it might appear across all parallel routes. This PR uses a map to track unique segments with a composite key used to identify the discovered segments.

Additionally, this refactors the function to be iterative rather than recursive. We keep track of a queue of segments to be processed (a tuple of [loaderTree, segments]), and as we discover new parallel route paths, we process the queue further.

Fixes #73793
Closes #73871 (h/t to @icyJoseph for identifying the cause of the memory consumption)

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Dec 13, 2024
Copy link
Member Author

ztanner commented Dec 13, 2024

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

@ijjk
Copy link
Member

ijjk commented Dec 13, 2024

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js 12-13-refactor_collectapppagesegments Change
buildDuration 18.8s 15.6s N/A
buildDurationCached 14.8s 12.8s N/A
nodeModulesSize 410 MB 410 MB ⚠️ +6.19 kB
nextStartRea..uration (ms) 478ms 471ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js 12-13-refactor_collectapppagesegments Change
1187-HASH.js gzip 50.8 kB 50.8 kB N/A
8276.HASH.js gzip 169 B 168 B N/A
8377-HASH.js gzip 5.36 kB 5.36 kB N/A
bccd1874-HASH.js gzip 53 kB 53 kB N/A
framework-HASH.js gzip 57.5 kB 57.5 kB N/A
main-app-HASH.js gzip 232 B 235 B N/A
main-HASH.js gzip 34 kB 34 kB N/A
webpack-HASH.js gzip 1.71 kB 1.71 kB N/A
Overall change 0 B 0 B
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js 12-13-refactor_collectapppagesegments Change
polyfills-HASH.js gzip 39.4 kB 39.4 kB
Overall change 39.4 kB 39.4 kB
Client Pages
vercel/next.js canary vercel/next.js 12-13-refactor_collectapppagesegments Change
_app-HASH.js gzip 193 B 193 B
_error-HASH.js gzip 193 B 193 B
amp-HASH.js gzip 512 B 510 B N/A
css-HASH.js gzip 343 B 342 B N/A
dynamic-HASH.js gzip 1.84 kB 1.84 kB
edge-ssr-HASH.js gzip 265 B 265 B
head-HASH.js gzip 363 B 362 B N/A
hooks-HASH.js gzip 393 B 392 B N/A
image-HASH.js gzip 4.49 kB 4.49 kB N/A
index-HASH.js gzip 268 B 268 B
link-HASH.js gzip 2.35 kB 2.34 kB N/A
routerDirect..HASH.js gzip 328 B 328 B
script-HASH.js gzip 397 B 397 B
withRouter-HASH.js gzip 323 B 326 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 3.59 kB 3.59 kB
Client Build Manifests
vercel/next.js canary vercel/next.js 12-13-refactor_collectapppagesegments Change
_buildManifest.js gzip 749 B 746 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js 12-13-refactor_collectapppagesegments Change
index.html gzip 525 B 523 B N/A
link.html gzip 539 B 537 B N/A
withRouter.html gzip 520 B 521 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js 12-13-refactor_collectapppagesegments Change
edge-ssr.js gzip 128 kB 128 kB N/A
page.js gzip 203 kB 203 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js 12-13-refactor_collectapppagesegments Change
middleware-b..fest.js gzip 672 B 667 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 31.2 kB 31.2 kB N/A
edge-runtime..pack.js gzip 844 B 844 B
Overall change 844 B 844 B
Next Runtimes
vercel/next.js canary vercel/next.js 12-13-refactor_collectapppagesegments Change
523-experime...dev.js gzip 322 B 322 B
523.runtime.dev.js gzip 314 B 314 B
app-page-exp...dev.js gzip 323 kB 323 kB
app-page-exp..prod.js gzip 127 kB 127 kB
app-page-tur..prod.js gzip 140 kB 140 kB
app-page-tur..prod.js gzip 135 kB 135 kB
app-page.run...dev.js gzip 313 kB 313 kB
app-page.run..prod.js gzip 123 kB 123 kB
app-route-ex...dev.js gzip 37.3 kB 37.3 kB
app-route-ex..prod.js gzip 25.3 kB 25.3 kB
app-route-tu..prod.js gzip 25.4 kB 25.4 kB
app-route-tu..prod.js gzip 25.2 kB 25.2 kB
app-route.ru...dev.js gzip 38.9 kB 38.9 kB
app-route.ru..prod.js gzip 25.2 kB 25.2 kB
pages-api-tu..prod.js gzip 9.67 kB 9.67 kB
pages-api.ru...dev.js gzip 11.6 kB 11.6 kB
pages-api.ru..prod.js gzip 9.67 kB 9.67 kB
pages-turbo...prod.js gzip 21.7 kB 21.7 kB
pages.runtim...dev.js gzip 27.4 kB 27.4 kB
pages.runtim..prod.js gzip 21.7 kB 21.7 kB
server.runti..prod.js gzip 916 kB 916 kB
Overall change 2.36 MB 2.36 MB
build cache Overall increase ⚠️
vercel/next.js canary vercel/next.js 12-13-refactor_collectapppagesegments Change
0.pack gzip 2.05 MB 2.05 MB ⚠️ +3.34 kB
index.pack gzip 71.8 kB 71.2 kB N/A
Overall change 2.05 MB 2.05 MB ⚠️ +3.34 kB
Diff details
Diff for main-HASH.js

Diff too large to display

Commit: 8e35d9f

@ztanner ztanner marked this pull request as ready for review December 13, 2024 15:27
@ztanner ztanner requested review from wyattjoh and ijjk December 13, 2024 15:27
@ztanner ztanner merged commit 2889713 into canary Dec 13, 2024
113 checks passed
@ztanner ztanner deleted the 12-13-refactor_collectapppagesegments branch December 13, 2024 19:17
ztanner added a commit that referenced this pull request Dec 16, 2024
This refactors `collectAppPageSegments` to be more efficient especially
when dealing with multiple parallel routes, where the number of
combinations of segments can drastically increase how many segments are
returned per route. Previously this function only considered the
`children` parallel route. When it was updated to consider all possible
parallel routes in #73358, this had the unintended side effect of
increasing the amount of duplicate segments this function would have to
process.

We only need to return unique segments discovered from a particular
loader tree (representing a page), as opposed to the same segment as
many times as it might appear across all parallel routes. This PR uses a
map to track unique segments with a composite key used to identify the
discovered segments.

Additionally, this refactors the function to be iterative rather than
recursive. We keep track of a queue of segments to be processed (a tuple
of `[loaderTree, segments]`), and as we discover new parallel route
paths, we process the queue further.

Fixes #73793
Closes #73871 (h/t to @icyJoseph for identifying the cause of the memory
consumption)
ztanner added a commit that referenced this pull request Dec 16, 2024
@hdodov
Copy link
Contributor

hdodov commented Dec 18, 2024

Has this had a runtime performance impact as well? After updating to Next 15.1.0, we noticed random CPU usage increase on our servers, without much increase in traffic. Running in standalone mode in Docker, using dynamic and catch-all segments in the app router.

@ztanner do you have any idea?

@reh2ur
Copy link

reh2ur commented Dec 18, 2024

@hdodov Did you also see a continuous memory increase? We saw something looking like a memory leak when upgrading from 15.0.3 to 15.1.0, which is now resolved again after rolling that change back.

Screenshot 2024-12-18 at 14 19 57

@hdodov
Copy link
Contributor

hdodov commented Dec 18, 2024

@reh2ur I don't know about memory, but we had weird CPU usage increases during times of lower number of incoming requests:

image

As you can see, when request count is low, CPU usage is higher — makes no sense. I guess it could be related to that memory leak issue that you've had.

@reh2ur
Copy link

reh2ur commented Dec 18, 2024

We also had random CPU spikes, though I can't say right now if they have to do with request or not.

Screenshot 2024-12-18 at 14 35 21

@ztanner
Copy link
Member Author

ztanner commented Dec 18, 2024

Has this had a runtime performance impact as well? After updating to Next 15.1.0, we noticed random CPU usage increase on our servers, without much increase in traffic. Running in standalone mode in Docker, using dynamic and catch-all segments in the app router.

@ztanner do you have any idea?

The function in this PR runs at build time, not at request time. It’d be best to open a new issue with additional details about your particular situation!

@icyJoseph
Copy link
Contributor

Let's open a discussion first, people who have runtime memory usage issues, let's collect thoughts and environment information there, to try and create a reproduction set-up.

We can also tip and give suggestions on ways to try to figure out the issue over a discussion. These are notably hard to figure out, but even since pages router days, I've seen that, if they are indeed happening within Next.js, one can figure it out.

@reh2ur
Copy link

reh2ur commented Dec 18, 2024

Some guidance would be appreciated.

We had been affected by the memory leak of next/og with the Next 15 RC in the past, but with Next 15 stable this issue has been resolved.

It's strange now that we run into memory and CPU spike issues again with Next 15.1, coming from 15.0.3 and having no problems at all.

Our service is self-hosted with Docker and a standalone output.

I already tried building the app locally and starting it with NODE_OPTIONS='--inspect' to make a few memory dumps, but there was no increase visible at all.

We might need to automate some traffic to get more load on our local systems. However, it might also be that there is some specific functionality involved (og, sitemap, etc.) which is only really visible on production.

In the "worst" case we would need to redeploy the latest version to production, to get memory dumps from the production pod.

@icyJoseph
Copy link
Contributor

You can use oha to easily send waves of requests, and try to notice increase in memory usage

@reh2ur
Copy link

reh2ur commented Dec 18, 2024

Thanks for the suggestion of oha! I didn't know that tool.

In our company we have two NextJS apps running (in a monorepo), and both experience very strange CPU and Memory behavior after upgrading to 15.1 (and 15.1.1).

I just wanted to post this as well:

Screenshot 2024-12-18 at 18 25 07

Though it is for sure possible that we do something wrong with both apps or in the shared code, it would surprise me and I would be interested why Next 15.1 brings this to the light.

What could be things that leak memory and have such an impact on the CPU as well?

@hdodov
Copy link
Contributor

hdodov commented Dec 19, 2024

@reh2ur I've opened a discussion about this, let's chat there: #74129

As @ztanner already said:

The function in this PR runs at build time, not at request time. It’d be best to open a new issue with additional details about your particular situation!

…so this PR apparently isn't the cause for this.

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

Successfully merging this pull request may close these issues.

JavaScript heap out of memory during next build (Next 15.1)
6 participants