Apply the Just Noticeable Difference to suspense timeouts (#15367)

* Apply the Just Noticeable Difference boundary

* Clamp suspense timeout to expiration time

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

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() {

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(

packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js

@@ -1668,6 +1668,162 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       </React.Fragment>,
     );
   });
+
+  it('suspends for longer if something took a long (CPU bound) time to render', async () => {
+    function Foo() {
+      Scheduler.yieldValue('Foo');
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <AsyncText text="A" ms={5000} />
+        </Suspense>
+      );
+    }
+
+    ReactNoop.render(<Foo />);
+    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 (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <AsyncText text="A" ms={5000} />
+          <Suspense fallback={<Text text="Loading more..." />}>
+            <AsyncText text="B" ms={10000} />
+          </Suspense>
+        </Suspense>
+      );
+    }
+
+    ReactNoop.render(<Foo />);
+    // 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 (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <AsyncText text="A" ms={5000} />
+        </Suspense>
+      );
+    }
+
+    ReactNoop.interactiveUpdates(() => ReactNoop.render(<Foo />));
+    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: