Initial implementation of cache cleanup #22510
Conversation
…mponent, refresh) so we do not know which referent (HostRoot, CacheComponent) created and "owns" the cache. Since any one of these could "own" the cache, retaining in passive effects could double-increment the ref count (double on top of the initial refCount=1). So instead of retaining in passive effects, we use a new cloneCache(cache) operation each time we create a new reference to the same cache. Then, passive effects change to only *releasing* cache references that are no longer reachable (due to an updated cache value at some fiber or deletinga fiber).
josephsavona
changed the title
| @@ -2128,11 +2128,11 @@ function refreshCache<T>(fiber: Fiber, seedKey: ?() => T, seedValue: T) { | |||
| entangleLegacyQueueTransitions(root, provider, lane); | |||
| } | |||
|
|
|||
| const seededCache = new Map(); | |||
| const seededCache = createCache(); | |||
So something we'll need to fix in a follow up is that if there's a refresh, but the cache never commits (simple case: the Cache boundary is deleted before it has a chance to process the update) then cache needs to be released. Which means we need to retain here, too. Perhaps you could write a failing test for this scenario, and leave a comment here explaining why it doesn't retain yet.
I added a todo here and failing test cases (which are currently skipped)
| @@ -1877,8 +1880,12 @@ function commitRootImpl(root, renderPriorityLevel) { | |||
| ) { | |||
| if (!rootDoesHavePassiveEffects) { | |||
| rootDoesHavePassiveEffects = true; | |||
| pendingPassiveEffectsRemainingLanes = remainingLanes; | |||
we need to store this to know which lanes were remaining when flushPassiveEffects() runs asynchronously
|
Okay @acdlite this is ready for another review pass! |
| expect(Scheduler).toHaveYielded([ | ||
| // Since "two" is new, it should be double-invoked. | ||
| 'useLayoutEffect unmount "one"', | ||
| 'useLayoutEffect mount "one"', | ||
| ]); | ||
| expect(Scheduler).toFlushUntilNextPaint([ | ||
| // Cleanup and re-run "one" (and "two") since there is no dependencies array. | ||
| 'useLayoutEffect unmount "one"', | ||
| 'useLayoutEffect mount "one"', | ||
| 'useLayoutEffect mount "two"', | ||
|
|
||
| // Since "two" is new, it should be double-invoked. | ||
| 'useLayoutEffect unmount "two"', | ||
| 'useLayoutEffect mount "two"', | ||
| ]); |
This seems like a sign that something is not quite right...
josephsavona
changed the title
| const textCache = getCacheForType(createTextCache); | ||
| const record = textCache.data.get(text); | ||
| if (record !== undefined) { | ||
| signal.addEventListener('abort', () => { | ||
| Scheduler.unstable_yieldValue( | ||
| `Cache cleanup: ${text} [v${textCache.version}] (cached)`, |
I don't think this log tells us anything interesting, right? In an actual app you would only ever attach a single abort listener. I would remove it, to reduce noise in the tests.
| }); | ||
| // the refreshed cache boundary is unmounted and cleans up | ||
| expect(Scheduler).toHaveYielded([ | ||
| // note there is no non '(cached)' version, since the refresh cache was seeded |
The listener should be attached when you seed the cache, then. That's what you'd do in a real app.
This is a work in progress, posting early to share code.
Summary
This is an initial, partial implementation of a cleanup mechanism for the experimental Cache API. The idea is that consumers of the Cache API can register to be informed when a given Cache instance is no longer needed so that they can perform associated cleanup tasks to free resources stored in the cache. A canonical example would be cancelling pending network requests.
An overview of the high-level changes:
Cachetype from a Map of cache instances to be an object with the original Map of instances, a reference count (to count roughly "active references" to the cache instances - more below), and an AbortController.unstable_getCacheSignal(): AbortSignal, which is callable during render. It returns an AbortSignal tied to the lifetime of the cache - developers can listen for the 'abort' event on the signal, which React now triggers when a given cache instance is no longer referenced.AbortSignalis a web standard that is supported by other platform APIs; for example a signal can be passed tofetch()to trigger cancellation of an HTTP request.Cases handled:
For follow-ups:
How did you test this change?
yarn test --all- a few tests are still failing and i'll need some guidance on how to resolve those (i'll post more details later).The text was updated successfully, but these errors were encountered: