Profiler onRender only called when we do work (#19885)

If we didn't perform any work in the subtree, skip calling onRender.

Author: Brian Vaughn

packages/react-reconciler/src/ReactFiberBeginWork.new.js

@@ -1148,6 +1148,9 @@ function updateHostComponent(
     workInProgress.flags |= ContentReset;
   }
 
+  // React DevTools reads this flag.
+  workInProgress.flags |= PerformedWork;
+
   markRef(current, workInProgress);
   reconcileChildren(current, workInProgress, nextChildren, renderLanes);
   return workInProgress.child;

packages/react-reconciler/src/ReactFiberCompleteWork.new.js

@@ -77,6 +77,7 @@ import {
   LayoutMask,
   PassiveMask,
   StaticMask,
+  PerformedWork,
 } from './ReactFiberFlags';
 import invariant from 'shared/invariant';
 
@@ -987,9 +988,13 @@ function completeWork(
         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 onRender any time this fiber or its subtree are worked on.
+        if (
+          (flags & PerformedWork) !== NoFlags ||
+          (subtreeFlags & PerformedWork) !== NoFlags
+        ) {
+          newFlags |= OnRenderFlag;
+        }
 
         // Call onCommit only if the subtree contains layout work, or if it
         // contains deletions, since those might result in unmount work, which

packages/react/src/__tests__/ReactDOMTracing-test.internal.js

@@ -151,12 +151,17 @@ describe('ReactDOMTracing', () => {
         expect(
           onInteractionScheduledWorkCompleted,
         ).toHaveBeenLastNotifiedOfInteraction(interaction);
-        // TODO: This is 4 instead of 3 because this update was scheduled at
-        // idle priority, and idle updates are slightly higher priority than
-        // offscreen work. So it takes two render passes to finish it. Profiler
-        // calls `onRender` for the first render even though everything
-        // bails out.
-        expect(onRender).toHaveBeenCalledTimes(4);
+
+        if (gate(flags => flags.new)) {
+          expect(onRender).toHaveBeenCalledTimes(3);
+        } else {
+          // TODO: This is 4 instead of 3 because this update was scheduled at
+          // idle priority, and idle updates are slightly higher priority than
+          // offscreen work. So it takes two render passes to finish it. Profiler
+          // calls `onRender` for the first render even though everything
+          // bails out.
+          expect(onRender).toHaveBeenCalledTimes(4);
+        }
         expect(onRender).toHaveLastRenderedWithInteractions(
           new Set([interaction]),
         );
@@ -305,12 +310,16 @@ describe('ReactDOMTracing', () => {
         expect(
           onInteractionScheduledWorkCompleted,
         ).toHaveBeenLastNotifiedOfInteraction(interaction);
-        // TODO: This is 4 instead of 3 because this update was scheduled at
-        // idle priority, and idle updates are slightly higher priority than
-        // offscreen work. So it takes two render passes to finish it. Profiler
-        // calls `onRender` for the first render even though everything
-        // bails out.
-        expect(onRender).toHaveBeenCalledTimes(4);
+        if (gate(flags => flags.new)) {
+          expect(onRender).toHaveBeenCalledTimes(3);
+        } else {
+          // TODO: This is 4 instead of 3 because this update was scheduled at
+          // idle priority, and idle updates are slightly higher priority than
+          // offscreen work. So it takes two render passes to finish it. Profiler
+          // calls `onRender` for the first render even though everything
+          // bails out.
+          expect(onRender).toHaveBeenCalledTimes(4);
+        }
         expect(onRender).toHaveLastRenderedWithInteractions(
           new Set([interaction]),
         );

packages/react/src/__tests__/ReactProfiler-test.internal.js

@@ -362,24 +362,33 @@ describe('Profiler', () => {
 
         Scheduler.unstable_advanceTime(20); // 10 -> 30
 
-        // Updating a parent should report a re-render,
-        // since React technically did a little bit of work between the Profiler and the bailed out subtree.
         renderer.update(<App />);
 
-        expect(callback).toHaveBeenCalledTimes(1);
+        if (gate(flags => flags.new)) {
+          // None of the Profiler's subtree was rendered because App bailed out before the Profiler.
+          // So we expect onRender not to be called.
+          expect(callback).not.toHaveBeenCalled();
+        } else {
+          // Updating a parent reports a re-render,
+          // since React technically did a little bit of work between the Profiler and the bailed out subtree.
+          // This is not optimal but it's how the old reconciler fork works.
+          expect(callback).toHaveBeenCalledTimes(1);
 
-        call = callback.mock.calls[0];
+          call = callback.mock.calls[0];
 
-        expect(call).toHaveLength(enableSchedulerTracing ? 7 : 6);
-        expect(call[0]).toBe('test');
-        expect(call[1]).toBe('update');
-        expect(call[2]).toBe(0); // actual time
-        expect(call[3]).toBe(10); // base time
-        expect(call[4]).toBe(30); // start time
-        expect(call[5]).toBe(30); // commit time
-        expect(call[6]).toEqual(enableSchedulerTracing ? new Set() : undefined); // interaction events
+          expect(call).toHaveLength(enableSchedulerTracing ? 7 : 6);
+          expect(call[0]).toBe('test');
+          expect(call[1]).toBe('update');
+          expect(call[2]).toBe(0); // actual time
+          expect(call[3]).toBe(10); // base time
+          expect(call[4]).toBe(30); // start time
+          expect(call[5]).toBe(30); // commit time
+          expect(call[6]).toEqual(
+            enableSchedulerTracing ? new Set() : undefined,
+          ); // interaction events
 
-        callback.mockReset();
+          callback.mockReset();
+        }
 
         Scheduler.unstable_advanceTime(20); // 30 -> 50