Skip to content

Commit

Permalink
Properly reset Profiler nested-update flag (#20253)
Browse files Browse the repository at this point in the history
Previously this flag was not being reset correctly if a concurrent update followed a nested (sync) update. This PR fixes the behavior and adds a regression test.
  • Loading branch information
Brian Vaughn authored Nov 13, 2020
1 parent bd8bc5a commit 7548dd5
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 0 deletions.
5 changes: 5 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ import {
import {
markNestedUpdateScheduled,
recordCommitTime,
resetNestedUpdateFlag,
startProfilerTimer,
stopProfilerTimerIfRunningAndRecordDelta,
syncNestedUpdateFlag,
Expand Down Expand Up @@ -746,6 +747,10 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
// This is the entry point for every concurrent task, i.e. anything that
// goes through Scheduler.
function performConcurrentWorkOnRoot(root) {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
resetNestedUpdateFlag();
}

// Since we know we're in a React event, we can clear the current
// event time. The next update will compute a new event time.
currentEventTime = NoTimestamp;
Expand Down
5 changes: 5 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ import {
markNestedUpdateScheduled,
recordCommitTime,
recordPassiveEffectDuration,
resetNestedUpdateFlag,
startPassiveEffectTimer,
startProfilerTimer,
stopProfilerTimerIfRunningAndRecordDelta,
Expand Down Expand Up @@ -770,6 +771,10 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
// This is the entry point for every concurrent task, i.e. anything that
// goes through Scheduler.
function performConcurrentWorkOnRoot(root) {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
resetNestedUpdateFlag();
}

// Since we know we're in a React event, we can clear the current
// event time. The next update will compute a new event time.
currentEventTime = NoTimestamp;
Expand Down
8 changes: 8 additions & 0 deletions packages/react-reconciler/src/ReactProfilerTimer.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ function markNestedUpdateScheduled(): void {
}
}

function resetNestedUpdateFlag(): void {
if (enableProfilerNestedUpdatePhase) {
currentUpdateIsNested = false;
nestedUpdateScheduled = false;
}
}

function syncNestedUpdateFlag(): void {
if (enableProfilerNestedUpdatePhase) {
currentUpdateIsNested = nestedUpdateScheduled;
Expand Down Expand Up @@ -206,6 +213,7 @@ export {
recordCommitTime,
recordLayoutEffectDuration,
recordPassiveEffectDuration,
resetNestedUpdateFlag,
startLayoutEffectTimer,
startPassiveEffectTimer,
startProfilerTimer,
Expand Down
8 changes: 8 additions & 0 deletions packages/react-reconciler/src/ReactProfilerTimer.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ function markNestedUpdateScheduled(): void {
}
}

function resetNestedUpdateFlag(): void {
if (enableProfilerNestedUpdatePhase) {
currentUpdateIsNested = false;
nestedUpdateScheduled = false;
}
}

function syncNestedUpdateFlag(): void {
if (enableProfilerNestedUpdatePhase) {
currentUpdateIsNested = nestedUpdateScheduled;
Expand Down Expand Up @@ -206,6 +213,7 @@ export {
recordCommitTime,
recordLayoutEffectDuration,
recordPassiveEffectDuration,
resetNestedUpdateFlag,
startLayoutEffectTimer,
startPassiveEffectTimer,
startProfilerTimer,
Expand Down
48 changes: 48 additions & 0 deletions packages/react/src/__tests__/ReactProfiler-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,54 @@ describe('Profiler', () => {
expect(updateCall[5]).toBe(43); // commit time
});

it('should clear nested-update flag when multiple cascading renders are scheduled', () => {
loadModules({
enableSchedulerTracing,
useNoopRenderer: true,
});

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

React.useLayoutEffect(() => {
setDidMount(true);
}, []);

React.useEffect(() => {
if (didMount && !didMountAndUpdate) {
setDidMountAndUpdate(true);
}
}, [didMount, didMountAndUpdate]);

Scheduler.unstable_yieldValue(`${didMount}:${didMountAndUpdate}`);

return null;
}

const onRender = jest.fn();

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

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

describe('with regard to interruptions', () => {
it('should accumulate actual time after a scheduling interruptions', () => {
const callback = jest.fn();
Expand Down

0 comments on commit 7548dd5

Please sign in to comment.