Only call Profiler onRender when a descendant had work (#17223)

Author: Brian Vaughn

packages/react-reconciler/src/ReactFiberBeginWork.js

@@ -2893,7 +2893,12 @@ function beginWork(
         }
         case Profiler:
           if (enableProfilerTimer) {
-            workInProgress.effectTag |= Update;
+            // Profiler should only call onRender when one of its descendants actually rendered.
+            const hasChildWork =
+              workInProgress.childExpirationTime >= renderExpirationTime;
+            if (hasChildWork) {
+              workInProgress.effectTag |= Update;
+            }
           }
           break;
         case SuspenseComponent: {

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

@@ -295,6 +295,79 @@ describe('Profiler', () => {
         );
       });
 
+      it('does not report work done on a sibling', () => {
+        const callback = jest.fn();
+
+        const DoesNotUpdate = React.memo(function DoesNotUpdateInner() {
+          Scheduler.unstable_advanceTime(10);
+          return null;
+        }, () => true);
+
+        let updateProfilerSibling;
+
+        function ProfilerSibling() {
+          const [count, setCount] = React.useState(0);
+          updateProfilerSibling = () => setCount(count + 1);
+          return null;
+        }
+
+        function App() {
+          return (
+            <React.Fragment>
+              <React.Profiler id="test" onRender={callback}>
+                <DoesNotUpdate />
+              </React.Profiler>
+              <ProfilerSibling />
+            </React.Fragment>
+          );
+        }
+
+        const renderer = ReactTestRenderer.create(<App />);
+
+        expect(callback).toHaveBeenCalledTimes(1);
+
+        let call = callback.mock.calls[0];
+
+        expect(call).toHaveLength(enableSchedulerTracing ? 7 : 6);
+        expect(call[0]).toBe('test');
+        expect(call[1]).toBe('mount');
+        expect(call[2]).toBe(10); // actual time
+        expect(call[3]).toBe(10); // base time
+        expect(call[4]).toBe(0); // start time
+        expect(call[5]).toBe(10); // commit time
+        expect(call[6]).toEqual(enableSchedulerTracing ? new Set() : undefined); // interaction events
+
+        callback.mockReset();
+
+        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);
+
+        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
+
+        callback.mockReset();
+
+        Scheduler.unstable_advanceTime(20); // 30 -> 50
+
+        // Updating a sibling should not report a re-render.
+        ReactTestRenderer.act(updateProfilerSibling);
+
+        expect(callback).not.toHaveBeenCalled();
+      });
+
       it('logs render times for both mount and update', () => {
         const callback = jest.fn();
 
@@ -372,15 +445,15 @@ describe('Profiler', () => {
         Scheduler.unstable_advanceTime(5); // 0 -> 5
 
         ReactTestRenderer.create(
-          <>
+          <React.Fragment>
             <React.Profiler id="parent" onRender={callback}>
               <AdvanceTime byAmount={10}>
                 <React.Profiler id="child" onRender={callback}>
                   <AdvanceTime byAmount={20} />
                 </React.Profiler>
               </AdvanceTime>
             </React.Profiler>
-          </>,
+          </React.Fragment>,
         );
 
         expect(callback).toHaveBeenCalledTimes(2);
@@ -407,14 +480,14 @@ describe('Profiler', () => {
         Scheduler.unstable_advanceTime(5); // 0 -> 5
 
         ReactTestRenderer.create(
-          <>
+          <React.Fragment>
             <React.Profiler id="first" onRender={callback}>
               <AdvanceTime byAmount={20} />
             </React.Profiler>
             <React.Profiler id="second" onRender={callback}>
               <AdvanceTime byAmount={5} />
             </React.Profiler>
-          </>,
+          </React.Fragment>,
         );
 
         expect(callback).toHaveBeenCalledTimes(2);
@@ -440,13 +513,13 @@ describe('Profiler', () => {
         Scheduler.unstable_advanceTime(5); // 0 -> 5
 
         ReactTestRenderer.create(
-          <>
+          <React.Fragment>
             <AdvanceTime byAmount={20} />
             <React.Profiler id="test" onRender={callback}>
               <AdvanceTime byAmount={5} />
             </React.Profiler>
             <AdvanceTime byAmount={20} />
-          </>,
+          </React.Fragment>,
         );
 
         expect(callback).toHaveBeenCalledTimes(1);
@@ -471,28 +544,18 @@ describe('Profiler', () => {
           }
         }
 
-        class Pure extends React.PureComponent {
-          render() {
-            return this.props.children;
-          }
-        }
-
         const renderer = ReactTestRenderer.create(
           <React.Profiler id="outer" onRender={callback}>
             <Updater>
-              <React.Profiler id="middle" onRender={callback}>
-                <Pure>
-                  <React.Profiler id="inner" onRender={callback}>
-                    <div />
-                  </React.Profiler>
-                </Pure>
+              <React.Profiler id="inner" onRender={callback}>
+                <div />
               </React.Profiler>
             </Updater>
           </React.Profiler>,
         );
 
         // All profile callbacks are called for initial render
-        expect(callback).toHaveBeenCalledTimes(3);
+        expect(callback).toHaveBeenCalledTimes(2);
 
         callback.mockReset();
 
@@ -502,11 +565,11 @@ describe('Profiler', () => {
           });
         });
 
-        // Only call profile updates for paths that have re-rendered
-        // Since "inner" is beneath a pure component, it isn't called
-        expect(callback).toHaveBeenCalledTimes(2);
-        expect(callback.mock.calls[0][0]).toBe('middle');
-        expect(callback.mock.calls[1][0]).toBe('outer');
+        // Only call onRender for paths that have re-rendered.
+        // Since the Updater's props didn't change,
+        // React does not re-render its children.
+        expect(callback).toHaveBeenCalledTimes(1);
+        expect(callback.mock.calls[0][0]).toBe('outer');
       });
 
       it('decreases actual time but not base time when sCU prevents an update', () => {
@@ -1455,11 +1518,11 @@ describe('Profiler', () => {
         render() {
           instance = this;
           return (
-            <>
+            <React.Fragment>
               <Yield value="first" />
               {this.state.count}
               <Yield value="last" />
-            </>
+            </React.Fragment>
           );
         }
       }