Add dedicated LanePriority for Suspense retries (facebook#19287)

acdlite · web-flow · commit 965d08cfff50 · 2020-07-08T21:09:22.000-07:00
A "retry" is a special type of update that attempts to flip a Suspense
boundary from its placeholder state back to its primary/resolved state.

Currently, retries are given default priority, using the same algorithm
as normal updates and occupying range of lanes.

This adds a new range of lanes dedicated specifically to retries, and
gives them lower priority than normal updates.

A couple of existing tests were affected because retries are no longer
batched with normal updates; they commit in separate batches.

Not totally satisfied with this design, but I think things will snap more
into place once the rest of the Lanes changes (like how we handle
parallel Suspense transitions) have settled.
diff --git a/packages/react-reconciler/src/ReactFiberLane.js b/packages/react-reconciler/src/ReactFiberLane.js
@@ -26,7 +26,8 @@ export opaque type LanePriority =
   | 13
   | 14
   | 15
-  | 16;
+  | 16
+  | 17;
 export opaque type Lanes = number;
 export opaque type Lane = number;
 export opaque type LaneMap<T> = Array<T>;
@@ -42,23 +43,25 @@ import {
   NoPriority as NoSchedulerPriority,
 } from './SchedulerWithReactIntegration.new';
 
-export const SyncLanePriority: LanePriority = 16;
-const SyncBatchedLanePriority: LanePriority = 15;
+export const SyncLanePriority: LanePriority = 17;
+const SyncBatchedLanePriority: LanePriority = 16;
 
-const InputDiscreteHydrationLanePriority: LanePriority = 14;
-export const InputDiscreteLanePriority: LanePriority = 13;
+const InputDiscreteHydrationLanePriority: LanePriority = 15;
+export const InputDiscreteLanePriority: LanePriority = 14;
 
-const InputContinuousHydrationLanePriority: LanePriority = 12;
-export const InputContinuousLanePriority: LanePriority = 11;
+const InputContinuousHydrationLanePriority: LanePriority = 13;
+export const InputContinuousLanePriority: LanePriority = 12;
 
-const DefaultHydrationLanePriority: LanePriority = 10;
-export const DefaultLanePriority: LanePriority = 9;
+const DefaultHydrationLanePriority: LanePriority = 11;
+export const DefaultLanePriority: LanePriority = 10;
 
-const TransitionShortHydrationLanePriority: LanePriority = 8;
-export const TransitionShortLanePriority: LanePriority = 7;
+const TransitionShortHydrationLanePriority: LanePriority = 9;
+export const TransitionShortLanePriority: LanePriority = 8;
 
-const TransitionLongHydrationLanePriority: LanePriority = 6;
-export const TransitionLongLanePriority: LanePriority = 5;
+const TransitionLongHydrationLanePriority: LanePriority = 7;
+export const TransitionLongLanePriority: LanePriority = 6;
+
+const RetryLanePriority: LanePriority = 5;
 
 const SelectiveHydrationLanePriority: LanePriority = 4;
 
@@ -90,25 +93,30 @@ const InputContinuousUpdateRangeStart = 6;
 const InputContinuousUpdateRangeEnd = 8;
 
 export const DefaultHydrationLane: Lane = /*            */ 0b0000000000000000000000100000000;
-const DefaultLanes: Lanes = /*                          */ 0b0000000000000000011111100000000;
+export const DefaultLanes: Lanes = /*                   */ 0b0000000000000000000111100000000;
 const DefaultUpdateRangeStart = 9;
-const DefaultUpdateRangeEnd = 14;
+const DefaultUpdateRangeEnd = 12;
+
+const TransitionShortHydrationLane: Lane = /*           */ 0b0000000000000000001000000000000;
+const TransitionShortLanes: Lanes = /*                  */ 0b0000000000000011111000000000000;
+const TransitionShortUpdateRangeStart = 13;
+const TransitionShortUpdateRangeEnd = 17;
 
-const TransitionShortHydrationLane: Lane = /*           */ 0b0000000000000000100000000000000;
-const TransitionShortLanes: Lanes = /*                  */ 0b0000000000011111100000000000000;
-const TransitionShortUpdateRangeStart = 15;
-const TransitionShortUpdateRangeEnd = 20;
+const TransitionLongHydrationLane: Lane = /*            */ 0b0000000000000100000000000000000;
+const TransitionLongLanes: Lanes = /*                   */ 0b0000000001111100000000000000000;
+const TransitionLongUpdateRangeStart = 18;
+const TransitionLongUpdateRangeEnd = 22;
 
-const TransitionLongHydrationLane: Lane = /*            */ 0b0000000000100000000000000000000;
-const TransitionLongLanes: Lanes = /*                   */ 0b0000011111100000000000000000000;
-const TransitionLongUpdateRangeStart = 21;
-const TransitionLongUpdateRangeEnd = 26;
+// Includes all updates. Except Idle updates, which have special semantics.
+const UpdateRangeEnd = TransitionLongUpdateRangeEnd;
 
-export const SelectiveHydrationLane: Lane = /*          */ 0b0000110000000000000000000000000;
+const RetryLanes: Lanes = /*                            */ 0b0000011110000000000000000000000;
+const RetryRangeStart = 22;
+const RetryRangeEnd = 26;
+
+export const SelectiveHydrationLane: Lane = /*          */ 0b0000100000000000000000000000000;
 const SelectiveHydrationRangeEnd = 27;
 
-// Includes all non-Idle updates
-const UpdateRangeEnd = 27;
 const NonIdleLanes = /*                                 */ 0b0000111111111111111111111111111;
 
 export const IdleHydrationLane: Lane = /*               */ 0b0001000000000000000000000000000;
@@ -206,6 +214,12 @@ function getHighestPriorityLanes(lanes: Lanes | Lane): Lanes {
       return transitionLongLanes;
     }
   }
