The library for web and native user interfaces.
Mirrored from https://github.com/247TaxiDriver/QOS-React.git
Synced Nov 14, 05:30 UTC
Fix Suspense throttling mechanism (#26802)
The throttling mechanism for fallbacks should apply to both their appearance _and_ disappearance. This was mostly addressed by #26611. See that PR for additional context. However, a flaw in the implementation is that we only update the the timestamp used for throttling when the fallback initially appears. We don't update it when the real content pops in. If lots of content in separate Suspense trees loads around the same time, you can still get jank. The issue is fixed by updating the throttling timestamp whenever the visibility of a fallback changes. Not just when it appears.
Andrew Clark committed
May 16, 2023 at 15:03 UTC
4bfcd02b2cebcb390f5aff0d7747c60a55012d5d
3 files changed
+128
-11
packages/react-reconciler/src/ReactFiberCommitWork.js
+28
-9
@@ -55,6 +55,7 @@ import {
55
enableLegacyHidden,
56
enableHostSingletons,
57
diffInCommitPhase,
58
+ alwaysThrottleRetries,
59
} from 'shared/ReactFeatureFlags';
60
import {
61
FunctionComponent,
@@ -2905,17 +2906,35 @@ function commitMutationEffectsOnFiber(
2906
recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2907
commitReconciliationEffects(finishedWork);
2908
2909
+ // TODO: We should mark a flag on the Suspense fiber itself, rather than
2910
+ // relying on the Offscreen fiber having a flag also being marked. The
2911
+ // reason is that this offscreen fiber might not be part of the work-in-
2912
+ // progress tree! It could have been reused from a previous render. This
2913
+ // doesn't lead to incorrect behavior because we don't rely on the flag
2914
+ // check alone; we also compare the states explicitly below. But for
2915
+ // modeling purposes, we _should_ be able to rely on the flag check alone.
2916
+ // So this is a bit fragile.
2917
+ //
2918
+ // Also, all this logic could/should move to the passive phase so it
2919
+ // doesn't block paint.
2920
const offscreenFiber: Fiber = (finishedWork.child: any);
2909
-
2921
if (offscreenFiber.flags & Visibility) {
2911
- const newState: OffscreenState | null = offscreenFiber.memoizedState;
2912
- const isHidden = newState !== null;
2913
- if (isHidden) {
2914
- const wasHidden =
2915
- offscreenFiber.alternate !== null &&
2916
- offscreenFiber.alternate.memoizedState !== null;
2917
- if (!wasHidden) {
2918
- // TODO: Move to passive phase
2922
+ // Throttle the appearance and disappearance of Suspense fallbacks.
2923
+ const isShowingFallback =
2924
+ (finishedWork.memoizedState: SuspenseState | null) !== null;
2925
+ const wasShowingFallback =
2926
+ current !== null &&
2927
+ (current.memoizedState: SuspenseState | null) !== null;
2928
+
2929
+ if (alwaysThrottleRetries) {
2930
+ if (isShowingFallback !== wasShowingFallback) {
2931
+ // A fallback is either appearing or disappearing.
2932
+ markCommitTimeOfFallback();
2933
+ }
2934
+ } else {
2935
+ if (isShowingFallback && !wasShowingFallback) {
2936
+ // Old behavior. Only mark when a fallback appears, not when
2937
+ // it disappears.
2938
markCommitTimeOfFallback();
2939
}
2940
}
packages/react-reconciler/src/ReactFiberWorkLoop.js
+4
-2
@@ -370,8 +370,10 @@ let workInProgressRootConcurrentErrors: Array<CapturedValue<mixed>> | null =
370
let workInProgressRootRecoverableErrors: Array<CapturedValue<mixed>> | null =
371
null;
372
373
-// The most recent time we committed a fallback. This lets us ensure a train
374
-// model where we don't commit new loading states in too quick succession.
373
+// The most recent time we either committed a fallback, or when a fallback was
374
+// filled in with the resolved UI. This lets us throttle the appearance of new
375
+// content as it streams in, to minimize jank.
376
+// TODO: Think of a better name for this variable?
377
let globalMostRecentFallbackTime: number = 0;
378
const FALLBACK_THROTTLE_MS: number = 500;
379
packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
+96
@@ -1811,6 +1811,102 @@ describe('ReactSuspenseWithNoopRenderer', () => {
1811
);
1812
});
1813
1814
+ // @gate enableLegacyCache
1815
+ it('throttles content from appearing if a fallback was filled in recently', async () => {
1816
+ function Foo() {
1817
+ Scheduler.log('Foo');
1818
+ return (
1819
+ <>
1820
+ <Suspense fallback={<Text text="Loading A..." />}>
1821
+ <AsyncText text="A" />
1822
+ </Suspense>
1823
+ <Suspense fallback={<Text text="Loading B..." />}>
1824
+ <AsyncText text="B" />
1825
+ </Suspense>
1826
+ </>
1827
+ );
1828
+ }
1829
+
1830
+ ReactNoop.render(<Foo />);
1831
+ // Start rendering
1832
+ await waitForAll([
1833
+ 'Foo',
1834
+ 'Suspend! [A]',
1835
+ 'Loading A...',
1836
+ 'Suspend! [B]',
1837
+ 'Loading B...',
1838
+ ]);
1839
+ expect(ReactNoop).toMatchRenderedOutput(
1840
+ <>
1841
+ <span prop="Loading A..." />
1842
+ <span prop="Loading B..." />
1843
+ </>,
1844
+ );
1845
+
1846
+ // Resolve only A. B will still be loading.
1847
+ await act(async () => {
1848
+ await resolveText('A');
1849
+
1850
+ // If we didn't advance the time here, A would not commit; it would
1851
+ // be throttled because the fallback would have appeared too recently.
1852
+ Scheduler.unstable_advanceTime(10000);
1853
+ jest.advanceTimersByTime(10000);
1854
+ await waitForPaint(['A']);
1855
+ expect(ReactNoop).toMatchRenderedOutput(
1856
+ <>
1857
+ <span prop="A" />
1858
+ <span prop="Loading B..." />
1859
+ </>,
1860
+ );
1861
+ });
1862
+
1863
+ // Advance by a small amount of time. For testing purposes, this is meant
1864
+ // to be just under the throttling interval. It's a heurstic, though, so
1865
+ // if we adjust the heuristic we might have to update this test, too.
1866
+ Scheduler.unstable_advanceTime(400);
1867
+ jest.advanceTimersByTime(400);
1868
+
1869
+ // Now resolve B.
1870
+ await act(async () => {
1871
+ await resolveText('B');
1872
+ await waitForPaint(['B']);
1873
+
1874
+ if (gate(flags => flags.alwaysThrottleRetries)) {
1875
+ // B should not commit yet. Even though it's been a long time since its
1876
+ // fallback was shown, it hasn't been long since A appeared. So B's
1877
+ // appearance is throttled to reduce jank.
1878
+ expect(ReactNoop).toMatchRenderedOutput(
1879
+ <>
1880
+ <span prop="A" />
1881
+ <span prop="Loading B..." />
1882
+ </>,
1883
+ );
1884
+
1885
+ // Advance time a little bit more. Now it commits because enough time
1886
+ // has passed.
1887
+ Scheduler.unstable_advanceTime(100);
1888
+ jest.advanceTimersByTime(100);
1889
+ await waitForAll([]);
1890
+ expect(ReactNoop).toMatchRenderedOutput(
1891
+ <>
1892
+ <span prop="A" />
1893
+ <span prop="B" />
1894
+ </>,
1895
+ );
1896
+ } else {
1897
+ // Old behavior, gated until this rolls out at Meta:
1898
+ //
1899
+ // B appears immediately, without being throttled.
1900
+ expect(ReactNoop).toMatchRenderedOutput(
1901
+ <>
1902
+ <span prop="A" />
1903
+ <span prop="B" />
1904
+ </>,
1905
+ );
1906
+ }
1907
+ });
1908
+ });
1909
+
1910
// TODO: flip to "warns" when this is implemented again.
1911
// @gate enableLegacyCache
1912
it('does not warn when a low priority update suspends inside a high priority update for functional components', async () => {