From ede9170648d07a63cd282e6acb3ea1fe9e22ded9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 30 Jul 2020 23:43:03 -0500 Subject: [PATCH] Move passive logic out of layout phase (#19500) * setCurrentFiber per fiber, instead of per effect * Re-use safelyCallDestroy Part of the code in flushPassiveUnmountEffects is a duplicate of the code used for unmounting layout effects. I did some minor refactoring to so we could use the same function in both places. Closure will inline anyway so it doesn't affect code size or performance, just maintainability. * Don't check HookHasEffect during deletion We don't need to check HookHasEffect during a deletion; all effects are unmounted. So we also don't have to set HookHasEffect during a deletion, either. This allows us to remove the last remaining passive effect logic from the synchronous layout phase. --- .../src/ReactFiberCommitWork.new.js | 8 +- .../src/ReactFiberWorkLoop.new.js | 81 +++++++------------ 2 files changed, 32 insertions(+), 57 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 3732c81ce6..7efe961ffd 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -122,7 +122,6 @@ import { NoEffect as NoHookEffect, HasEffect as HookHasEffect, Layout as HookLayout, - Passive as HookPassive, } from './ReactHookEffectTags'; import {didWarnAboutReassigningProps} from './ReactFiberBeginWork.new'; import { @@ -206,7 +205,7 @@ function safelyDetachRef(current: Fiber) { } } -function safelyCallDestroy(current, destroy) { +export function safelyCallDestroy(current: Fiber, destroy: () => void) { if (__DEV__) { invokeGuardedCallback(null, destroy, null); if (hasCaughtError()) { @@ -876,10 +875,7 @@ function commitUnmount( do { 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; - } else { + if ((tag & HookLayout) !== NoHookEffect) { if ( enableProfilerTimer && enableProfilerCommitHooks && diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 7e6845f19a..d5a1a2e2ca 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -15,6 +15,7 @@ import type {Interaction} from 'scheduler/src/Tracing'; import type {SuspenseConfig} from './ReactFiberSuspenseConfig'; import type {SuspenseState} from './ReactFiberSuspenseComponent.new'; import type {Effect as HookEffect} from './ReactFiberHooks.new'; +import type {HookEffectTag} from './ReactHookEffectTags'; import type {StackCursor} from './ReactFiberStack.new'; import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new'; @@ -209,6 +210,7 @@ import { commitPassiveEffectDurations, commitResetTextContent, isSuspenseBoundaryBeingHidden, + safelyCallDestroy, } from './ReactFiberCommitWork.new'; import {enqueueUpdate} from './ReactUpdateQueue.new'; import {resetContextDependencies} from './ReactFiberNewContext.new'; @@ -2702,6 +2704,8 @@ function flushPassiveMountEffects(firstChild: Fiber): void { } function flushPassiveMountEffectsImpl(fiber: Fiber): void { + setCurrentDebugFiberInDEV(fiber); + const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; if (lastEffect !== null) { @@ -2715,7 +2719,6 @@ function flushPassiveMountEffectsImpl(fiber: Fiber): void { (tag & HookHasEffect) !== NoHookEffect ) { if (__DEV__) { - setCurrentDebugFiberInDEV(fiber); if ( enableProfilerTimer && enableProfilerCommitHooks && @@ -2742,7 +2745,6 @@ function flushPassiveMountEffectsImpl(fiber: Fiber): void { const error = clearCaughtError(); captureCommitPhaseError(fiber, error); } - resetCurrentDebugFiberInDEV(); } else { try { const create = effect.create; @@ -2769,6 +2771,8 @@ function flushPassiveMountEffectsImpl(fiber: Fiber): void { effect = next; } while (effect !== firstEffect); + + resetCurrentDebugFiberInDEV(); } } @@ -2813,7 +2817,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { case Block: { const primaryEffectTag = fiber.effectTag & Passive; if (primaryEffectTag !== NoEffect) { - flushPassiveUnmountEffectsImpl(fiber); + flushPassiveUnmountEffectsImpl(fiber, HookPassive | HookHasEffect); } } } @@ -2845,7 +2849,7 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree( case ForwardRef: case SimpleMemoComponent: case Block: { - flushPassiveUnmountEffectsImpl(fiber); + flushPassiveUnmountEffectsImpl(fiber, HookPassive); } } } @@ -2854,67 +2858,42 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree( } } -function flushPassiveUnmountEffectsImpl(fiber: Fiber): void { +function flushPassiveUnmountEffectsImpl( + fiber: Fiber, + // Tags to check for when deciding whether to unmount. e.g. to skip over + // layout effects + hookEffectTag: HookEffectTag, +): void { const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; if (lastEffect !== null) { + setCurrentDebugFiberInDEV(fiber); + const firstEffect = lastEffect.next; let effect = firstEffect; do { const {next, tag} = effect; - if ( - (tag & HookPassive) !== NoHookEffect && - (tag & HookHasEffect) !== NoHookEffect - ) { + if ((tag & hookEffectTag) === hookEffectTag) { const destroy = effect.destroy; - effect.destroy = undefined; - - 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(); + if (destroy !== undefined) { + effect.destroy = undefined; + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + fiber.mode & ProfileMode + ) { + startPassiveEffectTimer(); + safelyCallDestroy(fiber, destroy); + recordPassiveEffectDuration(fiber); } 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); - } + safelyCallDestroy(fiber, destroy); } } } - effect = next; } while (effect !== firstEffect); + + resetCurrentDebugFiberInDEV(); } }