Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PR Stack] Refactor mutation phase to use recursion #24308

Merged
merged 10 commits into from
Apr 8, 2022
Merged

[PR Stack] Refactor mutation phase to use recursion #24308

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ea7b2ec
Remove wrong return pointer warning
acdlite Apr 7, 2022
12d7a9a
Combine commitWork into single switch statement
acdlite Apr 7, 2022
e66e7a0
Inline commitWork into commitMutationOnFiber
acdlite Apr 7, 2022
54b5b32
Move Update flag check into each switch case
acdlite Apr 7, 2022
c99c5f1
Move ad hoc flag checks into main switch statement
acdlite Apr 8, 2022
bcc1b31
Move reportUncaughtErrorInDev to captureCommitPhaseError
acdlite Apr 8, 2022
f9e6aef
Wrap try-catch directly around each user function
acdlite Apr 8, 2022
481dece
Use recursion to traverse during mutation phase
acdlite Apr 8, 2022
46db4e9
Combine deletion phase into single recursive function
acdlite Apr 7, 2022
ec52a56
Fix: Don't call cWU if already unmounted
acdlite Apr 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Fix: Don't call cWU if already unmounted 
When a tree goes offscreen, we unmount all the effects just like we
would in a normal deletion. (Conceptually it _is_ a deletion; we keep
the fiber around so we can reuse its state if the tree mounts again.)

If an offscreen component gets deleted "for real", we shouldn't unmount
it again.

The fix is to track on the stack whether we're inside a hidden tree.

We already had a stack variable for this purpose, called
`offscreenSubtreeWasHidden`, in another part of the commit phase, so I
reused that variable instead of creating a new one. (The name is a bit
confusing: "was" refers to the current tree before this commit. So, the
"previous current".)

Co-authored-by: dan <dan.abramov@me.com>
  • Loading branch information
@acdlite @gaearon
acdlite and gaearon committed Apr 8, 2022
commit ec52a5698e2dfea7050a0b015f0b79abfb2d81b7
135 changes: 85 additions & 50 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1604,7 +1604,9 @@ function commitDeletionEffectsOnFiber(
// that don't modify the stack.
switch (deletedFiber.tag) {
case HostComponent: {
safelyDetachRef(deletedFiber, nearestMountedAncestor);
if (!offscreenSubtreeWasHidden) {
safelyDetachRef(deletedFiber, nearestMountedAncestor);
}
// Intentional fallthrough to next branch
}
// eslint-disable-next-line-no-fallthrough
Expand Down Expand Up @@ -1710,54 +1712,56 @@ function commitDeletionEffectsOnFiber(
case ForwardRef:
case MemoComponent:
case SimpleMemoComponent: {
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
if (updateQueue !== null) {
const lastEffect = updateQueue.lastEffect;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;

let effect = firstEffect;
do {
const {destroy, tag} = effect;
if (destroy !== undefined) {
if ((tag & HookInsertion) !== NoHookEffect) {
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
} else if ((tag & HookLayout) !== NoHookEffect) {
if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStarted(deletedFiber);
}

if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
deletedFiber.mode & ProfileMode
) {
startLayoutEffectTimer();
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
recordLayoutEffectDuration(deletedFiber);
} else {
if (!offscreenSubtreeWasHidden) {
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
if (updateQueue !== null) {
const lastEffect = updateQueue.lastEffect;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;

let effect = firstEffect;
do {
const {destroy, tag} = effect;
if (destroy !== undefined) {
if ((tag & HookInsertion) !== NoHookEffect) {
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
}
} else if ((tag & HookLayout) !== NoHookEffect) {
if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStarted(deletedFiber);
}

if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStopped();
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
deletedFiber.mode & ProfileMode
) {
startLayoutEffectTimer();
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
recordLayoutEffectDuration(deletedFiber);
} else {
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
}

if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStopped();
}
}
}
}
effect = effect.next;
} while (effect !== firstEffect);
effect = effect.next;
} while (effect !== firstEffect);
}
}
}

