Skip to content

Commit

Permalink
Profiler onRender only called when we do work
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 committed Sep 22, 2020
1 parent fbb0375 commit feffd94
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 feffd94

Please sign in to comment.