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

[Segment Cache] Use LRU to manage cache data #73486

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

acdlite
Copy link
Contributor

@acdlite acdlite commented Dec 3, 2024

Based on:


This implements an LRU for data stored in the Segment Cache.

For now I've hardcoded in a cache size of 50mb, but as a follow up, we should look into creating a dynamic limit based on device memory constraints.

Segments and Route entries are managed by separate LRUs. We could combine them into a single LRU, but because they are separate types, we'd need to wrap each one in an extra LRU node (to maintain monomorphism, at the cost of additional memory).

@ijjk
Copy link
Member

ijjk commented Dec 3, 2024

Tests Passed

@ijjk
Copy link
Member

ijjk commented Dec 3, 2024

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary acdlite/next.js segment-cache-lru Change
buildDuration 20.2s 17.5s N/A
buildDurationCached 16.6s 14.6s N/A
nodeModulesSize 409 MB 409 MB ⚠️ +244 kB
nextStartRea..uration (ms) 450ms 455ms N/A
Client Bundles (main, webpack) Overall increase ⚠️
vercel/next.js canary acdlite/next.js segment-cache-lru Change
1187-HASH.js gzip 49.6 kB 50.1 kB ⚠️ +454 B
8276.HASH.js gzip 169 B 168 B N/A
8377-HASH.js gzip 5.3 kB 5.3 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 233 B 235 B N/A
main-HASH.js gzip 33.7 kB 33.7 kB N/A
webpack-HASH.js gzip 1.71 kB 1.71 kB N/A
Overall change 49.6 kB 50.1 kB ⚠️ +454 B
Legacy Client Bundles (polyfills)
vercel/next.js canary acdlite/next.js segment-cache-lru 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 acdlite/next.js segment-cache-lru Change
_app-HASH.js gzip 193 B 193 B
_error-HASH.js gzip 193 B 193 B
amp-HASH.js gzip 513 B 511 B N/A
css-HASH.js gzip 343 B 342 B N/A
dynamic-HASH.js gzip 1.84 kB 1.84 kB N/A
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.44 kB 4.43 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 1.75 kB 1.75 kB
Client Build Manifests
vercel/next.js canary acdlite/next.js segment-cache-lru Change
_buildManifest.js gzip 746 B 747 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary acdlite/next.js segment-cache-lru Change
index.html gzip 523 B 524 B N/A
link.html gzip 537 B 537 B
withRouter.html gzip 519 B 521 B N/A
Overall change 537 B 537 B
Edge SSR bundle Size Overall increase ⚠️
vercel/next.js canary acdlite/next.js segment-cache-lru Change
edge-ssr.js gzip 128 kB 128 kB N/A
page.js gzip 200 kB 202 kB ⚠️ +1.86 kB
Overall change 200 kB 202 kB ⚠️ +1.86 kB
Middleware size
vercel/next.js canary acdlite/next.js segment-cache-lru Change
middleware-b..fest.js gzip 667 B 664 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 31.1 kB 31.1 kB N/A
edge-runtime..pack.js gzip 844 B 844 B
Overall change 844 B 844 B
Next Runtimes Overall increase ⚠️
vercel/next.js canary acdlite/next.js segment-cache-lru Change
797-experime...dev.js gzip 322 B 322 B
797.runtime.dev.js gzip 314 B 314 B
app-page-exp...dev.js gzip 321 kB 322 kB ⚠️ +637 B
app-page-exp..prod.js gzip 126 kB 126 kB ⚠️ +412 B
app-page-tur..prod.js gzip 139 kB 139 kB ⚠️ +411 B
app-page-tur..prod.js gzip 134 kB 135 kB ⚠️ +406 B
app-page.run...dev.js gzip 312 kB 312 kB ⚠️ +627 B
app-page.run..prod.js gzip 122 kB 122 kB ⚠️ +409 B
app-route-ex...dev.js gzip 36.8 kB 36.8 kB
app-route-ex..prod.js gzip 25 kB 25 kB
app-route-tu..prod.js gzip 25 kB 25 kB
app-route-tu..prod.js gzip 24.8 kB 24.8 kB
app-route.ru...dev.js gzip 38.5 kB 38.5 kB
app-route.ru..prod.js gzip 24.8 kB 24.8 kB
pages-api-tu..prod.js gzip 9.56 kB 9.56 kB
pages-api.ru...dev.js gzip 11.4 kB 11.4 kB
pages-api.ru..prod.js gzip 9.56 kB 9.56 kB
pages-turbo...prod.js gzip 21.3 kB 21.3 kB
pages.runtim...dev.js gzip 27 kB 27 kB
pages.runtim..prod.js gzip 21.3 kB 21.3 kB
server.runti..prod.js gzip 916 kB 916 kB
Overall change 2.35 MB 2.35 MB ⚠️ +2.9 kB
build cache Overall increase ⚠️
vercel/next.js canary acdlite/next.js segment-cache-lru Change
0.pack gzip 2.04 MB 2.03 MB N/A
index.pack gzip 145 kB 146 kB ⚠️ +350 B
Overall change 145 kB 146 kB ⚠️ +350 B
Diff details
Diff for 1187-HASH.js

