From 4ebaeae40dec52c84c968a25f1f21f4d0f571336 Mon Sep 17 00:00:00 2001 From: Luna Ruan Date: Fri, 8 Apr 2022 11:28:20 -0500 Subject: [PATCH] moved mutation code to passive (#24251) This PR moves the code for transition tracing in the mutation phase that adds transitions to the pending callbacks object (to be called sometime later after paint) from the mutation to the passive phase. Things to think about: Passive effects can be flushed before or after paint. How do we make sure that we get the correct end time for the interaction? --- .../src/ReactFiberCommitWork.new.js | 63 ++++++++++--------- .../src/ReactFiberCommitWork.old.js | 63 ++++++++++--------- .../src/ReactFiberCompleteWork.new.js | 26 ++++++++ .../src/ReactFiberCompleteWork.old.js | 26 ++++++++ .../src/ReactFiberWorkLoop.new.js | 51 ++++++++------- .../src/ReactFiberWorkLoop.old.js | 51 ++++++++------- 6 files changed, 176 insertions(+), 104 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index d1bbdcdde156d..6d3b0e555a64f 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -2227,32 +2227,6 @@ function commitMutationEffectsOnFiber( // because of the shared reconciliation logic below. const flags = finishedWork.flags; - if (enableTransitionTracing) { - switch (finishedWork.tag) { - case HostRoot: { - const state = finishedWork.memoizedState; - const transitions = state.transitions; - if (transitions !== null) { - transitions.forEach(transition => { - // TODO(luna) Do we want to log TransitionStart in the startTransition callback instead? - addTransitionStartCallbackToPendingTransition({ - transitionName: transition.name, - startTime: transition.startTime, - }); - - addTransitionCompleteCallbackToPendingTransition({ - transitionName: transition.name, - startTime: transition.startTime, - }); - }); - - clearTransitionsForLanes(root, lanes); - state.transitions = null; - } - } - } - } - if (flags & ContentReset) { commitResetTextContent(finishedWork); } @@ -2639,12 +2613,17 @@ function reappearLayoutEffects_complete(subtreeRoot: Fiber) { export function commitPassiveMountEffects( root: FiberRoot, finishedWork: Fiber, + committedLanes: Lanes, ): void { nextEffect = finishedWork; - commitPassiveMountEffects_begin(finishedWork, root); + commitPassiveMountEffects_begin(finishedWork, root, committedLanes); } -function commitPassiveMountEffects_begin(subtreeRoot: Fiber, root: FiberRoot) { +function commitPassiveMountEffects_begin( + subtreeRoot: Fiber, + root: FiberRoot, + committedLanes: Lanes, +) { while (nextEffect !== null) { const fiber = nextEffect; const firstChild = fiber.child; @@ -2652,7 +2631,7 @@ function commitPassiveMountEffects_begin(subtreeRoot: Fiber, root: FiberRoot) { ensureCorrectReturnPointer(firstChild, fiber); nextEffect = firstChild; } else { - commitPassiveMountEffects_complete(subtreeRoot, root); + commitPassiveMountEffects_complete(subtreeRoot, root, committedLanes); } } } @@ -2660,13 +2639,15 @@ function commitPassiveMountEffects_begin(subtreeRoot: Fiber, root: FiberRoot) { function commitPassiveMountEffects_complete( subtreeRoot: Fiber, root: FiberRoot, + committedLanes: Lanes, ) { while (nextEffect !== null) { const fiber = nextEffect; + if ((fiber.flags & Passive) !== NoFlags) { setCurrentDebugFiberInDEV(fiber); try { - commitPassiveMountOnFiber(root, fiber); + commitPassiveMountOnFiber(root, fiber, committedLanes); } catch (error) { reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); @@ -2693,6 +2674,7 @@ function commitPassiveMountEffects_complete( function commitPassiveMountOnFiber( finishedRoot: FiberRoot, finishedWork: Fiber, + committedLanes: Lanes, ): void { switch (finishedWork.tag) { case FunctionComponent: @@ -2734,6 +2716,27 @@ function commitPassiveMountOnFiber( } } } + + if (enableTransitionTracing) { + const transitions = finishedWork.memoizedState.transitions; + if (transitions !== null) { + transitions.forEach(transition => { + // TODO(luna) Do we want to log TransitionStart in the startTransition callback instead? + addTransitionStartCallbackToPendingTransition({ + transitionName: transition.name, + startTime: transition.startTime, + }); + + addTransitionCompleteCallbackToPendingTransition({ + transitionName: transition.name, + startTime: transition.startTime, + }); + }); + + clearTransitionsForLanes(finishedRoot, committedLanes); + finishedWork.memoizedState.transitions = null; + } + } break; } case LegacyHiddenComponent: diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index bff8b2781310b..9a8ba5bcd6d57 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -2227,32 +2227,6 @@ function commitMutationEffectsOnFiber( // because of the shared reconciliation logic below. const flags = finishedWork.flags; - if (enableTransitionTracing) { - switch (finishedWork.tag) { - case HostRoot: { - const state = finishedWork.memoizedState; - const transitions = state.transitions; - if (transitions !== null) { - transitions.forEach(transition => { - // TODO(luna) Do we want to log TransitionStart in the startTransition callback instead? - addTransitionStartCallbackToPendingTransition({ - transitionName: transition.name, - startTime: transition.startTime, - }); - - addTransitionCompleteCallbackToPendingTransition({ - transitionName: transition.name, - startTime: transition.startTime, - }); - }); - - clearTransitionsForLanes(root, lanes); - state.transitions = null; - } - } - } - } - if (flags & ContentReset) { commitResetTextContent(finishedWork); } @@ -2639,12 +2613,17 @@ function reappearLayoutEffects_complete(subtreeRoot: Fiber) { export function commitPassiveMountEffects( root: FiberRoot, finishedWork: Fiber, + committedLanes: Lanes, ): void { nextEffect = finishedWork; - commitPassiveMountEffects_begin(finishedWork, root); + commitPassiveMountEffects_begin(finishedWork, root, committedLanes); } -function commitPassiveMountEffects_begin(subtreeRoot: Fiber, root: FiberRoot) { +function commitPassiveMountEffects_begin( + subtreeRoot: Fiber, + root: FiberRoot, + committedLanes: Lanes, +) { while (nextEffect !== null) { const fiber = nextEffect; const firstChild = fiber.child; @@ -2652,7 +2631,7 @@ function commitPassiveMountEffects_begin(subtreeRoot: Fiber, root: FiberRoot) { ensureCorrectReturnPointer(firstChild, fiber); nextEffect = firstChild; } else { - commitPassiveMountEffects_complete(subtreeRoot, root); + commitPassiveMountEffects_complete(subtreeRoot, root, committedLanes); } } } @@ -2660,13 +2639,15 @@ function commitPassiveMountEffects_begin(subtreeRoot: Fiber, root: FiberRoot) { function commitPassiveMountEffects_complete( subtreeRoot: Fiber, root: FiberRoot, + committedLanes: Lanes, ) { while (nextEffect !== null) { const fiber = nextEffect; + if ((fiber.flags & Passive) !== NoFlags) { setCurrentDebugFiberInDEV(fiber); try { - commitPassiveMountOnFiber(root, fiber); + commitPassiveMountOnFiber(root, fiber, committedLanes); } catch (error) { reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); @@ -2693,6 +2674,7 @@ function commitPassiveMountEffects_complete( function commitPassiveMountOnFiber( finishedRoot: FiberRoot, finishedWork: Fiber, + committedLanes: Lanes, ): void { switch (finishedWork.tag) { case FunctionComponent: @@ -2734,6 +2716,27 @@ function commitPassiveMountOnFiber( } } } + + if (enableTransitionTracing) { + const transitions = finishedWork.memoizedState.transitions; + if (transitions !== null) { + transitions.forEach(transition => { + // TODO(luna) Do we want to log TransitionStart in the startTransition callback instead? + addTransitionStartCallbackToPendingTransition({ + transitionName: transition.name, + startTime: transition.startTime, + }); + + addTransitionCompleteCallbackToPendingTransition({ + transitionName: transition.name, + startTime: transition.startTime, + }); + }); + + clearTransitionsForLanes(finishedRoot, committedLanes); + finishedWork.memoizedState.transitions = null; + } + } break; } case LegacyHiddenComponent: diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index bea984c19f1ce..979f76b51415d 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -153,6 +153,7 @@ import { popRenderLanes, getRenderTargetTime, subtreeRenderLanes, + getWorkInProgressTransitions, } from './ReactFiberWorkLoop.new'; import { OffscreenLane, @@ -862,6 +863,17 @@ function completeWork( } case HostRoot: { const fiberRoot = (workInProgress.stateNode: FiberRoot); + + if (enableTransitionTracing) { + const transitions = getWorkInProgressTransitions(); + // We set the Passive flag here because if there are new transitions, + // we will need to schedule callbacks and process the transitions, + // which we do in the passive phase + if (transitions !== null) { + workInProgress.flags |= Passive; + } + } + if (enableCache) { popRootTransition(fiberRoot, renderLanes); @@ -918,6 +930,14 @@ function completeWork( } updateHostContainer(current, workInProgress); bubbleProperties(workInProgress); + if (enableTransitionTracing) { + if ((workInProgress.subtreeFlags & Visibility) !== NoFlags) { + // If any of our suspense children toggle visibility, this means that + // the pending boundaries array needs to be updated, which we only + // do in the passive phase. + workInProgress.flags |= Passive; + } + } return null; } case HostComponent: { @@ -1187,6 +1207,12 @@ function completeWork( const offscreenFiber: Fiber = (workInProgress.child: any); offscreenFiber.flags |= Visibility; + // If the suspended state of the boundary changes, we need to schedule + // a passive effect, which is when we process the transitions + if (enableTransitionTracing) { + offscreenFiber.flags |= Passive; + } + // TODO: This will still suspend a synchronous tree if anything // in the concurrent tree already suspended during this render. // This is a known bug. diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js index ef3d4f7979f29..4b8fa1083cfaf 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js @@ -153,6 +153,7 @@ import { popRenderLanes, getRenderTargetTime, subtreeRenderLanes, + getWorkInProgressTransitions, } from './ReactFiberWorkLoop.old'; import { OffscreenLane, @@ -862,6 +863,17 @@ function completeWork( } case HostRoot: { const fiberRoot = (workInProgress.stateNode: FiberRoot); + + if (enableTransitionTracing) { + const transitions = getWorkInProgressTransitions(); + // We set the Passive flag here because if there are new transitions, + // we will need to schedule callbacks and process the transitions, + // which we do in the passive phase + if (transitions !== null) { + workInProgress.flags |= Passive; + } + } + if (enableCache) { popRootTransition(fiberRoot, renderLanes); @@ -918,6 +930,14 @@ function completeWork( } updateHostContainer(current, workInProgress); bubbleProperties(workInProgress); + if (enableTransitionTracing) { + if ((workInProgress.subtreeFlags & Visibility) !== NoFlags) { + // If any of our suspense children toggle visibility, this means that + // the pending boundaries array needs to be updated, which we only + // do in the passive phase. + workInProgress.flags |= Passive; + } + } return null; } case HostComponent: { @@ -1187,6 +1207,12 @@ function completeWork( const offscreenFiber: Fiber = (workInProgress.child: any); offscreenFiber.flags |= Visibility; + // If the suspended state of the boundary changes, we need to schedule + // a passive effect, which is when we process the transitions + if (enableTransitionTracing) { + offscreenFiber.flags |= Passive; + } + // TODO: This will still suspend a synchronous tree if anything // in the concurrent tree already suspended during this render. // This is a known bug. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 75d9c76b55d18..bbd4acdf9e9be 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2322,27 +2322,6 @@ function commitRootImpl( // If layout work was scheduled, flush it now. flushSyncCallbacks(); - if (enableTransitionTracing) { - const prevPendingTransitionCallbacks = currentPendingTransitionCallbacks; - const prevRootTransitionCallbacks = root.transitionCallbacks; - if ( - prevPendingTransitionCallbacks !== null && - prevRootTransitionCallbacks !== null - ) { - // TODO(luna) Refactor this code into the Host Config - const endTime = now(); - currentPendingTransitionCallbacks = null; - - scheduleCallback(IdleSchedulerPriority, () => - processTransitionCallbacks( - prevPendingTransitionCallbacks, - endTime, - prevRootTransitionCallbacks, - ), - ); - } - } - if (__DEV__) { if (enableDebugTracing) { logCommitStopped(); @@ -2457,7 +2436,7 @@ function flushPassiveEffectsImpl() { executionContext |= CommitContext; commitPassiveUnmountEffects(root.current); - commitPassiveMountEffects(root, root.current); + commitPassiveMountEffects(root, root.current, lanes); // TODO: Move to commitPassiveMountEffects if (enableProfilerTimer && enableProfilerCommitHooks) { @@ -2487,6 +2466,34 @@ function flushPassiveEffectsImpl() { flushSyncCallbacks(); + if (enableTransitionTracing) { + const prevPendingTransitionCallbacks = currentPendingTransitionCallbacks; + const prevRootTransitionCallbacks = root.transitionCallbacks; + if ( + prevPendingTransitionCallbacks !== null && + prevRootTransitionCallbacks !== null + ) { + // TODO(luna) Refactor this code into the Host Config + // TODO(luna) The end time here is not necessarily accurate + // because passive effects could be called before paint + // (synchronously) or after paint (normally). We need + // to come up with a way to get the correct end time for both cases. + // One solution is in the host config, if the passive effects + // have not yet been run, make a call to flush the passive effects + // right after paint. + const endTime = now(); + currentPendingTransitionCallbacks = null; + + scheduleCallback(IdleSchedulerPriority, () => + processTransitionCallbacks( + prevPendingTransitionCallbacks, + endTime, + prevRootTransitionCallbacks, + ), + ); + } + } + if (__DEV__) { // If additional passive effects were scheduled, increment a counter. If this // exceeds the limit, we'll fire a warning. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 17bfc935bc90f..8e2dd32c9f166 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2322,27 +2322,6 @@ function commitRootImpl( // If layout work was scheduled, flush it now. flushSyncCallbacks(); - if (enableTransitionTracing) { - const prevPendingTransitionCallbacks = currentPendingTransitionCallbacks; - const prevRootTransitionCallbacks = root.transitionCallbacks; - if ( - prevPendingTransitionCallbacks !== null && - prevRootTransitionCallbacks !== null - ) { - // TODO(luna) Refactor this code into the Host Config - const endTime = now(); - currentPendingTransitionCallbacks = null; - - scheduleCallback(IdleSchedulerPriority, () => - processTransitionCallbacks( - prevPendingTransitionCallbacks, - endTime, - prevRootTransitionCallbacks, - ), - ); - } - } - if (__DEV__) { if (enableDebugTracing) { logCommitStopped(); @@ -2457,7 +2436,7 @@ function flushPassiveEffectsImpl() { executionContext |= CommitContext; commitPassiveUnmountEffects(root.current); - commitPassiveMountEffects(root, root.current); + commitPassiveMountEffects(root, root.current, lanes); // TODO: Move to commitPassiveMountEffects if (enableProfilerTimer && enableProfilerCommitHooks) { @@ -2487,6 +2466,34 @@ function flushPassiveEffectsImpl() { flushSyncCallbacks(); + if (enableTransitionTracing) { + const prevPendingTransitionCallbacks = currentPendingTransitionCallbacks; + const prevRootTransitionCallbacks = root.transitionCallbacks; + if ( + prevPendingTransitionCallbacks !== null && + prevRootTransitionCallbacks !== null + ) { + // TODO(luna) Refactor this code into the Host Config + // TODO(luna) The end time here is not necessarily accurate + // because passive effects could be called before paint + // (synchronously) or after paint (normally). We need + // to come up with a way to get the correct end time for both cases. + // One solution is in the host config, if the passive effects + // have not yet been run, make a call to flush the passive effects + // right after paint. + const endTime = now(); + currentPendingTransitionCallbacks = null; + + scheduleCallback(IdleSchedulerPriority, () => + processTransitionCallbacks( + prevPendingTransitionCallbacks, + endTime, + prevRootTransitionCallbacks, + ), + ); + } + } + if (__DEV__) { // If additional passive effects were scheduled, increment a counter. If this // exceeds the limit, we'll fire a warning.