From df2c6361cbce78451c6233e3b38768b831994e25 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 22 Sep 2020 13:16:27 -0500 Subject: [PATCH] Consolidate commit phase hook functions (#19864) There were a few pairs of commit phase functions that were almost identical except for one detail. I've refactored them a bit to consolidate their implementations: - Lifted error handling logic when mounting a fiber's passive hook effects to surround the entire list, instead of surrounding each effect. - Lifted profiler duration tracking to surround the entire list. In both cases, this matches the corresponding code for the layout phase. The naming is still a bit of a mess but I'm not too concerned because my next step is to refactor each commit sub-phase (layout, mutation) so that we can store values on the JS stack. So the existing function boundaries are about to change, anyway. --- .../src/ReactFiberCommitWork.new.js | 193 ++++++------------ .../src/ReactFiberWorkLoop.new.js | 34 ++- 2 files changed, 86 insertions(+), 141 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 0027250803638..60b416c945f26 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -20,10 +20,7 @@ import type {FiberRoot} from './ReactInternalTypes'; import type {Lanes} from './ReactFiberLane'; import type {SuspenseState} from './ReactFiberSuspenseComponent.new'; import type {UpdateQueue} from './ReactUpdateQueue.new'; -import type { - Effect as HookEffect, - FunctionComponentUpdateQueue, -} from './ReactFiberHooks.new'; +import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new'; import type {Wakeable} from 'shared/ReactTypes'; import type {ReactPriorityLevel} from './ReactInternalTypes'; import type {OffscreenState} from './ReactFiberOffscreenComponent'; @@ -343,42 +340,6 @@ function commitHookEffectListUnmount( } } -// TODO: Remove this duplication. -function commitHookEffectListUnmount2( - // Tags to check for when deciding whether to unmount. e.g. to skip over layout effects - hookFlags: HookFlags, - fiber: Fiber, - nearestMountedAncestor: Fiber | null, -): 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 & hookFlags) === hookFlags) { - const destroy = effect.destroy; - if (destroy !== undefined) { - effect.destroy = undefined; - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - fiber.mode & ProfileMode - ) { - startPassiveEffectTimer(); - safelyCallDestroy(fiber, nearestMountedAncestor, destroy); - recordPassiveEffectDuration(fiber); - } else { - safelyCallDestroy(fiber, nearestMountedAncestor, destroy); - } - } - } - effect = next; - } while (effect !== firstEffect); - } -} - function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) { const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; @@ -429,83 +390,6 @@ function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) { } } -function invokePassiveEffectCreate(effect: HookEffect): void { - const create = effect.create; - effect.destroy = create(); -} - -// TODO: Remove this duplication. -function commitHookEffectListMount2(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__) { - 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, fiber.return, error); - } - } else { - try { - const create = effect.create; - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - fiber.mode & ProfileMode - ) { - try { - startPassiveEffectTimer(); - effect.destroy = create(); - } finally { - recordPassiveEffectDuration(fiber); - } - } else { - effect.destroy = create(); - } - // TODO: This is missing the warning that exists in commitHookEffectListMount. - // The warning refers to useEffect but only applies to useLayoutEffect. - } catch (error) { - invariant(fiber !== null, 'Should be working on an effect.'); - captureCommitPhaseError(fiber, fiber.return, error); - } - } - } - - effect = next; - } while (effect !== firstEffect); - } -} - function commitProfilerPassiveEffect( finishedRoot: FiberRoot, finishedWork: Fiber, @@ -1894,17 +1778,31 @@ function commitResetTextContent(current: Fiber): void { resetTextContent(current.stateNode); } -function commitPassiveWork(finishedWork: Fiber): void { +function commitPassiveUnmountInsideDeletedTree(finishedWork: Fiber): void { switch (finishedWork.tag) { case FunctionComponent: case ForwardRef: case SimpleMemoComponent: case Block: { - commitHookEffectListUnmount2( - HookPassive | HookHasEffect, - finishedWork, - finishedWork.return, - ); + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + startPassiveEffectTimer(); + commitHookEffectListUnmount( + HookPassive | HookHasEffect, + finishedWork, + finishedWork.return, + ); + recordPassiveEffectDuration(finishedWork); + } else { + commitHookEffectListUnmount( + HookPassive | HookHasEffect, + finishedWork, + finishedWork.return, + ); + } break; } } @@ -1918,16 +1816,32 @@ function commitPassiveUnmount( case FunctionComponent: case ForwardRef: case SimpleMemoComponent: - case Block: - commitHookEffectListUnmount2( - HookPassive, - current, - nearestMountedAncestor, - ); + case Block: { + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + current.mode & ProfileMode + ) { + startPassiveEffectTimer(); + commitHookEffectListUnmount( + HookPassive, + current, + nearestMountedAncestor, + ); + recordPassiveEffectDuration(current); + } else { + commitHookEffectListUnmount( + HookPassive, + current, + nearestMountedAncestor, + ); + } + break; + } } } -function commitPassiveLifeCycles( +function commitPassiveMount( finishedRoot: FiberRoot, finishedWork: Fiber, ): void { @@ -1936,7 +1850,20 @@ function commitPassiveLifeCycles( case ForwardRef: case SimpleMemoComponent: case Block: { - commitHookEffectListMount2(finishedWork); + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + startPassiveEffectTimer(); + try { + commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork); + } finally { + recordPassiveEffectDuration(finishedWork); + } + } else { + commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork); + } break; } case Profiler: { @@ -1956,6 +1883,6 @@ export { commitAttachRef, commitDetachRef, commitPassiveUnmount, - commitPassiveWork, - commitPassiveLifeCycles, + commitPassiveUnmountInsideDeletedTree, + commitPassiveMount, }; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 6192fc7c422ac..1f9cf3235cf81 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -191,9 +191,9 @@ import { commitPlacement, commitWork, commitDeletion, - commitPassiveUnmount, - commitPassiveWork, - commitPassiveLifeCycles as commitPassiveEffectOnFiber, + commitPassiveUnmount as commitPassiveUnmountOnFiber, + commitPassiveUnmountInsideDeletedTree as commitPassiveUnmountInsideDeletedTreeOnFiber, + commitPassiveMount as commitPassiveMountOnFiber, commitDetachRef, commitAttachRef, commitResetTextContent, @@ -2458,9 +2458,27 @@ function flushPassiveMountEffects(root, firstChild: Fiber): void { } if ((fiber.flags & Passive) !== NoFlags) { - setCurrentDebugFiberInDEV(fiber); - commitPassiveEffectOnFiber(root, fiber); - resetCurrentDebugFiberInDEV(); + if (__DEV__) { + setCurrentDebugFiberInDEV(fiber); + invokeGuardedCallback( + null, + commitPassiveMountOnFiber, + null, + root, + fiber, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(fiber, fiber.return, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + commitPassiveMountOnFiber(root, fiber); + } catch (error) { + captureCommitPhaseError(fiber, fiber.return, error); + } + } } fiber = fiber.sibling; @@ -2496,7 +2514,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { const primaryFlags = fiber.flags & Passive; if (primaryFlags !== NoFlags) { setCurrentDebugFiberInDEV(fiber); - commitPassiveWork(fiber); + commitPassiveUnmountInsideDeletedTreeOnFiber(fiber); resetCurrentDebugFiberInDEV(); } @@ -2525,7 +2543,7 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree( if ((fiberToDelete.flags & PassiveStatic) !== NoFlags) { setCurrentDebugFiberInDEV(fiberToDelete); - commitPassiveUnmount(fiberToDelete, nearestMountedAncestor); + commitPassiveUnmountOnFiber(fiberToDelete, nearestMountedAncestor); resetCurrentDebugFiberInDEV(); } }