Profiler: Improve nested-update checks (#20299)
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.
Brian Vaughn committed
Nov 20, 2020 at 14:46 UTC
e6a0f276307fcb2f1c5bc41d630c5e4c9e95a037
3 files changed
+93
-9
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+2
-2
@@ -542,7 +542,7 @@ export function scheduleUpdateOnFiber(
542
543
if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) {
544
if (
545
- executionContext === CommitContext &&
545
+ (executionContext & CommitContext) !== NoContext &&
546
root === rootCommittingMutationOrLayoutEffects
547
) {
548
if (fiber.mode & ProfileMode) {
@@ -2240,7 +2240,7 @@ function commitRootImpl(root, renderPriorityLevel) {
2240
}
2241
}
2242
2243
- if (remainingLanes === SyncLane) {
2243
+ if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
2244
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
2245
markNestedUpdateScheduled();
2246
}
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+2
-2
@@ -542,7 +542,7 @@ export function scheduleUpdateOnFiber(
542
543
if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) {
544
if (
545
- executionContext === CommitContext &&
545
+ (executionContext & CommitContext) !== NoContext &&
546
root === rootCommittingMutationOrLayoutEffects
547
) {
548
if (fiber.mode & ProfileMode) {
@@ -2240,7 +2240,7 @@ function commitRootImpl(root, renderPriorityLevel) {
2240
}
2241
}
2242
2243
- if (remainingLanes === SyncLane) {
2243
+ if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
2244
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
2245
markNestedUpdateScheduled();
2246
}
packages/react/src/__tests__/ReactProfiler-test.internal.js
+89
-5
@@ -748,6 +748,52 @@ describe('Profiler', () => {
748
expect(onRender.mock.calls[2][1]).toBe('update');
749
});
750
751
+ it('is properly distinguish updates and nested-updates when there is more than sync remaining work', () => {
752
+ loadModules({
753
+ enableSchedulerTracing,
754
+ useNoopRenderer: true,
755
+ });
756
+
757
+ function Component() {
758
+ const [didMount, setDidMount] = React.useState(false);
759
+
760
+ React.useLayoutEffect(() => {
761
+ setDidMount(true);
762
+ }, []);
763
+ Scheduler.unstable_yieldValue(didMount);
764
+ return didMount;
765
+ }
766
+
767
+ const onRender = jest.fn();
768
+
769
+ // Schedule low-priority work.
770
+ Scheduler.unstable_runWithPriority(
771
+ Scheduler.unstable_LowPriority,
772
+ () => {
773
+ ReactNoop.render(
774
+ <React.Profiler id="root" onRender={onRender}>
775
+ <Component />
776
+ </React.Profiler>,
777
+ );
778
+ },
779
+ );
780
+
781
+ // Flush sync work with a nested upate
782
+ ReactNoop.flushSync(() => {
783
+ ReactNoop.render(
784
+ <React.Profiler id="root" onRender={onRender}>
785
+ <Component />
786
+ </React.Profiler>,
787
+ );
788
+ });
789
+ expect(Scheduler).toHaveYielded([false, true]);
790
+
791
+ // Verify that the nested update inside of the sync work is appropriately tagged.
792
+ expect(onRender).toHaveBeenCalledTimes(2);
793
+ expect(onRender.mock.calls[0][1]).toBe('mount');
794
+ expect(onRender.mock.calls[1][1]).toBe('nested-update');
795
+ });
796
+
797
describe('with regard to interruptions', () => {
798
it('should accumulate actual time after a scheduling interruptions', () => {
799
const callback = jest.fn();
@@ -2582,14 +2628,50 @@ describe('Profiler', () => {
2628
2629
expect(Scheduler).toHaveYielded(['Component:false', 'Component:true']);
2630
expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(1);
2631
+ expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
2632
if (ReactFeatureFlags.enableSchedulerTracing) {
2586
- expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
2633
expect(onNestedUpdateScheduled.mock.calls[0][1]).toMatchInteractions([
2634
interactionCreation,
2635
]);
2636
}
2637
});
2638
2639
+ it('is called when a function component schedules a batched update during a layout effect', () => {
2640
+ function Component() {
2641
+ const [didMount, setDidMount] = React.useState(false);
2642
+ React.useLayoutEffect(() => {
2643
+ ReactNoop.batchedUpdates(() => {
2644
+ setDidMount(true);
2645
+ });
2646
+ }, []);
2647
+ Scheduler.unstable_yieldValue(`Component:${didMount}`);
2648
+ return didMount;
2649
+ }
2650
+
2651
+ const onNestedUpdateScheduled = jest.fn();
2652
+ const onRender = jest.fn();
2653
+
2654
+ ReactNoop.render(
2655
+ <React.Profiler
2656
+ id="root"
2657
+ onNestedUpdateScheduled={onNestedUpdateScheduled}
2658
+ onRender={onRender}>
2659
+ <Component />
2660
+ </React.Profiler>,
2661
+ );
2662
+ expect(Scheduler).toFlushAndYield([
2663
+ 'Component:false',
2664
+ 'Component:true',
2665
+ ]);
2666
+
2667
+ expect(onRender).toHaveBeenCalledTimes(2);
2668
+ expect(onRender.mock.calls[0][1]).toBe('mount');
2669
+ expect(onRender.mock.calls[1][1]).toBe('nested-update');
2670
+
2671
+ expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(1);
2672
+ expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('root');
2673
+ });
2674
+
2675
it('bubbles up and calls all ancestor Profilers', () => {
2676
function Component() {
2677
const [didMount, setDidMount] = React.useState(false);
@@ -2819,8 +2901,8 @@ describe('Profiler', () => {
2901
'Component:true:false',
2902
]);
2903
expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(1);
2904
+ expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
2905
if (ReactFeatureFlags.enableSchedulerTracing) {
2823
- expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
2906
expect(onNestedUpdateScheduled.mock.calls[0][1]).toMatchInteractions([
2907
interactionCreation,
2908
]);
@@ -2853,8 +2935,8 @@ describe('Profiler', () => {
2935
'Component:true:true',
2936
]);
2937
expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(2);
2938
+ expect(onNestedUpdateScheduled.mock.calls[1][0]).toBe('test');
2939
if (ReactFeatureFlags.enableSchedulerTracing) {
2857
- expect(onNestedUpdateScheduled.mock.calls[1][0]).toBe('test');
2940
expect(onNestedUpdateScheduled.mock.calls[1][1]).toMatchInteractions([
2941
interactionUpdate,
2942
]);
@@ -2902,8 +2984,8 @@ describe('Profiler', () => {
2984
2985
expect(Scheduler).toHaveYielded(['Component:false', 'Component:true']);
2986
expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(1);
2987
+ expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
2988
if (ReactFeatureFlags.enableSchedulerTracing) {
2906
- expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
2989
expect(onNestedUpdateScheduled.mock.calls[0][1]).toMatchInteractions([
2990
interactionCreation,
2991
]);
@@ -2974,8 +3056,8 @@ describe('Profiler', () => {
3056
'Component:true:true',
3057
]);
3058
expect(onNestedUpdateScheduled).toHaveBeenCalledTimes(1);
3059
+ expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
3060
if (ReactFeatureFlags.enableSchedulerTracing) {
2978
- expect(onNestedUpdateScheduled.mock.calls[0][0]).toBe('test');
3061
expect(onNestedUpdateScheduled.mock.calls[0][1]).toMatchInteractions([
3062
interactionCreation,
3063
]);
@@ -3016,6 +3098,8 @@ describe('Profiler', () => {
3098
expect(Scheduler).toHaveYielded(['Component:true']);
3099
expect(onNestedUpdateScheduled).not.toHaveBeenCalled();
3100
});
3101
+
3102
+ // TODO Add hydration tests to ensure we don't have false positives called.
3103
});
3104
});
3105