Expiration: Do nothing except disable time slicing (#21345)

We have a feature called "expiration" whose purpose is to prevent a concurrent update from being starved by higher priority events. If a lane is CPU-bound for too long, we finish the rest of the work synchronously without allowing further interruptions. In the current implementation, we do this in sort of a roundabout way: once a lane is determined to have expired, we entangle it with SyncLane and switch to the synchronous work loop. There are a few flaws with the approach. One is that SyncLane has a particular semantic meaning besides its non-yieldiness. For example, `flushSync` will force remaining Sync work to finish; currently, that also includes expired work, which isn't an intended behavior, but rather an artifact of the implementation. An event worse example is that passive effects triggered by a Sync update are flushed synchronously, before paint, so that its result is guaranteed to be observed by the next discrete event. But expired work has no such requirement: we're flushing expired effects before paint unnecessarily. Aside from the behaviorial implications, the current implementation has proven to be fragile: more than once, we've accidentally regressed performance due to a subtle change in how expiration is handled. This PR aims to radically simplify how we model starvation protection by scaling back the implementation as much as possible. In this new model, if a lane is expired, we disable time slicing. That's it. We don't entangle it with SyncLane. The only thing we do is skip the call to `shouldYield` in between each time slice. This is identical to how we model synchronous-by-default updates in React 18.
facebook · Apr 24, 2021 · 4874042 · 4874042
1 parent 0f5ebf3
commit 4874042
Show file tree

Hide file tree

Showing 10 changed files with 213 additions and 294 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js
@@ -401,7 +401,6 @@ export function markStarvedLanesAsExpired(
   // expiration time. If so, we'll assume the update is being starved and mark
   // it as expired to force it to finish.
   let lanes = pendingLanes;
-  let expiredLanes = 0;
   while (lanes > 0) {
     const index = pickArbitraryLaneIndex(lanes);
     const lane = 1 << index;
@@ -420,15 +419,11 @@ export function markStarvedLanesAsExpired(
       }
     } else if (expirationTime <= currentTime) {
       // This lane expired
-      expiredLanes |= lane;
+      root.expiredLanes |= lane;
     }
 
     lanes &= ~lane;
   }
-
-  if (expiredLanes !== 0) {
-    markRootExpired(root, expiredLanes);
-  }
 }
 
 // This returns the highest priority pending lanes regardless of whether they
