Remove PassiveSubtreeTag optimization

Instead, always schedule a passive traversal for a subtree containing a deleted node. The overhead of doing this (during the passive phase) is small, and effects are so common that a cleanup in the subtree is likely. This saves some bytes.
facebook · Jul 29, 2020 · cba61f7 · cba61f7
1 parent 7d618ed
commit cba61f7
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 60 deletions.
diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js
@@ -15,16 +15,8 @@ import type {Fiber} from './ReactInternalTypes';
 import type {Lanes} from './ReactFiberLane';
 
 import getComponentName from 'shared/getComponentName';
-import {
-  Deletion,
-  NoEffect,
-  PassiveMask,
-  Placement,
-} from './ReactSideEffectTags';
-import {
-  NoEffect as NoSubtreeTag,
-  Passive as PassiveSubtreeTag,
-} from './ReactSubtreeTags';
+import {Deletion, Placement} from './ReactSideEffectTags';
+import {Passive as PassiveSubtreeTag} from './ReactSubtreeTags';
 import {
   getIteratorFn,
   REACT_ELEMENT_TYPE,
@@ -303,13 +295,11 @@ function ChildReconciler(shouldTrackSideEffects) {
       // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
       returnFiber.effectTag |= Deletion;
 
-      // If we are deleting a subtree that contains a passive effect,
-      // mark the parent so that we're sure to traverse after commit and run any unmount functions.
-      const primaryEffectTag = childToDelete.effectTag & PassiveMask;
-      const primarySubtreeTag = childToDelete.subtreeTag & PassiveSubtreeTag;
-      if (primaryEffectTag !== NoEffect || primarySubtreeTag !== NoSubtreeTag) {
-        returnFiber.subtreeTag |= PassiveSubtreeTag;
-      }
+      // We are deleting a subtree that may contain a passive effect.
+      // Mark the parent so we traverse this path after commit and run any unmount functions.
+      // This may cause us to traverse unnecessarily in some cases, but effects are common,
+      // and the cost of over traversing is small (just the path to the deleted node).
+      returnFiber.subtreeTag |= PassiveSubtreeTag;
     } else {
       deletions.push(childToDelete);
     }

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
@@ -64,13 +64,9 @@ import {
   Ref,
   Deletion,
   ForceUpdateForLegacySuspense,
-  PassiveMask,
   StaticMask,
 } from './ReactSideEffectTags';
-import {
-  NoEffect as NoSubtreeTag,
-  Passive as PassiveSubtreeTag,
-} from './ReactSubtreeTags';
+import {Passive as PassiveSubtreeTag} from './ReactSubtreeTags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import {
   debugRenderPhaseSideEffectsForStrictMode,
@@ -2079,15 +2075,11 @@ function updateSuspensePrimaryChildren(
       // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
       workInProgress.effectTag |= Deletion;
 
-      // If we are deleting a subtree that contains a passive effect,
-      // mark the parent so that we're sure to traverse after commit and run any unmount functions.
-      const primaryEffectTag =
-        currentFallbackChildFragment.effectTag & PassiveMask;
-      const primarySubtreeTag =
-        currentFallbackChildFragment.subtreeTag & PassiveSubtreeTag;
-      if (primaryEffectTag !== NoEffect || primarySubtreeTag !== NoSubtreeTag) {
-        workInProgress.subtreeTag |= PassiveSubtreeTag;
-      }
+      // We are deleting a subtree that may contain a passive effect.
+      // Mark the parent so we traverse this path after commit and run any unmount functions.
+      // This may cause us to traverse unnecessarily in some cases, but effects are common,
+      // and the cost of over traversing is small (just the path to the deleted node).
+      workInProgress.subtreeTag |= PassiveSubtreeTag;
     } else {
       deletions.push(currentFallbackChildFragment);
     }
@@ -3073,13 +3065,11 @@ function remountFiber(
       // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
       returnFiber.effectTag |= Deletion;
 
-      // If we are deleting a subtree that contains a passive effect,
-      // mark the parent so that we're sure to traverse after commit and run any unmount functions.
-      const primaryEffectTag = current.effectTag & PassiveMask;
-      const primarySubtreeTag = current.subtreeTag & PassiveSubtreeTag;
-      if (primaryEffectTag !== NoEffect || primarySubtreeTag !== NoSubtreeTag) {
-        returnFiber.subtreeTag |= PassiveSubtreeTag;
-      }
+      // We are deleting a subtree that may contain a passive effect.
+      // Mark the parent so we traverse this path after commit and run any unmount functions.
+      // This may cause us to traverse unnecessarily in some cases, but effects are common,
+      // and the cost of over traversing is small (just the path to the deleted node).
+      returnFiber.subtreeTag |= PassiveSubtreeTag;
     } else {
       deletions.push(current);
     }

diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js
@@ -24,17 +24,8 @@ import {
   HostRoot,
   SuspenseComponent,
 } from './ReactWorkTags';
-import {
-  Deletion,
-  Hydrating,
-  NoEffect,
-  PassiveMask,
-  Placement,
-} from './ReactSideEffectTags';
-import {
-  NoEffect as NoSubtreeTag,
-  Passive as PassiveSubtreeTag,
-} from './ReactSubtreeTags';
+import {Deletion, Hydrating, Placement} from './ReactSideEffectTags';
+import {Passive as PassiveSubtreeTag} from './ReactSubtreeTags';
 import invariant from 'shared/invariant';
 
 import {
@@ -141,13 +132,11 @@ function deleteHydratableInstance(
     // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
     returnFiber.effectTag |= Deletion;
 
-    // If we are deleting a subtree that contains a passive effect,
-    // mark the parent so that we're sure to traverse after commit and run any unmount functions.
-    const primaryEffectTag = childToDelete.effectTag & PassiveMask;
-    const primarySubtreeTag = childToDelete.subtreeTag & PassiveSubtreeTag;
-    if (primaryEffectTag !== NoEffect || primarySubtreeTag !== NoSubtreeTag) {
-      returnFiber.subtreeTag |= PassiveSubtreeTag;
-    }
+    // We are deleting a subtree that may contain a passive effect.
+    // Mark the parent so we traverse this path after commit and run any unmount functions.
+    // This may cause us to traverse unnecessarily in some cases, but effects are common,
+    // and the cost of over traversing is small (just the path to the deleted node).
+    returnFiber.subtreeTag |= PassiveSubtreeTag;
   } else {
     deletions.push(childToDelete);
   }

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
@@ -1454,7 +1454,7 @@ describe('ReactSuspense', () => {
       ]);
     });
 
-    it('should call onInteractionScheduledWorkCompleted after suspending', done => {
+    it('should call onInteractionScheduledWorkCompleted after suspending', () => {
       const subscriber = {
         onInteractionScheduledWorkCompleted: jest.fn(),
         onInteractionTraced: jest.fn(),
@@ -1512,13 +1512,11 @@ describe('ReactSuspense', () => {
         jest.advanceTimersByTime(1000);
 
         expect(Scheduler).toHaveYielded(['Promise resolved [C]']);
-        expect(Scheduler).toFlushExpired([
+        expect(Scheduler).toFlushAndYield([
           // Even though the promise for C was thrown three times, we should only
           // re-render once.
           'C',
         ]);
-
-        done();
       });
 
       expect(