Expand All @@ -1769,14 +1773,16 @@ function commitDeletionEffectsOnFiber(
return;
}
case ClassComponent: {
safelyDetachRef(deletedFiber, nearestMountedAncestor);
const instance = deletedFiber.stateNode;
if (typeof instance.componentWillUnmount === 'function') {
safelyCallComponentWillUnmount(
deletedFiber,
nearestMountedAncestor,
instance,
);
if (!offscreenSubtreeWasHidden) {
safelyDetachRef(deletedFiber, nearestMountedAncestor);
const instance = deletedFiber.stateNode;
if (typeof instance.componentWillUnmount === 'function') {
safelyCallComponentWillUnmount(
deletedFiber,
nearestMountedAncestor,
instance,
);
}
}
recursivelyTraverseDeletionEffects(
finishedRoot,
Expand All @@ -1796,6 +1802,27 @@ function commitDeletionEffectsOnFiber(
);
return;
}
case OffscreenComponent: {
// If this offscreen component is hidden, we already unmounted it. Before
// deleting the children, track that it's already unmounted so that we
// don't attempt to unmount the effects again.
// TODO: If the tree is hidden, in most cases we should be able to skip
// over the nested children entirely. An exception is we haven't yet found
// the topmost host node to delete, which we already track on the stack.
// But the other case is portals, which need to be detached no matter how
// deeply they are nested. We should use a subtree flag to track whether a
// subtree includes a nested portal.
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
offscreenSubtreeWasHidden =
prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
recursivelyTraverseDeletionEffects(
finishedRoot,
nearestMountedAncestor,
deletedFiber,
);
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
break;
}
default: {
recursivelyTraverseDeletionEffects(
finishedRoot,
Expand Down Expand Up @@ -2203,13 +2230,21 @@ function commitMutationEffectsOnFiber(
return;
}
case OffscreenComponent: {
const wasHidden = current !== null && current.memoizedState !== null;

// Before committing the children, track on the stack whether this
// offscreen subtree was already hidden, so that we don't unmount the
// effects again.
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
recursivelyTraverseMutationEffects(root, finishedWork, lanes);
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;

commitReconciliationEffects(finishedWork);

if (flags & Visibility) {
const newState: OffscreenState | null = finishedWork.memoizedState;
const isHidden = newState !== null;
const wasHidden = current !== null && current.memoizedState !== null;
const offscreenBoundary: Fiber = finishedWork;

if (supportsMutation) {
Expand Down
135 changes: 85 additions & 50 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1604,7 +1604,9 @@ function commitDeletionEffectsOnFiber(
// that don't modify the stack.
switch (deletedFiber.tag) {
case HostComponent: {
safelyDetachRef(deletedFiber, nearestMountedAncestor);
if (!offscreenSubtreeWasHidden) {
safelyDetachRef(deletedFiber, nearestMountedAncestor);
}
// Intentional fallthrough to next branch
}
// eslint-disable-next-line-no-fallthrough
Expand Down Expand Up @@ -1710,54 +1712,56 @@ function commitDeletionEffectsOnFiber(
case ForwardRef:
case MemoComponent:
case SimpleMemoComponent: {
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
if (updateQueue !== null) {
const lastEffect = updateQueue.lastEffect;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;

let effect = firstEffect;
do {
const {destroy, tag} = effect;
if (destroy !== undefined) {
if ((tag & HookInsertion) !== NoHookEffect) {
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
} else if ((tag & HookLayout) !== NoHookEffect) {
if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStarted(deletedFiber);
}

if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
deletedFiber.mode & ProfileMode
) {
startLayoutEffectTimer();
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
recordLayoutEffectDuration(deletedFiber);
} else {
if (!offscreenSubtreeWasHidden) {
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
if (updateQueue !== null) {
const lastEffect = updateQueue.lastEffect;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;

let effect = firstEffect;
do {
const {destroy, tag} = effect;
if (destroy !== undefined) {
if ((tag & HookInsertion) !== NoHookEffect) {
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
}
} else if ((tag & HookLayout) !== NoHookEffect) {
if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStarted(deletedFiber);
}

if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStopped();
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
deletedFiber.mode & ProfileMode
) {
startLayoutEffectTimer();
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
recordLayoutEffectDuration(deletedFiber);
} else {
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
}

if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStopped();
}
}
}
}
effect = effect.next;
} while (effect !== firstEffect);
effect = effect.next;
} while (effect !== firstEffect);
}
}
}

Expand All @@ -1769,14 +1773,16 @@ function commitDeletionEffectsOnFiber(
return;
}
case ClassComponent: {
safelyDetachRef(deletedFiber, nearestMountedAncestor);
const instance = deletedFiber.stateNode;
if (typeof instance.componentWillUnmount === 'function') {
safelyCallComponentWillUnmount(
deletedFiber,
nearestMountedAncestor,
instance,
);
if (!offscreenSubtreeWasHidden) {
safelyDetachRef(deletedFiber, nearestMountedAncestor);
const instance = deletedFiber.stateNode;
if (typeof instance.componentWillUnmount === 'function') {
safelyCallComponentWillUnmount(
deletedFiber,
nearestMountedAncestor,
instance,
);
}
}
recursivelyTraverseDeletionEffects(
finishedRoot,
Expand All @@ -1796,6 +1802,27 @@ function commitDeletionEffectsOnFiber(
);
return;
}
case OffscreenComponent: {
// If this offscreen component is hidden, we already unmounted it. Before
// deleting the children, track that it's already unmounted so that we
// don't attempt to unmount the effects again.
// TODO: If the tree is hidden, in most cases we should be able to skip
// over the nested children entirely. An exception is we haven't yet found
// the topmost host node to delete, which we already track on the stack.
// But the other case is portals, which need to be detached no matter how
// deeply they are nested. We should use a subtree flag to track whether a
// subtree includes a nested portal.
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
offscreenSubtreeWasHidden =
prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
recursivelyTraverseDeletionEffects(
finishedRoot,
nearestMountedAncestor,
deletedFiber,
);
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
break;
}
default: {
recursivelyTraverseDeletionEffects(
finishedRoot,
Expand Down Expand Up @@ -2203,13 +2230,21 @@ function commitMutationEffectsOnFiber(
return;
}
case OffscreenComponent: {
gaearon marked this conversation as resolved.
Show resolved Hide resolved
const wasHidden = current !== null && current.memoizedState !== null;

// Before committing the children, track on the stack whether this
// offscreen subtree was already hidden, so that we don't unmount the
// effects again.
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
recursivelyTraverseMutationEffects(root, finishedWork, lanes);
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;

commitReconciliationEffects(finishedWork);

if (flags & Visibility) {
const newState: OffscreenState | null = finishedWork.memoizedState;
const isHidden = newState !== null;
const wasHidden = current !== null && current.memoizedState !== null;
const offscreenBoundary: Fiber = finishedWork;

if (supportsMutation) {
Expand Down
1 change: 0 additions & 1 deletion packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1980,7 +1980,6 @@ describe('ReactSuspenseEffectsSemantics', () => {

// Destroy layout and passive effects in the errored tree.
'App destroy layout',
'ThrowsInWillUnmount componentWillUnmount',
'Text:Fallback destroy layout',
'Text:Outside destroy layout',
'Text:Inside destroy passive',
Expand Down
Loading