Remove dependency on Offscreen Fiber updateQueue for React Cache #23229

lunaruan · 2022-02-02T23:40:15Z

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.

sizebot · 2022-02-02T23:44:14Z

Comparing: 80059bb...c8157b2

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	130.78 kB	130.78 kB	=	41.92 kB	41.90 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	135.76 kB	135.65 kB	=	43.40 kB	43.33 kB
facebook-www/ReactDOM-prod.classic.js	=	432.17 kB	431.80 kB	=	79.29 kB	79.18 kB
facebook-www/ReactDOM-prod.modern.js	=	422.09 kB	421.76 kB	=	77.83 kB	77.74 kB
facebook-www/ReactDOMForked-prod.classic.js	=	432.17 kB	431.80 kB	=	79.30 kB	79.19 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against c8157b2

acdlite · 2022-02-02T23:48:29Z

packages/react-reconciler/src/ReactFiberBeginWork.new.js

@@ -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) {


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.

acdlite · 2022-02-02T23:53:29Z

packages/react-reconciler/src/ReactFiberCompleteWork.old.js

+        ) {
+          prevState = workInProgress.alternate.memoizedState;
+        }
+        if (prevState !== null && prevState.cachePool !== null) {


So then here, you would remove this conditional completely. Always pop.

acdlite · 2022-02-03T22:25:27Z

packages/react-reconciler/src/ReactFiberBeginWork.new.js

@@ -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) {


This check should be removed, right?

acdlite · 2022-02-18T02:15:16Z

packages/react-reconciler/src/ReactFiberCompleteWork.new.js

@@ -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) {


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.

acdlite · 2022-02-18T02:15:38Z

packages/react-reconciler/src/ReactFiberUnwindWork.old.js

@@ -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) {


I don't think there is a "current" here

There's only "interruptedWork" and "renderLanes" so we'd have to access alternate

You can pass it in as an argument from the caller in completeUnitOfWork

acdlite · 2022-02-18T03:07:16Z

packages/react-reconciler/src/ReactFiberCacheComponent.new.js

-    // 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


This function doesn't need to return anything anymore, right? So this whole else block could just be:

push(resumedCache, prevCachePool.pool, offscreenWorkInProgress);

acdlite · 2022-02-18T03:12:18Z

packages/react-reconciler/src/ReactFiberBeginWork.old.js

-          );
-        }
+        const prevCachePool = prevState !== null ? prevState.cachePool : null;
+        spawnedCachePool = restoreSpawnedCachePool(


Let's make move the let spawnedCachePool declaration into this block? We no longer reference it beyond the lifetime of this block.

acdlite · 2022-02-18T03:12:27Z

packages/react-reconciler/src/ReactFiberBeginWork.old.js

+        // using the same cache. Unless the parent changed, since that means
+        // there was a refresh.
+        if (current !== null) {
+          restoreSpawnedCachePool(workInProgress, null);


Maybe let's call this pushSpawnedCachePool to align it more with the other push/pop functions?

acdlite

Nice!

acdlite · 2022-02-18T18:32:16Z

packages/react-reconciler/src/ReactFiberBeginWork.old.js

@@ -640,7 +640,6 @@ function updateOffscreenComponent(
  // If this is not null, this is a cache pool that was carried over from the


This comment no longer makes sense, let's delete it

acdlite · 2022-02-18T18:33:21Z

packages/react-reconciler/src/ReactFiberBeginWork.old.js

+        // 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);


Nit: Let's move this block right above pushRenderLanes since they are always called together

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

lunaruan requested a review from acdlite February 2, 2022 23:40

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 2, 2022

lunaruan force-pushed the remove_cache_optimization branch from 5a6239a to bcfc9d0 Compare February 2, 2022 23:45

acdlite reviewed Feb 2, 2022

View reviewed changes

acdlite reviewed Feb 3, 2022

View reviewed changes

remove dependency on updateQueue for React Cache

179e40e

lunaruan force-pushed the remove_cache_optimization branch 2 times, most recently from 7008630 to 2f1eab9 Compare February 17, 2022 23:55

acdlite reviewed Feb 18, 2022

View reviewed changes

lunaruan force-pushed the remove_cache_optimization branch from 2f1eab9 to 330b720 Compare February 18, 2022 02:50

lunaruan requested a review from acdlite February 18, 2022 02:51

acdlite reviewed Feb 18, 2022

View reviewed changes

lunaruan force-pushed the remove_cache_optimization branch 3 times, most recently from a6cbb21 to 32695bb Compare February 18, 2022 03:32

acdlite approved these changes Feb 18, 2022

View reviewed changes

lunaruan force-pushed the remove_cache_optimization branch from 32695bb to 0d81d7d Compare February 18, 2022 22:35

tmp

c8157b2

lunaruan force-pushed the remove_cache_optimization branch from 0d81d7d to c8157b2 Compare February 18, 2022 22:36

lunaruan merged commit 40eaa22 into facebook:main Feb 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependency on Offscreen Fiber updateQueue for React Cache #23229

Remove dependency on Offscreen Fiber updateQueue for React Cache #23229

lunaruan commented Feb 2, 2022

sizebot commented Feb 2, 2022 •

edited

acdlite Feb 2, 2022 •

edited

acdlite Feb 2, 2022

acdlite Feb 3, 2022

acdlite Feb 18, 2022

acdlite Feb 18, 2022

lunaruan Feb 18, 2022

lunaruan Feb 18, 2022

acdlite Feb 18, 2022

acdlite Feb 18, 2022

acdlite Feb 18, 2022

acdlite Feb 18, 2022

acdlite left a comment

acdlite Feb 18, 2022

acdlite Feb 18, 2022

		@@ -640,7 +640,6 @@ function updateOffscreenComponent(
		// If this is not null, this is a cache pool that was carried over from the

Remove dependency on Offscreen Fiber updateQueue for React Cache #23229

Remove dependency on Offscreen Fiber updateQueue for React Cache #23229

Conversation

lunaruan commented Feb 2, 2022

sizebot commented Feb 2, 2022 • edited

Critical size changes

Significant size changes

acdlite Feb 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sizebot commented Feb 2, 2022 •

edited

acdlite Feb 2, 2022 •

edited