From 7548dd573edae9444e2f3c8c15691c5caefdc3c1 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 13 Nov 2020 14:29:06 -0500 Subject: [PATCH] Properly reset Profiler nested-update flag (#20253) 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. --- .../src/ReactFiberWorkLoop.new.js | 5 ++ .../src/ReactFiberWorkLoop.old.js | 5 ++ .../src/ReactProfilerTimer.new.js | 8 ++++ .../src/ReactProfilerTimer.old.js | 8 ++++ .../__tests__/ReactProfiler-test.internal.js | 48 +++++++++++++++++++ 5 files changed, 74 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 4ccd2ea67e8b..17346a73660f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -199,6 +199,7 @@ import { import { markNestedUpdateScheduled, recordCommitTime, + resetNestedUpdateFlag, startProfilerTimer, stopProfilerTimerIfRunningAndRecordDelta, syncNestedUpdateFlag, @@ -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; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 609bd8016b9c..b4852f120db9 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -213,6 +213,7 @@ import { markNestedUpdateScheduled, recordCommitTime, recordPassiveEffectDuration, + resetNestedUpdateFlag, startPassiveEffectTimer, startProfilerTimer, stopProfilerTimerIfRunningAndRecordDelta, @@ -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; diff --git a/packages/react-reconciler/src/ReactProfilerTimer.new.js b/packages/react-reconciler/src/ReactProfilerTimer.new.js index 95395b6898de..235074b78ad8 100644 --- a/packages/react-reconciler/src/ReactProfilerTimer.new.js +++ b/packages/react-reconciler/src/ReactProfilerTimer.new.js @@ -68,6 +68,13 @@ function markNestedUpdateScheduled(): void { } } +function resetNestedUpdateFlag(): void { + if (enableProfilerNestedUpdatePhase) { + currentUpdateIsNested = false; + nestedUpdateScheduled = false; + } +} + function syncNestedUpdateFlag(): void { if (enableProfilerNestedUpdatePhase) { currentUpdateIsNested = nestedUpdateScheduled; @@ -206,6 +213,7 @@ export { recordCommitTime, recordLayoutEffectDuration, recordPassiveEffectDuration, + resetNestedUpdateFlag, startLayoutEffectTimer, startPassiveEffectTimer, startProfilerTimer, diff --git a/packages/react-reconciler/src/ReactProfilerTimer.old.js b/packages/react-reconciler/src/ReactProfilerTimer.old.js index 95395b6898de..235074b78ad8 100644 --- a/packages/react-reconciler/src/ReactProfilerTimer.old.js +++ b/packages/react-reconciler/src/ReactProfilerTimer.old.js @@ -68,6 +68,13 @@ function markNestedUpdateScheduled(): void { } } +function resetNestedUpdateFlag(): void { + if (enableProfilerNestedUpdatePhase) { + currentUpdateIsNested = false; + nestedUpdateScheduled = false; + } +} + function syncNestedUpdateFlag(): void { if (enableProfilerNestedUpdatePhase) { currentUpdateIsNested = nestedUpdateScheduled; @@ -206,6 +213,7 @@ export { recordCommitTime, recordLayoutEffectDuration, recordPassiveEffectDuration, + resetNestedUpdateFlag, startLayoutEffectTimer, startPassiveEffectTimer, startProfilerTimer, diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index 92b30d9280fa..831861941a24 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -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( + + + , + ); + }); + 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();