Move passive logic out of layout phase (facebook#19500)

acdlite · web-flow · commit ede9170648d0 · 2020-07-30T21:43:03.000-07:00
* setCurrentFiber per fiber, instead of per effect

* Re-use safelyCallDestroy

Part of the code in flushPassiveUnmountEffects is a duplicate of the
code used for unmounting layout effects. I did some minor refactoring to
so we could use the same function in both places.

Closure will inline anyway so it doesn't affect code size or
performance, just maintainability.

* Don't check HookHasEffect during deletion

We don't need to check HookHasEffect during a deletion; all effects are
unmounted.

So we also don't have to set HookHasEffect during a deletion, either.

This allows us to remove the last remaining passive effect logic from
the synchronous layout phase.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -122,7 +122,6 @@ import {
   NoEffect as NoHookEffect,
   HasEffect as HookHasEffect,
   Layout as HookLayout,
-  Passive as HookPassive,
 } from './ReactHookEffectTags';
 import {didWarnAboutReassigningProps} from './ReactFiberBeginWork.new';
 import {
@@ -206,7 +205,7 @@ function safelyDetachRef(current: Fiber) {
   }
 }
 
-function safelyCallDestroy(current, destroy) {
+export function safelyCallDestroy(current: Fiber, destroy: () => void) {
   if (__DEV__) {
     invokeGuardedCallback(null, destroy, null);
     if (hasCaughtError()) {
@@ -876,10 +875,7 @@ function commitUnmount(
           do {
             const {destroy, tag} = effect;
             if (destroy !== undefined) {
-              if ((tag & HookPassive) !== NoHookEffect) {
-                // TODO: Consider if we can move this block out of the synchronous commit phase
-                effect.tag |= HookHasEffect;
-              } else {
+              if ((tag & HookLayout) !== NoHookEffect) {
                 if (
                   enableProfilerTimer &&
                   enableProfilerCommitHooks &&
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -15,6 +15,7 @@ import type {Interaction} from 'scheduler/src/Tracing';
 import type {SuspenseConfig} from './ReactFiberSuspenseConfig';
 import type {SuspenseState} from './ReactFiberSuspenseComponent.new';
 import type {Effect as HookEffect} from './ReactFiberHooks.new';
+import type {HookEffectTag} from './ReactHookEffectTags';
 import type {StackCursor} from './ReactFiberStack.new';
 import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new';
 
@@ -209,6 +210,7 @@ import {
   commitPassiveEffectDurations,
   commitResetTextContent,
   isSuspenseBoundaryBeingHidden,
+  safelyCallDestroy,
 } from './ReactFiberCommitWork.new';
 import {enqueueUpdate} from './ReactUpdateQueue.new';
 import {resetContextDependencies} from './ReactFiberNewContext.new';
@@ -2702,6 +2704,8 @@ function flushPassiveMountEffects(firstChild: Fiber): void {
 }
 
 function flushPassiveMountEffectsImpl(fiber: Fiber): void {
+  setCurrentDebugFiberInDEV(fiber);
+
   const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
   const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
   if (lastEffect !== null) {
@@ -2715,7 +2719,6 @@ function flushPassiveMountEffectsImpl(fiber: Fiber): void {
         (tag & HookHasEffect) !== NoHookEffect
       ) {
         if (__DEV__) {
-          setCurrentDebugFiberInDEV(fiber);
           if (
             enableProfilerTimer &&
             enableProfilerCommitHooks &&
@@ -2742,7 +2745,6 @@ function flushPassiveMountEffectsImpl(fiber: Fiber): void {
             const error = clearCaughtError();
             captureCommitPhaseError(fiber, error);
           }
-          resetCurrentDebugFiberInDEV();
         } else {
           try {
             const create = effect.create;
@@ -2769,6 +2771,8 @@ function flushPassiveMountEffectsImpl(fiber: Fiber): void {
 
       effect = next;
     } while (effect !== firstEffect);
+
+    resetCurrentDebugFiberInDEV();
   }
 }
 
@@ -2813,7 +2817,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
       case Block: {
         const primaryEffectTag = fiber.effectTag & Passive;
         if (primaryEffectTag !== NoEffect) {
-          flushPassiveUnmountEffectsImpl(fiber);
+          flushPassiveUnmountEffectsImpl(fiber, HookPassive | HookHasEffect);
         }
       }
     }
@@ -2845,7 +2849,7 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree(
         case ForwardRef:
         case SimpleMemoComponent:
         case Block: {
-          flushPassiveUnmountEffectsImpl(fiber);
+          flushPassiveUnmountEffectsImpl(fiber, HookPassive);
         }
       }
     }
@@ -2854,67 +2858,42 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree(
   }
 }
 
-function flushPassiveUnmountEffectsImpl(fiber: Fiber): void {
+function flushPassiveUnmountEffectsImpl(
+  fiber: Fiber,
+  // Tags to check for when deciding whether to unmount. e.g. to skip over
+  // layout effects
+  hookEffectTag: HookEffectTag,
+): void {
   const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
   const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
   if (lastEffect !== null) {
+    setCurrentDebugFiberInDEV(fiber);
+
     const firstEffect = lastEffect.next;
     let effect = firstEffect;
     do {
       const {next, tag} = effect;
-      if (
-        (tag & HookPassive) !== NoHookEffect &&
-        (tag & HookHasEffect) !== NoHookEffect
-      ) {
+      if ((tag & hookEffectTag) === hookEffectTag) {
         const destroy = effect.destroy;
-        effect.destroy = undefined;
-
-        if (typeof destroy === 'function') {
-          if (__DEV__) {
-            setCurrentDebugFiberInDEV(fiber);
-            if (
-              enableProfilerTimer &&
-              enableProfilerCommitHooks &&
-              fiber.mode & ProfileMode
-            ) {
-              startPassiveEffectTimer();
-              invokeGuardedCallback(null, destroy, null);
-              recordPassiveEffectDuration(fiber);
-            } else {
-              invokeGuardedCallback(null, destroy, null);
-            }
-            if (hasCaughtError()) {
-              invariant(fiber !== null, 'Should be working on an effect.');
-              const error = clearCaughtError();
-              captureCommitPhaseError(fiber, error);
-            }
-            resetCurrentDebugFiberInDEV();
+        if (destroy !== undefined) {
+          effect.destroy = undefined;
+          if (
+            enableProfilerTimer &&
+            enableProfilerCommitHooks &&
+            fiber.mode & ProfileMode
+          ) {
+            startPassiveEffectTimer();
+            safelyCallDestroy(fiber, destroy);
+            recordPassiveEffectDuration(fiber);
           } else {
-            try {
-              if (
-                enableProfilerTimer &&
-                enableProfilerCommitHooks &&
-                fiber.mode & ProfileMode
-              ) {
-                try {
-                  startPassiveEffectTimer();
-                  destroy();
-                } finally {
-                  recordPassiveEffectDuration(fiber);
-                }
-              } else {
-                destroy();
-              }
-            } catch (error) {
-              invariant(fiber !== null, 'Should be working on an effect.');
-              captureCommitPhaseError(fiber, error);
-            }
+            safelyCallDestroy(fiber, destroy);
           }
         }
       }
-
       effect = next;
     } while (effect !== firstEffect);
+
+    resetCurrentDebugFiberInDEV();
   }
 }
 
