Skip to content

Commit 7548dd5

Browse files
author
Brian Vaughn
authored
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.
1 parent bd8bc5a commit 7548dd5

5 files changed

Lines changed: 74 additions & 0 deletions

File tree

‎packages/react-reconciler/src/ReactFiberWorkLoop.new.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,7 @@ import {
199199
import {
200200
markNestedUpdateScheduled,
201201
recordCommitTime,
202+
resetNestedUpdateFlag,
202203
startProfilerTimer,
203204
stopProfilerTimerIfRunningAndRecordDelta,
204205
syncNestedUpdateFlag,
@@ -746,6 +747,10 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
746747
// This is the entry point for every concurrent task, i.e. anything that
747748
// goes through Scheduler.
748749
function performConcurrentWorkOnRoot(root) {
750+
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
751+
resetNestedUpdateFlag();
752+
}
753+
749754
// Since we know we're in a React event, we can clear the current
750755
// event time. The next update will compute a new event time.
751756
currentEventTime = NoTimestamp;

‎packages/react-reconciler/src/ReactFiberWorkLoop.old.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,7 @@ import {
213213
markNestedUpdateScheduled,
214214
recordCommitTime,
215215
recordPassiveEffectDuration,
216+
resetNestedUpdateFlag,
216217
startPassiveEffectTimer,
217218
startProfilerTimer,
218219
stopProfilerTimerIfRunningAndRecordDelta,
@@ -770,6 +771,10 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
770771
// This is the entry point for every concurrent task, i.e. anything that
771772
// goes through Scheduler.
772773
function performConcurrentWorkOnRoot(root) {
774+
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
775+
resetNestedUpdateFlag();
776+
}
777+
773778
// Since we know we're in a React event, we can clear the current
774779
// event time. The next update will compute a new event time.
775780
currentEventTime = NoTimestamp;

‎packages/react-reconciler/src/ReactProfilerTimer.new.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,13 @@ function markNestedUpdateScheduled(): void {
6868
}
6969
}
7070

71+
function resetNestedUpdateFlag(): void {
72+
if (enableProfilerNestedUpdatePhase) {
73+
currentUpdateIsNested = false;
74+
nestedUpdateScheduled = false;
75+
}
76+
}
77+
7178
function syncNestedUpdateFlag(): void {
7279
if (enableProfilerNestedUpdatePhase) {
7380
currentUpdateIsNested = nestedUpdateScheduled;
@@ -206,6 +213,7 @@ export {
206213
recordCommitTime,
207214
recordLayoutEffectDuration,
208215
recordPassiveEffectDuration,
216+
resetNestedUpdateFlag,
209217
startLayoutEffectTimer,
210218
startPassiveEffectTimer,
211219
startProfilerTimer,

‎packages/react-reconciler/src/ReactProfilerTimer.old.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,13 @@ function markNestedUpdateScheduled(): void {
6868
}
6969
}
7070

71+
function resetNestedUpdateFlag(): void {
72+
if (enableProfilerNestedUpdatePhase) {
73+
currentUpdateIsNested = false;
74+
nestedUpdateScheduled = false;
75+
}
76+
}
77+
7178
function syncNestedUpdateFlag(): void {
7279
if (enableProfilerNestedUpdatePhase) {
7380
currentUpdateIsNested = nestedUpdateScheduled;
@@ -206,6 +213,7 @@ export {
206213
recordCommitTime,
207214
recordLayoutEffectDuration,
208215
recordPassiveEffectDuration,
216+
resetNestedUpdateFlag,
209217
startLayoutEffectTimer,
210218
startPassiveEffectTimer,
211219
startProfilerTimer,

‎packages/react/src/__tests__/ReactProfiler-test.internal.js‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -700,6 +700,54 @@ describe('Profiler', () => {
700700
expect(updateCall[5]).toBe(43); // commit time
701701
});
702702

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+
703751
describe('with regard to interruptions', () => {
704752
it('should accumulate actual time after a scheduling interruptions', () => {
705753
const callback = jest.fn();

0 commit comments

Comments
 (0)