@@ -459,16 +454,22 @@ export function includesOnlyTransitions(lanes: Lanes) {
 }
 
 export function shouldTimeSlice(root: FiberRoot, lanes: Lanes) {
-  if (!enableSyncDefaultUpdates) {
+  if ((lanes & root.expiredLanes) !== NoLanes) {
+    // At least one of these lanes expired. To prevent additional starvation,
+    // finish rendering without yielding execution.
+    return false;
+  }
+  if (enableSyncDefaultUpdates) {
+    const SyncDefaultLanes =
+      InputContinuousHydrationLane |
+      InputContinuousLane |
+      DefaultHydrationLane |
+      DefaultLane;
+    // TODO: Check for root override, once that lands
+    return (lanes & SyncDefaultLanes) === NoLanes;
+  } else {
     return true;
   }
-  const SyncDefaultLanes =
-    InputContinuousHydrationLane |
-    InputContinuousLane |
-    DefaultHydrationLane |
-    DefaultLane;
-  // TODO: Check for root override, once that lands
-  return (lanes & SyncDefaultLanes) === NoLanes;
 }
 
 export function isTransitionLane(lane: Lane) {
@@ -613,14 +614,6 @@ export function markRootPinged(
   root.pingedLanes |= root.suspendedLanes & pingedLanes;
 }
 
-export function markRootExpired(root: FiberRoot, expiredLanes: Lanes) {
-  const entanglements = root.entanglements;
-  const SyncLaneIndex = 0;
-  entanglements[SyncLaneIndex] |= expiredLanes;
-  root.entangledLanes |= SyncLane;
-  root.pendingLanes |= SyncLane;
-}
-
 export function markRootMutableRead(root: FiberRoot, updateLane: Lane) {
   root.mutableReadLanes |= updateLane & root.pendingLanes;
 }
@@ -634,6 +627,7 @@ export function markRootFinished(root: FiberRoot, remainingLanes: Lanes) {
   root.suspendedLanes = 0;
   root.pingedLanes = 0;
 
+  root.expiredLanes &= remainingLanes;
   root.mutableReadLanes &= remainingLanes;
 
   root.entangledLanes &= remainingLanes;

diff --git a/packages/react-reconciler/src/ReactFiberLane.old.js b/packages/react-reconciler/src/ReactFiberLane.old.js
@@ -401,7 +401,6 @@ export function markStarvedLanesAsExpired(
   // expiration time. If so, we'll assume the update is being starved and mark
   // it as expired to force it to finish.
   let lanes = pendingLanes;
-  let expiredLanes = 0;
   while (lanes > 0) {
     const index = pickArbitraryLaneIndex(lanes);
     const lane = 1 << index;
@@ -420,15 +419,11 @@ export function markStarvedLanesAsExpired(
       }
     } else if (expirationTime <= currentTime) {
       // This lane expired
-      expiredLanes |= lane;
+      root.expiredLanes |= lane;
     }
 
     lanes &= ~lane;
   }
-
-  if (expiredLanes !== 0) {
-    markRootExpired(root, expiredLanes);
-  }
 }
 
 // This returns the highest priority pending lanes regardless of whether they
@@ -459,16 +454,22 @@ export function includesOnlyTransitions(lanes: Lanes) {
 }
 
 export function shouldTimeSlice(root: FiberRoot, lanes: Lanes) {
-  if (!enableSyncDefaultUpdates) {
+  if ((lanes & root.expiredLanes) !== NoLanes) {
+    // At least one of these lanes expired. To prevent additional starvation,
+    // finish rendering without yielding execution.
+    return false;
+  }
+  if (enableSyncDefaultUpdates) {
+    const SyncDefaultLanes =
+      InputContinuousHydrationLane |
+      InputContinuousLane |
+      DefaultHydrationLane |
+      DefaultLane;
+    // TODO: Check for root override, once that lands
+    return (lanes & SyncDefaultLanes) === NoLanes;
+  } else {
     return true;
   }
-  const SyncDefaultLanes =
-    InputContinuousHydrationLane |
-    InputContinuousLane |
-    DefaultHydrationLane |
-    DefaultLane;
-  // TODO: Check for root override, once that lands
-  return (lanes & SyncDefaultLanes) === NoLanes;
 }
 
 export function isTransitionLane(lane: Lane) {
@@ -613,14 +614,6 @@ export function markRootPinged(
   root.pingedLanes |= root.suspendedLanes & pingedLanes;
 }
 
-export function markRootExpired(root: FiberRoot, expiredLanes: Lanes) {
-  const entanglements = root.entanglements;
-  const SyncLaneIndex = 0;
-  entanglements[SyncLaneIndex] |= expiredLanes;
-  root.entangledLanes |= SyncLane;
-  root.pendingLanes |= SyncLane;
-}
-
 export function markRootMutableRead(root: FiberRoot, updateLane: Lane) {
   root.mutableReadLanes |= updateLane & root.pendingLanes;
 }
@@ -634,6 +627,7 @@ export function markRootFinished(root: FiberRoot, remainingLanes: Lanes) {
   root.suspendedLanes = 0;
   root.pingedLanes = 0;
 
+  root.expiredLanes &= remainingLanes;
   root.mutableReadLanes &= remainingLanes;
 
   root.entangledLanes &= remainingLanes;

diff --git a/packages/react-reconciler/src/ReactFiberRoot.new.js b/packages/react-reconciler/src/ReactFiberRoot.new.js
@@ -50,6 +50,7 @@ function FiberRootNode(containerInfo, tag, hydrate) {
   this.pendingLanes = NoLanes;
   this.suspendedLanes = NoLanes;
   this.pingedLanes = NoLanes;
+  this.expiredLanes = NoLanes;
   this.mutableReadLanes = NoLanes;
   this.finishedLanes = NoLanes;
 

diff --git a/packages/react-reconciler/src/ReactFiberRoot.old.js b/packages/react-reconciler/src/ReactFiberRoot.old.js
@@ -50,6 +50,7 @@ function FiberRootNode(containerInfo, tag, hydrate) {
   this.pendingLanes = NoLanes;
   this.suspendedLanes = NoLanes;
   this.pingedLanes = NoLanes;
+  this.expiredLanes = NoLanes;
   this.mutableReadLanes = NoLanes;
   this.finishedLanes = NoLanes;
 

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -159,7 +159,7 @@ import {
   markRootUpdated,
   markRootSuspended as markRootSuspended_dontCallThisOneDirectly,
   markRootPinged,
-  markRootExpired,
+  markRootEntangled,
   markRootFinished,
   getHighestPriorityLane,
   addFiberToLanesMap,
@@ -787,22 +787,17 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
     return null;
   }
 
+  // We disable time-slicing in some cases: if the work has been CPU-bound
+  // for too long ("expired" work, to prevent starvation), or we're in
+  // sync-updates-by-default mode.
   // TODO: We only check `didTimeout` defensively, to account for a Scheduler
   // bug we're still investigating. Once the bug in Scheduler is fixed,
   // we can remove this, since we track expiration ourselves.
-  if (!disableSchedulerTimeoutInWorkLoop && didTimeout) {
-    // Something expired. Flush synchronously until there's no expired
-    // work left.
-    markRootExpired(root, lanes);
-    // This will schedule a synchronous callback.
-    ensureRootIsScheduled(root, now());
-    return null;
-  }
-
-  let exitStatus = shouldTimeSlice(root, lanes)
-    ? renderRootConcurrent(root, lanes)
-    : // Time slicing is disabled for default updates in this root.
-      renderRootSync(root, lanes);
+  let exitStatus =
+    shouldTimeSlice(root, lanes) &&
+    (disableSchedulerTimeoutInWorkLoop || !didTimeout)
+      ? renderRootConcurrent(root, lanes)
+      : renderRootSync(root, lanes);
   if (exitStatus !== RootIncomplete) {
     if (exitStatus === RootErrored) {
       executionContext |= RetryAfterError;
@@ -990,16 +985,7 @@ function performSyncWorkOnRoot(root) {
   flushPassiveEffects();
 
   let lanes = getNextLanes(root, NoLanes);
-  if (includesSomeLane(lanes, SyncLane)) {
-    if (
-      root === workInProgressRoot &&
-      includesSomeLane(lanes, workInProgressRootRenderLanes)
-    ) {
-      // There's a partial tree, and at least one of its lanes has expired. Finish
-      // rendering it before rendering the rest of the expired work.
-      lanes = workInProgressRootRenderLanes;
-    }
-  } else {
+  if (!includesSomeLane(lanes, SyncLane)) {
     // There's no remaining sync work left.
     ensureRootIsScheduled(root, now());
     return null;
@@ -1052,11 +1038,9 @@ function performSyncWorkOnRoot(root) {
   return null;
 }
 
-// TODO: Do we still need this API? I think we can delete it. Was only used
-// internally.
 export function flushRoot(root: FiberRoot, lanes: Lanes) {
   if (lanes !== NoLanes) {
-    markRootExpired(root, lanes);
+    markRootEntangled(root, mergeLanes(lanes, SyncLane));
     ensureRootIsScheduled(root, now());
     if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
       resetRenderTimer();

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -159,7 +159,7 @@ import {
   markRootUpdated,
   markRootSuspended as markRootSuspended_dontCallThisOneDirectly,
   markRootPinged,
-  markRootExpired,
+  markRootEntangled,
   markRootFinished,
   getHighestPriorityLane,
   addFiberToLanesMap,
@@ -787,22 +787,17 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
     return null;
   }
 
+  // We disable time-slicing in some cases: if the work has been CPU-bound
+  // for too long ("expired" work, to prevent starvation), or we're in
+  // sync-updates-by-default mode.
   // TODO: We only check `didTimeout` defensively, to account for a Scheduler
   // bug we're still investigating. Once the bug in Scheduler is fixed,
   // we can remove this, since we track expiration ourselves.
-  if (!disableSchedulerTimeoutInWorkLoop && didTimeout) {
-    // Something expired. Flush synchronously until there's no expired
-    // work left.
-    markRootExpired(root, lanes);
-    // This will schedule a synchronous callback.
-    ensureRootIsScheduled(root, now());
-    return null;
-  }
-
-  let exitStatus = shouldTimeSlice(root, lanes)
-    ? renderRootConcurrent(root, lanes)
-    : // Time slicing is disabled for default updates in this root.
-      renderRootSync(root, lanes);
+  let exitStatus =
+    shouldTimeSlice(root, lanes) &&
+    (disableSchedulerTimeoutInWorkLoop || !didTimeout)
+      ? renderRootConcurrent(root, lanes)
+      : renderRootSync(root, lanes);
   if (exitStatus !== RootIncomplete) {
     if (exitStatus === RootErrored) {
       executionContext |= RetryAfterError;
@@ -990,16 +985,7 @@ function performSyncWorkOnRoot(root) {
   flushPassiveEffects();
 
   let lanes = getNextLanes(root, NoLanes);
-  if (includesSomeLane(lanes, SyncLane)) {
-    if (
-      root === workInProgressRoot &&
-      includesSomeLane(lanes, workInProgressRootRenderLanes)
-    ) {
-      // There's a partial tree, and at least one of its lanes has expired. Finish
-      // rendering it before rendering the rest of the expired work.
-      lanes = workInProgressRootRenderLanes;
-    }
-  } else {
+  if (!includesSomeLane(lanes, SyncLane)) {
     // There's no remaining sync work left.
     ensureRootIsScheduled(root, now());
     return null;
@@ -1052,11 +1038,9 @@ function performSyncWorkOnRoot(root) {
   return null;
 }
 
-// TODO: Do we still need this API? I think we can delete it. Was only used
-// internally.
 export function flushRoot(root: FiberRoot, lanes: Lanes) {
   if (lanes !== NoLanes) {
-    markRootExpired(root, lanes);
+    markRootEntangled(root, mergeLanes(lanes, SyncLane));
     ensureRootIsScheduled(root, now());
     if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
       resetRenderTimer();

diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js
@@ -228,6 +228,7 @@ type BaseFiberRootProperties = {|
   pendingLanes: Lanes,
   suspendedLanes: Lanes,
   pingedLanes: Lanes,
+  expiredLanes: Lanes,
   mutableReadLanes: Lanes,
 
   finishedLanes: Lanes,