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

Renew prefetch cache entry after update from server #61573

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

ztanner
Copy link
Member

@ztanner ztanner commented Feb 2, 2024

What

When a prefetch cache entry becomes "stale", it'll remain stale until eventually it gets evicted. However during that stale window, the cache is never revalidated, and so instant navigations stop working and data is fetched from origin every navigation.

Why

The lastUsedTime entry on the prefetch cache is currently only updated after the first read. Once it becomes stale, applyFlightData's recursive functions will see that there is no longer a reusable prefetch cache entry, and will trigger a lazy fetch of the new data on every subsequent navigation.

How

This updates prefetch cache handling to always ensure we’re using a fresh or reusable prefetch cache entry. This means that stale prefetch entries will be refreshed on a navigation event. As a result of this, I’ve had to disable one of our tests that relies on this stale cache behavior. It’s not ideal that we’re blocked on the loading boundary when fetching child segment data—ideally we can refactor this to cache the loading component in the CacheNode and copy it over on navigations, similar to how ‘head’ is handled. I’ll work on this in a separate PR.

Note: The new client cache test for this is disabled in PPR for the same reason as the other tests: auto prefetching with PPR navigations is currently loading fresh data rather than reusing the prefetch cache.

Fixes #58969
Fixes #58723
Closes NEXT-1904

@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from 8563e34 to a55d8b3 Compare February 2, 2024 13:41
@ijjk
Copy link
Member

ijjk commented Feb 2, 2024

Tests Passed

@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from a55d8b3 to 058c955 Compare February 2, 2024 13:45
@ijjk
Copy link
Member

ijjk commented Feb 2, 2024

Stats from current PR

Default Build
General
vercel/next.js canary vercel/next.js 02-02-renew_router_cache_after_update_from_server Change
buildDuration 14.1s 14.2s ⚠️ +107ms
buildDurationCached 7.4s 8.3s ⚠️ +841ms
nodeModulesSize 196 MB 196 MB N/A
nextStartRea..uration (ms) 421ms 434ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js 02-02-renew_router_cache_after_update_from_server Change
1068-HASH.js gzip 30.3 kB 30.3 kB N/A
3f784ff6-HASH.js gzip 53.5 kB 53.5 kB N/A
4944-HASH.js gzip 5.04 kB 5.03 kB N/A
8423.HASH.js gzip 181 B 181 B
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 241 B 241 B
main-HASH.js gzip 32.1 kB 32.1 kB N/A
webpack-HASH.js gzip 1.7 kB 1.7 kB
Overall change 47.3 kB 47.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js 02-02-renew_router_cache_after_update_from_server Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js 02-02-renew_router_cache_after_update_from_server Change
_app-HASH.js gzip 196 B 196 B
_error-HASH.js gzip 184 B 183 B N/A
amp-HASH.js gzip 503 B 504 B N/A
css-HASH.js gzip 323 B 324 B N/A
dynamic-HASH.js gzip 2.5 kB 2.51 kB N/A
edge-ssr-HASH.js gzip 258 B 259 B N/A
head-HASH.js gzip 353 B 351 B N/A
hooks-HASH.js gzip 370 B 370 B
image-HASH.js gzip 4.21 kB 4.2 kB N/A
index-HASH.js gzip 259 B 259 B
link-HASH.js gzip 2.68 kB 2.67 kB N/A
routerDirect..HASH.js gzip 313 B 314 B N/A
script-HASH.js gzip 386 B 385 B N/A
withRouter-HASH.js gzip 309 B 311 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 931 B 931 B
Client Build Manifests
vercel/next.js canary vercel/next.js 02-02-renew_router_cache_after_update_from_server Change
_buildManifest.js gzip 485 B 484 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js 02-02-renew_router_cache_after_update_from_server Change
index.html gzip 529 B 528 B N/A
link.html gzip 541 B 540 B N/A
withRouter.html gzip 524 B 522 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js 02-02-renew_router_cache_after_update_from_server Change
edge-ssr.js gzip 2.29 kB 2.29 kB N/A
page.js gzip 2.98 kB 2.98 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js 02-02-renew_router_cache_after_update_from_server Change
middleware-b..fest.js gzip 623 B 626 B N/A
middleware-r..fest.js gzip 151 B 151 B
middleware.js gzip 466 B 465 B N/A
edge-runtime..pack.js gzip 839 B 839 B
Overall change 990 B 990 B
Next Runtimes
vercel/next.js canary vercel/next.js 02-02-renew_router_cache_after_update_from_server Change
app-page-exp...dev.js gzip 166 kB 166 kB
app-page-exp..prod.js gzip 95.9 kB 95.9 kB
app-page-tur..prod.js gzip 97.6 kB 97.6 kB
app-page-tur..prod.js gzip 92 kB 92 kB
app-page.run...dev.js gzip 136 kB 136 kB
app-page.run..prod.js gzip 90.6 kB 90.6 kB
app-route-ex...dev.js gzip 22 kB 22 kB
app-route-ex..prod.js gzip 14.9 kB 14.9 kB
app-route-tu..prod.js gzip 14.9 kB 14.9 kB
app-route-tu..prod.js gzip 14.6 kB 14.6 kB
app-route.ru...dev.js gzip 21.7 kB 21.7 kB
app-route.ru..prod.js gzip 14.6 kB 14.6 kB
pages-api-tu..prod.js gzip 9.47 kB 9.47 kB
pages-api.ru...dev.js gzip 9.74 kB 9.74 kB
pages-api.ru..prod.js gzip 9.47 kB 9.47 kB
pages-turbo...prod.js gzip 22.1 kB 22.1 kB
pages.runtim...dev.js gzip 22.7 kB 22.7 kB
pages.runtim..prod.js gzip 22.1 kB 22.1 kB
server.runti..prod.js gzip 50.1 kB 50.1 kB
Overall change 927 kB 927 kB
build cache
vercel/next.js canary vercel/next.js 02-02-renew_router_cache_after_update_from_server Change
0.pack gzip 1.49 MB 1.49 MB N/A
index.pack gzip 104 kB 104 kB N/A
Overall change 0 B 0 B
Diff details
Diff for 1068-HASH.js