+  const retryLanes = RetryLanes & lanes;
+  if (retryLanes !== NoLanes) {
+    return_highestLanePriority = RetryLanePriority;
+    return_updateRangeEnd = RetryRangeEnd;
+    return retryLanes;
+  }
   if (lanes & SelectiveHydrationLane) {
     return_highestLanePriority = SelectiveHydrationLanePriority;
     return_updateRangeEnd = SelectiveHydrationRangeEnd;
@@ -275,6 +289,7 @@ export function lanePriorityToSchedulerPriority(
     case TransitionLongHydrationLanePriority:
     case TransitionLongLanePriority:
     case SelectiveHydrationLanePriority:
+    case RetryLanePriority:
       return NormalSchedulerPriority;
     case IdleHydrationLanePriority:
     case IdleLanePriority:
@@ -537,6 +552,9 @@ export function findUpdateLane(
     case TransitionLongLanePriority:
       // Should be handled by findTransitionLane instead
       break;
+    case RetryLanePriority:
+      // Should be handled by findRetryLane instead
+      break;
     case IdleLanePriority:
       let lane = findLane(IdleUpdateRangeStart, IdleUpdateRangeEnd, wipLanes);
       if (lane === NoLane) {
@@ -604,6 +622,19 @@ export function findTransitionLane(
   );
 }
 
+// To ensure consistency across multiple updates in the same event, this should
+// be pure function, so that it always returns the same lane for given inputs.
+export function findRetryLane(wipLanes: Lanes): Lane {
+  // This is a fork of `findUpdateLane` designed specifically for Suspense
+  // "retries" — a special update that attempts to flip a Suspense boundary
+  // from its placeholder state to its primary/resolved state.
+  let lane = findLane(RetryRangeStart, RetryRangeEnd, wipLanes);
+  if (lane === NoLane) {
+    lane = pickArbitraryLane(RetryLanes);
+  }
+  return lane;
+}
+
 function findLane(start, end, skipLanes) {
   // This finds the first bit between the `start` and `end` positions that isn't
   // in `skipLanes`.
@@ -808,6 +839,11 @@ export function getBumpedLaneForHydration(
     case TransitionLongLanePriority:
       lane = TransitionLongHydrationLane;
       break;
+    case RetryLanePriority:
+      // Shouldn't be reachable under normal circumstances, so there's no
+      // dedicated lane for retry priority. Use the one for long transitions.
+      lane = TransitionLongHydrationLane;
+      break;
     case SelectiveHydrationLanePriority:
       lane = SelectiveHydrationLane;
       break;
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -125,6 +125,7 @@ import {
   NoTimestamp,
   findUpdateLane,
   findTransitionLane,
+  findRetryLane,
   includesSomeLane,
   isSubsetOfLanes,
   mergeLanes,
@@ -472,6 +473,28 @@ export function requestUpdateLane(
   return lane;
 }
 
+function requestRetryLane(fiber: Fiber) {
+  // This is a fork of `requestUpdateLane` designed specifically for Suspense
+  // "retries" — a special update that attempts to flip a Suspense boundary
+  // from its placeholder state to its primary/resolved state.
+
+  // Special cases
+  const mode = fiber.mode;
+  if ((mode & BlockingMode) === NoMode) {
+    return (SyncLane: Lane);
+  } else if ((mode & ConcurrentMode) === NoMode) {
+    return getCurrentPriorityLevel() === ImmediateSchedulerPriority
+      ? (SyncLane: Lane)
+      : (SyncBatchedLane: Lane);
+  }
+
+  // See `requestUpdateLane` for explanation of `currentEventWipLanes`
+  if (currentEventWipLanes === NoLanes) {
+    currentEventWipLanes = workInProgressRootIncludedLanes;
+  }
+  return findRetryLane(currentEventWipLanes);
+}
+
 export function scheduleUpdateOnFiber(
   fiber: Fiber,
   lane: Lane,
@@ -2708,10 +2731,7 @@ function retryTimedOutBoundary(boundaryFiber: Fiber, retryLane: Lane) {
   // suspended it has resolved, which means at least part of the tree was
   // likely unblocked. Try rendering again, at a new expiration time.
   if (retryLane === NoLane) {
-    const suspenseConfig = null; // Retries don't carry over the already committed update.
-    // TODO: Should retries get their own lane? Maybe it could share with
-    // transitions.
-    retryLane = requestUpdateLane(boundaryFiber, suspenseConfig);
+    retryLane = requestRetryLane(boundaryFiber);
   }
   // TODO: Special case idle priority?
   const eventTime = requestEventTime();
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -148,6 +148,7 @@ import {
   NoTimestamp,
   findUpdateLane,
   findTransitionLane,
+  findRetryLane,
   includesSomeLane,
   isSubsetOfLanes,
   mergeLanes,
@@ -495,6 +496,28 @@ export function requestUpdateLane(
   return lane;
 }
 
+function requestRetryLane(fiber: Fiber) {
+  // This is a fork of `requestUpdateLane` designed specifically for Suspense
+  // "retries" — a special update that attempts to flip a Suspense boundary
+  // from its placeholder state to its primary/resolved state.
+
+  // Special cases
+  const mode = fiber.mode;
+  if ((mode & BlockingMode) === NoMode) {
+    return (SyncLane: Lane);
+  } else if ((mode & ConcurrentMode) === NoMode) {
+    return getCurrentPriorityLevel() === ImmediateSchedulerPriority
+      ? (SyncLane: Lane)
+      : (SyncBatchedLane: Lane);
+  }
+
+  // See `requestUpdateLane` for explanation of `currentEventWipLanes`
+  if (currentEventWipLanes === NoLanes) {
+    currentEventWipLanes = workInProgressRootIncludedLanes;
+  }
+  return findRetryLane(currentEventWipLanes);
+}
+
 export function scheduleUpdateOnFiber(
   fiber: Fiber,
   lane: Lane,
@@ -2855,10 +2878,7 @@ function retryTimedOutBoundary(boundaryFiber: Fiber, retryLane: Lane) {
   // suspended it has resolved, which means at least part of the tree was
   // likely unblocked. Try rendering again, at a new expiration time.
   if (retryLane === NoLane) {
-    const suspenseConfig = null; // Retries don't carry over the already committed update.
-    // TODO: Should retries get their own lane? Maybe it could share with
-    // transitions.
-    retryLane = requestUpdateLane(boundaryFiber, suspenseConfig);
+    retryLane = requestRetryLane(boundaryFiber);
   }
   // TODO: Special case idle priority?
   const eventTime = requestEventTime();
diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.internal.js
@@ -219,6 +219,11 @@ describe('ReactSuspense', () => {
     await resolve1();
     ReactNoop.render(element);
     expect(Scheduler).toFlushWithoutYielding();
+
+    // Force fallback to commit.
+    // TODO: Should be able to use `act` here.
+    jest.runAllTimers();
+
     expect(ReactNoop.getChildren()).toEqual([
       text('Waiting Tier 2'),
       text('Done'),
diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
@@ -623,6 +623,17 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       // The new reconciler batches everything together, so it finishes without
       // suspending again.
       'Sibling',
+
+      // NOTE: The final of the update got pushed into a lower priority range of
+      // lanes, leading to the extra intermediate render. This is because when
+      // we schedule the fourth update, we're already in the middle of rendering
+      // the three others. Since there are only three lanes in the default
+      // range, the fourth lane is shifted to slightly lower priority. This
+      // could easily change when we tweak our batching heuristics. Ideally,
+      // they'd all have default priority and render in a single batch.
+      'Suspend! [Step 3]',
+      'Sibling',
+
       'Step 4',
     ]);
   });
@@ -3896,4 +3907,59 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     expect(Scheduler).toHaveYielded(['Promise resolved [B]', 'B']);
     expect(root).toMatchRenderedOutput(<span prop="B" />);
   });
+
+  it('retries have lower priority than normal updates', async () => {
+    const {useState} = React;
+
+    let setText;
+    function UpdatingText() {
+      const [text, _setText] = useState('A');
+      setText = _setText;
+      return <Text text={text} />;
+    }
+
+    const root = ReactNoop.createRoot();
+    await ReactNoop.act(async () => {
+      root.render(
+        <>
+          <UpdatingText />
+          <Suspense fallback={<Text text="Loading..." />}>
+            <AsyncText text="Async" />
+          </Suspense>
+        </>,
+      );
+    });
+    expect(Scheduler).toHaveYielded(['A', 'Suspend! [Async]', 'Loading...']);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span prop="A" />
+        <span prop="Loading..." />
+      </>,
+    );
+
+    await ReactNoop.act(async () => {
+      // Resolve the promise. This will trigger a retry.
+      await resolveText('Async');
+      expect(Scheduler).toHaveYielded(['Promise resolved [Async]']);
+      // Before the retry happens, schedule a new update.
+      setText('B');
+
+      // The update should be allowed to finish before the retry is attempted.
+      expect(Scheduler).toFlushUntilNextPaint(['B']);
+      expect(root).toMatchRenderedOutput(
+        <>
+          <span prop="B" />
+          <span prop="Loading..." />
+        </>,
+      );
+    });
+    // Then do the retry.
+    expect(Scheduler).toHaveYielded(['Async']);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span prop="B" />
+        <span prop="Async" />
+      </>,
+    );
+  });
 });
diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js
@@ -3923,7 +3923,6 @@ describe('Profiler', () => {
 
         expect(onPostCommit).toHaveBeenCalledTimes(1);
         expect(onPostCommit.mock.calls[0][4]).toMatchInteractions([
-          initialRenderInteraction,
           highPriUpdateInteraction,
         ]);
 
