From 87c023b1c1b00d6776b7031f6e105913ead355da Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 22 Sep 2020 14:47:13 -0400 Subject: [PATCH] Profiler onRender only called when we do work (#19885) If we didn't perform any work in the subtree, skip calling onRender. --- .../src/ReactFiberBeginWork.new.js | 3 ++ .../src/ReactFiberCompleteWork.new.js | 11 ++++-- .../ReactDOMTracing-test.internal.js | 33 ++++++++++------- .../__tests__/ReactProfiler-test.internal.js | 35 ++++++++++++------- 4 files changed, 54 insertions(+), 28 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index aa8d1c17ae3f9..198d07b87727d 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/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; diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index f4b8380deaa3d..1c77996185319 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/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 diff --git a/packages/react/src/__tests__/ReactDOMTracing-test.internal.js b/packages/react/src/__tests__/ReactDOMTracing-test.internal.js index e2c6bbe8f59dd..7be6513a5737d 100644 --- a/packages/react/src/__tests__/ReactDOMTracing-test.internal.js +++ b/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]), ); diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index ea7dead7e0250..35125e70ac2d6 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/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(); - 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