Skip to content

Commit

Permalink
Profiler: Improve nested-update checks (#20299)
Browse files Browse the repository at this point in the history
Previous checks were too naive when it comes to pending lower-pri work or batched updates. This commit adds two new (previously failing) tests and fixes.
  • Loading branch information
Brian Vaughn authored Nov 20, 2020
1 parent 2cf8f7f commit e6a0f27
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 9 deletions.
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,7 @@ export function scheduleUpdateOnFiber(

if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) {
if (
executionContext === CommitContext &&
(executionContext & CommitContext) !== NoContext &&
root === rootCommittingMutationOrLayoutEffects
) {
if (fiber.mode & ProfileMode) {
Expand Down Expand Up @@ -2240,7 +2240,7 @@ function commitRootImpl(root, renderPriorityLevel) {
}
}

if (remainingLanes === SyncLane) {
if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
markNestedUpdateScheduled();
}
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,7 @@ export function scheduleUpdateOnFiber(

if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) {
if (
executionContext === CommitContext &&
(executionContext & CommitContext) !== NoContext &&
root === rootCommittingMutationOrLayoutEffects
) {
if (fiber.mode & ProfileMode) {
Expand Down Expand Up @@ -2240,7 +2240,7 @@ function commitRootImpl(root, renderPriorityLevel) {
}
}

if (remainingLanes === SyncLane) {
if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
markNestedUpdateScheduled();
}
Expand Down
94 changes: 89 additions & 5 deletions packages/react/src/__tests__/ReactProfiler-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,52 @@ describe('Profiler', () => {
expect(onRender.mock.calls[2][1]).toBe('update');
});

it('is properly distinguish updates and nested-updates when there is more than sync remaining work', () => {
loadModules({
enableSchedulerTracing,
useNoopRenderer: true,
});

function Component() {
const [didMount, setDidMount] = React.useState(false);

React.useLayoutEffect(() => {
setDidMount(true);
}, []);
Scheduler.unstable_yieldValue(didMount);
return didMount;
}

const onRender = jest.fn();

// Schedule low-priority work.
Scheduler.unstable_runWithPriority(
Scheduler.unstable_LowPriority,
() => {
ReactNoop.render(
<React.Profiler id="root" onRender={onRender}>
<Component />
</React.Profiler>,
);
},
);

// Flush sync work with a nested upate
ReactNoop.flushSync(() => {
ReactNoop.render(
<React.Profiler id="root" onRender={onRender}>
<Component />
</React.Profiler>,
);
});
expect(Scheduler).toHaveYielded([false, true]);

// Verify that the nested update inside of the sync work is appropriately tagged.
expect(onRender).toHaveBeenCalledTimes(2);
expect(onRender.mock.calls[0][1]).toBe('mount');
expect(onRender.mock.calls[1][1]).toBe('nested-update');
});

describe('with regard to interruptions', () => {
it('should accumulate actual time after a scheduling interruptions', () => {
const callback = jest.fn();
Expand Down Expand Up @@ -2582,14 +2628,50 @@ describe('Profiler', () => {

expect(Scheduler).toHaveYielded(['Component:false', 'Component:true']);
expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(1);
expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
if (ReactFeatureFlags.enableSchedulerTracing) {
expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
expect(onNestedUpdateScheduled.mock.calls[0][1]).toMatchInteractions([
interactionCreation,
]);
}
});

it('is called when a function component schedules a batched update during a layout effect', () => {
function Component() {
const [didMount, setDidMount] = React.useState(false);
React.useLayoutEffect(() => {
ReactNoop.batchedUpdates(() => {
setDidMount(true);
});
}, []);
Scheduler.unstable_yieldValue(`Component:${didMount}`);
return didMount;
}

const onNestedUpdateScheduled = jest.fn();
const onRender = jest.fn();

ReactNoop.render(
<React.Profiler
id="root"
onNestedUpdateScheduled={onNestedUpdateScheduled}
onRender={onRender}>
<Component />
</React.Profiler>,
);
expect(Scheduler).toFlushAndYield([
'Component:false',
'Component:true',
]);

expect(onRender).toHaveBeenCalledTimes(2);
expect(onRender.mock.calls[0][1]).toBe('mount');
expect(onRender.mock.calls[1][1]).toBe('nested-update');

expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(1);
expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('root');
});

it('bubbles up and calls all ancestor Profilers', () => {
function Component() {
const [didMount, setDidMount] = React.useState(false);
Expand Down Expand Up @@ -2819,8 +2901,8 @@ describe('Profiler', () => {
'Component:true:false',
]);
expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(1);
expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
if (ReactFeatureFlags.enableSchedulerTracing) {
expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
expect(onNestedUpdateScheduled.mock.calls[0][1]).toMatchInteractions([
interactionCreation,
]);
Expand Down Expand Up @@ -2853,8 +2935,8 @@ describe('Profiler', () => {
'Component:true:true',
]);
expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(2);
expect(onNestedUpdateScheduled.mock.calls[1][0]).toBe('test');
if (ReactFeatureFlags.enableSchedulerTracing) {
expect(onNestedUpdateScheduled.mock.calls[1][0]).toBe('test');
expect(onNestedUpdateScheduled.mock.calls[1][1]).toMatchInteractions([
interactionUpdate,
]);
Expand Down Expand Up @@ -2902,8 +2984,8 @@ describe('Profiler', () => {

expect(Scheduler).toHaveYielded(['Component:false', 'Component:true']);
expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(1);
expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
if (ReactFeatureFlags.enableSchedulerTracing) {
expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
expect(onNestedUpdateScheduled.mock.calls[0][1]).toMatchInteractions([
interactionCreation,
]);
Expand Down Expand Up @@ -2974,8 +3056,8 @@ describe('Profiler', () => {
'Component:true:true',
]);
expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(1);
expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
if (ReactFeatureFlags.enableSchedulerTracing) {
expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
expect(onNestedUpdateScheduled.mock.calls[0][1]).toMatchInteractions([
interactionCreation,
]);
Expand Down Expand Up @@ -3016,6 +3098,8 @@ describe('Profiler', () => {
expect(Scheduler).toHaveYielded(['Component:true']);
expect(onNestedUpdateScheduled).not.toHaveBeenCalled();
});

// TODO Add hydration tests to ensure we don't have false positives called.
});
});

Expand Down

0 comments on commit e6a0f27

Please sign in to comment.