Diff too large to display

Commit: f507d83

@ztanner ztanner marked this pull request as ready for review February 2, 2024 13:56
@ztanner ztanner force-pushed the 02-01-seed_prefetch_cache_with_initial_page branch from 86c1971 to 508f8a5 Compare February 6, 2024 15:14
@ztanner ztanner requested a review from a team as a code owner February 6, 2024 15:14
@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from 058c955 to 433aa22 Compare February 6, 2024 15:14
@ztanner ztanner force-pushed the 02-01-seed_prefetch_cache_with_initial_page branch 4 times, most recently from 70a9616 to eae1430 Compare February 6, 2024 22:24
@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from 433aa22 to d3f1cc5 Compare February 6, 2024 22:24
@ztanner ztanner force-pushed the 02-01-seed_prefetch_cache_with_initial_page branch from eae1430 to bc62046 Compare February 6, 2024 23:05
@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from d3f1cc5 to 1645efa Compare February 6, 2024 23:05
@ztanner ztanner force-pushed the 02-01-seed_prefetch_cache_with_initial_page branch 2 times, most recently from 95339b9 to 8544ddb Compare February 7, 2024 21:12
@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from a61bdd8 to 7187ea6 Compare February 7, 2024 21:13
@ztanner ztanner force-pushed the 02-01-seed_prefetch_cache_with_initial_page branch 2 times, most recently from 17c9a20 to ec7f968 Compare February 7, 2024 21:52
@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from 7187ea6 to 4b2e379 Compare February 7, 2024 21:53
@ztanner ztanner force-pushed the 02-01-seed_prefetch_cache_with_initial_page branch from ec7f968 to 407fa0e Compare February 8, 2024 00:53
@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from 4b2e379 to 4ad233e Compare February 8, 2024 00:53
@AkifumiSato
Copy link
Contributor

If this pull request is merged, I will close #58997.

@ztanner ztanner force-pushed the 02-01-seed_prefetch_cache_with_initial_page branch 2 times, most recently from 654953d to 1f62f69 Compare February 9, 2024 19:43
@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from 4ad233e to 06c0610 Compare February 9, 2024 19:44
@ztanner ztanner force-pushed the 02-01-seed_prefetch_cache_with_initial_page branch from 1f62f69 to 8c37ac6 Compare February 13, 2024 15:16
Base automatically changed from 02-01-seed_prefetch_cache_with_initial_page to canary February 13, 2024 15:42
@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from 06c0610 to ef85241 Compare February 13, 2024 16:28
@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from ef85241 to b57039a Compare February 19, 2024 16:51
@ztanner ztanner force-pushed the 02-02-renew_router_cache_after_update_from_server branch from b57039a to f507d83 Compare February 19, 2024 18:01
Copy link
Contributor

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the #DigDeep

@@ -11,7 +11,7 @@ export function fillLazyItemsTillLeafWithHead(
routerState: FlightRouterState,
cacheNodeSeedData: CacheNodeSeedData | null,
head: React.ReactNode,
wasPrefetched?: boolean
hasReusablePrefetch?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

I think is argument is no longer needed? For the same reason you removed it from the PPR implementation

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's still needed but for a more subtle reason -- we set this flag to true specifically for auto prefetches that don't have a reusable cache status. So that means fresh entries technically don't fall into this categorization.

When attempting to remove it I was noticing that once we renewed the stale prefetch, the lazy fetch was never happening, because fillLazyItemsTillLeafWithHead would always hit this condition since there would always be an existing cache node.

For now I'm going to leave it as-is but I'd like to better understand why fillLazyItemsTillLeafWithHead is triggering any lazy fetch behavior rather than fillCacheWithNewSubtreeData.

@ztanner ztanner merged commit d371f64 into canary Feb 20, 2024
74 checks passed
@ztanner ztanner deleted the 02-02-renew_router_cache_after_update_from_server branch February 20, 2024 17:07
@github-actions github-actions bot added the locked label Mar 6, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent router cache behavior Router Cache not working after first invalidation
4 participants