From 9b894feb4091bdbf1dc28e75d471dc528ea4da7b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 22 Jul 2020 11:20:38 -0400 Subject: [PATCH] More cleanup of passive effects TODOs * Re-added sibling/return/child fiber detaches. * Clear deletions array properly after mutation or passive effects --- .../src/ReactFiberCommitWork.new.js | 22 +++------- .../src/ReactFiberWorkLoop.new.js | 43 +++++++++++++------ .../src/ReactSideEffectTags.js | 1 + 3 files changed, 38 insertions(+), 28 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 0e7c548d5bc58..b80c2315784fe 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -881,17 +881,10 @@ function commitUnmount( if ((tag & HookPassive) !== NoHookEffect) { effect.tag |= HookHasEffect; - // subtreeTags currently bubble in resetChildLanes which doens't get called for unmounted subtrees. - // So we need to bubble this up manually to the root (or to the nearest ancestor that already has it). + // subtreeTags bubble in resetChildLanes which doens't get called for unmounted subtrees. + // So in the case of unmounts, we need to bubble passive effects explicitly. let ancestor = current.return; while (ancestor !== null) { - if ( - (ancestor.subtreeTag & PassiveSubtreeTag) !== - NoSubtreeTag - ) { - break; - } - ancestor.subtreeTag |= PassiveSubtreeTag; const alternate = ancestor.alternate; if (alternate !== null) { @@ -1035,8 +1028,11 @@ function commitNestedUnmounts( } function detachFiberMutation(fiber: Fiber) { - // Cut off the return pointers to disconnect it from the tree. Ideally, we - // should clear the child pointer of the parent alternate to let this + // Cut off the return pointers to disconnect it from the tree. + // Note that we can't clear child or sibling pointers yet, + // because they may be required for passive effects. + // These pointers will be cleared in a separate pass. + // Ideally, we should clear the child pointer of the parent alternate to let this // get GC:ed but we don't know which for sure which parent is the current // one so we'll settle for GC:ing the subtree of this child. This child // itself will be GC:ed when the parent updates the next time. @@ -1045,8 +1041,6 @@ function detachFiberMutation(fiber: Fiber) { // traversal in a later effect. See PR #16820. We now clear the sibling // field after effects, see: detachFiberAfterEffects. fiber.alternate = null; - // TODO (effects) Don't clear this yet because Passive effects might need it. - //fiber.child = null; fiber.dependencies = null; fiber.firstEffect = null; fiber.lastEffect = null; @@ -1055,8 +1049,6 @@ function detachFiberMutation(fiber: Fiber) { fiber.pendingProps = null; fiber.return = null; fiber.stateNode = null; - // TODO (effects) Don't clear this yet because Passive effects might need it. - //fiber.updateQueue = null; if (__DEV__) { fiber._debugOwner = null; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index deb32516eec35..0f0f621750995 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2406,15 +2406,16 @@ function commitMutationEffects( ) { let fiber = firstChild; while (fiber !== null) { - if (fiber.deletions !== null) { - commitMutationEffectsDeletions( - fiber.deletions, - root, - renderPriorityLevel, - ); + const deletions = fiber.deletions; + if (deletions !== null) { + commitMutationEffectsDeletions(deletions, root, renderPriorityLevel); - // TODO (effects) Detach sibling pointers for deleted Fibers - // TODO (effects) Clear deletion arrays + // If there are no pending passive effects, clear the deletions Array. + const primaryEffectTag = fiber.effectTag & PassiveMask; + const primarySubtreeTag = fiber.subtreeTag & PassiveSubtreeTag; + if (primaryEffectTag === NoEffect && primarySubtreeTag === NoSubtreeTag) { + fiber.deletions = null; + } } if (fiber.child !== null) { @@ -2548,7 +2549,13 @@ function commitMutationEffectsDeletions( captureCommitPhaseError(childToDelete, error); } } - // Don't clear the Deletion effect yet; we also use it to know when we need to detach refs later. + + // If there are no pending passive effects, it's safe to detach remaining pointers now. + const primarySubtreeTag = childToDelete.subtreeTag & PassiveSubtreeTag; + const primaryEffectTag = childToDelete.effectTag & PassiveMask; + if (primarySubtreeTag === NoSubtreeTag && primaryEffectTag === NoEffect) { + detachFiberAfterEffects(childToDelete); + } } } @@ -2777,7 +2784,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { for (let i = 0; i < deletions.length; i++) { const fiberToDelete = deletions[i]; // If this fiber (or anything below it) has passive effects then traverse the subtree. - const primaryEffectTag = fiberToDelete.effectTag & (Passive | Update); + const primaryEffectTag = fiberToDelete.effectTag & PassiveMask; const primarySubtreeTag = fiberToDelete.subtreeTag & PassiveSubtreeTag; if ( primarySubtreeTag !== NoSubtreeTag || @@ -2785,7 +2792,13 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { ) { flushPassiveUnmountEffects(fiberToDelete); } + + // Now that passive effects have been processed, it's safe to detach lingering pointers. + detachFiberAfterEffects(fiberToDelete); } + + // Clear deletions now that passive effects have been procssed. + fiber.deletions = null; } const didBailout = @@ -2809,7 +2822,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { case ForwardRef: case SimpleMemoComponent: case Block: { - const primaryEffectTag = fiber.effectTag & (Passive | Update); + const primaryEffectTag = fiber.effectTag & PassiveMask; if (primaryEffectTag !== NoEffect) { flushPassiveUnmountEffectsImpl(fiber); } @@ -2934,8 +2947,6 @@ function flushPassiveEffectsImpl() { flushPassiveUnmountEffects(root.current); flushPassiveMountEffects(root.current); - // TODO (effects) Detach sibling pointers for deleted Fibers - if (enableProfilerTimer && enableProfilerCommitHooks) { const profilerEffects = pendingPassiveProfilerEffects; pendingPassiveProfilerEffects = []; @@ -4034,3 +4045,9 @@ export function act(callback: () => Thenable): Thenable { }; } } + +function detachFiberAfterEffects(fiber: Fiber): void { + fiber.child = null; + fiber.sibling = null; + fiber.updateQueue = null; +} diff --git a/packages/react-reconciler/src/ReactSideEffectTags.js b/packages/react-reconciler/src/ReactSideEffectTags.js index b01aa52aba6e6..f360938e6d754 100644 --- a/packages/react-reconciler/src/ReactSideEffectTags.js +++ b/packages/react-reconciler/src/ReactSideEffectTags.js @@ -26,6 +26,7 @@ export const Snapshot = /* */ 0b000000100000000; export const Passive = /* */ 0b000001000000000; export const PassiveUnmountPendingDev = /* */ 0b010000000000000; export const Hydrating = /* */ 0b000010000000000; + export const HydratingAndUpdate = /* */ 0b000010000000100; // Passive & Update & Callback & Ref & Snapshot