From fbc64ba84b651ef8a5d8326cb722d5d3db9f6b43 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 23 Sep 2020 15:19:45 -0400 Subject: [PATCH 1/6] Avoid traversing return path during commit Commit phase durations (layout and passive) are stored on the nearest (ancestor) Profiler and bubble up during the commit phase. This bubbling used to be implemented by traversing the return path each time we finished working on a Profiler to find the next nearest Profiler. This commit removes that traversal. Instead, we maintain a stack of the current and next-nearest Profiler ancestor while recursing the tree. This stack is maintained in the work loop (since that's whre the recursive functions are) and referenced in commit work (since that's generally where tag-specific operations are). This is not obviously an improvement. It's just an idea I'm sharing for discussion purposes. --- .../src/ReactFiberCommitWork.new.js | 25 ++++-------- .../src/ReactFiberWorkLoop.new.js | 39 +++++++++++++++++++ 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 6d47fac3f4661..631978b1e75e9 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -121,6 +121,7 @@ import { resolveRetryWakeable, markCommitTimeOfFallback, schedulePassiveEffectCallback, + penultimateProfilerOnStack, } from './ReactFiberWorkLoop.new'; import { NoFlags as NoHookEffect, @@ -427,15 +428,9 @@ function commitProfilerPassiveEffect( // Bubble times to the next nearest ancestor Profiler. // After we process that Profiler, we'll bubble further up. - // TODO: Use JS Stack instead - let parentFiber = finishedWork.return; - while (parentFiber !== null) { - if (parentFiber.tag === Profiler) { - const parentStateNode = parentFiber.stateNode; - parentStateNode.passiveEffectDuration += passiveEffectDuration; - break; - } - parentFiber = parentFiber.return; + if (penultimateProfilerOnStack !== null) { + const stateNode = penultimateProfilerOnStack.stateNode; + stateNode.passiveEffectDuration += passiveEffectDuration; } break; } @@ -736,15 +731,9 @@ function commitLifeCycles( // Propagate layout effect durations to the next nearest Profiler ancestor. // Do not reset these values until the next render so DevTools has a chance to read them first. - // TODO: Use JS Stack instead - let parentFiber = finishedWork.return; - while (parentFiber !== null) { - if (parentFiber.tag === Profiler) { - const parentStateNode = parentFiber.stateNode; - parentStateNode.effectDuration += effectDuration; - break; - } - parentFiber = parentFiber.return; + if (penultimateProfilerOnStack !== null) { + const stateNode = penultimateProfilerOnStack.stateNode; + stateNode.effectDuration += effectDuration; } } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index bd8ef8ce5e3c6..58bd78315bb6c 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -22,6 +22,7 @@ import { enableSuspenseServerRenderer, replayFailedUnitOfWorkWithInvokeGuardedCallback, enableProfilerTimer, + enableProfilerCommitHooks, enableSchedulerTracing, warnAboutUnmockedScheduler, deferRenderPhaseUpdateToNextBatch, @@ -116,6 +117,7 @@ import { SimpleMemoComponent, Block, ScopeComponent, + Profiler, } from './ReactWorkTags'; import {LegacyRoot} from './ReactRootTags'; import { @@ -373,6 +375,10 @@ let isFlushingPassiveEffects = false; let focusedInstanceHandle: null | Fiber = null; let shouldFireAfterActiveInstanceBlur: boolean = false; +// Used to avoid traversing the return path to find the nearest Profiler ancestor during commit. +let nearestProfilerOnStack: Fiber | null = null; +export let penultimateProfilerOnStack: Fiber | null = null; + export function getWorkInProgressRoot(): FiberRoot | null { return workInProgressRoot; } @@ -2362,6 +2368,15 @@ function commitLayoutEffects( ) { let fiber = firstChild; while (fiber !== null) { + let prevPenultimateProfiler = null; + if (enableProfilerTimer && enableProfilerCommitHooks) { + if (fiber.tag === Profiler) { + prevPenultimateProfiler = penultimateProfilerOnStack; + penultimateProfilerOnStack = nearestProfilerOnStack; + nearestProfilerOnStack = fiber; + } + } + if (fiber.child !== null) { const primarySubtreeFlags = fiber.subtreeFlags & LayoutMask; if (primarySubtreeFlags !== NoFlags) { @@ -2391,6 +2406,14 @@ function commitLayoutEffects( captureCommitPhaseError(fiber, fiber.return, error); } } + + if (enableProfilerTimer && enableProfilerCommitHooks) { + if (fiber.tag === Profiler) { + nearestProfilerOnStack = penultimateProfilerOnStack; + penultimateProfilerOnStack = prevPenultimateProfiler; + } + } + fiber = fiber.sibling; } } @@ -2452,6 +2475,15 @@ export function flushPassiveEffects(): boolean { function flushPassiveMountEffects(root, firstChild: Fiber): void { let fiber = firstChild; while (fiber !== null) { + let prevPenultimateProfiler = null; + if (enableProfilerTimer && enableProfilerCommitHooks) { + if (fiber.tag === Profiler) { + prevPenultimateProfiler = penultimateProfilerOnStack; + penultimateProfilerOnStack = nearestProfilerOnStack; + nearestProfilerOnStack = fiber; + } + } + const primarySubtreeFlags = fiber.subtreeFlags & PassiveMask; if (fiber.child !== null && primarySubtreeFlags !== NoFlags) { @@ -2482,6 +2514,13 @@ function flushPassiveMountEffects(root, firstChild: Fiber): void { } } + if (enableProfilerTimer && enableProfilerCommitHooks) { + if (fiber.tag === Profiler) { + nearestProfilerOnStack = penultimateProfilerOnStack; + penultimateProfilerOnStack = prevPenultimateProfiler; + } + } + fiber = fiber.sibling; } } From 17043993c5883ccba69ed69143abd48827261c27 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 23 Sep 2020 15:37:54 -0400 Subject: [PATCH 2/6] Moved Profiler commit-phase bubbling into work loop This enables us to only track a single item in the stack and requires less ugly coupling between the work loop and commit work. --- .../src/ReactFiberCommitWork.new.js | 15 --------- .../src/ReactFiberWorkLoop.new.js | 31 ++++++++++++------- 2 files changed, 20 insertions(+), 26 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 631978b1e75e9..a030b38c2024a 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -121,7 +121,6 @@ import { resolveRetryWakeable, markCommitTimeOfFallback, schedulePassiveEffectCallback, - penultimateProfilerOnStack, } from './ReactFiberWorkLoop.new'; import { NoFlags as NoHookEffect, @@ -425,13 +424,6 @@ function commitProfilerPassiveEffect( ); } } - - // Bubble times to the next nearest ancestor Profiler. - // After we process that Profiler, we'll bubble further up. - if (penultimateProfilerOnStack !== null) { - const stateNode = penultimateProfilerOnStack.stateNode; - stateNode.passiveEffectDuration += passiveEffectDuration; - } break; } default: @@ -728,13 +720,6 @@ function commitLifeCycles( ); } } - - // Propagate layout effect durations to the next nearest Profiler ancestor. - // Do not reset these values until the next render so DevTools has a chance to read them first. - if (penultimateProfilerOnStack !== null) { - const stateNode = penultimateProfilerOnStack.stateNode; - stateNode.effectDuration += effectDuration; - } } } return; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 58bd78315bb6c..149142e56ba3e 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -377,7 +377,6 @@ let shouldFireAfterActiveInstanceBlur: boolean = false; // Used to avoid traversing the return path to find the nearest Profiler ancestor during commit. let nearestProfilerOnStack: Fiber | null = null; -export let penultimateProfilerOnStack: Fiber | null = null; export function getWorkInProgressRoot(): FiberRoot | null { return workInProgressRoot; @@ -2368,11 +2367,10 @@ function commitLayoutEffects( ) { let fiber = firstChild; while (fiber !== null) { - let prevPenultimateProfiler = null; + let prevProfilerOnStack = null; if (enableProfilerTimer && enableProfilerCommitHooks) { if (fiber.tag === Profiler) { - prevPenultimateProfiler = penultimateProfilerOnStack; - penultimateProfilerOnStack = nearestProfilerOnStack; + prevProfilerOnStack = nearestProfilerOnStack; nearestProfilerOnStack = fiber; } } @@ -2409,8 +2407,14 @@ function commitLayoutEffects( if (enableProfilerTimer && enableProfilerCommitHooks) { if (fiber.tag === Profiler) { - nearestProfilerOnStack = penultimateProfilerOnStack; - penultimateProfilerOnStack = prevPenultimateProfiler; + // Propagate layout effect durations to the next nearest Profiler ancestor. + // Do not reset these values until the next render so DevTools has a chance to read them first. + if (prevProfilerOnStack !== null) { + prevProfilerOnStack.stateNode.effectDuration += + fiber.stateNode.effectDuration; + } + + nearestProfilerOnStack = prevProfilerOnStack; } } @@ -2475,11 +2479,10 @@ export function flushPassiveEffects(): boolean { function flushPassiveMountEffects(root, firstChild: Fiber): void { let fiber = firstChild; while (fiber !== null) { - let prevPenultimateProfiler = null; + let prevProfilerOnStack = null; if (enableProfilerTimer && enableProfilerCommitHooks) { if (fiber.tag === Profiler) { - prevPenultimateProfiler = penultimateProfilerOnStack; - penultimateProfilerOnStack = nearestProfilerOnStack; + prevProfilerOnStack = nearestProfilerOnStack; nearestProfilerOnStack = fiber; } } @@ -2516,8 +2519,14 @@ function flushPassiveMountEffects(root, firstChild: Fiber): void { if (enableProfilerTimer && enableProfilerCommitHooks) { if (fiber.tag === Profiler) { - nearestProfilerOnStack = penultimateProfilerOnStack; - penultimateProfilerOnStack = prevPenultimateProfiler; + // Bubble times to the next nearest ancestor Profiler. + // After we process that Profiler, we'll bubble further up. + if (prevProfilerOnStack !== null) { + prevProfilerOnStack.stateNode.passiveEffectDuration += + fiber.stateNode.passiveEffectDuration; + } + + nearestProfilerOnStack = prevProfilerOnStack; } } From 254914711acae1d02cf7c140d09cdf43086bbf2f Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 25 Sep 2020 10:30:30 -0400 Subject: [PATCH 3/6] Fixed incorrectly named commitPassiveUnmount/commitPassiveUnmountInsideDeletedTree functions --- packages/react-reconciler/src/ReactFiberCommitWork.new.js | 4 ++-- packages/react-reconciler/src/ReactFiberWorkLoop.new.js | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index a030b38c2024a..8f975bab91dfc 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1764,7 +1764,7 @@ function commitResetTextContent(current: Fiber): void { resetTextContent(current.stateNode); } -function commitPassiveUnmountInsideDeletedTree(finishedWork: Fiber): void { +function commitPassiveUnmount(finishedWork: Fiber): void { switch (finishedWork.tag) { case FunctionComponent: case ForwardRef: @@ -1794,7 +1794,7 @@ function commitPassiveUnmountInsideDeletedTree(finishedWork: Fiber): void { } } -function commitPassiveUnmount( +function commitPassiveUnmountInsideDeletedTree( current: Fiber, nearestMountedAncestor: Fiber | null, ): void { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 149142e56ba3e..109d6b4abd124 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2563,7 +2563,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { const primaryFlags = fiber.flags & Passive; if (primaryFlags !== NoFlags) { setCurrentDebugFiberInDEV(fiber); - commitPassiveUnmountInsideDeletedTreeOnFiber(fiber); + commitPassiveUnmountOnFiber(fiber); resetCurrentDebugFiberInDEV(); } @@ -2592,7 +2592,10 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree( if ((fiberToDelete.flags & PassiveStatic) !== NoFlags) { setCurrentDebugFiberInDEV(fiberToDelete); - commitPassiveUnmountOnFiber(fiberToDelete, nearestMountedAncestor); + commitPassiveUnmountInsideDeletedTreeOnFiber( + fiberToDelete, + nearestMountedAncestor, + ); resetCurrentDebugFiberInDEV(); } } From c76729f11a0f9eac849b37b2038e6c23471680b9 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 25 Sep 2020 12:55:45 -0400 Subject: [PATCH 4/6] Moved commit layout effects code from work loop to commit work Merged the three functions responsible for handling layout effects (commitLayoutEffects, commitLayoutEffectsImpl, and commitLifeCycles) into two functions (recursivelyCommitLayoutEffects and commitLayoutEffectsForFiber) and relocated them into the work loop. --- .../src/ReactFiberCommitWork.new.js | 601 +++++++++++------- .../src/ReactFiberWorkLoop.new.js | 102 +-- 2 files changed, 368 insertions(+), 335 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 8f975bab91dfc..2416dcc5b15b6 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -70,11 +70,16 @@ import { Snapshot, Update, Callback, + LayoutMask, PassiveMask, + Ref, } from './ReactFiberFlags'; import getComponentName from 'shared/getComponentName'; import invariant from 'shared/invariant'; - +import { + resetCurrentFiber as resetCurrentDebugFiberInDEV, + setCurrentFiber as setCurrentDebugFiberInDEV, +} from './ReactCurrentFiber'; import {onCommitUnmount} from './ReactFiberDevToolsHook.new'; import {resolveDefaultProps} from './ReactFiberLazyComponent.new'; import { @@ -130,6 +135,9 @@ import { } from './ReactHookEffectTags'; import {didWarnAboutReassigningProps} from './ReactFiberBeginWork.new'; +// Used to avoid traversing the return path to find the nearest Profiler ancestor during commit. +let nearestProfilerOnStack: Fiber | null = null; + let didWarnAboutUndefinedSnapshotBeforeUpdate: Set | null = null; if (__DEV__) { didWarnAboutUndefinedSnapshotBeforeUpdate = new Set(); @@ -432,98 +440,260 @@ function commitProfilerPassiveEffect( } } -function commitLifeCycles( - finishedRoot: FiberRoot, - current: Fiber | null, +function recursivelyCommitLayoutEffects( finishedWork: Fiber, + root: FiberRoot, committedLanes: Lanes, -): void { +) { switch (finishedWork.tag) { - case FunctionComponent: - case ForwardRef: - case SimpleMemoComponent: - case Block: { - // At this point layout effects have already been destroyed (during mutation phase). - // This is done to prevent sibling component effects from interfering with each other, - // e.g. a destroy function in one component should never override a ref set - // by a create function in another component during the same commit. - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); - commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); - } finally { - recordLayoutEffectDuration(finishedWork); + case Profiler: { + let prevProfilerOnStack = null; + if (enableProfilerTimer && enableProfilerCommitHooks) { + prevProfilerOnStack = nearestProfilerOnStack; + nearestProfilerOnStack = finishedWork; + } + + let child = finishedWork.child; + while (child !== null) { + const primaryFlags = child.flags & LayoutMask; + const primarySubtreeFlags = child.subtreeFlags & LayoutMask; + if (primaryFlags !== NoFlags || primarySubtreeFlags !== NoFlags) { + recursivelyCommitLayoutEffects(child, root, committedLanes); } + child = child.sibling; + } + + if (__DEV__) { + setCurrentDebugFiberInDEV(finishedWork); + invokeGuardedCallback( + null, + commitLayoutEffectsForFiber, + null, + finishedWork, + root, + committedLanes, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } + resetCurrentDebugFiberInDEV(); } else { - commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); + try { + commitLayoutEffectsForFiber(finishedWork, root, committedLanes); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } } - if ((finishedWork.subtreeFlags & PassiveMask) !== NoFlags) { - schedulePassiveEffectCallback(); + if (enableProfilerTimer && enableProfilerCommitHooks) { + // Propagate layout effect durations to the next nearest Profiler ancestor. + // Do not reset these values until the next render so DevTools has a chance to read them first. + if (prevProfilerOnStack !== null) { + prevProfilerOnStack.stateNode.effectDuration += + finishedWork.stateNode.effectDuration; + } + + nearestProfilerOnStack = prevProfilerOnStack; } - return; + break; } - case ClassComponent: { - const instance = finishedWork.stateNode; - if (finishedWork.flags & Update) { - if (current === null) { - // We could update instance props and state here, - // but instead we rely on them being set during last render. - // TODO: revisit this when we implement resuming. - if (__DEV__) { + + // case Offscreen: { + // TODO: Fast path to invoke all nested layout effects when Offscren goes from hidden to visible. + // break; + // } + + default: { + let child = finishedWork.child; + while (child !== null) { + const primaryFlags = child.flags & LayoutMask; + const primarySubtreeFlags = child.subtreeFlags & LayoutMask; + if (primaryFlags !== NoFlags || primarySubtreeFlags !== NoFlags) { + recursivelyCommitLayoutEffects(child, root, committedLanes); + } + child = child.sibling; + } + + if (__DEV__) { + setCurrentDebugFiberInDEV(finishedWork); + invokeGuardedCallback( + null, + commitLayoutEffectsForFiber, + null, + finishedWork, + root, + committedLanes, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + commitLayoutEffectsForFiber(finishedWork, root, committedLanes); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } + } + break; + } + } +} + +function commitLayoutEffectsForFiber( + finishedWork: Fiber, + finishedRoot: FiberRoot, + committedLanes: Lanes, +) { + const {alternate: current, flags, tag} = finishedWork; + + if (flags & (Update | Callback)) { + switch (tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: + case Block: { + // At this point layout effects have already been destroyed (during mutation phase). + // This is done to prevent sibling component effects from interfering with each other, + // e.g. a destroy function in one component should never override a ref set + // by a create function in another component during the same commit. + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + try { + startLayoutEffectTimer(); + commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); + } finally { + recordLayoutEffectDuration(finishedWork); + } + } else { + commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); + } + + if ((finishedWork.subtreeFlags & PassiveMask) !== NoFlags) { + schedulePassiveEffectCallback(); + } + break; + } + case ClassComponent: { + const instance = finishedWork.stateNode; + if (flags & Update) { + if (current === null) { + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. + if (__DEV__) { + if ( + finishedWork.type === finishedWork.elementType && + !didWarnAboutReassigningProps + ) { + if (instance.props !== finishedWork.memoizedProps) { + console.error( + 'Expected %s props to match memoized props before ' + + 'componentDidMount. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.props`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + if (instance.state !== finishedWork.memoizedState) { + console.error( + 'Expected %s state to match memoized state before ' + + 'componentDidMount. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.state`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + } + } if ( - finishedWork.type === finishedWork.elementType && - !didWarnAboutReassigningProps + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode ) { - if (instance.props !== finishedWork.memoizedProps) { - console.error( - 'Expected %s props to match memoized props before ' + - 'componentDidMount. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.props`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); + try { + startLayoutEffectTimer(); + instance.componentDidMount(); + } finally { + recordLayoutEffectDuration(finishedWork); } - if (instance.state !== finishedWork.memoizedState) { - console.error( - 'Expected %s state to match memoized state before ' + - 'componentDidMount. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.state`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); - } - } - } - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); + } else { instance.componentDidMount(); - } finally { - recordLayoutEffectDuration(finishedWork); } } else { - instance.componentDidMount(); + const prevProps = + finishedWork.elementType === finishedWork.type + ? current.memoizedProps + : resolveDefaultProps(finishedWork.type, current.memoizedProps); + const prevState = current.memoizedState; + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. + if (__DEV__) { + if ( + finishedWork.type === finishedWork.elementType && + !didWarnAboutReassigningProps + ) { + if (instance.props !== finishedWork.memoizedProps) { + console.error( + 'Expected %s props to match memoized props before ' + + 'componentDidUpdate. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.props`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + if (instance.state !== finishedWork.memoizedState) { + console.error( + 'Expected %s state to match memoized state before ' + + 'componentDidUpdate. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.state`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + } + } + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + try { + startLayoutEffectTimer(); + instance.componentDidUpdate( + prevProps, + prevState, + instance.__reactInternalSnapshotBeforeUpdate, + ); + } finally { + recordLayoutEffectDuration(finishedWork); + } + } else { + instance.componentDidUpdate( + prevProps, + prevState, + instance.__reactInternalSnapshotBeforeUpdate, + ); + } } - } else { - const prevProps = - finishedWork.elementType === finishedWork.type - ? current.memoizedProps - : resolveDefaultProps(finishedWork.type, current.memoizedProps); - const prevState = current.memoizedState; - // We could update instance props and state here, - // but instead we rely on them being set during last render. - // TODO: revisit this when we implement resuming. + } + + // TODO: I think this is now always non-null by the time it reaches the + // commit phase. Consider removing the type check. + const updateQueue: UpdateQueue< + *, + > | null = (finishedWork.updateQueue: any); + if (updateQueue !== null) { if (__DEV__) { if ( finishedWork.type === finishedWork.elementType && @@ -532,7 +702,7 @@ function commitLifeCycles( if (instance.props !== finishedWork.memoizedProps) { console.error( 'Expected %s props to match memoized props before ' + - 'componentDidUpdate. ' + + 'processing the update queue. ' + 'This might either be because of a bug in React, or because ' + 'a component reassigns its own `this.props`. ' + 'Please file an issue.', @@ -542,7 +712,7 @@ function commitLifeCycles( if (instance.state !== finishedWork.memoizedState) { console.error( 'Expected %s state to match memoized state before ' + - 'componentDidUpdate. ' + + 'processing the update queue. ' + 'This might either be because of a bug in React, or because ' + 'a component reassigns its own `this.state`. ' + 'Please file an issue.', @@ -551,196 +721,151 @@ function commitLifeCycles( } } } - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); - instance.componentDidUpdate( - prevProps, - prevState, - instance.__reactInternalSnapshotBeforeUpdate, - ); - } finally { - recordLayoutEffectDuration(finishedWork); - } - } else { - instance.componentDidUpdate( - prevProps, - prevState, - instance.__reactInternalSnapshotBeforeUpdate, - ); - } + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. + commitUpdateQueue(finishedWork, updateQueue, instance); } + break; } - - // TODO: I think this is now always non-null by the time it reaches the - // commit phase. Consider removing the type check. - const updateQueue: UpdateQueue< - *, - > | null = (finishedWork.updateQueue: any); - if (updateQueue !== null) { - if (__DEV__) { - if ( - finishedWork.type === finishedWork.elementType && - !didWarnAboutReassigningProps - ) { - if (instance.props !== finishedWork.memoizedProps) { - console.error( - 'Expected %s props to match memoized props before ' + - 'processing the update queue. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.props`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); - } - if (instance.state !== finishedWork.memoizedState) { - console.error( - 'Expected %s state to match memoized state before ' + - 'processing the update queue. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.state`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); + case HostRoot: { + // TODO: I think this is now always non-null by the time it reaches the + // commit phase. Consider removing the type check. + const updateQueue: UpdateQueue< + *, + > | null = (finishedWork.updateQueue: any); + if (updateQueue !== null) { + let instance = null; + if (finishedWork.child !== null) { + switch (finishedWork.child.tag) { + case HostComponent: + instance = getPublicInstance(finishedWork.child.stateNode); + break; + case ClassComponent: + instance = finishedWork.child.stateNode; + break; } } + commitUpdateQueue(finishedWork, updateQueue, instance); } - // We could update instance props and state here, - // but instead we rely on them being set during last render. - // TODO: revisit this when we implement resuming. - commitUpdateQueue(finishedWork, updateQueue, instance); + break; } - return; - } - case HostRoot: { - // TODO: I think this is now always non-null by the time it reaches the - // commit phase. Consider removing the type check. - const updateQueue: UpdateQueue< - *, - > | null = (finishedWork.updateQueue: any); - if (updateQueue !== null) { - let instance = null; - if (finishedWork.child !== null) { - switch (finishedWork.child.tag) { - case HostComponent: - instance = getPublicInstance(finishedWork.child.stateNode); - break; - case ClassComponent: - instance = finishedWork.child.stateNode; - break; - } + case HostComponent: { + const instance: Instance = finishedWork.stateNode; + + // Renderers may schedule work to be done after host components are mounted + // (eg DOM renderer may schedule auto-focus for inputs and form controls). + // These effects should only be committed when components are first mounted, + // aka when there is no current/alternate. + if (current === null && flags & Update) { + const type = finishedWork.type; + const props = finishedWork.memoizedProps; + commitMount(instance, type, props, finishedWork); } - commitUpdateQueue(finishedWork, updateQueue, instance); - } - return; - } - case HostComponent: { - const instance: Instance = finishedWork.stateNode; - // Renderers may schedule work to be done after host components are mounted - // (eg DOM renderer may schedule auto-focus for inputs and form controls). - // These effects should only be committed when components are first mounted, - // aka when there is no current/alternate. - if (current === null && finishedWork.flags & Update) { - const type = finishedWork.type; - const props = finishedWork.memoizedProps; - commitMount(instance, type, props, finishedWork); + break; } + case HostText: { + // We have no life-cycles associated with text. + break; + } + case HostPortal: { + // We have no life-cycles associated with portals. + break; + } + case Profiler: { + if (enableProfilerTimer) { + const {onCommit, onRender} = finishedWork.memoizedProps; + const {effectDuration} = finishedWork.stateNode; - return; - } - case HostText: { - // We have no life-cycles associated with text. - return; - } - case HostPortal: { - // We have no life-cycles associated with portals. - return; - } - case Profiler: { - if (enableProfilerTimer) { - const {onCommit, onRender} = finishedWork.memoizedProps; - const {effectDuration} = finishedWork.stateNode; - const flags = finishedWork.flags; - - const commitTime = getCommitTime(); - - const OnRenderFlag = Update; - const OnCommitFlag = Callback; + const commitTime = getCommitTime(); - if ( - (flags & OnRenderFlag) !== NoFlags && - typeof onRender === 'function' - ) { - if (enableSchedulerTracing) { - onRender( - finishedWork.memoizedProps.id, - current === null ? 'mount' : 'update', - finishedWork.actualDuration, - finishedWork.treeBaseDuration, - finishedWork.actualStartTime, - commitTime, - finishedRoot.memoizedInteractions, - ); - } else { - onRender( - finishedWork.memoizedProps.id, - current === null ? 'mount' : 'update', - finishedWork.actualDuration, - finishedWork.treeBaseDuration, - finishedWork.actualStartTime, - commitTime, - ); - } - } + const OnRenderFlag = Update; + const OnCommitFlag = Callback; - if (enableProfilerCommitHooks) { if ( - (flags & OnCommitFlag) !== NoFlags && - typeof onCommit === 'function' + (flags & OnRenderFlag) !== NoFlags && + typeof onRender === 'function' ) { if (enableSchedulerTracing) { - onCommit( + onRender( finishedWork.memoizedProps.id, current === null ? 'mount' : 'update', - effectDuration, + finishedWork.actualDuration, + finishedWork.treeBaseDuration, + finishedWork.actualStartTime, commitTime, finishedRoot.memoizedInteractions, ); } else { - onCommit( + onRender( finishedWork.memoizedProps.id, current === null ? 'mount' : 'update', - effectDuration, + finishedWork.actualDuration, + finishedWork.treeBaseDuration, + finishedWork.actualStartTime, commitTime, ); } } + + if (enableProfilerCommitHooks) { + if ( + (flags & OnCommitFlag) !== NoFlags && + typeof onCommit === 'function' + ) { + if (enableSchedulerTracing) { + onCommit( + finishedWork.memoizedProps.id, + current === null ? 'mount' : 'update', + effectDuration, + commitTime, + finishedRoot.memoizedInteractions, + ); + } else { + onCommit( + finishedWork.memoizedProps.id, + current === null ? 'mount' : 'update', + effectDuration, + commitTime, + ); + } + } + } } + break; } - return; + case SuspenseComponent: { + commitSuspenseHydrationCallbacks(finishedRoot, finishedWork); + break; + } + case SuspenseListComponent: + case IncompleteClassComponent: + case FundamentalComponent: + case ScopeComponent: + case OffscreenComponent: + case LegacyHiddenComponent: + break; + + default: + invariant( + false, + 'This unit of work tag should not have side-effects. This error is ' + + 'likely caused by a bug in React. Please file an issue.', + ); } - case SuspenseComponent: { - commitSuspenseHydrationCallbacks(finishedRoot, finishedWork); - return; + } + + if (enableScopeAPI) { + // TODO: This is a temporary solution that allowed us to transition away from React Flare on www. + if (flags & Ref && tag !== ScopeComponent) { + commitAttachRef(finishedWork); + } + } else { + if (flags & Ref) { + commitAttachRef(finishedWork); } - case SuspenseListComponent: - case IncompleteClassComponent: - case FundamentalComponent: - case ScopeComponent: - case OffscreenComponent: - case LegacyHiddenComponent: - return; } - invariant( - false, - 'This unit of work tag should not have side-effects. This error is ' + - 'likely caused by a bug in React. Please file an issue.', - ); } function hideOrUnhideAllChildren(finishedWork, isHidden) { @@ -1990,7 +2115,6 @@ export { commitPlacement, commitDeletion, commitWork, - commitLifeCycles, commitAttachRef, commitDetachRef, commitPassiveUnmount, @@ -2000,4 +2124,5 @@ export { invokeLayoutEffectUnmountInDEV, invokePassiveEffectMountInDEV, invokePassiveEffectUnmountInDEV, + recursivelyCommitLayoutEffects, }; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 109d6b4abd124..b4b2bfd11d5e7 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -128,7 +128,6 @@ import { Ref, ContentReset, Snapshot, - Callback, Passive, PassiveStatic, Incomplete, @@ -192,7 +191,6 @@ import { } from './ReactFiberThrow.new'; import { commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber, - commitLifeCycles as commitLayoutEffectOnFiber, commitPlacement, commitWork, commitDeletion, @@ -207,6 +205,7 @@ import { invokePassiveEffectMountInDEV, invokeLayoutEffectUnmountInDEV, invokePassiveEffectUnmountInDEV, + recursivelyCommitLayoutEffects, } from './ReactFiberCommitWork.new'; import {enqueueUpdate} from './ReactUpdateQueue.new'; import {resetContextDependencies} from './ReactFiberNewContext.new'; @@ -327,6 +326,9 @@ let workInProgressRootRenderTargetTime: number = Infinity; // suspense heuristics and opt out of rendering more content. const RENDER_TIMEOUT_MS = 500; +// Used to avoid traversing the return path to find the nearest Profiler ancestor during commit. +let nearestProfilerOnStack: Fiber | null = null; + function resetRenderTimer() { workInProgressRootRenderTargetTime = now() + RENDER_TIMEOUT_MS; } @@ -375,9 +377,6 @@ let isFlushingPassiveEffects = false; let focusedInstanceHandle: null | Fiber = null; let shouldFireAfterActiveInstanceBlur: boolean = false; -// Used to avoid traversing the return path to find the nearest Profiler ancestor during commit. -let nearestProfilerOnStack: Fiber | null = null; - export function getWorkInProgressRoot(): FiberRoot | null { return workInProgressRoot; } @@ -1942,7 +1941,7 @@ function commitRootImpl(root, renderPriorityLevel) { markLayoutEffectsStarted(lanes); } - commitLayoutEffects(finishedWork, root, lanes); + recursivelyCommitLayoutEffects(finishedWork, root, lanes); if (__DEV__) { if (enableDebugTracing) { @@ -2360,97 +2359,6 @@ export function schedulePassiveEffectCallback() { } } -function commitLayoutEffects( - firstChild: Fiber, - root: FiberRoot, - committedLanes: Lanes, -) { - let fiber = firstChild; - while (fiber !== null) { - let prevProfilerOnStack = null; - if (enableProfilerTimer && enableProfilerCommitHooks) { - if (fiber.tag === Profiler) { - prevProfilerOnStack = nearestProfilerOnStack; - nearestProfilerOnStack = fiber; - } - } - - if (fiber.child !== null) { - const primarySubtreeFlags = fiber.subtreeFlags & LayoutMask; - if (primarySubtreeFlags !== NoFlags) { - commitLayoutEffects(fiber.child, root, committedLanes); - } - } - - if (__DEV__) { - setCurrentDebugFiberInDEV(fiber); - invokeGuardedCallback( - null, - commitLayoutEffectsImpl, - null, - fiber, - root, - committedLanes, - ); - if (hasCaughtError()) { - const error = clearCaughtError(); - captureCommitPhaseError(fiber, fiber.return, error); - } - resetCurrentDebugFiberInDEV(); - } else { - try { - commitLayoutEffectsImpl(fiber, root, committedLanes); - } catch (error) { - captureCommitPhaseError(fiber, fiber.return, error); - } - } - - if (enableProfilerTimer && enableProfilerCommitHooks) { - if (fiber.tag === Profiler) { - // Propagate layout effect durations to the next nearest Profiler ancestor. - // Do not reset these values until the next render so DevTools has a chance to read them first. - if (prevProfilerOnStack !== null) { - prevProfilerOnStack.stateNode.effectDuration += - fiber.stateNode.effectDuration; - } - - nearestProfilerOnStack = prevProfilerOnStack; - } - } - - fiber = fiber.sibling; - } -} - -function commitLayoutEffectsImpl( - fiber: Fiber, - root: FiberRoot, - committedLanes: Lanes, -) { - const flags = fiber.flags; - - setCurrentDebugFiberInDEV(fiber); - - if (flags & (Update | Callback)) { - const current = fiber.alternate; - commitLayoutEffectOnFiber(root, current, fiber, committedLanes); - } - - if (enableScopeAPI) { - // TODO: This is a temporary solution that allowed us to transition away - // from React Flare on www. - if (flags & Ref && fiber.tag !== ScopeComponent) { - commitAttachRef(fiber); - } - } else { - if (flags & Ref) { - commitAttachRef(fiber); - } - } - - resetCurrentDebugFiberInDEV(); -} - export function flushPassiveEffects(): boolean { // Returns whether passive effects were flushed. if (pendingPassiveEffectsRenderPriority !== NoSchedulerPriority) { From 721c813e5a2454ee4cb90558871376ffb3b30495 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 29 Sep 2020 14:14:35 -0400 Subject: [PATCH 5/6] Lifted commit layout switch/case to the outer recursive function And split the per-tag-type logic into separate helper functions. This keeps all of the tag checks on the same level. --- .../src/ReactFiberCommitWork.new.js | 722 +++++++++--------- .../src/ReactFiberWorkLoop.new.js | 25 +- 2 files changed, 403 insertions(+), 344 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 2416dcc5b15b6..34a23650301bc 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -17,7 +17,6 @@ import type { } from './ReactFiberHostConfig'; import type {Fiber} from './ReactInternalTypes'; import type {FiberRoot} from './ReactInternalTypes'; -import type {Lanes} from './ReactFiberLane'; import type {SuspenseState} from './ReactFiberSuspenseComponent.new'; import type {UpdateQueue} from './ReactUpdateQueue.new'; import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new'; @@ -77,6 +76,7 @@ import { import getComponentName from 'shared/getComponentName'; import invariant from 'shared/invariant'; import { + current as currentDebugFiberInDEV, resetCurrentFiber as resetCurrentDebugFiberInDEV, setCurrentFiber as setCurrentDebugFiberInDEV, } from './ReactCurrentFiber'; @@ -442,10 +442,10 @@ function commitProfilerPassiveEffect( function recursivelyCommitLayoutEffects( finishedWork: Fiber, - root: FiberRoot, - committedLanes: Lanes, + finishedRoot: FiberRoot, ) { - switch (finishedWork.tag) { + const {flags, tag} = finishedWork; + switch (tag) { case Profiler: { let prevProfilerOnStack = null; if (enableProfilerTimer && enableProfilerCommitHooks) { @@ -458,31 +458,65 @@ function recursivelyCommitLayoutEffects( const primaryFlags = child.flags & LayoutMask; const primarySubtreeFlags = child.subtreeFlags & LayoutMask; if (primaryFlags !== NoFlags || primarySubtreeFlags !== NoFlags) { - recursivelyCommitLayoutEffects(child, root, committedLanes); + if (__DEV__) { + const prevCurrentFiberInDEV = currentDebugFiberInDEV; + setCurrentDebugFiberInDEV(child); + invokeGuardedCallback( + null, + recursivelyCommitLayoutEffects, + null, + child, + finishedRoot, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(child, finishedWork, error); + } + if (prevCurrentFiberInDEV !== null) { + setCurrentDebugFiberInDEV(prevCurrentFiberInDEV); + } else { + resetCurrentDebugFiberInDEV(); + } + } else { + try { + recursivelyCommitLayoutEffects(child, finishedRoot); + } catch (error) { + captureCommitPhaseError(child, finishedWork, error); + } + } } child = child.sibling; } - if (__DEV__) { - setCurrentDebugFiberInDEV(finishedWork); - invokeGuardedCallback( - null, - commitLayoutEffectsForFiber, - null, - finishedWork, - root, - committedLanes, - ); - if (hasCaughtError()) { - const error = clearCaughtError(); - captureCommitPhaseError(finishedWork, finishedWork.return, error); - } - resetCurrentDebugFiberInDEV(); - } else { - try { - commitLayoutEffectsForFiber(finishedWork, root, committedLanes); - } catch (error) { - captureCommitPhaseError(finishedWork, finishedWork.return, error); + const primaryFlags = flags & (Update | Callback); + if (primaryFlags !== NoFlags) { + if (enableProfilerTimer) { + if (__DEV__) { + const prevCurrentFiberInDEV = currentDebugFiberInDEV; + setCurrentDebugFiberInDEV(finishedWork); + invokeGuardedCallback( + null, + commitLayoutEffectsForProfiler, + null, + finishedWork, + finishedRoot, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } + if (prevCurrentFiberInDEV !== null) { + setCurrentDebugFiberInDEV(prevCurrentFiberInDEV); + } else { + resetCurrentDebugFiberInDEV(); + } + } else { + try { + commitLayoutEffectsForProfiler(finishedWork, finishedRoot); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } + } } } @@ -510,159 +544,43 @@ function recursivelyCommitLayoutEffects( const primaryFlags = child.flags & LayoutMask; const primarySubtreeFlags = child.subtreeFlags & LayoutMask; if (primaryFlags !== NoFlags || primarySubtreeFlags !== NoFlags) { - recursivelyCommitLayoutEffects(child, root, committedLanes); - } - child = child.sibling; - } - - if (__DEV__) { - setCurrentDebugFiberInDEV(finishedWork); - invokeGuardedCallback( - null, - commitLayoutEffectsForFiber, - null, - finishedWork, - root, - committedLanes, - ); - if (hasCaughtError()) { - const error = clearCaughtError(); - captureCommitPhaseError(finishedWork, finishedWork.return, error); - } - resetCurrentDebugFiberInDEV(); - } else { - try { - commitLayoutEffectsForFiber(finishedWork, root, committedLanes); - } catch (error) { - captureCommitPhaseError(finishedWork, finishedWork.return, error); - } - } - break; - } - } -} - -function commitLayoutEffectsForFiber( - finishedWork: Fiber, - finishedRoot: FiberRoot, - committedLanes: Lanes, -) { - const {alternate: current, flags, tag} = finishedWork; - - if (flags & (Update | Callback)) { - switch (tag) { - case FunctionComponent: - case ForwardRef: - case SimpleMemoComponent: - case Block: { - // At this point layout effects have already been destroyed (during mutation phase). - // This is done to prevent sibling component effects from interfering with each other, - // e.g. a destroy function in one component should never override a ref set - // by a create function in another component during the same commit. - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); - commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); - } finally { - recordLayoutEffectDuration(finishedWork); - } - } else { - commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); - } - - if ((finishedWork.subtreeFlags & PassiveMask) !== NoFlags) { - schedulePassiveEffectCallback(); - } - break; - } - case ClassComponent: { - const instance = finishedWork.stateNode; - if (flags & Update) { - if (current === null) { - // We could update instance props and state here, - // but instead we rely on them being set during last render. - // TODO: revisit this when we implement resuming. - if (__DEV__) { - if ( - finishedWork.type === finishedWork.elementType && - !didWarnAboutReassigningProps - ) { - if (instance.props !== finishedWork.memoizedProps) { - console.error( - 'Expected %s props to match memoized props before ' + - 'componentDidMount. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.props`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); - } - if (instance.state !== finishedWork.memoizedState) { - console.error( - 'Expected %s state to match memoized state before ' + - 'componentDidMount. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.state`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); - } - } + if (__DEV__) { + const prevCurrentFiberInDEV = currentDebugFiberInDEV; + setCurrentDebugFiberInDEV(child); + invokeGuardedCallback( + null, + recursivelyCommitLayoutEffects, + null, + child, + finishedRoot, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(child, finishedWork, error); } - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); - instance.componentDidMount(); - } finally { - recordLayoutEffectDuration(finishedWork); - } + if (prevCurrentFiberInDEV !== null) { + setCurrentDebugFiberInDEV(prevCurrentFiberInDEV); } else { - instance.componentDidMount(); + resetCurrentDebugFiberInDEV(); } } else { - const prevProps = - finishedWork.elementType === finishedWork.type - ? current.memoizedProps - : resolveDefaultProps(finishedWork.type, current.memoizedProps); - const prevState = current.memoizedState; - // We could update instance props and state here, - // but instead we rely on them being set during last render. - // TODO: revisit this when we implement resuming. - if (__DEV__) { - if ( - finishedWork.type === finishedWork.elementType && - !didWarnAboutReassigningProps - ) { - if (instance.props !== finishedWork.memoizedProps) { - console.error( - 'Expected %s props to match memoized props before ' + - 'componentDidUpdate. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.props`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); - } - if (instance.state !== finishedWork.memoizedState) { - console.error( - 'Expected %s state to match memoized state before ' + - 'componentDidUpdate. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.state`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); - } - } + try { + recursivelyCommitLayoutEffects(child, finishedRoot); + } catch (error) { + captureCommitPhaseError(child, finishedWork, error); } + } + } + child = child.sibling; + } + + const primaryFlags = flags & (Update | Callback); + if (primaryFlags !== NoFlags) { + switch (tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: + case Block: { if ( enableProfilerTimer && enableProfilerCommitHooks && @@ -670,189 +588,63 @@ function commitLayoutEffectsForFiber( ) { try { startLayoutEffectTimer(); - instance.componentDidUpdate( - prevProps, - prevState, - instance.__reactInternalSnapshotBeforeUpdate, + commitHookEffectListMount( + HookLayout | HookHasEffect, + finishedWork, ); } finally { recordLayoutEffectDuration(finishedWork); } } else { - instance.componentDidUpdate( - prevProps, - prevState, - instance.__reactInternalSnapshotBeforeUpdate, + commitHookEffectListMount( + HookLayout | HookHasEffect, + finishedWork, ); } - } - } - // TODO: I think this is now always non-null by the time it reaches the - // commit phase. Consider removing the type check. - const updateQueue: UpdateQueue< - *, - > | null = (finishedWork.updateQueue: any); - if (updateQueue !== null) { - if (__DEV__) { - if ( - finishedWork.type === finishedWork.elementType && - !didWarnAboutReassigningProps - ) { - if (instance.props !== finishedWork.memoizedProps) { - console.error( - 'Expected %s props to match memoized props before ' + - 'processing the update queue. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.props`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); - } - if (instance.state !== finishedWork.memoizedState) { - console.error( - 'Expected %s state to match memoized state before ' + - 'processing the update queue. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.state`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); - } + if ((finishedWork.subtreeFlags & PassiveMask) !== NoFlags) { + schedulePassiveEffectCallback(); } + break; } - // We could update instance props and state here, - // but instead we rely on them being set during last render. - // TODO: revisit this when we implement resuming. - commitUpdateQueue(finishedWork, updateQueue, instance); - } - break; - } - case HostRoot: { - // TODO: I think this is now always non-null by the time it reaches the - // commit phase. Consider removing the type check. - const updateQueue: UpdateQueue< - *, - > | null = (finishedWork.updateQueue: any); - if (updateQueue !== null) { - let instance = null; - if (finishedWork.child !== null) { - switch (finishedWork.child.tag) { - case HostComponent: - instance = getPublicInstance(finishedWork.child.stateNode); - break; - case ClassComponent: - instance = finishedWork.child.stateNode; - break; - } + case ClassComponent: { + // NOTE: Layout effect durations are measured within this function. + commitLayoutEffectsForClassComponent(finishedWork); + break; } - commitUpdateQueue(finishedWork, updateQueue, instance); - } - break; - } - case HostComponent: { - const instance: Instance = finishedWork.stateNode; - - // Renderers may schedule work to be done after host components are mounted - // (eg DOM renderer may schedule auto-focus for inputs and form controls). - // These effects should only be committed when components are first mounted, - // aka when there is no current/alternate. - if (current === null && flags & Update) { - const type = finishedWork.type; - const props = finishedWork.memoizedProps; - commitMount(instance, type, props, finishedWork); - } - - break; - } - case HostText: { - // We have no life-cycles associated with text. - break; - } - case HostPortal: { - // We have no life-cycles associated with portals. - break; - } - case Profiler: { - if (enableProfilerTimer) { - const {onCommit, onRender} = finishedWork.memoizedProps; - const {effectDuration} = finishedWork.stateNode; - - const commitTime = getCommitTime(); - - const OnRenderFlag = Update; - const OnCommitFlag = Callback; - - if ( - (flags & OnRenderFlag) !== NoFlags && - typeof onRender === 'function' - ) { - if (enableSchedulerTracing) { - onRender( - finishedWork.memoizedProps.id, - current === null ? 'mount' : 'update', - finishedWork.actualDuration, - finishedWork.treeBaseDuration, - finishedWork.actualStartTime, - commitTime, - finishedRoot.memoizedInteractions, - ); - } else { - onRender( - finishedWork.memoizedProps.id, - current === null ? 'mount' : 'update', - finishedWork.actualDuration, - finishedWork.treeBaseDuration, - finishedWork.actualStartTime, - commitTime, - ); - } + case HostRoot: { + commitLayoutEffectsForHostRoot(finishedWork); + break; } - - if (enableProfilerCommitHooks) { - if ( - (flags & OnCommitFlag) !== NoFlags && - typeof onCommit === 'function' - ) { - if (enableSchedulerTracing) { - onCommit( - finishedWork.memoizedProps.id, - current === null ? 'mount' : 'update', - effectDuration, - commitTime, - finishedRoot.memoizedInteractions, - ); - } else { - onCommit( - finishedWork.memoizedProps.id, - current === null ? 'mount' : 'update', - effectDuration, - commitTime, - ); - } - } + case HostComponent: { + commitLayoutEffectsForHostComponent(finishedWork); + break; + } + case SuspenseComponent: { + commitSuspenseHydrationCallbacks(finishedRoot, finishedWork); + break; + } + case FundamentalComponent: + case HostPortal: + case HostText: + case IncompleteClassComponent: + case LegacyHiddenComponent: + case OffscreenComponent: + case ScopeComponent: + case SuspenseListComponent: { + // We have no life-cycles associated with these component types. + break; + } + default: { + invariant( + false, + 'This unit of work tag should not have side-effects. This error is ' + + 'likely caused by a bug in React. Please file an issue.', + ); } } - break; - } - case SuspenseComponent: { - commitSuspenseHydrationCallbacks(finishedRoot, finishedWork); - break; } - case SuspenseListComponent: - case IncompleteClassComponent: - case FundamentalComponent: - case ScopeComponent: - case OffscreenComponent: - case LegacyHiddenComponent: - break; - - default: - invariant( - false, - 'This unit of work tag should not have side-effects. This error is ' + - 'likely caused by a bug in React. Please file an issue.', - ); + break; } } @@ -868,6 +660,254 @@ function commitLayoutEffectsForFiber( } } +function commitLayoutEffectsForProfiler( + finishedWork: Fiber, + finishedRoot: FiberRoot, +) { + if (enableProfilerTimer) { + const flags = finishedWork.flags; + const current = finishedWork.alternate; + + const {onCommit, onRender} = finishedWork.memoizedProps; + const {effectDuration} = finishedWork.stateNode; + + const commitTime = getCommitTime(); + + const OnRenderFlag = Update; + const OnCommitFlag = Callback; + + if ((flags & OnRenderFlag) !== NoFlags && typeof onRender === 'function') { + if (enableSchedulerTracing) { + onRender( + finishedWork.memoizedProps.id, + current === null ? 'mount' : 'update', + finishedWork.actualDuration, + finishedWork.treeBaseDuration, + finishedWork.actualStartTime, + commitTime, + finishedRoot.memoizedInteractions, + ); + } else { + onRender( + finishedWork.memoizedProps.id, + current === null ? 'mount' : 'update', + finishedWork.actualDuration, + finishedWork.treeBaseDuration, + finishedWork.actualStartTime, + commitTime, + ); + } + } + + if (enableProfilerCommitHooks) { + if ( + (flags & OnCommitFlag) !== NoFlags && + typeof onCommit === 'function' + ) { + if (enableSchedulerTracing) { + onCommit( + finishedWork.memoizedProps.id, + current === null ? 'mount' : 'update', + effectDuration, + commitTime, + finishedRoot.memoizedInteractions, + ); + } else { + onCommit( + finishedWork.memoizedProps.id, + current === null ? 'mount' : 'update', + effectDuration, + commitTime, + ); + } + } + } + } +} + +function commitLayoutEffectsForClassComponent(finishedWork: Fiber) { + const instance = finishedWork.stateNode; + const current = finishedWork.alternate; + if (finishedWork.flags & Update) { + if (current === null) { + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. + if (__DEV__) { + if ( + finishedWork.type === finishedWork.elementType && + !didWarnAboutReassigningProps + ) { + if (instance.props !== finishedWork.memoizedProps) { + console.error( + 'Expected %s props to match memoized props before ' + + 'componentDidMount. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.props`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + if (instance.state !== finishedWork.memoizedState) { + console.error( + 'Expected %s state to match memoized state before ' + + 'componentDidMount. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.state`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + } + } + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + try { + startLayoutEffectTimer(); + instance.componentDidMount(); + } finally { + recordLayoutEffectDuration(finishedWork); + } + } else { + instance.componentDidMount(); + } + } else { + const prevProps = + finishedWork.elementType === finishedWork.type + ? current.memoizedProps + : resolveDefaultProps(finishedWork.type, current.memoizedProps); + const prevState = current.memoizedState; + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. + if (__DEV__) { + if ( + finishedWork.type === finishedWork.elementType && + !didWarnAboutReassigningProps + ) { + if (instance.props !== finishedWork.memoizedProps) { + console.error( + 'Expected %s props to match memoized props before ' + + 'componentDidUpdate. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.props`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + if (instance.state !== finishedWork.memoizedState) { + console.error( + 'Expected %s state to match memoized state before ' + + 'componentDidUpdate. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.state`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + } + } + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + try { + startLayoutEffectTimer(); + instance.componentDidUpdate( + prevProps, + prevState, + instance.__reactInternalSnapshotBeforeUpdate, + ); + } finally { + recordLayoutEffectDuration(finishedWork); + } + } else { + instance.componentDidUpdate( + prevProps, + prevState, + instance.__reactInternalSnapshotBeforeUpdate, + ); + } + } + } + + // TODO: I think this is now always non-null by the time it reaches the + // commit phase. Consider removing the type check. + const updateQueue: UpdateQueue<*> | null = (finishedWork.updateQueue: any); + if (updateQueue !== null) { + if (__DEV__) { + if ( + finishedWork.type === finishedWork.elementType && + !didWarnAboutReassigningProps + ) { + if (instance.props !== finishedWork.memoizedProps) { + console.error( + 'Expected %s props to match memoized props before ' + + 'processing the update queue. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.props`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + if (instance.state !== finishedWork.memoizedState) { + console.error( + 'Expected %s state to match memoized state before ' + + 'processing the update queue. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.state`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + } + } + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. + commitUpdateQueue(finishedWork, updateQueue, instance); + } +} + +function commitLayoutEffectsForHostRoot(finishedWork: Fiber) { + // TODO: I think this is now always non-null by the time it reaches the + // commit phase. Consider removing the type check. + const updateQueue: UpdateQueue<*> | null = (finishedWork.updateQueue: any); + if (updateQueue !== null) { + let instance = null; + if (finishedWork.child !== null) { + switch (finishedWork.child.tag) { + case HostComponent: + instance = getPublicInstance(finishedWork.child.stateNode); + break; + case ClassComponent: + instance = finishedWork.child.stateNode; + break; + } + } + commitUpdateQueue(finishedWork, updateQueue, instance); + } +} + +function commitLayoutEffectsForHostComponent(finishedWork: Fiber) { + const instance: Instance = finishedWork.stateNode; + const current = finishedWork.alternate; + + // Renderers may schedule work to be done after host components are mounted + // (eg DOM renderer may schedule auto-focus for inputs and form controls). + // These effects should only be committed when components are first mounted, + // aka when there is no current/alternate. + if (current === null && finishedWork.flags & Update) { + const type = finishedWork.type; + const props = finishedWork.memoizedProps; + commitMount(instance, type, props, finishedWork); + } +} + function hideOrUnhideAllChildren(finishedWork, isHidden) { if (supportsMutation) { // We only have the top Fiber that was inserted but we need to recurse down its diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index b4b2bfd11d5e7..4f40bde182310 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1941,7 +1941,27 @@ function commitRootImpl(root, renderPriorityLevel) { markLayoutEffectsStarted(lanes); } - recursivelyCommitLayoutEffects(finishedWork, root, lanes); + if (__DEV__) { + setCurrentDebugFiberInDEV(finishedWork); + invokeGuardedCallback( + null, + recursivelyCommitLayoutEffects, + null, + finishedWork, + root, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseErrorOnRoot(finishedWork, finishedWork, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + recursivelyCommitLayoutEffects(finishedWork, root); + } catch (error) { + captureCommitPhaseErrorOnRoot(finishedWork, finishedWork, error); + } + } if (__DEV__) { if (enableDebugTracing) { @@ -2257,8 +2277,7 @@ function commitMutationEffectsImpl( commitDetachRef(current); } if (enableScopeAPI) { - // TODO: This is a temporary solution that allowed us to transition away - // from React Flare on www. + // TODO: This is a temporary solution that allowed us to transition away from React Flare on www. if (fiber.tag === ScopeComponent) { commitAttachRef(fiber); } From fbb628bbbe0cb19c9d982c2cc8eac31687a1f105 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 29 Sep 2020 15:28:57 -0400 Subject: [PATCH 6/6] Fixed the issue with the Suspense fuzz tester --- .../src/ReactFiberCommitWork.new.js | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 34a23650301bc..d8843eb0ca0d4 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -455,9 +455,8 @@ function recursivelyCommitLayoutEffects( let child = finishedWork.child; while (child !== null) { - const primaryFlags = child.flags & LayoutMask; - const primarySubtreeFlags = child.subtreeFlags & LayoutMask; - if (primaryFlags !== NoFlags || primarySubtreeFlags !== NoFlags) { + const primarySubtreeFlags = finishedWork.subtreeFlags & LayoutMask; + if (primarySubtreeFlags !== NoFlags) { if (__DEV__) { const prevCurrentFiberInDEV = currentDebugFiberInDEV; setCurrentDebugFiberInDEV(child); @@ -541,9 +540,8 @@ function recursivelyCommitLayoutEffects( default: { let child = finishedWork.child; while (child !== null) { - const primaryFlags = child.flags & LayoutMask; - const primarySubtreeFlags = child.subtreeFlags & LayoutMask; - if (primaryFlags !== NoFlags || primarySubtreeFlags !== NoFlags) { + const primarySubtreeFlags = finishedWork.subtreeFlags & LayoutMask; + if (primarySubtreeFlags !== NoFlags) { if (__DEV__) { const prevCurrentFiberInDEV = currentDebugFiberInDEV; setCurrentDebugFiberInDEV(child); @@ -644,18 +642,18 @@ function recursivelyCommitLayoutEffects( } } } - break; - } - } - if (enableScopeAPI) { - // TODO: This is a temporary solution that allowed us to transition away from React Flare on www. - if (flags & Ref && tag !== ScopeComponent) { - commitAttachRef(finishedWork); - } - } else { - if (flags & Ref) { - commitAttachRef(finishedWork); + if (enableScopeAPI) { + // TODO: This is a temporary solution that allowed us to transition away from React Flare on www. + if (flags & Ref && tag !== ScopeComponent) { + commitAttachRef(finishedWork); + } + } else { + if (flags & Ref) { + commitAttachRef(finishedWork); + } + } + break; } } }