Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only call Profiler onRender if we performed work #19885

Merged
merged 1 commit into from
Sep 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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