Track deletions using an array on the parent

Adds back the `deletions` array and uses it in the commit phase. We use a trick where the first time we hit a deletion effect, we commit all the deletion effects that belong to that parent. This is an incremental step away from using the effect list and toward a DFS + subtreeFlags traversal. This will help determine whether the regression is caused by, say, pushing the same fiber into the deletions array multiple times.
facebook · Dec 7, 2020 · de75315 · de75315
1 parent 1377e46
commit de75315
Show file tree

Hide file tree

Showing 9 changed files with 203 additions and 39 deletions.
diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js
@@ -13,7 +13,7 @@ import type {Fiber} from './ReactInternalTypes';
 import type {Lanes} from './ReactFiberLane.new';
 
 import getComponentName from 'shared/getComponentName';
-import {Placement, Deletion} from './ReactFiberFlags';
+import {Deletion, ChildDeletion, Placement} from './ReactFiberFlags';
 import {
   getIteratorFn,
   REACT_ELEMENT_TYPE,
@@ -276,6 +276,23 @@ function ChildReconciler(shouldTrackSideEffects) {
     }
     childToDelete.nextEffect = null;
     childToDelete.flags = Deletion;
+
+    let deletions = returnFiber.deletions;
+    if (deletions === null) {
+      deletions = returnFiber.deletions = [childToDelete];
+      returnFiber.flags |= ChildDeletion;
+    } else {
+      deletions.push(childToDelete);
+    }
+    // Stash a reference to the return fiber's deletion array on each of the
+    // deleted children. This is really weird, but it's a temporary workaround
+    // while we're still using the effect list to traverse effect fibers. A
+    // better workaround would be to follow the `.return` pointer in the commit
+    // phase, but unfortunately we can't assume that `.return` points to the
+    // correct fiber, even in the commit phase, because `findDOMNode` might
+    // mutate it.
+    // TODO: Remove this line.
+    childToDelete.deletions = deletions;
   }
 
   function deleteRemainingChildren(

diff --git a/packages/react-reconciler/src/ReactChildFiber.old.js b/packages/react-reconciler/src/ReactChildFiber.old.js
@@ -13,7 +13,7 @@ import type {Fiber} from './ReactInternalTypes';
 import type {Lanes} from './ReactFiberLane.old';
 
 import getComponentName from 'shared/getComponentName';
-import {Placement, Deletion} from './ReactFiberFlags';
+import {Deletion, ChildDeletion, Placement} from './ReactFiberFlags';
 import {
   getIteratorFn,
   REACT_ELEMENT_TYPE,
@@ -276,6 +276,23 @@ function ChildReconciler(shouldTrackSideEffects) {
     }
     childToDelete.nextEffect = null;
     childToDelete.flags = Deletion;
+
+    let deletions = returnFiber.deletions;
+    if (deletions === null) {
+      deletions = returnFiber.deletions = [childToDelete];
+      returnFiber.flags |= ChildDeletion;
+    } else {
+      deletions.push(childToDelete);
+    }
+    // Stash a reference to the return fiber's deletion array on each of the
+    // deleted children. This is really weird, but it's a temporary workaround
+    // while we're still using the effect list to traverse effect fibers. A
+    // better workaround would be to follow the `.return` pointer in the commit
+    // phase, but unfortunately we can't assume that `.return` points to the
+    // correct fiber, even in the commit phase, because `findDOMNode` might
+    // mutate it.
+    // TODO: Remove this line.
+    childToDelete.deletions = deletions;
   }
 
   function deleteRemainingChildren(
@@ -1125,6 +1142,7 @@ function ChildReconciler(shouldTrackSideEffects) {
         } else {
           if (
             child.elementType === elementType ||
+            // Keep this check inline so it only runs on the false path:
             (__DEV__
               ? isCompatibleFamilyForHotReloading(child, element)
               : false) ||

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
@@ -61,6 +61,7 @@ import {
   Update,
   Ref,
   Deletion,
+  ChildDeletion,
   ForceUpdateForLegacySuspense,
 } from './ReactFiberFlags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
@@ -2007,6 +2008,14 @@ function updateSuspensePrimaryChildren(
     currentFallbackChildFragment.nextEffect = null;
     currentFallbackChildFragment.flags = Deletion;
     workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment;
+    let deletions = workInProgress.deletions;
+    if (deletions === null) {
+      deletions = workInProgress.deletions = [currentFallbackChildFragment];
+      workInProgress.flags |= ChildDeletion;
+    } else {
+      deletions.push(currentFallbackChildFragment);
+    }
+    currentFallbackChildFragment.deletions = deletions;
   }
 
   workInProgress.child = primaryChildFragment;
@@ -2061,21 +2070,23 @@ function updateSuspenseFallbackChildren(
         currentPrimaryChildFragment.treeBaseDuration;
     }
 
-    // The fallback fiber was added as a deletion effect during the first pass.
-    // However, since we're going to remain on the fallback, we no longer want
-    // to delete it. So we need to remove it from the list. Deletions are stored
-    // on the same list as effects. We want to keep the effects from the primary
-    // tree. So we copy the primary child fragment's effect list, which does not
-    // include the fallback deletion effect.
-    const progressedLastEffect = primaryChildFragment.lastEffect;
-    if (progressedLastEffect !== null) {
-      workInProgress.firstEffect = primaryChildFragment.firstEffect;
-      workInProgress.lastEffect = progressedLastEffect;
-      progressedLastEffect.nextEffect = null;
-    } else {
-      // TODO: Reset this somewhere else? Lol legacy mode is so weird.
-      workInProgress.firstEffect = workInProgress.lastEffect = null;
+    if (currentFallbackChildFragment !== null) {
+      // The fallback fiber was added as a deletion effect during the first
+      // pass. However, since we're going to remain on the fallback, we no
+      // longer want to delete it. So we need to remove it from the list.
+      // Deletions are stored on the same list as effects, and are always added
+      // to the front. So we know that the first effect must be the fallback
+      // deletion effect, and everything after that is from the primary free.
+      const firstPrimaryTreeEffect = currentFallbackChildFragment.nextEffect;
+      if (firstPrimaryTreeEffect !== null) {
+        workInProgress.firstEffect = firstPrimaryTreeEffect;
+      } else {
+        // TODO: Reset this somewhere else? Lol legacy mode is so weird.
+        workInProgress.firstEffect = workInProgress.lastEffect = null;
+      }
     }
+
+    workInProgress.deletions = null;
   } else {
     primaryChildFragment = createWorkInProgressOffscreenFiber(
       currentPrimaryChildFragment,
@@ -2982,6 +2993,15 @@ function remountFiber(
     current.nextEffect = null;
     current.flags = Deletion;
 
+    let deletions = returnFiber.deletions;
+    if (deletions === null) {
+      deletions = returnFiber.deletions = [current];
+      returnFiber.flags |= ChildDeletion;
+    } else {
+      deletions.push(current);
+    }
+    current.deletions = deletions;
+
     newWorkInProgress.flags |= Placement;
 
     // Restart work from the new fiber.

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js
@@ -61,6 +61,7 @@ import {
   Update,
   Ref,
   Deletion,
+  ChildDeletion,
   ForceUpdateForLegacySuspense,
 } from './ReactFiberFlags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
@@ -2007,6 +2008,14 @@ function updateSuspensePrimaryChildren(
     currentFallbackChildFragment.nextEffect = null;
     currentFallbackChildFragment.flags = Deletion;
     workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment;
+    let deletions = workInProgress.deletions;
+    if (deletions === null) {
+      deletions = workInProgress.deletions = [currentFallbackChildFragment];
+      workInProgress.flags |= ChildDeletion;
+    } else {
+      deletions.push(currentFallbackChildFragment);
+    }
+    currentFallbackChildFragment.deletions = deletions;
   }
 
   workInProgress.child = primaryChildFragment;
@@ -2061,21 +2070,23 @@ function updateSuspenseFallbackChildren(
         currentPrimaryChildFragment.treeBaseDuration;
     }
 
-    // The fallback fiber was added as a deletion effect during the first pass.
-    // However, since we're going to remain on the fallback, we no longer want
-    // to delete it. So we need to remove it from the list. Deletions are stored
-    // on the same list as effects. We want to keep the effects from the primary
-    // tree. So we copy the primary child fragment's effect list, which does not
-    // include the fallback deletion effect.
-    const progressedLastEffect = primaryChildFragment.lastEffect;
-    if (progressedLastEffect !== null) {
-      workInProgress.firstEffect = primaryChildFragment.firstEffect;
-      workInProgress.lastEffect = progressedLastEffect;
-      progressedLastEffect.nextEffect = null;
-    } else {
-      // TODO: Reset this somewhere else? Lol legacy mode is so weird.
-      workInProgress.firstEffect = workInProgress.lastEffect = null;
+    if (currentFallbackChildFragment !== null) {
+      // The fallback fiber was added as a deletion effect during the first
+      // pass. However, since we're going to remain on the fallback, we no
+      // longer want to delete it. So we need to remove it from the list.
+      // Deletions are stored on the same list as effects, and are always added
+      // to the front. So we know that the first effect must be the fallback
+      // deletion effect, and everything after that is from the primary free.
+      const firstPrimaryTreeEffect = currentFallbackChildFragment.nextEffect;
+      if (firstPrimaryTreeEffect !== null) {
+        workInProgress.firstEffect = firstPrimaryTreeEffect;
+      } else {
+        // TODO: Reset this somewhere else? Lol legacy mode is so weird.
+        workInProgress.firstEffect = workInProgress.lastEffect = null;
+      }
     }
+
+    workInProgress.deletions = null;
   } else {
     primaryChildFragment = createWorkInProgressOffscreenFiber(
       currentPrimaryChildFragment,
@@ -2982,6 +2993,15 @@ function remountFiber(
     current.nextEffect = null;
     current.flags = Deletion;
 
+    let deletions = returnFiber.deletions;
+    if (deletions === null) {
+      deletions = returnFiber.deletions = [current];
+      returnFiber.flags |= ChildDeletion;
+    } else {
+      deletions.push(current);
+    }
+    current.deletions = deletions;
+
     newWorkInProgress.flags |= Placement;
 
     // Restart work from the new fiber.

diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js
@@ -1864,9 +1864,6 @@ const HooksDispatcherOnMount: Dispatcher = {
 
   unstable_isNewReconciler: enableNewReconciler,
 };
-if (enableCache) {
-  (HooksDispatcherOnMount: Dispatcher).getCacheForType = getCacheForType;
-}
 
 const HooksDispatcherOnUpdate: Dispatcher = {
   readContext,

diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js
@@ -24,7 +24,7 @@ import {
   HostRoot,
   SuspenseComponent,
 } from './ReactWorkTags';
-import {Deletion, Placement, Hydrating} from './ReactFiberFlags';
+import {Deletion, ChildDeletion, Placement, Hydrating} from './ReactFiberFlags';
 import invariant from 'shared/invariant';
 
 import {
@@ -137,6 +137,15 @@ function deleteHydratableInstance(
   } else {
     returnFiber.firstEffect = returnFiber.lastEffect = childToDelete;
   }
+
+  let deletions = returnFiber.deletions;
+  if (deletions === null) {
+    deletions = returnFiber.deletions = [childToDelete];
+    returnFiber.flags |= ChildDeletion;
+  } else {
+    deletions.push(childToDelete);
+  }
+  childToDelete.deletions = deletions;
 }
 
 function insertNonHydratedInstance(returnFiber: Fiber, fiber: Fiber) {

diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js
@@ -24,7 +24,7 @@ import {
   HostRoot,
   SuspenseComponent,
 } from './ReactWorkTags';
-import {Deletion, Placement, Hydrating} from './ReactFiberFlags';
+import {Deletion, ChildDeletion, Placement, Hydrating} from './ReactFiberFlags';
 import invariant from 'shared/invariant';
 
 import {
@@ -137,6 +137,15 @@ function deleteHydratableInstance(
   } else {
     returnFiber.firstEffect = returnFiber.lastEffect = childToDelete;
   }
+
+  let deletions = returnFiber.deletions;
+  if (deletions === null) {
+    deletions = returnFiber.deletions = [childToDelete];
+    returnFiber.flags |= ChildDeletion;
+  } else {
+    deletions.push(childToDelete);
+  }
+  childToDelete.deletions = deletions;
 }
 
 function insertNonHydratedInstance(returnFiber: Fiber, fiber: Fiber) {

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -1821,6 +1821,7 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
         // Mark the parent fiber as incomplete and clear its effect list.
         returnFiber.firstEffect = returnFiber.lastEffect = null;
         returnFiber.flags |= Incomplete;
+        returnFiber.deletions = null;
       }
     }
 
@@ -2384,7 +2385,7 @@ function commitMutationEffects(root: FiberRoot, renderPriorityLevel) {
     // bitmap value, we remove the secondary effects from the effect tag and
     // switch on that value.
     const primaryFlags = flags & (Placement | Update | Deletion | Hydrating);
-    switch (primaryFlags) {
+    outer: switch (primaryFlags) {
       case Placement: {
         commitPlacement(nextEffect);
         // Clear the "placement" from effect tag so that we know that this is
@@ -2424,7 +2425,35 @@ function commitMutationEffects(root: FiberRoot, renderPriorityLevel) {
         break;
       }
       case Deletion: {
-        commitDeletion(root, nextEffect, renderPriorityLevel);
+        // Reached a deletion effect. Instead of commit this effect like we
+        // normally do, we're going to use the `deletions` array of the parent.
+        // However, because the effect list is sorted in depth-first order, we
+        // can't wait until we reach the parent node, because the child effects
+        // will have run in the meantime.
+        //
+        // So instead, we use a trick where the first time we hit a deletion
+        // effect, we commit all the deletion effects that belong to that parent.
+        //
+        // This is an incremental step away from using the effect list and
+        // toward a DFS + subtreeFlags traversal.
+        //
+        // A reference to the deletion array of the parent is also stored on
+        // each of the deletions. This is really weird. It would be better to
+        // follow the `.return` pointer, but unfortunately we can't assume that
+        // `.return` points to the correct fiber, even in the commit phase,
+        // because `findDOMNode` might mutate it.
+        const deletedChild = nextEffect;
+        const deletions = deletedChild.deletions;
+        if (deletions !== null) {
+          for (let i = 0; i < deletions.length; i++) {
+            const deletion = deletions[i];
+            // Clear the deletion effect so that we don't delete this node more
+            // than once.
+            deletion.flags &= ~Deletion;
+            deletion.deletions = null;
+            commitDeletion(root, deletion, renderPriorityLevel);
+          }
+        }
         break;
       }
     }