Diff too large to display

Diff for main-HASH.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js

Diff too large to display

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

Diff too large to display

Diff for app-page.runtime.prod.js

Diff too large to display

Commit: 6ae540a

skipDeployment: true,
})
if (isNextDev || skipped) {
test('ppr is disabled', () => {})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test('ppr is disabled', () => {})
test('disabled in development / deployment', () => {})

Comment on lines 287 to 289
getPage() {
return page
}
Copy link
Member

Choose a reason for hiding this comment

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

If you intended to introduce this as part of this changeset I think you'll also need to add it to the BrowserInterface. Though maybe better to delete for now until we use it in a test so we can validate it's plumbed correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I don't need it until a later PR, I'll move it to that one

Comment on lines 153 to 155

// We use an LRU for memory management. We must update this whenever we add or
// remove a new cache entry, or when an entry changes size.
// TODO: I chose the max size somewhat arbitrarily. Consider setting this based
// on navigator.deviceMemory, or some other heuristic. We should make this
// customizable via the Next.js config, too.
Copy link
Member

Choose a reason for hiding this comment

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

I think this detached the comment about storing search params in the segment cache, as it used to live next to the creation of the segmentCacheMap

Copy link
Contributor Author

@acdlite acdlite Dec 6, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

yea, it felt like it was floating when it was above the LRU comment. I think it just needs to be moved to 164

@acdlite acdlite force-pushed the segment-cache-lru branch 2 times, most recently from 3d823b2 to 86e39dc Compare December 6, 2024 16:35
This implements an LRU for data stored in the Segment Cache.

For now I've hardcoded in a cache size of 50mb, but as a follow up, we
should look into creating a dynamic limit based on device
memory constraints.

Segments and Route entries are managed by separate LRUs. We could
combine them into a single LRU, but because they are separate types,
we'd need to wrap each one in an extra LRU node (to maintain
monomorphism, at the cost of additional memory).
@acdlite acdlite requested a review from ztanner December 6, 2024 16:36
@acdlite acdlite merged commit 144fd73 into vercel:canary Dec 6, 2024
86 checks passed
acdlite added a commit that referenced this pull request Dec 6, 2024
Based on

- #73434
- #73486 

---


During a navigation, we should be able to skip the dynamic request if
the prefetched data does not contain any dynamic holes. In the previous
cache implementation, we only tracked this per route; in the Segment
Cache, we must track this per segment.

This updates the SegmentCacheEntry and CacheNodeSeedData types to
include an `isPartial` field. The field is always false during a dynamic
render, or when PPR is disabled.

This PR does not change any behavior; it only adds the new field.
acdlite added a commit that referenced this pull request Dec 6, 2024
Based on:

- #73434
- #73486 
- #73528

---

Similar to #73528, but for the head, which is delivered separately from
the segments. We can only skip the dynamic request if this value is
`false`.
devjiwonchoi pushed a commit that referenced this pull request Dec 9, 2024
Based on:

- #73434
---

This implements an LRU for data stored in the Segment Cache.

For now I've hardcoded in a cache size of 50mb, but as a follow up, we
should look into creating a dynamic limit based on device memory
constraints.

Segments and Route entries are managed by separate LRUs. We could
combine them into a single LRU, but because they are separate types,
we'd need to wrap each one in an extra LRU node (to maintain
monomorphism, at the cost of additional memory).
devjiwonchoi pushed a commit that referenced this pull request Dec 9, 2024
Based on

- #73434
- #73486 

---


During a navigation, we should be able to skip the dynamic request if
the prefetched data does not contain any dynamic holes. In the previous
cache implementation, we only tracked this per route; in the Segment
Cache, we must track this per segment.

This updates the SegmentCacheEntry and CacheNodeSeedData types to
include an `isPartial` field. The field is always false during a dynamic
render, or when PPR is disabled.

This PR does not change any behavior; it only adds the new field.
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.

3 participants