Improve deduping of concurrent 'use cache' invocations#91830
Improve deduping of concurrent 'use cache' invocations#91830unstubbable merged 2 commits intocanaryfrom
'use cache' invocations#91830Conversation
Tests Passed |
48c94c3 to
ab1f2a2
Compare
Stats from current PR🔴 1 regression
📊 All Metrics📖 Metrics GlossaryDev Server Metrics:
Build Metrics:
Change Thresholds:
⚡ Dev Server
📦 Dev Server (Webpack) (Legacy)📦 Dev Server (Webpack)
⚡ Production Builds
📦 Production Builds (Webpack) (Legacy)📦 Production Builds (Webpack)
📦 Bundle SizesBundle Sizes⚡ TurbopackClient Main Bundles
Server Middleware
Build DetailsBuild Manifests
📦 WebpackClient Main Bundles
Polyfills
Pages
Server Edge SSR
Middleware
Build DetailsBuild Manifests
Build Cache
🔄 Shared (bundler-independent)Runtimes
📎 Tarball URL |
577e315 to
b50b94b
Compare
'use cache' invocations'use cache' invocations
80691d4 to
cf63000
Compare
cf63000 to
5ddc140
Compare
With #71286, we implemented deduping of cache entries under certain circumstances. For example, the following constructed example was fixed with the PR: ```js async function getCachedRandom() { 'use cache' return Math.random() } const rand1 = await getCachedRandom() const rand2 = await getCachedRandom() assert(rand1 === rand2) ``` However, this implementation relied on awaiting the two calls sequentially. When rendering components, this can usually not be guaranteed. E.g. the following example was not properly deduped: ```jsx async function Cached() { 'use cache' return <p>{Math.random()}</p> } export default function Page() { return ( <> <Cached /> <Cached /> </> ) } ``` This did render the same value, but only because we triggered two render passes, and the last cached value was used for both elements in the final render pass. But during the first render pass, the `Cached` function was called twice, and the cache entry was also set twice. With #75786, we also fixed the render scenario by wrapping the cached function in `React.cache`. This however did not work for route handlers. E.g. the first example rewritten as follows, and used in a route handler, still wouldn't be deduped: ```js const [rand1, rand2] = await Promise.all([ getCachedRandom(), getCachedRandom(), ]) ``` Furthermore, with this solution, nested cached functions could not be deduped across different outer cache scopes. This is because each cache scope creates its own `React.cache` scope. Example: ```jsx async function Inner() { 'use cache' return <p>{Math.random()}</p> } async function Outer1() { 'use cache' return <Inner /> } async function Outer2() { 'use cache' return <Inner /> } export default function Page() { return ( <> <Outer1 /> <Outer2 /> </> ) } ``` This PR introduces two-layer invocation tracking that deduplicates these cases without changing how cache handlers are implemented. Only the first invocation (the "leader") performs the cache handler lookup and generation. Subsequent invocations ("joiners") tee the leader's result stream instead. The deduplication scope starts after the Resume Data Cache (RDC) lookup and before the cache handler `get`. The RDC phase is excluded because it throws synchronous errors (dynamic usage errors) that need individual stack traces per call site, and RDC lookups are local with no network savings from deduplication. Intra-request deduplication is stored on the `WorkStore` and keyed by `serializedCacheKey` (the coarse key, which is safe because root params are identical within a single request). Cross-request deduplication is stored in a module-scope map keyed by `cacheHandlerKey`, which may include root params on the warm path. Cross-request joiners must await metadata for root param verification before forking the stream. If the key mismatches (different root params), the joiner retries with a recomputed key. Because metadata is checked before `fork()` is called, a mismatched joiner never consumes a stream from the wrong entry. Stream tee-ing is lazy via the `SharedCacheEntry` class: the leader calls `fork()` to get its copy, and each joiner calls `fork()` on demand. If no joiners exist, only one tee occurs. The `SharedCacheResult` discriminated union wraps either a `SharedCacheEntry` (for the cached case) or a hanging promise (for `prerender-dynamic`). Each invocation still decodes the stream independently via `createFromReadableStream` with its own `temporaryReferences`, because sharing the decoded result would cause cache poisoning when components receive different non-serializable props (e.g. `children`). This adds significant complexity to the `cache()` function. Two classes help keep it manageable: `SharedCacheEntry` encapsulates stream ownership and lazy tee-ing so that call sites don't need to reason about which streams have been consumed or need cloning. `ResolvableSharedCacheResult` manages the deferred promise, map registration, and lazy cleanup. Entries stay in the dedup maps until collection completes (so late-arriving invocations can join while the leader streams), then clean up automatically on resolve or reject. A follow-up refactoring to extract the cache handler lookup and generation into a separate function would help break up the function's size.
5ddc140 to
631d825
Compare
gnoff
left a comment
There was a problem hiding this comment.
Leaving some general comments first. mostly for posterity
-
I think we want to move to a world where cache entries can be streamed not just from early joiners but late ones as well. And not just in process but from the cache handler too. The most extreme version would be being able to write to the cache handler and read from it simultaneously. This PR demonstrates one of the biggest challenges with this which is what do you do if the key changes while you are generating the entry. You'd have to error or you'd have to never assume a key more general than the most specific one possible but this then defeats the purpose of caching since taken to it's limit (supporting arbitrary cookies) you are never going to match any entries but your own.
-
More actionable than 1 we should consider reimplementing the internals using node streams. We already don't support Cache Components in edge runtime. We can likely make this layering more efficient by converting to web only at the interface with the cache handler.
| if (currentTime > entry.timestamp + entry.revalidate * 1000) { | ||
| // If this is stale, and we're not in a prerender (i.e. this is | ||
| // dynamic render), then we should warm up the cache with a fresh | ||
| // revalidated entry. | ||
| const result = await generateCacheEntry( | ||
| workStore, |
There was a problem hiding this comment.
I know this was already in the codebase but isn't this wrong? When we are rendering a dynamic render we should be tolerant of stale results. we should kick of a background revalidation. but are we blocking here instead?
In a prerender path we should consider the stale entry a miss and force an eager revalidation b/c we don't want to prerender something stale and then lock it into the outer cache (ISR or nested "use cache") for much longer than we might have otherwise expected
There was a problem hiding this comment.
I know this was already in the codebase but isn't this wrong? When we are rendering a dynamic render we should be tolerant of stale results. we should kick of a background revalidation. but are we blocking here instead?
Yes, that is wrong. There was a community fix submitted recently: #92636
In a prerender path we should consider the stale entry a miss and force an eager revalidation b/c we don't want to prerender something stale and then lock it into the outer cache (ISR or nested "use cache") for much longer than we might have otherwise expected
That's how it already works.
With #71286, we implemented deduping of cache entries under certain circumstances. For example, the following constructed example was fixed with the PR:
However, this implementation relied on awaiting the two calls sequentially. When rendering components, this can usually not be guaranteed. E.g. the following example was not properly deduped:
This did render the same value, but only because we triggered two render passes, and the last cached value was used for both elements in the final render pass. But during the first render pass, the
Cachedfunction was called twice, and the cache entry was also set twice.With #75786, we also fixed the render scenario by wrapping the cached function in
React.cache. This however did not work for route handlers. E.g. the first example rewritten as follows, and used in a route handler, still wouldn't be deduped:Furthermore, with this solution, nested cached functions could not be deduped across different outer cache scopes. This is because each cache scope creates its own
React.cachescope. Example:This PR introduces two-layer invocation tracking that deduplicates these cases without changing how cache handlers are implemented. Only the first invocation (the "leader") performs the cache handler lookup and generation. Subsequent invocations ("joiners") tee the leader's result stream instead.
The deduplication scope starts after the Resume Data Cache (RDC) lookup and before the cache handler
get. The RDC phase is excluded because it throws synchronous errors (dynamic usage errors) that need individual stack traces per call site, and RDC lookups are local with no network savings from deduplication.Intra-request deduplication is stored on the
WorkStoreand keyed byserializedCacheKey(the coarse key, which is safe because root params are identical within a single request). Cross-request deduplication is stored in a module-scope map keyed bycacheHandlerKey, which may include root params on the warm path. Cross-request joiners must await metadata for root param verification before forking the stream. If the key mismatches (different root params), the joiner retries with a recomputed key. Because metadata is checked beforefork()is called, a mismatched joiner never consumes a stream from the wrong entry.Stream tee-ing is lazy via the
SharedCacheEntryclass: the leader callsfork()to get its copy, and each joiner callsfork()on demand. If no joiners exist, only one tee occurs. TheSharedCacheResultdiscriminated union wraps either aSharedCacheEntry(for the cached case) or a hanging promise (forprerender-dynamic).Each invocation still decodes the stream independently via
createFromReadableStreamwith its owntemporaryReferences, because sharing the decoded result would cause cache poisoning when components receive different non-serializable props (e.g.children).Admittedly, this adds significant complexity to the
cache()function. Two classes help keep it manageable:SharedCacheEntryencapsulates stream ownership and lazy tee-ing so that call sites don't need to reason about which streams have been consumed or need cloning.ResolvableSharedCacheResultmanages the deferred promise, map registration, and lazy cleanup. Entries stay in the dedup maps until collection completes (so late-arriving invocations can join while the leader streams), then clean up automatically on resolve or reject.A follow-up refactoring to extract the cache handler lookup and generation into a separate function would help break up the function's size.
closes #78703