@samitouri / QOS-React-2 / commits / 7548dd573e

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.

Brian Vaughn committed Nov 13, 2020 at 14:29 UTC 7548dd573edae9444e2f3c8c15691c5caefdc3c1
5 files changed +74
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+5
@@ -199,6 +199,7 @@ import {
199 import {
200 markNestedUpdateScheduled,
201 recordCommitTime,
202 + resetNestedUpdateFlag,
203 startProfilerTimer,
204 stopProfilerTimerIfRunningAndRecordDelta,
205 syncNestedUpdateFlag,
@@ -746,6 +747,10 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
747 // This is the entry point for every concurrent task, i.e. anything that
748 // goes through Scheduler.
749 function performConcurrentWorkOnRoot(root) {
750 + if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
751 + resetNestedUpdateFlag();
752 + }
753 +
754 // Since we know we're in a React event, we can clear the current
755 // event time. The next update will compute a new event time.
756 currentEventTime = NoTimestamp;
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+5
@@ -213,6 +213,7 @@ import {
213 markNestedUpdateScheduled,
214 recordCommitTime,
215 recordPassiveEffectDuration,
216 + resetNestedUpdateFlag,
217 startPassiveEffectTimer,
218 startProfilerTimer,
219 stopProfilerTimerIfRunningAndRecordDelta,
@@ -770,6 +771,10 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
771 // This is the entry point for every concurrent task, i.e. anything that
772 // goes through Scheduler.
773 function performConcurrentWorkOnRoot(root) {
774 + if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
775 + resetNestedUpdateFlag();
776 + }
777 +
778 // Since we know we're in a React event, we can clear the current
779 // event time. The next update will compute a new event time.
780 currentEventTime = NoTimestamp;
packages/react-reconciler/src/ReactProfilerTimer.new.js
+8
@@ -68,6 +68,13 @@ function markNestedUpdateScheduled(): void {
68 }
69 }
70
71 +function resetNestedUpdateFlag(): void {
72 + if (enableProfilerNestedUpdatePhase) {
73 + currentUpdateIsNested = false;
74 + nestedUpdateScheduled = false;
75 + }
76 +}
77 +
78 function syncNestedUpdateFlag(): void {
79 if (enableProfilerNestedUpdatePhase) {
80 currentUpdateIsNested = nestedUpdateScheduled;
@@ -206,6 +213,7 @@ export {
213 recordCommitTime,
214 recordLayoutEffectDuration,
215 recordPassiveEffectDuration,
216 + resetNestedUpdateFlag,
217 startLayoutEffectTimer,
218 startPassiveEffectTimer,
219 startProfilerTimer,
packages/react-reconciler/src/ReactProfilerTimer.old.js
+8
@@ -68,6 +68,13 @@ function markNestedUpdateScheduled(): void {
68 }
69 }
70
71 +function resetNestedUpdateFlag(): void {
72 + if (enableProfilerNestedUpdatePhase) {
73 + currentUpdateIsNested = false;
74 + nestedUpdateScheduled = false;
75 + }
76 +}
77 +
78 function syncNestedUpdateFlag(): void {
79 if (enableProfilerNestedUpdatePhase) {
80 currentUpdateIsNested = nestedUpdateScheduled;
@@ -206,6 +213,7 @@ export {
213 recordCommitTime,
214 recordLayoutEffectDuration,
215 recordPassiveEffectDuration,
216 + resetNestedUpdateFlag,
217 startLayoutEffectTimer,
218 startPassiveEffectTimer,
219 startProfilerTimer,
packages/react/src/__tests__/ReactProfiler-test.internal.js
+48
@@ -700,6 +700,54 @@ describe('Profiler', () => {
700 expect(updateCall[5]).toBe(43); // commit time
701 });
702
703 + it('should clear nested-update flag when multiple cascading renders are scheduled', () => {
704 + loadModules({
705 + enableSchedulerTracing,
706 + useNoopRenderer: true,
707 + });
708 +
709 + function Component() {
710 + const [didMount, setDidMount] = React.useState(false);
711 + const [didMountAndUpdate, setDidMountAndUpdate] = React.useState(
712 + false,
713 + );
714 +
715 + React.useLayoutEffect(() => {
716 + setDidMount(true);
717 + }, []);
718 +
719 + React.useEffect(() => {
720 + if (didMount && !didMountAndUpdate) {
721 + setDidMountAndUpdate(true);
722 + }
723 + }, [didMount, didMountAndUpdate]);
724 +
725 + Scheduler.unstable_yieldValue(`${didMount}:${didMountAndUpdate}`);
726 +
727 + return null;
728 + }
729 +
730 + const onRender = jest.fn();
731 +
732 + ReactNoop.act(() => {
733 + ReactNoop.render(
734 + <React.Profiler id="root" onRender={onRender}>
735 + <Component />
736 + </React.Profiler>,
737 + );
738 + });
739 + expect(Scheduler).toHaveYielded([
740 + 'false:false',
741 + 'true:false',
742 + 'true:true',
743 + ]);
744 +
745 + expect(onRender).toHaveBeenCalledTimes(3);
746 + expect(onRender.mock.calls[0][1]).toBe('mount');
747 + expect(onRender.mock.calls[1][1]).toBe('nested-update');
748 + expect(onRender.mock.calls[2][1]).toBe('update');
749 + });
750 +
751 describe('with regard to interruptions', () => {
752 it('should accumulate actual time after a scheduling interruptions', () => {
753 const callback = jest.fn();