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);
 returnworkInProgress.child;

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

@@ -77,6 +77,7 @@ import {
 LayoutMask,
 PassiveMask,
 StaticMask,
+PerformedWork,
 }from'./ReactFiberFlags';
 importinvariantfrom'shared/invariant';
 
@@ -987,9 +988,13 @@ function completeWork(
 constflags=workInProgress.flags;
 letnewFlags=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(
 newSet([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(
 newSet([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 ? newSet() : 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 ? newSet() : undefined,
+);// interaction events
 
-callback.mockReset();
+callback.mockReset();
+}
 
 Scheduler.unstable_advanceTime(20);// 30 -> 50