-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Remove dependency on Offscreen Fiber updateQueue for React Cache #23229
Conversation
Comparing: 80059bb...c8157b2 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
5a6239a
to
bcfc9d0
Compare
@@ -662,10 +662,13 @@ function updateOffscreenComponent( | |||
nextBaseLanes = mergeLanes(prevBaseLanes, renderLanes); | |||
if (enableCache) { | |||
// Save the cache pool so we can resume later. | |||
const prevCachePool = prevState.cachePool; | |||
if (prevCachePool !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic around when to push and pop is too confusing. I was being too clever when I wrote this code originally.
Let's make it so if there's no spawned cache pool we need to restore, instead of pushing nothing, we push the current value to the stack again. Then in complete/unwind, you can just pop without any conditional checks.
) { | ||
prevState = workInProgress.alternate.memoizedState; | ||
} | ||
if (prevState !== null && prevState.cachePool !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So then here, you would remove this conditional completely. Always pop.
@@ -657,17 +657,18 @@ function updateOffscreenComponent( | |||
// We're hidden, and we're not rendering at Offscreen. We will bail out | |||
// and resume this tree later. | |||
let nextBaseLanes; | |||
const prevCachePool = prevState !== null ? prevState.cachePool : null; | |||
if (prevCachePool !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should be removed, right?
7008630
to
2f1eab9
Compare
@@ -1550,8 +1550,7 @@ function completeWork( | |||
// Run passive effects to retain/release the cache. | |||
workInProgress.flags |= Passive; | |||
} | |||
const spawnedCachePool: SpawnedCachePool | null = (workInProgress.updateQueue: any); | |||
if (spawnedCachePool !== null) { | |||
if (workInProgress.alternate !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can you replace all the workInProgress.alternate
occurrences in this block to current
instead? Slightly more optimal because there are fewer property lookups, and also fewer places we need to update when .alternate
is renamed.
@@ -218,8 +217,7 @@ function unwindInterruptedWork(interruptedWork: Fiber, renderLanes: Lanes) { | |||
case LegacyHiddenComponent: | |||
popRenderLanes(interruptedWork); | |||
if (enableCache) { | |||
const spawnedCachePool: SpawnedCachePool | null = (interruptedWork.updateQueue: any); | |||
if (spawnedCachePool !== null) { | |||
if (interruptedWork.alternate !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a "current" here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's only "interruptedWork" and "renderLanes" so we'd have to access alternate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can pass it in as an argument from the caller in completeUnitOfWork
2f1eab9
to
330b720
Compare
// Return the cache pool to signal that we did in fact push it. We will | ||
// assign this to the field on the fiber so we know to pop the context. | ||
return prevCachePool; | ||
const nextParentCache = isPrimaryRenderer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't need to return anything anymore, right? So this whole else
block could just be:
push(resumedCache, prevCachePool.pool, offscreenWorkInProgress);
); | ||
} | ||
const prevCachePool = prevState !== null ? prevState.cachePool : null; | ||
spawnedCachePool = restoreSpawnedCachePool( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make move the let spawnedCachePool
declaration into this block? We no longer reference it beyond the lifetime of this block.
// using the same cache. Unless the parent changed, since that means | ||
// there was a refresh. | ||
if (current !== null) { | ||
restoreSpawnedCachePool(workInProgress, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's call this pushSpawnedCachePool
to align it more with the other push/pop functions?
a6cbb21
to
32695bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@@ -640,7 +640,6 @@ function updateOffscreenComponent( | |||
// If this is not null, this is a cache pool that was carried over from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment no longer makes sense, let's delete it
// push the cache pool even though we're going to bail out | ||
// because otherwise there'd be a context mismatch | ||
if (current !== null) { | ||
pushSpawnedCachePool(workInProgress, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Let's move this block right above pushRenderLanes
since they are always called together
32695bb
to
0d81d7d
Compare
0d81d7d
to
c8157b2
Compare
Summary: This sync includes the following changes: - **[4de99b3ca](facebook/react@4de99b3ca )**: fix getSnapshot warning when a selector returns NaN ([#23333](facebook/react#23333)) //<OGURA Daiki>// - **[40eaa22d9](facebook/react@40eaa22d9 )**: Remove dependency on Offscreen Fiber updateQueue for React Cache ([#23229](facebook/react#23229)) //<Luna Ruan>// - **[caf6d4707](facebook/react@caf6d4707 )**: Enable enableCache on Test Renderer native ([#23314](facebook/react#23314)) //<David McCabe>// - **[419ccc2b1](facebook/react@419ccc2b1 )**: Land skipUnmountedBoundaries experiment ([#23322](facebook/react#23322)) //<Andrew Clark>// - **[54f785bc5](facebook/react@54f785bc5 )**: Disallow comments as DOM containers for createRoot ([#23321](facebook/react#23321)) //<Andrew Clark>// - **[e9aa9592c](facebook/react@e9aa9592c )**: change ReactBatchConfig.transition //<Luna Ruan>// - **[51c8411d9](facebook/react@51c8411d9 )**: Log a recoverable error whenever hydration fails ([#23319](facebook/react#23319)) //<Andrew Clark>// - **[79ed5e18f](facebook/react@79ed5e18f )**: Delete vestigial RetryAfterError logic ([#23312](facebook/react#23312)) //<Andrew Clark>// - **[80059bb73](facebook/react@80059bb73 )**: Switch to client rendering if root receives update ([#23309](facebook/react#23309)) //<Andrew Clark>// - **[f7f7ed089](facebook/react@f7f7ed089 )**: Allow suspending in the shell during hydration ([#23304](facebook/react#23304)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 27b5699...4de99b3 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D34399162 fbshipit-source-id: 5c49e2bdcf63eb6a601cfa6a4e4b8f2e1f83e2dd
…ebook#23229) We need to use the Offscreen Fiber's update queue for interaction tracing. This PR removes the optimization that React Cache uses to not need to push and pop the cache in special circumstances and defaults to always pushing and popping the cache as long as there was a previous cache.
Summary: This sync includes the following changes: - **[4de99b3ca](facebook/react@4de99b3ca )**: fix getSnapshot warning when a selector returns NaN ([facebook#23333](facebook/react#23333)) //<OGURA Daiki>// - **[40eaa22d9](facebook/react@40eaa22d9 )**: Remove dependency on Offscreen Fiber updateQueue for React Cache ([facebook#23229](facebook/react#23229)) //<Luna Ruan>// - **[caf6d4707](facebook/react@caf6d4707 )**: Enable enableCache on Test Renderer native ([facebook#23314](facebook/react#23314)) //<David McCabe>// - **[419ccc2b1](facebook/react@419ccc2b1 )**: Land skipUnmountedBoundaries experiment ([facebook#23322](facebook/react#23322)) //<Andrew Clark>// - **[54f785bc5](facebook/react@54f785bc5 )**: Disallow comments as DOM containers for createRoot ([facebook#23321](facebook/react#23321)) //<Andrew Clark>// - **[e9aa9592c](facebook/react@e9aa9592c )**: change ReactBatchConfig.transition //<Luna Ruan>// - **[51c8411d9](facebook/react@51c8411d9 )**: Log a recoverable error whenever hydration fails ([facebook#23319](facebook/react#23319)) //<Andrew Clark>// - **[79ed5e18f](facebook/react@79ed5e18f )**: Delete vestigial RetryAfterError logic ([facebook#23312](facebook/react#23312)) //<Andrew Clark>// - **[80059bb73](facebook/react@80059bb73 )**: Switch to client rendering if root receives update ([facebook#23309](facebook/react#23309)) //<Andrew Clark>// - **[f7f7ed089](facebook/react@f7f7ed089 )**: Allow suspending in the shell during hydration ([facebook#23304](facebook/react#23304)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 27b5699...4de99b3 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D34399162 fbshipit-source-id: 5c49e2bdcf63eb6a601cfa6a4e4b8f2e1f83e2dd
We need to use the Offscreen Fiber's update queue for interaction tracing. This PR removes the optimization that React Cache uses to not need to push and pop the cache in special circumstances and defaults to always pushing and popping the cache as long as there was a previous cache.