From e1c2b5da2cb467e929e311f975c847c98b8da8ef Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 11 May 2023 00:28:14 -0400 Subject: [PATCH] Fix Suspense throttling mechanism 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. --- .../src/ReactFiberCommitWork.js | 37 +++++-- .../src/ReactFiberWorkLoop.js | 6 +- .../ReactSuspenseWithNoopRenderer-test.js | 96 +++++++++++++++++++ 3 files changed, 128 insertions(+), 11 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index b540929ce0352..1960a4707e244 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -55,6 +55,7 @@ import { enableLegacyHidden, enableHostSingletons, diffInCommitPhase, + alwaysThrottleRetries, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -2905,17 +2906,35 @@ function commitMutationEffectsOnFiber( recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); + // TODO: We should mark a flag on the Suspense fiber itself, rather than + // relying on the Offscreen fiber having a flag also being marked. The + // reason is that this offscreen fiber might not be part of the work-in- + // progress tree! It could have been reused from a previous render. This + // doesn't lead to incorrect behavior because we don't rely on the flag + // check alone; we also compare the states explicitly below. But for + // modeling purposes, we _should_ be able to rely on the flag check alone. + // So this is a bit fragile. + // + // Also, all this logic could/should move to the passive phase so it + // doesn't block paint. const offscreenFiber: Fiber = (finishedWork.child: any); - if (offscreenFiber.flags & Visibility) { - const newState: OffscreenState | null = offscreenFiber.memoizedState; - const isHidden = newState !== null; - if (isHidden) { - const wasHidden = - offscreenFiber.alternate !== null && - offscreenFiber.alternate.memoizedState !== null; - if (!wasHidden) { - // TODO: Move to passive phase + // Throttle the appearance and disappearance of Suspense fallbacks. + const isShowingFallback = + (finishedWork.memoizedState: SuspenseState | null) !== null; + const wasShowingFallback = + current !== null && + (current.memoizedState: SuspenseState | null) !== null; + + if (alwaysThrottleRetries) { + if (isShowingFallback !== wasShowingFallback) { + // A fallback is either appearing or disappearing. + markCommitTimeOfFallback(); + } + } else { + if (isShowingFallback && !wasShowingFallback) { + // Old behavior. Only mark when a fallback appears, not when + // it disappears. markCommitTimeOfFallback(); } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index f0a0d4e228160..f6d1d7f7a5849 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -370,8 +370,10 @@ let workInProgressRootConcurrentErrors: Array> | null = let workInProgressRootRecoverableErrors: Array> | null = null; -// The most recent time we committed a fallback. This lets us ensure a train -// model where we don't commit new loading states in too quick succession. +// The most recent time we either committed a fallback, or when a fallback was +// filled in with the resolved UI. This lets us throttle the appearance of new +// content as it streams in, to minimize jank. +// TODO: Think of a better name for this variable? let globalMostRecentFallbackTime: number = 0; const FALLBACK_THROTTLE_MS: number = 500; diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index e6019ffd40583..fc1aa38708b8d 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -1811,6 +1811,102 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); }); + // @gate enableLegacyCache + it('throttles content from appearing if a fallback was filled in recently', async () => { + function Foo() { + Scheduler.log('Foo'); + return ( + <> + }> + + + }> + + + + ); + } + + ReactNoop.render(); + // Start rendering + await waitForAll([ + 'Foo', + 'Suspend! [A]', + 'Loading A...', + 'Suspend! [B]', + 'Loading B...', + ]); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + + // Resolve only A. B will still be loading. + await act(async () => { + await resolveText('A'); + + // If we didn't advance the time here, A would not commit; it would + // be throttled because the fallback would have appeared too recently. + Scheduler.unstable_advanceTime(10000); + jest.advanceTimersByTime(10000); + await waitForPaint(['A']); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + }); + + // Advance by a small amount of time. For testing purposes, this is meant + // to be just under the throttling interval. It's a heurstic, though, so + // if we adjust the heuristic we might have to update this test, too. + Scheduler.unstable_advanceTime(400); + jest.advanceTimersByTime(400); + + // Now resolve B. + await act(async () => { + await resolveText('B'); + await waitForPaint(['B']); + + if (gate(flags => flags.alwaysThrottleRetries)) { + // B should not commit yet. Even though it's been a long time since its + // fallback was shown, it hasn't been long since A appeared. So B's + // appearance is throttled to reduce jank. + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + + // Advance time a little bit more. Now it commits because enough time + // has passed. + Scheduler.unstable_advanceTime(100); + jest.advanceTimersByTime(100); + await waitForAll([]); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + } else { + // Old behavior, gated until this rolls out at Meta: + // + // B appears immediately, without being throttled. + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + } + }); + }); + // TODO: flip to "warns" when this is implemented again. // @gate enableLegacyCache it('does not warn when a low priority update suspends inside a high priority update for functional components', async () => {