Skip to content

Commit

Permalink
Consolidate commit phase hook functions (#19864)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@acdlite
acdlite authored Sep 22, 2020
1 parent c91c1c4 commit 7355bf5
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 141 deletions.
193 changes: 60 additions & 133 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
}
Expand All @@ -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 {
Expand All @@ -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: {
Expand All @@ -1956,6 +1883,6 @@ export {
commitAttachRef,
commitDetachRef,
commitPassiveUnmount,
commitPassiveWork,
commitPassiveLifeCycles,
commitPassiveUnmountInsideDeletedTree,
commitPassiveMount,
};
34 changes: 26 additions & 8 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -2496,7 +2514,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
const primaryFlags = fiber.flags & Passive;
if (primaryFlags !== NoFlags) {
setCurrentDebugFiberInDEV(fiber);
commitPassiveWork(fiber);
commitPassiveUnmountInsideDeletedTreeOnFiber(fiber);
resetCurrentDebugFiberInDEV();
}

Expand Down Expand Up @@ -2525,7 +2543,7 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree(

if ((fiberToDelete.flags & PassiveStatic) !== NoFlags) {
setCurrentDebugFiberInDEV(fiberToDelete);
commitPassiveUnmount(fiberToDelete, nearestMountedAncestor);
commitPassiveUnmountOnFiber(fiberToDelete, nearestMountedAncestor);
resetCurrentDebugFiberInDEV();
}
}
Expand Down

0 comments on commit 7355bf5

Please sign in to comment.