Skip to content

Commit

Permalink
Profiler onRender only called when we do work (#19885)
Browse files Browse the repository at this point in the history
If we didn't perform any work in the subtree, skip calling onRender.
  • Loading branch information
Brian Vaughn authored Sep 22, 2020
1 parent 92c7e49 commit 87c023b
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 28 deletions.
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
11 changes: 8 additions & 3 deletions packages/react-reconciler/src/ReactFiberCompleteWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ import {
LayoutMask,
PassiveMask,
StaticMask,
PerformedWork,
} from './ReactFiberFlags';
import invariant from 'shared/invariant';

Expand Down Expand Up @@ -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
Expand Down
33 changes: 21 additions & 12 deletions packages/react/src/__tests__/ReactDOMTracing-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]),
);
Expand Down Expand Up @@ -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]),
);
Expand Down
35 changes: 22 additions & 13 deletions packages/react/src/__tests__/ReactProfiler-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 87c023b

Please sign in to comment.