From bfe385ff4c8ff4d22bd6263c79b2316e17604944 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 22 Jul 2020 15:54:20 -0400 Subject: [PATCH 01/11] Effects list refactor continued: passive effects traversal * Adds new Passive subtree tag value. * Bubbles passive flag to ancestors in the case of an unmount. * Adds recursive traversal for passive effects (mounts and unmounts). * Removes pendingPassiveHookEffectsMount and pendingPassiveHookEffectsUnmount arrays from work loop. * Re-adds sibling and child pointer detaching (temporarily removed in previous PR). * Addresses some minor TODO comments left over from previous PRs. --- .../src/ReactFiberCommitWork.new.js | 72 +-- .../src/ReactFiberHydrationContext.new.js | 2 +- .../src/ReactFiberWorkLoop.new.js | 464 +++++++++++------- .../src/ReactSideEffectTags.js | 1 + .../react-reconciler/src/ReactSubtreeTags.js | 9 +- 5 files changed, 348 insertions(+), 200 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index d854ae52aa25..b0dbb7585a8e 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -68,6 +68,8 @@ import { Placement, Snapshot, Update, + Passive, + PassiveUnmountPendingDev, } from './ReactSideEffectTags'; import getComponentName from 'shared/getComponentName'; import invariant from 'shared/invariant'; @@ -115,9 +117,8 @@ import { captureCommitPhaseError, resolveRetryWakeable, markCommitTimeOfFallback, - enqueuePendingPassiveHookEffectMount, - enqueuePendingPassiveHookEffectUnmount, enqueuePendingPassiveProfilerEffect, + schedulePassiveEffectCallback, } from './ReactFiberWorkLoop.new'; import { NoEffect as NoHookEffect, @@ -130,6 +131,10 @@ import { updateDeprecatedEventListeners, unmountDeprecatedResponderListeners, } from './ReactFiberDeprecatedEvents.new'; +import { + NoEffect as NoSubtreeTag, + Passive as PassiveSubtreeTag, +} from './ReactSubtreeTags'; let didWarnAboutUndefinedSnapshotBeforeUpdate: Set | null = null; if (__DEV__) { @@ -381,26 +386,6 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) { } } -function schedulePassiveEffects(finishedWork: Fiber) { - const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); - const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; - if (lastEffect !== null) { - const firstEffect = lastEffect.next; - let effect = firstEffect; - do { - const {next, tag} = effect; - if ( - (tag & HookPassive) !== NoHookEffect && - (tag & HookHasEffect) !== NoHookEffect - ) { - enqueuePendingPassiveHookEffectUnmount(finishedWork, effect); - enqueuePendingPassiveHookEffectMount(finishedWork, effect); - } - effect = next; - } while (effect !== firstEffect); - } -} - export function commitPassiveEffectDurations( finishedRoot: FiberRoot, finishedWork: Fiber, @@ -486,7 +471,9 @@ function commitLifeCycles( commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); } - schedulePassiveEffects(finishedWork); + if ((finishedWork.subtreeTag & PassiveSubtreeTag) !== NoSubtreeTag) { + schedulePassiveEffectCallback(); + } return; } case ClassComponent: { @@ -892,7 +879,35 @@ function commitUnmount( const {destroy, tag} = effect; if (destroy !== undefined) { if ((tag & HookPassive) !== NoHookEffect) { - enqueuePendingPassiveHookEffectUnmount(current, effect); + effect.tag |= HookHasEffect; + + // 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) { + ancestor.subtreeTag |= PassiveSubtreeTag; + const alternate = ancestor.alternate; + if (alternate !== null) { + alternate.subtreeTag |= PassiveSubtreeTag; + } + + ancestor = ancestor.return; + } + + current.effectTag |= Passive; + + if (__DEV__) { + // This flag is used to avoid warning about an update to an unmounted component + // if the component has a passive unmount scheduled. + // Presumably the listener would be cleaned up by that unmount. + current.effectTag |= PassiveUnmountPendingDev; + const alternate = current.alternate; + if (alternate !== null) { + alternate.effectTag |= PassiveUnmountPendingDev; + } + } + + schedulePassiveEffectCallback(); } else { if ( enableProfilerTimer && @@ -1013,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. @@ -1023,7 +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; - fiber.child = null; fiber.dependencies = null; fiber.firstEffect = null; fiber.lastEffect = null; @@ -1032,7 +1049,6 @@ function detachFiberMutation(fiber: Fiber) { fiber.pendingProps = null; fiber.return = null; fiber.stateNode = null; - fiber.updateQueue = null; if (__DEV__) { fiber._debugOwner = null; } diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index 5071f76fb5fe..cc9965e2ea81 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -124,7 +124,7 @@ function deleteHydratableInstance( const childToDelete = createFiberFromHostInstanceForDeletion(); childToDelete.stateNode = instance; childToDelete.return = returnFiber; - childToDelete.effectTag = Deletion; + const deletions = returnFiber.deletions; if (deletions === null) { returnFiber.deletions = [childToDelete]; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 0540d53df553..ca0e0d131695 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -16,6 +16,7 @@ import type {SuspenseConfig} from './ReactFiberSuspenseConfig'; import type {SuspenseState} from './ReactFiberSuspenseComponent.new'; import type {Effect as HookEffect} from './ReactFiberHooks.new'; import type {StackCursor} from './ReactFiberStack.new'; +import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new'; import { warnAboutDeprecatedLifecycles, @@ -49,6 +50,11 @@ import { flushSyncCallbackQueue, scheduleSyncCallback, } from './SchedulerWithReactIntegration.new'; +import { + NoEffect as NoHookEffect, + HasEffect as HookHasEffect, + Passive as HookPassive, +} from './ReactHookEffectTags'; import { logCommitStarted, logCommitStopped, @@ -134,12 +140,14 @@ import { BeforeMutationMask, MutationMask, LayoutMask, + PassiveMask, } from './ReactSideEffectTags'; import { NoEffect as NoSubtreeTag, - BeforeMutation, - Mutation, - Layout, + BeforeMutation as BeforeMutationSubtreeTag, + Mutation as MutationSubtreeTag, + Layout as LayoutSubtreeTag, + Passive as PassiveSubtreeTag, } from './ReactSubtreeTags'; import { NoLanePriority, @@ -327,8 +335,6 @@ let rootDoesHavePassiveEffects: boolean = false; let rootWithPendingPassiveEffects: FiberRoot | null = null; let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoSchedulerPriority; let pendingPassiveEffectsLanes: Lanes = NoLanes; -let pendingPassiveHookEffectsMount: Array = []; -let pendingPassiveHookEffectsUnmount: Array = []; let pendingPassiveProfilerEffects: Array = []; let rootsWithPendingDiscreteUpdates: Set | null = null; @@ -1882,13 +1888,16 @@ function resetChildLanes(completedWork: Fiber) { const effectTag = child.effectTag; if ((effectTag & BeforeMutationMask) !== NoEffect) { - subtreeTag |= BeforeMutation; + subtreeTag |= BeforeMutationSubtreeTag; } if ((effectTag & MutationMask) !== NoEffect) { - subtreeTag |= Mutation; + subtreeTag |= MutationSubtreeTag; } if ((effectTag & LayoutMask) !== NoEffect) { - subtreeTag |= Layout; + subtreeTag |= LayoutSubtreeTag; + } + if ((effectTag & PassiveMask) !== NoEffect) { + subtreeTag |= PassiveSubtreeTag; } // When a fiber is cloned, its actualDuration is reset to 0. This value will @@ -1929,13 +1938,16 @@ function resetChildLanes(completedWork: Fiber) { const effectTag = child.effectTag; if ((effectTag & BeforeMutationMask) !== NoEffect) { - subtreeTag |= BeforeMutation; + subtreeTag |= BeforeMutationSubtreeTag; } if ((effectTag & MutationMask) !== NoEffect) { - subtreeTag |= Mutation; + subtreeTag |= MutationSubtreeTag; } if ((effectTag & LayoutMask) !== NoEffect) { - subtreeTag |= Layout; + subtreeTag |= LayoutSubtreeTag; + } + if ((effectTag & PassiveMask) !== NoEffect) { + subtreeTag |= PassiveSubtreeTag; } child = child.sibling; @@ -2170,6 +2182,17 @@ function commitRootImpl(root, renderPriorityLevel) { markLayoutEffectsStopped(); } + // If there are pending passive effects, schedule a callback to process them. + if ((root.current.subtreeTag & PassiveSubtreeTag) !== NoSubtreeTag) { + if (!rootDoesHavePassiveEffects) { + rootDoesHavePassiveEffects = true; + scheduleCallback(NormalSchedulerPriority, () => { + flushPassiveEffects(); + return null; + }); + } + } + // Tell Scheduler to yield at the end of the frame, so the browser has an // opportunity to paint. requestPaint(); @@ -2201,8 +2224,6 @@ function commitRootImpl(root, renderPriorityLevel) { rootWithPendingPassiveEffects = root; pendingPassiveEffectsLanes = lanes; pendingPassiveEffectsRenderPriority = renderPriorityLevel; - } else { - // TODO (effects) Detach sibling pointers for deleted Fibers } // Read this again, since an effect might have updated it @@ -2312,7 +2333,7 @@ function commitBeforeMutationEffects(firstChild: Fiber) { } if (fiber.child !== null) { - const primarySubtreeTag = fiber.subtreeTag & BeforeMutation; + const primarySubtreeTag = fiber.subtreeTag & BeforeMutationSubtreeTag; if (primarySubtreeTag !== NoSubtreeTag) { commitBeforeMutationEffects(fiber.child); } @@ -2396,19 +2417,20 @@ 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) Don't clear this yet; we may need to cleanup passive effects - fiber.deletions = null; + // 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) { - const primarySubtreeTag = fiber.subtreeTag & Mutation; + const primarySubtreeTag = fiber.subtreeTag & MutationSubtreeTag; if (primarySubtreeTag !== NoSubtreeTag) { commitMutationEffects(fiber.child, root, renderPriorityLevel); } @@ -2538,7 +2560,23 @@ 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); + } + } +} + +export function schedulePassiveEffectCallback() { + if (!rootDoesHavePassiveEffects) { + rootDoesHavePassiveEffects = true; + scheduleCallback(NormalSchedulerPriority, () => { + flushPassiveEffects(); + return null; + }); } } @@ -2550,7 +2588,7 @@ function commitLayoutEffects( let fiber = firstChild; while (fiber !== null) { if (fiber.child !== null) { - const primarySubtreeTag = fiber.subtreeTag & Layout; + const primarySubtreeTag = fiber.subtreeTag & LayoutSubtreeTag; if (primarySubtreeTag !== NoSubtreeTag) { commitLayoutEffects(fiber.child, root, committedLanes); } @@ -2645,44 +2683,239 @@ export function enqueuePendingPassiveProfilerEffect(fiber: Fiber): void { } } -export function enqueuePendingPassiveHookEffectMount( - fiber: Fiber, - effect: HookEffect, -): void { - pendingPassiveHookEffectsMount.push(effect, fiber); - if (!rootDoesHavePassiveEffects) { - rootDoesHavePassiveEffects = true; - scheduleCallback(NormalSchedulerPriority, () => { - flushPassiveEffects(); - return null; - }); - } +function invokePassiveEffectCreate(effect: HookEffect): void { + const create = effect.create; + effect.destroy = create(); } -export function enqueuePendingPassiveHookEffectUnmount( - fiber: Fiber, - effect: HookEffect, -): void { - pendingPassiveHookEffectsUnmount.push(effect, fiber); - if (__DEV__) { - fiber.effectTag |= PassiveUnmountPendingDev; - const alternate = fiber.alternate; - if (alternate !== null) { - alternate.effectTag |= PassiveUnmountPendingDev; +function flushPassiveMountEffects(firstChild: Fiber): void { + let fiber = firstChild; + while (fiber !== null) { + const didBailout = + fiber.alternate !== null && fiber.alternate.child === fiber.child; + const primarySubtreeTag = fiber.subtreeTag & PassiveSubtreeTag; + + if ( + fiber.child !== null && + !didBailout && + primarySubtreeTag !== NoSubtreeTag + ) { + flushPassiveMountEffects(fiber.child); } + + if ((fiber.effectTag & Update) !== NoEffect) { + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: + case Block: { + flushPassiveMountEffectsImpl(fiber); + } + } + } + + fiber = fiber.sibling; } - if (!rootDoesHavePassiveEffects) { - rootDoesHavePassiveEffects = true; - scheduleCallback(NormalSchedulerPriority, () => { - flushPassiveEffects(); - return null; - }); +} + +function flushPassiveMountEffectsImpl(fiber: Fiber): void { + const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); + const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + let effect = firstEffect; + do { + const {next, tag} = effect; + + if ( + (tag & HookPassive) !== NoHookEffect && + (tag & HookHasEffect) !== NoHookEffect + ) { + if (__DEV__) { + setCurrentDebugFiberInDEV(fiber); + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + fiber.mode & ProfileMode + ) { + startPassiveEffectTimer(); + invokeGuardedCallback( + null, + invokePassiveEffectCreate, + null, + effect, + ); + recordPassiveEffectDuration(fiber); + } else { + invokeGuardedCallback( + null, + invokePassiveEffectCreate, + null, + effect, + ); + } + if (hasCaughtError()) { + invariant(fiber !== null, 'Should be working on an effect.'); + const error = clearCaughtError(); + captureCommitPhaseError(fiber, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + const create = effect.create; + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + fiber.mode & ProfileMode + ) { + try { + startPassiveEffectTimer(); + effect.destroy = create(); + } finally { + recordPassiveEffectDuration(fiber); + } + } else { + effect.destroy = create(); + } + } catch (error) { + invariant(fiber !== null, 'Should be working on an effect.'); + captureCommitPhaseError(fiber, error); + } + } + } + + effect = next; + } while (effect !== firstEffect); } } -function invokePassiveEffectCreate(effect: HookEffect): void { - const create = effect.create; - effect.destroy = create(); +function flushPassiveUnmountEffects(firstChild: Fiber): void { + let fiber = firstChild; + while (fiber !== null) { + const deletions = fiber.deletions; + if (deletions !== null) { + 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 & PassiveMask; + const primarySubtreeTag = fiberToDelete.subtreeTag & PassiveSubtreeTag; + if ( + primarySubtreeTag !== NoSubtreeTag || + primaryEffectTag !== NoEffect + ) { + 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 = + fiber.alternate !== null && fiber.alternate.child === fiber.child; + if (!didBailout) { + const child = fiber.child; + if (child !== null) { + // If any children have passive effects then traverse the subtree. + // Note that this requires checking subtreeTag of the current Fiber, + // rather than the subtreeTag/effectsTag of the first child, + // since that would not cover passive effects in siblings. + const primarySubtreeTag = fiber.subtreeTag & PassiveSubtreeTag; + if (primarySubtreeTag !== NoSubtreeTag) { + flushPassiveUnmountEffects(child); + } + } + } + + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: + case Block: { + const primaryEffectTag = fiber.effectTag & PassiveMask; + if (primaryEffectTag !== NoEffect) { + flushPassiveUnmountEffectsImpl(fiber); + } + } + } + + fiber = fiber.sibling; + } +} + +function flushPassiveUnmountEffectsImpl(fiber: Fiber): void { + const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); + const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + let effect = firstEffect; + do { + const {next, tag} = effect; + if ( + (tag & HookPassive) !== NoHookEffect && + (tag & HookHasEffect) !== NoHookEffect + ) { + const destroy = effect.destroy; + effect.destroy = undefined; + + if (__DEV__) { + fiber.effectTag &= ~PassiveUnmountPendingDev; + const alternate = fiber.alternate; + if (alternate !== null) { + alternate.effectTag &= ~PassiveUnmountPendingDev; + } + } + + if (typeof destroy === 'function') { + if (__DEV__) { + setCurrentDebugFiberInDEV(fiber); + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + fiber.mode & ProfileMode + ) { + startPassiveEffectTimer(); + invokeGuardedCallback(null, destroy, null); + recordPassiveEffectDuration(fiber); + } else { + invokeGuardedCallback(null, destroy, null); + } + if (hasCaughtError()) { + invariant(fiber !== null, 'Should be working on an effect.'); + const error = clearCaughtError(); + captureCommitPhaseError(fiber, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + fiber.mode & ProfileMode + ) { + try { + startPassiveEffectTimer(); + destroy(); + } finally { + recordPassiveEffectDuration(fiber); + } + } else { + destroy(); + } + } catch (error) { + invariant(fiber !== null, 'Should be working on an effect.'); + captureCommitPhaseError(fiber, error); + } + } + } + } + + effect = next; + } while (effect !== firstEffect); + } } function flushPassiveEffectsImpl() { @@ -2724,117 +2957,8 @@ function flushPassiveEffectsImpl() { // e.g. a destroy function in one component may unintentionally override a ref // value set by a create function in another component. // Layout effects have the same constraint. - - // First pass: Destroy stale passive effects. - const unmountEffects = pendingPassiveHookEffectsUnmount; - pendingPassiveHookEffectsUnmount = []; - for (let i = 0; i < unmountEffects.length; i += 2) { - const effect = ((unmountEffects[i]: any): HookEffect); - const fiber = ((unmountEffects[i + 1]: any): Fiber); - const destroy = effect.destroy; - effect.destroy = undefined; - - if (__DEV__) { - fiber.effectTag &= ~PassiveUnmountPendingDev; - const alternate = fiber.alternate; - if (alternate !== null) { - alternate.effectTag &= ~PassiveUnmountPendingDev; - } - } - - if (typeof destroy === 'function') { - if (__DEV__) { - setCurrentDebugFiberInDEV(fiber); - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - fiber.mode & ProfileMode - ) { - startPassiveEffectTimer(); - invokeGuardedCallback(null, destroy, null); - recordPassiveEffectDuration(fiber); - } else { - invokeGuardedCallback(null, destroy, null); - } - if (hasCaughtError()) { - invariant(fiber !== null, 'Should be working on an effect.'); - const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); - } - resetCurrentDebugFiberInDEV(); - } else { - try { - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - fiber.mode & ProfileMode - ) { - try { - startPassiveEffectTimer(); - destroy(); - } finally { - recordPassiveEffectDuration(fiber); - } - } else { - destroy(); - } - } catch (error) { - invariant(fiber !== null, 'Should be working on an effect.'); - captureCommitPhaseError(fiber, error); - } - } - } - } - // Second pass: Create new passive effects. - const mountEffects = pendingPassiveHookEffectsMount; - pendingPassiveHookEffectsMount = []; - for (let i = 0; i < mountEffects.length; i += 2) { - const effect = ((mountEffects[i]: any): HookEffect); - const fiber = ((mountEffects[i + 1]: any): Fiber); - if (__DEV__) { - setCurrentDebugFiberInDEV(fiber); - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - fiber.mode & ProfileMode - ) { - startPassiveEffectTimer(); - invokeGuardedCallback(null, invokePassiveEffectCreate, null, effect); - recordPassiveEffectDuration(fiber); - } else { - invokeGuardedCallback(null, invokePassiveEffectCreate, null, effect); - } - if (hasCaughtError()) { - invariant(fiber !== null, 'Should be working on an effect.'); - const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); - } - resetCurrentDebugFiberInDEV(); - } else { - try { - const create = effect.create; - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - fiber.mode & ProfileMode - ) { - try { - startPassiveEffectTimer(); - effect.destroy = create(); - } finally { - recordPassiveEffectDuration(fiber); - } - } else { - effect.destroy = create(); - } - } catch (error) { - invariant(fiber !== null, 'Should be working on an effect.'); - captureCommitPhaseError(fiber, error); - } - } - } - - // TODO (effects) Detach sibling pointers for deleted Fibers + flushPassiveUnmountEffects(root.current); + flushPassiveMountEffects(root.current); if (enableProfilerTimer && enableProfilerCommitHooks) { const profilerEffects = pendingPassiveProfilerEffects; @@ -3934,3 +4058,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 8b63f2f22f8c..b01aa52aba6e 100644 --- a/packages/react-reconciler/src/ReactSideEffectTags.js +++ b/packages/react-reconciler/src/ReactSideEffectTags.js @@ -43,3 +43,4 @@ export const ForceUpdateForLegacySuspense = /* */ 0b100000000000000; export const BeforeMutationMask = /* */ 0b000001100001010; export const MutationMask = /* */ 0b000010010011110; export const LayoutMask = /* */ 0b000000010100100; +export const PassiveMask = /* */ 0b000001000000000; diff --git a/packages/react-reconciler/src/ReactSubtreeTags.js b/packages/react-reconciler/src/ReactSubtreeTags.js index 7cc1d7acd090..536d9e074b44 100644 --- a/packages/react-reconciler/src/ReactSubtreeTags.js +++ b/packages/react-reconciler/src/ReactSubtreeTags.js @@ -9,7 +9,8 @@ export type SubtreeTag = number; -export const NoEffect = /* */ 0b000; -export const BeforeMutation = /* */ 0b001; -export const Mutation = /* */ 0b010; -export const Layout = /* */ 0b100; +export const NoEffect = /* */ 0b0000; +export const BeforeMutation = /* */ 0b0001; +export const Mutation = /* */ 0b0010; +export const Layout = /* */ 0b0100; +export const Passive = /* */ 0b1000; From fe2e311db5db1ae3ed2b1263ac577438e45bb204 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 27 Jul 2020 13:13:27 -0400 Subject: [PATCH 02/11] Add persistent, Static subtreeTag This allows certain concepts to persist without recalculting them, e.g. whether a subtree contains passive effects or portals. This helps for cases like nested unmounts that contain deep passive effects. --- packages/react-reconciler/src/ReactFiber.new.js | 7 +++++-- .../src/ReactFiberCommitWork.new.js | 13 ------------- packages/react-reconciler/src/ReactSubtreeTags.js | 5 +++++ 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index 53133777866c..be4e1ea7011a 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -30,7 +30,10 @@ import { enableBlocksAPI, } from 'shared/ReactFeatureFlags'; import {NoEffect, Placement} from './ReactSideEffectTags'; -import {NoEffect as NoSubtreeEffect} from './ReactSubtreeTags'; +import { + NoEffect as NoSubtreeEffect, + Static as StaticSubtreeEffects, +} from './ReactSubtreeTags'; import {ConcurrentRoot, BlockingRoot} from './ReactRootTags'; import { IndeterminateComponent, @@ -290,7 +293,6 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { // We already have an alternate. // Reset the effect tag. workInProgress.effectTag = NoEffect; - workInProgress.subtreeTag = NoSubtreeEffect; workInProgress.deletions = null; // The effect list is no longer valid. @@ -308,6 +310,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { } } + workInProgress.subtreeTag = current.subtreeTag & StaticSubtreeEffects; workInProgress.childLanes = current.childLanes; workInProgress.lanes = current.lanes; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index b0dbb7585a8e..00dc62682451 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -881,19 +881,6 @@ function commitUnmount( if ((tag & HookPassive) !== NoHookEffect) { effect.tag |= HookHasEffect; - // 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) { - ancestor.subtreeTag |= PassiveSubtreeTag; - const alternate = ancestor.alternate; - if (alternate !== null) { - alternate.subtreeTag |= PassiveSubtreeTag; - } - - ancestor = ancestor.return; - } - current.effectTag |= Passive; if (__DEV__) { diff --git a/packages/react-reconciler/src/ReactSubtreeTags.js b/packages/react-reconciler/src/ReactSubtreeTags.js index 536d9e074b44..a1edfd50ec45 100644 --- a/packages/react-reconciler/src/ReactSubtreeTags.js +++ b/packages/react-reconciler/src/ReactSubtreeTags.js @@ -14,3 +14,8 @@ export const BeforeMutation = /* */ 0b0001; export const Mutation = /* */ 0b0010; export const Layout = /* */ 0b0100; export const Passive = /* */ 0b1000; + +// Union of tags that don't get reset on clones. +// This allows certain concepts to persist without recalculting them, +// e.g. whether a subtree contains passive effects or portals. +export const Static = /* */ 0b1000; From 2876d61f583e3619db0cfd99a4628049b7bd88cf Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 28 Jul 2020 16:53:09 -0400 Subject: [PATCH 03/11] Add static effectTag bit for passive effect cleanup after unmount --- .../src/ReactChildFiber.new.js | 19 +++++- .../react-reconciler/src/ReactFiber.new.js | 14 ++--- .../src/ReactFiberBeginWork.new.js | 27 ++++++++- .../src/ReactFiberHooks.new.js | 8 ++- .../src/ReactFiberHydrationContext.new.js | 20 ++++++- .../src/ReactFiberWorkLoop.new.js | 2 +- .../src/ReactSideEffectTags.js | 60 +++++++++++-------- .../react-reconciler/src/ReactSubtreeTags.js | 5 -- .../SchedulingProfiler-test.internal.js | 4 ++ 9 files changed, 115 insertions(+), 44 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js index c090768ca1c2..c0c131bb928b 100644 --- a/packages/react-reconciler/src/ReactChildFiber.new.js +++ b/packages/react-reconciler/src/ReactChildFiber.new.js @@ -15,7 +15,16 @@ import type {Fiber} from './ReactInternalTypes'; import type {Lanes} from './ReactFiberLane'; import getComponentName from 'shared/getComponentName'; -import {Placement, Deletion} from './ReactSideEffectTags'; +import { + Deletion, + NoEffect, + PassiveMask, + Placement, +} from './ReactSideEffectTags'; +import { + NoEffect as NoSubtreeTag, + Passive as PassiveSubtreeTag, +} from './ReactSubtreeTags'; import { getIteratorFn, REACT_ELEMENT_TYPE, @@ -293,6 +302,14 @@ function ChildReconciler(shouldTrackSideEffects) { returnFiber.deletions = [childToDelete]; // 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; + } } else { deletions.push(childToDelete); } diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index be4e1ea7011a..60e9379e8ce5 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -29,11 +29,8 @@ import { enableScopeAPI, enableBlocksAPI, } from 'shared/ReactFeatureFlags'; -import {NoEffect, Placement} from './ReactSideEffectTags'; -import { - NoEffect as NoSubtreeEffect, - Static as StaticSubtreeEffects, -} from './ReactSubtreeTags'; +import {NoEffect, Placement, StaticMask} from './ReactSideEffectTags'; +import {NoEffect as NoSubtreeEffect} from './ReactSubtreeTags'; import {ConcurrentRoot, BlockingRoot} from './ReactRootTags'; import { IndeterminateComponent, @@ -291,8 +288,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.type = current.type; // We already have an alternate. - // Reset the effect tag. - workInProgress.effectTag = NoEffect; + workInProgress.subtreeTag = NoSubtreeEffect; workInProgress.deletions = null; // The effect list is no longer valid. @@ -310,7 +306,9 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { } } - workInProgress.subtreeTag = current.subtreeTag & StaticSubtreeEffects; + // Reset all effects except static ones. + // Static effects are not specific to a render. + workInProgress.effectTag = current.effectTag & StaticMask; workInProgress.childLanes = current.childLanes; workInProgress.lanes = current.lanes; diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index 4c068edc0fa7..244472069a80 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -64,7 +64,13 @@ import { Ref, Deletion, ForceUpdateForLegacySuspense, + PassiveMask, + StaticMask, } from './ReactSideEffectTags'; +import { + NoEffect as NoSubtreeTag, + Passive as PassiveSubtreeTag, +} from './ReactSubtreeTags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import { debugRenderPhaseSideEffectsForStrictMode, @@ -2064,13 +2070,24 @@ function updateSuspensePrimaryChildren( if (currentFallbackChildFragment !== null) { // Delete the fallback child fragment currentFallbackChildFragment.nextEffect = null; - currentFallbackChildFragment.effectTag = Deletion; + currentFallbackChildFragment.effectTag = + (currentFallbackChildFragment.effectTag & StaticMask) | Deletion; workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment; const deletions = workInProgress.deletions; if (deletions === null) { workInProgress.deletions = [currentFallbackChildFragment]; // 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; + } } else { deletions.push(currentFallbackChildFragment); } @@ -3055,6 +3072,14 @@ function remountFiber( returnFiber.deletions = [current]; // 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; + } } else { deletions.push(current); } diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index c5f766759e0d..544abf636b97 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -50,6 +50,7 @@ import {createDeprecatedResponderListener} from './ReactFiberDeprecatedEvents.ne import { Update as UpdateEffect, Passive as PassiveEffect, + PassiveStatic as PassiveStaticEffect, } from './ReactSideEffectTags'; import { HasEffect as HookHasEffect, @@ -1270,7 +1271,7 @@ function mountEffect( } } return mountEffectImpl( - UpdateEffect | PassiveEffect, + UpdateEffect | PassiveEffect | PassiveStaticEffect, HookPassive, create, deps, @@ -1288,7 +1289,7 @@ function updateEffect( } } return updateEffectImpl( - UpdateEffect | PassiveEffect, + UpdateEffect | PassiveEffect | PassiveStaticEffect, HookPassive, create, deps, @@ -1631,7 +1632,8 @@ function mountOpaqueIdentifier(): OpaqueIDType | void { const setId = mountState(id)[1]; if ((currentlyRenderingFiber.mode & BlockingMode) === NoMode) { - currentlyRenderingFiber.effectTag |= UpdateEffect | PassiveEffect; + currentlyRenderingFiber.effectTag |= + UpdateEffect | PassiveEffect | PassiveStaticEffect; pushEffect( HookHasEffect | HookPassive, () => { diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index cc9965e2ea81..81201f82257e 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -24,7 +24,17 @@ import { HostRoot, SuspenseComponent, } from './ReactWorkTags'; -import {Deletion, Hydrating, Placement} from './ReactSideEffectTags'; +import { + Deletion, + Hydrating, + NoEffect, + PassiveMask, + Placement, +} from './ReactSideEffectTags'; +import { + NoEffect as NoSubtreeTag, + Passive as PassiveSubtreeTag, +} from './ReactSubtreeTags'; import invariant from 'shared/invariant'; import { @@ -130,6 +140,14 @@ function deleteHydratableInstance( returnFiber.deletions = [childToDelete]; // 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; + } } else { deletions.push(childToDelete); } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index ca0e0d131695..04f57c4ac992 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2835,7 +2835,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { case ForwardRef: case SimpleMemoComponent: case Block: { - const primaryEffectTag = fiber.effectTag & PassiveMask; + const primaryEffectTag = fiber.effectTag & Passive; if (primaryEffectTag !== NoEffect) { flushPassiveUnmountEffectsImpl(fiber); } diff --git a/packages/react-reconciler/src/ReactSideEffectTags.js b/packages/react-reconciler/src/ReactSideEffectTags.js index b01aa52aba6e..2c2aa129d41a 100644 --- a/packages/react-reconciler/src/ReactSideEffectTags.js +++ b/packages/react-reconciler/src/ReactSideEffectTags.js @@ -10,37 +10,49 @@ export type SideEffectTag = number; // Don't change these two values. They're used by React Dev Tools. -export const NoEffect = /* */ 0b000000000000000; -export const PerformedWork = /* */ 0b000000000000001; +export const NoEffect = /* */ 0b0000000000000000; +export const PerformedWork = /* */ 0b0000000000000001; // You can change the rest (and add more). -export const Placement = /* */ 0b000000000000010; -export const Update = /* */ 0b000000000000100; -export const PlacementAndUpdate = /* */ 0b000000000000110; -export const Deletion = /* */ 0b000000000001000; -export const ContentReset = /* */ 0b000000000010000; -export const Callback = /* */ 0b000000000100000; -export const DidCapture = /* */ 0b000000001000000; -export const Ref = /* */ 0b000000010000000; -export const Snapshot = /* */ 0b000000100000000; -export const Passive = /* */ 0b000001000000000; -export const PassiveUnmountPendingDev = /* */ 0b010000000000000; -export const Hydrating = /* */ 0b000010000000000; -export const HydratingAndUpdate = /* */ 0b000010000000100; +export const Placement = /* */ 0b0000000000000010; +export const Update = /* */ 0b0000000000000100; +export const PlacementAndUpdate = /* */ 0b0000000000000110; +export const Deletion = /* */ 0b0000000000001000; +export const ContentReset = /* */ 0b0000000000010000; +export const Callback = /* */ 0b0000000000100000; +export const DidCapture = /* */ 0b0000000001000000; +export const Ref = /* */ 0b0000000010000000; +export const Snapshot = /* */ 0b0000000100000000; +export const Passive = /* */ 0b0000001000000000; +export const PassiveUnmountPendingDev = /* */ 0b0010000000000000; +export const Hydrating = /* */ 0b0000010000000000; +export const HydratingAndUpdate = /* */ 0b0000010000000100; // Passive & Update & Callback & Ref & Snapshot -export const LifecycleEffectMask = /* */ 0b000001110100100; +export const LifecycleEffectMask = /* */ 0b0000001110100100; // Union of all host effects -export const HostEffectMask = /* */ 0b000011111111111; +export const HostEffectMask = /* */ 0b0000011111111111; // These are not really side effects, but we still reuse this field. -export const Incomplete = /* */ 0b000100000000000; -export const ShouldCapture = /* */ 0b001000000000000; -export const ForceUpdateForLegacySuspense = /* */ 0b100000000000000; +export const Incomplete = /* */ 0b0000100000000000; +export const ShouldCapture = /* */ 0b0001000000000000; +export const ForceUpdateForLegacySuspense = /* */ 0b0100000000000000; + +// Static tags describe aspects of a fiber that are not specific to a render, +// e.g. a fiber uses a passive effect (even if there are no updates on this particular render). +// This enables us to defer more work in the unmount case, +// since we can defer traversing the tree during layout to look for Passive effects, +// and instead rely on the static flag as a signal that there may be cleanup work. +export const PassiveStatic = /* */ 0b1000000000000000; // Union of side effect groupings as pertains to subtreeTag -export const BeforeMutationMask = /* */ 0b000001100001010; -export const MutationMask = /* */ 0b000010010011110; -export const LayoutMask = /* */ 0b000000010100100; -export const PassiveMask = /* */ 0b000001000000000; +export const BeforeMutationMask = /* */ 0b0000001100001010; +export const MutationMask = /* */ 0b0000010010011110; +export const LayoutMask = /* */ 0b0000000010100100; +export const PassiveMask = /* */ 0b1000001000000000; + +// Union of tags that don't get reset on clones. +// This allows certain concepts to persist without recalculting them, +// e.g. whether a subtree contains passive effects or portals. +export const StaticMask = /* */ 0b1000000000000000; diff --git a/packages/react-reconciler/src/ReactSubtreeTags.js b/packages/react-reconciler/src/ReactSubtreeTags.js index a1edfd50ec45..536d9e074b44 100644 --- a/packages/react-reconciler/src/ReactSubtreeTags.js +++ b/packages/react-reconciler/src/ReactSubtreeTags.js @@ -14,8 +14,3 @@ export const BeforeMutation = /* */ 0b0001; export const Mutation = /* */ 0b0010; export const Layout = /* */ 0b0100; export const Passive = /* */ 0b1000; - -// Union of tags that don't get reset on clones. -// This allows certain concepts to persist without recalculting them, -// e.g. whether a subtree contains passive effects or portals. -export const Static = /* */ 0b1000; diff --git a/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js b/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js index 8086079b56f3..64a81fed0059 100644 --- a/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js +++ b/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js @@ -502,7 +502,11 @@ describe('SchedulingProfiler', () => { '--render-start-1024', '--render-stop', '--commit-start-1024', + '--layout-effects-start-1024', + '--layout-effects-stop', '--commit-stop', + '--passive-effects-start-1024', + '--passive-effects-stop', ]); }); From 0c59d069aae3c7d0654344cb10ae83c5c9cc3b24 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 28 Jul 2020 17:07:53 -0400 Subject: [PATCH 04/11] Added gate to SchedulingProfiler test --- .../SchedulingProfiler-test.internal.js | 67 +++++++++++++------ 1 file changed, 45 insertions(+), 22 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js b/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js index 64a81fed0059..4a184a815743 100644 --- a/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js +++ b/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js @@ -486,28 +486,51 @@ describe('SchedulingProfiler', () => { ReactTestRenderer.create(, {unstable_isConcurrent: true}); }); - expect(marks.map(normalizeCodeLocInfo)).toEqual([ - '--schedule-render-512', - '--render-start-512', - '--render-stop', - '--commit-start-512', - '--layout-effects-start-512', - '--layout-effects-stop', - '--commit-stop', - '--passive-effects-start-512', - toggleComponentStacks( - '--schedule-state-update-1024-Example-\n in Example (at **)', - ), - '--passive-effects-stop', - '--render-start-1024', - '--render-stop', - '--commit-start-1024', - '--layout-effects-start-1024', - '--layout-effects-stop', - '--commit-stop', - '--passive-effects-start-1024', - '--passive-effects-stop', - ]); + gate(({old}) => { + if (old) { + expect(marks.map(normalizeCodeLocInfo)).toEqual([ + '--schedule-render-512', + '--render-start-512', + '--render-stop', + '--commit-start-512', + '--layout-effects-start-512', + '--layout-effects-stop', + '--commit-stop', + '--passive-effects-start-512', + toggleComponentStacks( + '--schedule-state-update-1024-Example-\n in Example (at **)', + ), + '--passive-effects-stop', + '--render-start-1024', + '--render-stop', + '--commit-start-1024', + '--commit-stop', + ]); + } else { + expect(marks.map(normalizeCodeLocInfo)).toEqual([ + '--schedule-render-512', + '--render-start-512', + '--render-stop', + '--commit-start-512', + '--layout-effects-start-512', + '--layout-effects-stop', + '--commit-stop', + '--passive-effects-start-512', + toggleComponentStacks( + '--schedule-state-update-1024-Example-\n in Example (at **)', + ), + '--passive-effects-stop', + '--render-start-1024', + '--render-stop', + '--commit-start-1024', + '--layout-effects-start-1024', + '--layout-effects-stop', + '--commit-stop', + '--passive-effects-start-1024', + '--passive-effects-stop', + ]); + } + }); }); // @gate enableSchedulingProfiler From 7d618ede1794700c287552a1f5e816e6f2586a43 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 29 Jul 2020 11:30:51 -0400 Subject: [PATCH 05/11] Defer Fiber field clenaup to passive phase for all but return pointer --- .../src/ReactFiberCommitWork.new.js | 33 +++++++------------ .../src/ReactFiberWorkLoop.new.js | 14 ++++++++ 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 00dc62682451..1517a183c2cf 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1015,30 +1015,19 @@ function commitNestedUnmounts( } function detachFiberMutation(fiber: Fiber) { - // 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 + // Cut off the return pointer to disconnect it from the tree. + // This enables us to detect and warn against state updates on an unmounted component. + // It also prevents events from bubbling from within disconnected components. + // + // Ideally, we should also 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. - // Note: we cannot null out sibling here, otherwise it can cause issues - // with findDOMNode and how it requires the sibling field to carry out - // traversal in a later effect. See PR #16820. We now clear the sibling - // field after effects, see: detachFiberAfterEffects. - fiber.alternate = null; - fiber.dependencies = null; - fiber.firstEffect = null; - fiber.lastEffect = null; - fiber.memoizedProps = null; - fiber.memoizedState = null; - fiber.pendingProps = null; + // 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. + // + // Note that we can't clear child or sibling pointers yet. + // They're needed for passive effects and for findDOMNode. + // We defer those fields, and all other cleanup, to the passive phase (see detachFiberAfterEffects). fiber.return = null; - fiber.stateNode = null; - if (__DEV__) { - fiber._debugOwner = null; - } } function emptyPortalContainer(current: Fiber) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 04f57c4ac992..7142d1b36f60 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -4060,7 +4060,21 @@ export function act(callback: () => Thenable): Thenable { } function detachFiberAfterEffects(fiber: Fiber): void { + // Null out fields to improve GC for references that may be lingering (e.g. DevTools). + // Note that we already cleared the return pointer in detachFiberMutation(). + fiber.alternate = null; fiber.child = null; + fiber.dependencies = null; + fiber.firstEffect = null; + fiber.lastEffect = null; + fiber.memoizedProps = null; + fiber.memoizedState = null; + fiber.pendingProps = null; fiber.sibling = null; + fiber.stateNode = null; fiber.updateQueue = null; + + if (__DEV__) { + fiber._debugOwner = null; + } } From cba61f7f443d7a93d4072c6bd23147a2d9ee2ed4 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 29 Jul 2020 12:25:46 -0400 Subject: [PATCH 06/11] 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. --- .../src/ReactChildFiber.new.js | 24 ++++---------- .../src/ReactFiberBeginWork.new.js | 32 +++++++------------ .../src/ReactFiberHydrationContext.new.js | 25 ++++----------- .../__tests__/ReactSuspense-test.internal.js | 6 ++-- 4 files changed, 27 insertions(+), 60 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js index c0c131bb928b..b43ed8bb8183 100644 --- 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 index 244472069a80..fa2445664b6f 100644 --- 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 index 81201f82257e..914370df882e 100644 --- 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 index 3c7440502957..8c709cb59397 100644 --- 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( From 9dc5188110e14bcf3f73c0dcfa7ebca0d6849f95 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 29 Jul 2020 13:58:41 -0400 Subject: [PATCH 07/11] Tweaks made in response to PR feedback: * Added Deletion effect to PassiveMask. * Removed the no longer needed explicit bubbling of Passive subtreeTag during deletion. * Removed unnecessary deletion effectTag from fallback child. * Detached alternate.return as well during layout. * Removed PassiveStaticEffect from hook update. * Removed unnecessary call to detachFiberAfterEffects. --- .../react-reconciler/src/ReactChildFiber.new.js | 7 ------- .../src/ReactFiberBeginWork.new.js | 16 ---------------- .../src/ReactFiberCommitWork.new.js | 5 +++++ .../react-reconciler/src/ReactFiberHooks.new.js | 2 +- .../src/ReactFiberHydrationContext.new.js | 7 ------- .../src/ReactFiberWorkLoop.new.js | 8 -------- .../react-reconciler/src/ReactSideEffectTags.js | 2 +- 7 files changed, 7 insertions(+), 40 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js index b43ed8bb8183..e39660ba0b56 100644 --- a/packages/react-reconciler/src/ReactChildFiber.new.js +++ b/packages/react-reconciler/src/ReactChildFiber.new.js @@ -16,7 +16,6 @@ import type {Lanes} from './ReactFiberLane'; import getComponentName from 'shared/getComponentName'; import {Deletion, Placement} from './ReactSideEffectTags'; -import {Passive as PassiveSubtreeTag} from './ReactSubtreeTags'; import { getIteratorFn, REACT_ELEMENT_TYPE, @@ -294,12 +293,6 @@ function ChildReconciler(shouldTrackSideEffects) { returnFiber.deletions = [childToDelete]; // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions) returnFiber.effectTag |= Deletion; - - // 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 index fa2445664b6f..54962f2365d9 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -64,9 +64,7 @@ import { Ref, Deletion, ForceUpdateForLegacySuspense, - StaticMask, } from './ReactSideEffectTags'; -import {Passive as PassiveSubtreeTag} from './ReactSubtreeTags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import { debugRenderPhaseSideEffectsForStrictMode, @@ -2066,20 +2064,12 @@ function updateSuspensePrimaryChildren( if (currentFallbackChildFragment !== null) { // Delete the fallback child fragment currentFallbackChildFragment.nextEffect = null; - currentFallbackChildFragment.effectTag = - (currentFallbackChildFragment.effectTag & StaticMask) | Deletion; workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment; const deletions = workInProgress.deletions; if (deletions === null) { workInProgress.deletions = [currentFallbackChildFragment]; // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions) workInProgress.effectTag |= Deletion; - - // 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); } @@ -3064,12 +3054,6 @@ function remountFiber( returnFiber.deletions = [current]; // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions) returnFiber.effectTag |= Deletion; - - // 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/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 1517a183c2cf..a2410b972ca8 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1027,6 +1027,11 @@ function detachFiberMutation(fiber: Fiber) { // Note that we can't clear child or sibling pointers yet. // They're needed for passive effects and for findDOMNode. // We defer those fields, and all other cleanup, to the passive phase (see detachFiberAfterEffects). + const alternate = fiber.alternate; + if (alternate !== null) { + alternate.return = null; + fiber.alternate = null; + } fiber.return = null; } diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 544abf636b97..be63958074d7 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -1289,7 +1289,7 @@ function updateEffect( } } return updateEffectImpl( - UpdateEffect | PassiveEffect | PassiveStaticEffect, + UpdateEffect | PassiveEffect, HookPassive, create, deps, diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index 914370df882e..cc9965e2ea81 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -25,7 +25,6 @@ import { SuspenseComponent, } from './ReactWorkTags'; import {Deletion, Hydrating, Placement} from './ReactSideEffectTags'; -import {Passive as PassiveSubtreeTag} from './ReactSubtreeTags'; import invariant from 'shared/invariant'; import { @@ -131,12 +130,6 @@ function deleteHydratableInstance( returnFiber.deletions = [childToDelete]; // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions) returnFiber.effectTag |= Deletion; - - // 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/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 7142d1b36f60..3ab19b099cb5 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2560,13 +2560,6 @@ function commitMutationEffectsDeletions( captureCommitPhaseError(childToDelete, error); } } - - // 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); - } } } @@ -4062,7 +4055,6 @@ export function act(callback: () => Thenable): Thenable { function detachFiberAfterEffects(fiber: Fiber): void { // Null out fields to improve GC for references that may be lingering (e.g. DevTools). // Note that we already cleared the return pointer in detachFiberMutation(). - fiber.alternate = null; fiber.child = null; fiber.dependencies = null; fiber.firstEffect = null; diff --git a/packages/react-reconciler/src/ReactSideEffectTags.js b/packages/react-reconciler/src/ReactSideEffectTags.js index 2c2aa129d41a..9fcf0edf17cc 100644 --- a/packages/react-reconciler/src/ReactSideEffectTags.js +++ b/packages/react-reconciler/src/ReactSideEffectTags.js @@ -50,7 +50,7 @@ export const PassiveStatic = /* */ 0b1000000000000000; export const BeforeMutationMask = /* */ 0b0000001100001010; export const MutationMask = /* */ 0b0000010010011110; export const LayoutMask = /* */ 0b0000000010100100; -export const PassiveMask = /* */ 0b1000001000000000; +export const PassiveMask = /* */ 0b1000001000001000; // Union of tags that don't get reset on clones. // This allows certain concepts to persist without recalculting them, From a57840eca99b8fa8675d789e248d042efa9791fc Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 29 Jul 2020 15:45:33 -0400 Subject: [PATCH 08/11] Defer clearing deletions array until passive, remove unecessary didBailout check --- .../src/ReactFiberWorkLoop.new.js | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 3ab19b099cb5..716d33b0f40a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2183,7 +2183,7 @@ function commitRootImpl(root, renderPriorityLevel) { } // If there are pending passive effects, schedule a callback to process them. - if ((root.current.subtreeTag & PassiveSubtreeTag) !== NoSubtreeTag) { + if ((finishedWork.subtreeTag & PassiveSubtreeTag) !== NoSubtreeTag) { if (!rootDoesHavePassiveEffects) { rootDoesHavePassiveEffects = true; scheduleCallback(NormalSchedulerPriority, () => { @@ -2420,13 +2420,6 @@ function commitMutationEffects( const deletions = fiber.deletions; if (deletions !== null) { commitMutationEffectsDeletions(deletions, root, renderPriorityLevel); - - // 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) { @@ -2684,15 +2677,9 @@ function invokePassiveEffectCreate(effect: HookEffect): void { function flushPassiveMountEffects(firstChild: Fiber): void { let fiber = firstChild; while (fiber !== null) { - const didBailout = - fiber.alternate !== null && fiber.alternate.child === fiber.child; const primarySubtreeTag = fiber.subtreeTag & PassiveSubtreeTag; - if ( - fiber.child !== null && - !didBailout && - primarySubtreeTag !== NoSubtreeTag - ) { + if (fiber.child !== null && primarySubtreeTag !== NoSubtreeTag) { flushPassiveMountEffects(fiber.child); } @@ -4056,6 +4043,7 @@ function detachFiberAfterEffects(fiber: Fiber): void { // Null out fields to improve GC for references that may be lingering (e.g. DevTools). // Note that we already cleared the return pointer in detachFiberMutation(). fiber.child = null; + fiber.deletions = null; fiber.dependencies = null; fiber.firstEffect = null; fiber.lastEffect = null; From 6d926dadde31acbedc2914cd478d0743f14fff65 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 29 Jul 2020 15:50:33 -0400 Subject: [PATCH 09/11] Removed PassiveUnmountPendingDev in the new reconciler --- .../src/ReactFiberCommitWork.new.js | 12 ------- .../src/ReactFiberWorkLoop.new.js | 32 +++++++++++-------- .../src/ReactSideEffectTags.js | 1 + 3 files changed, 20 insertions(+), 25 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index a2410b972ca8..40286e21d263 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -69,7 +69,6 @@ import { Snapshot, Update, Passive, - PassiveUnmountPendingDev, } from './ReactSideEffectTags'; import getComponentName from 'shared/getComponentName'; import invariant from 'shared/invariant'; @@ -883,17 +882,6 @@ function commitUnmount( current.effectTag |= Passive; - if (__DEV__) { - // This flag is used to avoid warning about an update to an unmounted component - // if the component has a passive unmount scheduled. - // Presumably the listener would be cleaned up by that unmount. - current.effectTag |= PassiveUnmountPendingDev; - const alternate = current.alternate; - if (alternate !== null) { - alternate.effectTag |= PassiveUnmountPendingDev; - } - } - schedulePassiveEffectCallback(); } else { if ( diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 716d33b0f40a..ab0cecc7bb16 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -132,7 +132,7 @@ import { Snapshot, Callback, Passive, - PassiveUnmountPendingDev, + PassiveStatic, Incomplete, HostEffectMask, Hydrating, @@ -2841,14 +2841,6 @@ function flushPassiveUnmountEffectsImpl(fiber: Fiber): void { const destroy = effect.destroy; effect.destroy = undefined; - if (__DEV__) { - fiber.effectTag &= ~PassiveUnmountPendingDev; - const alternate = fiber.alternate; - if (alternate !== null) { - alternate.effectTag &= ~PassiveUnmountPendingDev; - } - } - if (typeof destroy === 'function') { if (__DEV__) { setCurrentDebugFiberInDEV(fiber); @@ -3334,10 +3326,24 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) { return; } - // If there are pending passive effects unmounts for this Fiber, - // we can assume that they would have prevented this update. - if ((fiber.effectTag & PassiveUnmountPendingDev) !== NoEffect) { - return; + if ((fiber.effectTag & PassiveStatic) !== NoEffect) { + const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); + if (updateQueue !== null) { + const lastEffect = updateQueue.lastEffect; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + + let effect = firstEffect; + do { + if (effect.destroy !== undefined) { + if ((effect.tag & HookPassive) !== NoHookEffect) { + return; + } + } + effect = effect.next; + } while (effect !== firstEffect); + } + } } // We show the whole stack but dedupe on the top component's name because diff --git a/packages/react-reconciler/src/ReactSideEffectTags.js b/packages/react-reconciler/src/ReactSideEffectTags.js index 9fcf0edf17cc..2af6d5369a39 100644 --- a/packages/react-reconciler/src/ReactSideEffectTags.js +++ b/packages/react-reconciler/src/ReactSideEffectTags.js @@ -24,6 +24,7 @@ export const DidCapture = /* */ 0b0000000001000000; export const Ref = /* */ 0b0000000010000000; export const Snapshot = /* */ 0b0000000100000000; export const Passive = /* */ 0b0000001000000000; +// TODO (effects) Remove this bit once the new reconciler is synced to the old. export const PassiveUnmountPendingDev = /* */ 0b0010000000000000; export const Hydrating = /* */ 0b0000010000000000; export const HydratingAndUpdate = /* */ 0b0000010000000100; From c1a1be521766a8ba746e7268ec044516c4a9dc50 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 29 Jul 2020 16:11:26 -0400 Subject: [PATCH 10/11] Removed unnecessary bailout check. Added a TODO for followup. --- .../src/ReactFiberCommitWork.new.js | 1 + .../src/ReactFiberWorkLoop.new.js | 22 ++++++++----------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 40286e21d263..9fc5b1f5b2cf 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -878,6 +878,7 @@ function commitUnmount( const {destroy, tag} = effect; if (destroy !== undefined) { if ((tag & HookPassive) !== NoHookEffect) { + // TODO: Consider if we can move this block out of the synchronous commit phase effect.tag |= HookHasEffect; current.effectTag |= Passive; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index ab0cecc7bb16..be7a63ec4f2d 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2794,19 +2794,15 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { fiber.deletions = null; } - const didBailout = - fiber.alternate !== null && fiber.alternate.child === fiber.child; - if (!didBailout) { - const child = fiber.child; - if (child !== null) { - // If any children have passive effects then traverse the subtree. - // Note that this requires checking subtreeTag of the current Fiber, - // rather than the subtreeTag/effectsTag of the first child, - // since that would not cover passive effects in siblings. - const primarySubtreeTag = fiber.subtreeTag & PassiveSubtreeTag; - if (primarySubtreeTag !== NoSubtreeTag) { - flushPassiveUnmountEffects(child); - } + const child = fiber.child; + if (child !== null) { + // If any children have passive effects then traverse the subtree. + // Note that this requires checking subtreeTag of the current Fiber, + // rather than the subtreeTag/effectsTag of the first child, + // since that would not cover passive effects in siblings. + const primarySubtreeTag = fiber.subtreeTag & PassiveSubtreeTag; + if (primarySubtreeTag !== NoSubtreeTag) { + flushPassiveUnmountEffects(child); } } From ae37c7c2620f8b202426dc4f94c2d8797a54544d Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 29 Jul 2020 16:31:39 -0400 Subject: [PATCH 11/11] Split flushPassiveUnmountEffects() into two functions One is used for mounted fibers, and one for unmounted subtrees. --- .../src/ReactFiberWorkLoop.new.js | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index be7a63ec4f2d..69992f6807a1 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2783,15 +2783,12 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { primarySubtreeTag !== NoSubtreeTag || primaryEffectTag !== NoEffect ) { - flushPassiveUnmountEffects(fiberToDelete); + flushPassiveUnmountEffectsInsideOfDeletedTree(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 child = fiber.child; @@ -2822,6 +2819,39 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { } } +function flushPassiveUnmountEffectsInsideOfDeletedTree( + firstChild: Fiber, +): void { + let fiber = firstChild; + while (fiber !== null) { + const child = fiber.child; + if (child !== null) { + // If any children have passive effects then traverse the subtree. + // Note that this requires checking subtreeTag of the current Fiber, + // rather than the subtreeTag/effectsTag of the first child, + // since that would not cover passive effects in siblings. + const primarySubtreeTag = fiber.subtreeTag & PassiveSubtreeTag; + if (primarySubtreeTag !== NoSubtreeTag) { + flushPassiveUnmountEffectsInsideOfDeletedTree(child); + } + } + + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: + case Block: { + const primaryEffectTag = fiber.effectTag & Passive; + if (primaryEffectTag !== NoEffect) { + flushPassiveUnmountEffectsImpl(fiber); + } + } + } + + fiber = fiber.sibling; + } +} + function flushPassiveUnmountEffectsImpl(fiber: Fiber): void { const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;