Don't call onCommit et al if there are no effects (#19863)

acdlite · Brian Vaughn · web-flow · commit 81aaee56afba · 2020-09-22T11:20:26.000-07:00
* Don't call onCommit et al if there are no effects

Checks `subtreeFlags` before scheduling an effect on the Profiler.

* Fix failing Profiler tests

The change to conditionally call Profiler commit hooks only if updates were scheduled broke a few of the Profiler tests. I've fixed the tests by either:
* Adding a no-op passive effect into the subtree or
* Converting onPostCommit to onCommit

When possible, I opted to add the no-op passive effect to the tests since that that hook is called later (during passive phase) so the test is a little broader. In a few cases, this required adding awkward act() wrappers so I opted to go with onCommit instead.

Co-authored-by: Brian Vaughn &lt;bvaughn@fb.com&gt;
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
@@ -60,8 +60,6 @@ import {
   Hydrating,
   ContentReset,
   DidCapture,
-  Update,
-  Passive,
   Ref,
   Deletion,
   ForceUpdateForLegacySuspense,
@@ -675,9 +673,6 @@ function updateProfiler(
   renderLanes: Lanes,
 ) {
   if (enableProfilerTimer) {
-    // TODO: Only call onRender et al if subtree has effects
-    workInProgress.flags |= Update | Passive;
-
     // Reset effect durations for the next eventual effect phase.
     // These are reset during render to allow the DevTools commit hook a chance to read them,
     const stateNode = workInProgress.stateNode;
@@ -3117,16 +3112,6 @@ function beginWork(
         }
         case Profiler:
           if (enableProfilerTimer) {
-            // Profiler should only call onRender when one of its descendants actually rendered.
-            // TODO: Only call onRender et al if subtree has effects
-            const hasChildWork = includesSomeLane(
-              renderLanes,
-              workInProgress.childLanes,
-            );
-            if (hasChildWork) {
-              workInProgress.flags |= Passive | Update;
-            }
-
             // Reset effect durations for the next eventual effect phase.
             // These are reset during render to allow the DevTools commit hook a chance to read them,
             const stateNode = workInProgress.stateNode;
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -68,6 +68,7 @@ import {
   Placement,
   Snapshot,
   Update,
+  Callback,
   PassiveMask,
 } from './ReactFiberFlags';
 import getComponentName from 'shared/getComponentName';
@@ -676,10 +677,17 @@ function commitLifeCycles(
       if (enableProfilerTimer) {
         const {onCommit, onRender} = finishedWork.memoizedProps;
         const {effectDuration} = finishedWork.stateNode;
+        const flags = finishedWork.flags;
 
         const commitTime = getCommitTime();
 
-        if (typeof onRender === 'function') {
+        const OnRenderFlag = Update;
+        const OnCommitFlag = Callback;
+
+        if (
+          (flags & OnRenderFlag) !== NoFlags &&
+          typeof onRender === 'function'
+        ) {
           if (enableSchedulerTracing) {
             onRender(
               finishedWork.memoizedProps.id,
@@ -703,7 +711,10 @@ function commitLifeCycles(
         }
 
         if (enableProfilerCommitHooks) {
-          if (typeof onCommit === 'function') {
+          if (
+            (flags & OnCommitFlag) !== NoFlags &&
+            typeof onCommit === 'function'
+          ) {
             if (enableSchedulerTracing) {
               onCommit(
                 finishedWork.memoizedProps.id,
diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js
@@ -67,10 +67,15 @@ import {
 import {
   Ref,
   Update,
+  Callback,
+  Passive,
+  Deletion,
   NoFlags,
   DidCapture,
   Snapshot,
   MutationMask,
+  LayoutMask,
+  PassiveMask,
   StaticMask,
 } from './ReactFiberFlags';
 import invariant from 'shared/invariant';
@@ -787,6 +792,8 @@ function bubbleProperties(completedWork: Fiber) {
   }
 
   completedWork.childLanes = newChildLanes;
+
+  return didBailout;
 }
 
 function completeWork(
@@ -804,7 +811,6 @@ function completeWork(
     case ForwardRef:
     case Fragment:
     case Mode:
-    case Profiler:
     case ContextConsumer:
     case MemoComponent:
       bubbleProperties(workInProgress);
@@ -966,6 +972,53 @@ function completeWork(
       bubbleProperties(workInProgress);
       return null;
     }
+    case Profiler: {
+      const didBailout = bubbleProperties(workInProgress);
+      if (!didBailout) {
+        // Use subtreeFlags to determine which commit callbacks should fire.
+        // TODO: Move this logic to the commit phase, since we already check if
+        // a fiber's subtree contains effects. Refactor the commit phase's
+        // depth-first traversal so that we can put work tag-specific logic
+        // before or after committing a subtree's effects.
+        const OnRenderFlag = Update;
+        const OnCommitFlag = Callback;
+        const OnPostCommitFlag = Passive;
+        const subtreeFlags = workInProgress.subtreeFlags;
+        const flags = workInProgress.flags;
+        let newFlags = flags;
+
+        // Call onRender any time this fiber or its subtree are worked on, even
+        // if there are no effects
+        newFlags |= OnRenderFlag;
+
+        // Call onCommit only if the subtree contains layout work, or if it
+        // contains deletions, since those might result in unmount work, which
+        // we include in the same measure.
+        // TODO: Can optimize by using a static flag to track whether a tree
+        // contains layout effects, like we do for passive effects.
+        if (
+          (flags & (LayoutMask | Deletion)) !== NoFlags ||
+          (subtreeFlags & (LayoutMask | Deletion)) !== NoFlags
+        ) {
+          newFlags |= OnCommitFlag;
+        }
+
+        // Call onPostCommit only if the subtree contains passive work.
+        // Don't have to check for deletions, because Deletion is already
+        // a passive flag.
+        if (
+          (flags & PassiveMask) !== NoFlags ||
+          (subtreeFlags & PassiveMask) !== NoFlags
+        ) {
+          newFlags |= OnPostCommitFlag;
+        }
+        workInProgress.flags = newFlags;
+      } else {
+        // This fiber and its subtree bailed out, so don't fire any callbacks.
+      }
+
+      return null;
+    }
     case SuspenseComponent: {
       popSuspenseContext(workInProgress);
       const nextState: null | SuspenseState = workInProgress.memoizedState;
diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js
