From c25c59c808d90bfa78c40d0a19527d384a490cc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Wed, 10 Apr 2019 17:16:27 -0700 Subject: [PATCH] Apply the Just Noticeable Difference to suspense timeouts (#15367) * Apply the Just Noticeable Difference boundary * Clamp suspense timeout to expiration time --- .../src/ReactFiberCompleteWork.js | 4 +- .../src/ReactFiberScheduler.new.js | 56 ++++++- .../src/ReactFiberScheduler.old.js | 36 +++- ...tSuspenseWithNoopRenderer-test.internal.js | 156 ++++++++++++++++++ 4 files changed, 239 insertions(+), 13 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 56d9dd5b3c8da..da5385eb3b059 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -692,9 +692,9 @@ function completeWork( // Mark the event time of the switching from fallback to normal children, // based on the start of when we first showed the fallback. This time // was given a normal pri expiration time at the time it was shown. - const fallbackExpirationTimeExpTime: ExpirationTime = + const fallbackExpirationTime: ExpirationTime = prevState.fallbackExpirationTime; - markRenderEventTime(fallbackExpirationTimeExpTime); + markRenderEventTime(fallbackExpirationTime); // Delete the fallback. // TODO: Would it be better to store the fallback fragment on diff --git a/packages/react-reconciler/src/ReactFiberScheduler.new.js b/packages/react-reconciler/src/ReactFiberScheduler.new.js index 669f121c5433f..9c1065a568781 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.new.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.new.js @@ -160,6 +160,8 @@ import { } from 'shared/ReactErrorUtils'; import {onCommitRoot} from './ReactFiberDevToolsHook'; +const ceil = Math.ceil; + const { ReactCurrentDispatcher, ReactCurrentOwner, @@ -893,10 +895,12 @@ function renderRoot( // track any event times. That can happen if we retried but nothing switched // from fallback to content. There's no reason to delay doing no work. if (workInProgressRootMostRecentEventTime !== Sync) { - const msUntilTimeout = computeMsUntilTimeout( + let msUntilTimeout = computeMsUntilTimeout( workInProgressRootMostRecentEventTime, + expirationTime, ); - if (msUntilTimeout > 0) { + // Don't bother with a very short suspense time. + if (msUntilTimeout > 10) { // The render is suspended, it hasn't timed out, and there's no lower // priority work to do. Instead of committing the fallback // immediately, wait for more data to arrive. @@ -1815,7 +1819,35 @@ export function resolveRetryThenable(boundaryFiber: Fiber, thenable: Thenable) { retryTimedOutBoundary(boundaryFiber); } -function computeMsUntilTimeout(mostRecentEventTime: ExpirationTime) { +// Computes the next Just Noticeable Difference (JND) boundary. +// The theory is that a person can't tell the difference between small differences in time. +// Therefore, if we wait a bit longer than necessary that won't translate to a noticeable +// difference in the experience. However, waiting for longer might mean that we can avoid +// showing an intermediate loading state. The longer we have already waited, the harder it +// is to tell small differences in time. Therefore, the longer we've already waited, +// the longer we can wait additionally. At some point we have to give up though. +// We pick a train model where the next boundary commits at a consistent schedule. +// These particular numbers are vague estimates. We expect to adjust them based on research. +function jnd(timeElapsed: number) { + return timeElapsed < 120 + ? 120 + : timeElapsed < 480 + ? 480 + : timeElapsed < 1080 + ? 1080 + : timeElapsed < 1920 + ? 1920 + : timeElapsed < 3000 + ? 3000 + : timeElapsed < 4320 + ? 4320 + : ceil(timeElapsed / 1960) * 1960; +} + +function computeMsUntilTimeout( + mostRecentEventTime: ExpirationTime, + committedExpirationTime: ExpirationTime, +) { if (disableYielding) { // Timeout immediately when yielding is disabled. return 0; @@ -1825,11 +1857,21 @@ function computeMsUntilTimeout(mostRecentEventTime: ExpirationTime) { const currentTimeMs: number = now(); const timeElapsed = currentTimeMs - eventTimeMs; - // TODO: Account for the Just Noticeable Difference - const timeoutMs = 150; - const msUntilTimeout = timeoutMs - timeElapsed; + let msUntilTimeout = jnd(timeElapsed) - timeElapsed; + + // Compute the time until this render pass would expire. + const timeUntilExpirationMs = + expirationTimeToMs(committedExpirationTime) + initialTimeMs - currentTimeMs; + + // Clamp the timeout to the expiration time. + // TODO: Once the event time is exact instead of inferred from expiration time + // we don't need this. + if (timeUntilExpirationMs < msUntilTimeout) { + msUntilTimeout = timeUntilExpirationMs; + } + // This is the value that is passed to `setTimeout`. - return msUntilTimeout < 0 ? 0 : msUntilTimeout; + return msUntilTimeout; } function checkForNestedUpdates() { diff --git a/packages/react-reconciler/src/ReactFiberScheduler.old.js b/packages/react-reconciler/src/ReactFiberScheduler.old.js index 8614c2eb15de7..c4184fbb9465d 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.old.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.old.js @@ -1241,6 +1241,22 @@ function workLoop(isYieldy) { } } +function jnd(timeElapsed: number) { + return timeElapsed < 120 + ? 120 + : timeElapsed < 480 + ? 480 + : timeElapsed < 1080 + ? 1080 + : timeElapsed < 1920 + ? 1920 + : timeElapsed < 3000 + ? 3000 + : timeElapsed < 4320 + ? 4320 + : Math.ceil(timeElapsed / 1960) * 1960; +} + function renderRoot(root: FiberRoot, isYieldy: boolean): void { invariant( !isWorking, @@ -1518,10 +1534,22 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void { const currentTimeMs: number = now(); const timeElapsed = currentTimeMs - eventTimeMs; - // TODO: Account for the Just Noticeable Difference - const timeoutMs = 150; - let msUntilTimeout = timeoutMs - timeElapsed; - msUntilTimeout = msUntilTimeout < 0 ? 0 : msUntilTimeout; + let msUntilTimeout = jnd(timeElapsed) - timeElapsed; + + if (msUntilTimeout < 10) { + // Don't bother with a very short suspense time. + msUntilTimeout = 0; + } else { + // Compute the time until this render pass would expire. + const timeUntilExpirationMs = + expirationTimeToMs(suspendedExpirationTime) + + originalStartTimeMs - + currentTimeMs; + // Clamp the timeout to the expiration time. + if (timeUntilExpirationMs < msUntilTimeout) { + msUntilTimeout = timeUntilExpirationMs; + } + } const rootExpirationTime = root.expirationTime; onSuspend( diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 89f3f2a698c62..46ace8065bfca 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -1668,6 +1668,162 @@ describe('ReactSuspenseWithNoopRenderer', () => { , ); }); + + it('suspends for longer if something took a long (CPU bound) time to render', async () => { + function Foo() { + Scheduler.yieldValue('Foo'); + return ( + }> + + + ); + } + + ReactNoop.render(); + Scheduler.advanceTime(100); + await advanceTimers(100); + // Start rendering + expect(Scheduler).toFlushAndYieldThrough(['Foo']); + // For some reason it took a long time to render Foo. + Scheduler.advanceTime(1250); + await advanceTimers(1250); + expect(Scheduler).toFlushAndYield([ + // A suspends + 'Suspend! [A]', + 'Loading...', + ]); + // We're now suspended and we haven't shown anything yet. + expect(ReactNoop.getChildren()).toEqual([]); + + // Flush some of the time + Scheduler.advanceTime(450); + await advanceTimers(450); + // Because we've already been waiting for so long we can + // wait a bit longer. Still nothing... + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop.getChildren()).toEqual([]); + + // Eventually we'll show the fallback. + Scheduler.advanceTime(500); + await advanceTimers(500); + // No need to rerender. + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); + + // Flush the promise completely + Scheduler.advanceTime(4500); + await advanceTimers(4500); + // Renders successfully + expect(Scheduler).toHaveYielded(['Promise resolved [A]']); + expect(Scheduler).toFlushAndYield(['A']); + expect(ReactNoop.getChildren()).toEqual([span('A')]); + }); + + it('suspends for longer if a fallback has been shown for a long time', async () => { + function Foo() { + Scheduler.yieldValue('Foo'); + return ( + }> + + }> + + + + ); + } + + ReactNoop.render(); + // Start rendering + expect(Scheduler).toFlushAndYield([ + 'Foo', + // A suspends + 'Suspend! [A]', + // B suspends + 'Suspend! [B]', + 'Loading more...', + 'Loading...', + ]); + // We're now suspended and we haven't shown anything yet. + expect(ReactNoop.getChildren()).toEqual([]); + + // Show the fallback. + Scheduler.advanceTime(400); + await advanceTimers(400); + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); + + // Wait a long time. + Scheduler.advanceTime(5000); + await advanceTimers(5000); + expect(Scheduler).toHaveYielded(['Promise resolved [A]']); + + // Retry with the new content. + expect(Scheduler).toFlushAndYield([ + 'A', + // B still suspends + 'Suspend! [B]', + 'Loading more...', + ]); + // Because we've already been waiting for so long we can + // wait a bit longer. Still nothing... + Scheduler.advanceTime(600); + await advanceTimers(600); + expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); + + // Eventually we'll show more content with inner fallback. + Scheduler.advanceTime(3000); + await advanceTimers(3000); + // No need to rerender. + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop.getChildren()).toEqual([ + span('A'), + span('Loading more...'), + ]); + + // Flush the last promise completely + Scheduler.advanceTime(4500); + await advanceTimers(4500); + // Renders successfully + expect(Scheduler).toHaveYielded(['Promise resolved [B]']); + expect(Scheduler).toFlushAndYield(['B']); + expect(ReactNoop.getChildren()).toEqual([span('A'), span('B')]); + }); + + it('does not suspend for very long after a higher priority update', async () => { + function Foo() { + Scheduler.yieldValue('Foo'); + return ( + }> + + + ); + } + + ReactNoop.interactiveUpdates(() => ReactNoop.render()); + expect(Scheduler).toFlushAndYieldThrough(['Foo']); + + // Advance some time. + Scheduler.advanceTime(100); + await advanceTimers(100); + + expect(Scheduler).toFlushAndYield([ + // A suspends + 'Suspend! [A]', + 'Loading...', + ]); + // We're now suspended and we haven't shown anything yet. + expect(ReactNoop.getChildren()).toEqual([]); + + // Flush some of the time + Scheduler.advanceTime(500); + await advanceTimers(500); + // We should have already shown the fallback. + // When we wrote this test, we inferred the start time of high priority + // updates as way earlier in the past. This test ensures that we don't + // use this assumption to add a very long JND. + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); + }); }); // TODO: