Track Event Time as the Start Time for Suspense (#15358)

* Track the earliest event time in this render Rebase * Track the time of the fallback being shown as an event time When we switch back from fallback to content, we made progress and we track the time from when we showed the fallback in the first place as the last time we made progress. * Don't retry if synchronous * Only suspend when we switch to fallback mode This ensures that we don't resuspend unnecessarily if we're just retrying the same exact boundary again. We can still unnecessarily suspend for nested boundaries. * Rename timedOutAt to fallbackExpirationTime * Account for suspense in devtools suspense test
facebook · Apr 10, 2019 · 4c78ac0 · 4c78ac0
1 parent 875d05d
commit 4c78ac0
Show file tree

Hide file tree

Showing 14 changed files with 200 additions and 228 deletions.
diff --git a/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js
@@ -251,7 +251,10 @@ describe('React hooks DevTools integration', () => {
       </div>,
       {unstable_isConcurrent: true},
     );
+
     expect(Scheduler).toFlushAndYield([]);
+    // Ensure we timeout any suspense time.
+    jest.advanceTimersByTime(1000);
     const fiber = renderer.root._currentFiber().child;
     if (__DEV__) {
       // First render was locked

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js
@@ -1413,7 +1413,8 @@ function updateSuspenseComponent(
     // Something in this boundary's subtree already suspended. Switch to
     // rendering the fallback children.
     nextState = {
-      timedOutAt: nextState !== null ? nextState.timedOutAt : NoWork,
+      fallbackExpirationTime:
+        nextState !== null ? nextState.fallbackExpirationTime : NoWork,
     };
     nextDidTimeout = true;
     workInProgress.effectTag &= ~DidCapture;

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js
@@ -63,7 +63,10 @@ import invariant from 'shared/invariant';
 import warningWithoutStack from 'shared/warningWithoutStack';
 import warning from 'shared/warning';
 
-import {NoWork} from './ReactFiberExpirationTime';
+import {
+  NoWork,
+  computeAsyncExpirationNoBucket,
+} from './ReactFiberExpirationTime';
 import {onCommitUnmount} from './ReactFiberDevToolsHook';
 import {startPhaseTimer, stopPhaseTimer} from './ReactDebugFiberPerf';
 import {getStackByFiberInDevAndProd} from './ReactCurrentFiber';
@@ -1272,11 +1275,15 @@ function commitSuspenseComponent(finishedWork: Fiber) {
   } else {
     newDidTimeout = true;
     primaryChildParent = finishedWork.child;
-    if (newState.timedOutAt === NoWork) {
+    if (newState.fallbackExpirationTime === NoWork) {
       // If the children had not already timed out, record the time.
       // This is used to compute the elapsed time during subsequent
       // attempts to render the children.
-      newState.timedOutAt = requestCurrentTime();
+      // We model this as a normal pri expiration time since that's
+      // how we infer start time for updates.
+      newState.fallbackExpirationTime = computeAsyncExpirationNoBucket(
+        requestCurrentTime(),
+      );
     }
   }
 

diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js
@@ -18,6 +18,7 @@ import type {
   ChildSet,
 } from './ReactFiberHostConfig';
 import type {ReactEventComponentInstance} from 'shared/ReactTypes';
+import type {SuspenseState} from './ReactFiberSuspenseComponent';
 
 import {
   IndeterminateComponent,
@@ -42,6 +43,7 @@ import {
   EventComponent,
   EventTarget,
 } from 'shared/ReactWorkTags';
+import {ConcurrentMode, NoContext} from './ReactTypeOfMode';
 import {
   Placement,
   Ref,
@@ -92,6 +94,7 @@ import {
   enableSuspenseServerRenderer,
   enableEventAPI,
 } from 'shared/ReactFeatureFlags';
+import {markRenderEventTime, renderDidSuspend} from './ReactFiberScheduler';
 
 function markUpdate(workInProgress: Fiber) {
   // Tag the fiber with an update effect. This turns a Placement into
@@ -665,7 +668,7 @@ function completeWork(
     case ForwardRef:
       break;
     case SuspenseComponent: {
-      const nextState = workInProgress.memoizedState;
+      const nextState: null | SuspenseState = workInProgress.memoizedState;
       if ((workInProgress.effectTag & DidCapture) !== NoEffect) {
         // Something suspended. Re-render with the fallback children.
         workInProgress.expirationTime = renderExpirationTime;
@@ -674,34 +677,58 @@ function completeWork(
       }
 
       const nextDidTimeout = nextState !== null;
-      const prevDidTimeout = current !== null && current.memoizedState !== null;
-
+      let prevDidTimeout = false;
       if (current === null) {
         // In cases where we didn't find a suitable hydration boundary we never
         // downgraded this to a DehydratedSuspenseComponent, but we still need to
         // pop the hydration state since we might be inside the insertion tree.
         popHydrationState(workInProgress);
-      } else if (!nextDidTimeout && prevDidTimeout) {
-        // We just switched from the fallback to the normal children. Delete
-        // the fallback.
-        // TODO: Would it be better to store the fallback fragment on
-        // the stateNode during the begin phase?
-        const currentFallbackChild: Fiber | null = (current.child: any).sibling;
-        if (currentFallbackChild !== null) {
-          // Deletions go at the beginning of the return fiber's effect list
-          const first = workInProgress.firstEffect;
-          if (first !== null) {
-            workInProgress.firstEffect = currentFallbackChild;
-            currentFallbackChild.nextEffect = first;
-          } else {
-            workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChild;
-            currentFallbackChild.nextEffect = null;
+      } else {
+        const prevState: null | SuspenseState = current.memoizedState;
+        prevDidTimeout = prevState !== null;
+        if (!nextDidTimeout && prevState !== null) {
+          // We just switched from the fallback to the normal children.
+
+          // 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 =
+            prevState.fallbackExpirationTime;
+          markRenderEventTime(fallbackExpirationTimeExpTime);
+
+          // Delete the fallback.
+          // TODO: Would it be better to store the fallback fragment on
+          // the stateNode during the begin phase?
+          const currentFallbackChild: Fiber | null = (current.child: any)
+            .sibling;
+          if (currentFallbackChild !== null) {
+            // Deletions go at the beginning of the return fiber's effect list
+            const first = workInProgress.firstEffect;
+            if (first !== null) {
+              workInProgress.firstEffect = currentFallbackChild;
+              currentFallbackChild.nextEffect = first;
+            } else {
+              workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChild;
+              currentFallbackChild.nextEffect = null;
+            }
+            currentFallbackChild.effectTag = Deletion;
           }
-          currentFallbackChild.effectTag = Deletion;
+        }
+      }
+
+      if (nextDidTimeout && !prevDidTimeout) {
+        // If this subtreee is running in concurrent mode we can suspend,
+        // otherwise we won't suspend.
+        // TODO: This will still suspend a synchronous tree if anything
+        // in the concurrent tree already suspended during this render.
+        // This is a known bug.
+        if ((workInProgress.mode & ConcurrentMode) !== NoContext) {
+          renderDidSuspend();
         }
       }
 
       if (supportsPersistence) {
+        // TODO: Only schedule updates if not prevDidTimeout.
         if (nextDidTimeout) {
           // If this boundary just timed out, schedule an effect to attach a
           // retry listener to the proimse. This flag is also used to hide the
@@ -710,6 +737,7 @@ function completeWork(
         }
       }
       if (supportsMutation) {
+        // TODO: Only schedule updates if these values are non equal, i.e. it changed.
         if (nextDidTimeout || prevDidTimeout) {
           // If this boundary just timed out, schedule an effect to attach a
           // retry listener to the proimse. This flag is also used to hide the

diff --git a/packages/react-reconciler/src/ReactFiberExpirationTime.js b/packages/react-reconciler/src/ReactFiberExpirationTime.js
@@ -70,6 +70,14 @@ export function computeAsyncExpiration(
   );
 }
 
+// Same as computeAsyncExpiration but without the bucketing logic. This is
+// used to compute timestamps instead of actual expiration times.
+export function computeAsyncExpirationNoBucket(
+  currentTime: ExpirationTime,
+): ExpirationTime {
+  return currentTime - LOW_PRIORITY_EXPIRATION / UNIT_SIZE;
+}
+
 // We intentionally set a higher expiration time for interactive updates in
 // dev than in production.
 //

diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js
@@ -34,6 +34,7 @@ import {
   flushPassiveEffects,
   requestCurrentTime,
   warnIfNotCurrentlyActingUpdatesInDev,
+  markRenderEventTime,
 } from './ReactFiberScheduler';
 
 import invariant from 'shared/invariant';
@@ -718,6 +719,16 @@ function updateReducer<S, I, A>(
           remainingExpirationTime = updateExpirationTime;
         }
       } else {
+        // This update does have sufficient priority.
+
+        // Mark the event time of this update as relevant to this render pass.
+        // TODO: This should ideally use the true event time of this update rather than
+        // its priority which is a derived and not reverseable value.
+        // TODO: We should skip this update if it was already committed but currently
+        // we have no way of detecting the difference between a committed and suspended
+        // update here.
+        markRenderEventTime(updateExpirationTime);
+
         // Process this update.
         if (update.eagerReducer === reducer) {
           // If this update was processed eagerly, and its reducer matches the

diff --git a/packages/react-reconciler/src/ReactFiberPendingPriority.js b/packages/react-reconciler/src/ReactFiberPendingPriority.js
@@ -219,23 +219,6 @@ function clearPing(root, completedTime) {
   }
 }
 
-export function findEarliestOutstandingPriorityLevel(
-  root: FiberRoot,
-  renderExpirationTime: ExpirationTime,
-): ExpirationTime {
-  let earliestExpirationTime = renderExpirationTime;
-
-  const earliestPendingTime = root.earliestPendingTime;
-  const earliestSuspendedTime = root.earliestSuspendedTime;
-  if (earliestPendingTime > earliestExpirationTime) {
-    earliestExpirationTime = earliestPendingTime;
-  }
-  if (earliestSuspendedTime > earliestExpirationTime) {
-    earliestExpirationTime = earliestSuspendedTime;
-  }
-  return earliestExpirationTime;
-}
-
 export function didExpireAtExpirationTime(
   root: FiberRoot,
   currentTime: ExpirationTime,

diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js
@@ -14,6 +14,7 @@ import {
   computeExpirationForFiber as computeExpirationForFiber_old,
   captureCommitPhaseError as captureCommitPhaseError_old,
   onUncaughtError as onUncaughtError_old,
+  markRenderEventTime as markRenderEventTime_old,
   renderDidSuspend as renderDidSuspend_old,
   renderDidError as renderDidError_old,
   pingSuspendedRoot as pingSuspendedRoot_old,
@@ -34,14 +35,14 @@ import {
   computeUniqueAsyncExpiration as computeUniqueAsyncExpiration_old,
   flushPassiveEffects as flushPassiveEffects_old,
   warnIfNotCurrentlyActingUpdatesInDev as warnIfNotCurrentlyActingUpdatesInDev_old,
-  inferStartTimeFromExpirationTime as inferStartTimeFromExpirationTime_old,
 } from './ReactFiberScheduler.old';
 
 import {
   requestCurrentTime as requestCurrentTime_new,
   computeExpirationForFiber as computeExpirationForFiber_new,
   captureCommitPhaseError as captureCommitPhaseError_new,
   onUncaughtError as onUncaughtError_new,
+  markRenderEventTime as markRenderEventTime_new,
   renderDidSuspend as renderDidSuspend_new,
   renderDidError as renderDidError_new,
   pingSuspendedRoot as pingSuspendedRoot_new,
@@ -62,7 +63,6 @@ import {
   computeUniqueAsyncExpiration as computeUniqueAsyncExpiration_new,
   flushPassiveEffects as flushPassiveEffects_new,
   warnIfNotCurrentlyActingUpdatesInDev as warnIfNotCurrentlyActingUpdatesInDev_new,
-  inferStartTimeFromExpirationTime as inferStartTimeFromExpirationTime_new,
 } from './ReactFiberScheduler.new';
 
 export const requestCurrentTime = enableNewScheduler
@@ -77,6 +77,9 @@ export const captureCommitPhaseError = enableNewScheduler
 export const onUncaughtError = enableNewScheduler
   ? onUncaughtError_new
   : onUncaughtError_old;
+export const markRenderEventTime = enableNewScheduler
+  ? markRenderEventTime_new
+  : markRenderEventTime_old;
 export const renderDidSuspend = enableNewScheduler
   ? renderDidSuspend_new
   : renderDidSuspend_old;
@@ -133,9 +136,6 @@ export const flushPassiveEffects = enableNewScheduler
 export const warnIfNotCurrentlyActingUpdatesInDev = enableNewScheduler
   ? warnIfNotCurrentlyActingUpdatesInDev_new
   : warnIfNotCurrentlyActingUpdatesInDev_old;
-export const inferStartTimeFromExpirationTime = enableNewScheduler
-  ? inferStartTimeFromExpirationTime_new
-  : inferStartTimeFromExpirationTime_old;
 
 export type Thenable = {
   then(resolve: () => mixed, reject?: () => mixed): void | Thenable,