Skip to content

Commit

Permalink
Track deletions using an array on the parent
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite committed Dec 7, 2020
1 parent 1377e46 commit de75315
Show file tree
Hide file tree
Showing 9 changed files with 203 additions and 39 deletions.
19 changes: 18 additions & 1 deletion packages/react-reconciler/src/ReactChildFiber.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand Down
20 changes: 19 additions & 1 deletion packages/react-reconciler/src/ReactChildFiber.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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) ||
Expand Down
48 changes: 34 additions & 14 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import {
Update,
Ref,
Deletion,
ChildDeletion,
ForceUpdateForLegacySuspense,
} from './ReactFiberFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
48 changes: 34 additions & 14 deletions packages/react-reconciler/src/ReactFiberBeginWork.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import {
Update,
Ref,
Deletion,
ChildDeletion,
ForceUpdateForLegacySuspense,
} from './ReactFiberFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
3 changes: 0 additions & 3 deletions packages/react-reconciler/src/ReactFiberHooks.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -1864,9 +1864,6 @@ const HooksDispatcherOnMount: Dispatcher = {

unstable_isNewReconciler: enableNewReconciler,
};
if (enableCache) {
(HooksDispatcherOnMount: Dispatcher).getCacheForType = getCacheForType;
}

const HooksDispatcherOnUpdate: Dispatcher = {
readContext,
Expand Down
11 changes: 10 additions & 1 deletion packages/react-reconciler/src/ReactFiberHydrationContext.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down
11 changes: 10 additions & 1 deletion packages/react-reconciler/src/ReactFiberHydrationContext.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down
33 changes: 31 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
}
Expand Down
Loading

0 comments on commit de75315

Please sign in to comment.