Don't shift interleaved updates to separate lane

Now that interleaved updates are added to a special queue, we no longer
need to shift them into their own lane. We can add to a lane that's
already in the middle of rendering without risk of tearing.

See facebook#20615 for more background.

I've only changed this in the new fork, and only behind the
enableTransitionEntanglements flag.

Most of this commit involves updating tests. The "shift-to-a-new" lane
trick was intentionally used in a handful of tests where two or more
updates need to be scheduled in different lanes. Most of these tests
were written before `startTransition` existed, and all of them were
written before transitions were assigned arbitrary lanes.

So I ported these tests to use `startTransition` instead, which is the
idiomatic way to mark an update as parallel.

I didn't change the old fork at all. Writing these tests in such a way
that they also pass in the old fork actually revealed a few flaws in the
current implementation regarding interrupting a suspended refresh
transition early, which is a good reminder that we should be writing our
tests using idiomatic patterns as much as we possibly can.

Author: acdlite

packages/react-reconciler/src/ReactFiberLane.new.js

@@ -512,56 +512,88 @@ export function findUpdateLane(
   lanePriority: LanePriority,
   wipLanes: Lanes,
 ): Lane {
-  switch (lanePriority) {
-    case NoLanePriority:
-      break;
-    case SyncLanePriority:
-      return SyncLane;
-    case SyncBatchedLanePriority:
-      return SyncBatchedLane;
-    case InputDiscreteLanePriority: {
-      const lane = pickArbitraryLane(InputDiscreteLanes & ~wipLanes);
-      if (lane === NoLane) {
-        // Shift to the next priority level
-        return findUpdateLane(InputContinuousLanePriority, wipLanes);
+  if (enableTransitionEntanglement) {
+    // Ignore wipLanes. Always assign to the same bit per priority.
+    switch (lanePriority) {
+      case NoLanePriority:
+        break;
+      case SyncLanePriority:
+        return SyncLane;
+      case SyncBatchedLanePriority:
+        return SyncBatchedLane;
+      case InputDiscreteLanePriority: {
+        return pickArbitraryLane(InputDiscreteLanes);
       }
-      return lane;
-    }
-    case InputContinuousLanePriority: {
-      const lane = pickArbitraryLane(InputContinuousLanes & ~wipLanes);
-      if (lane === NoLane) {
-        // Shift to the next priority level
-        return findUpdateLane(DefaultLanePriority, wipLanes);
+      case InputContinuousLanePriority: {
+        return pickArbitraryLane(InputContinuousLanes);
+      }
+      case DefaultLanePriority: {
+        return pickArbitraryLane(DefaultLanes);
       }
-      return lane;
+      case TransitionPriority: // Should be handled by findTransitionLane instead
+      case RetryLanePriority: // Should be handled by findRetryLane instead
+        break;
+      case IdleLanePriority:
+        return pickArbitraryLane(IdleLanes);
+      default:
+        // The remaining priorities are not valid for updates
+        break;
     }
-    case DefaultLanePriority: {
-      let lane = pickArbitraryLane(DefaultLanes & ~wipLanes);
-      if (lane === NoLane) {
-        // If all the default lanes are already being worked on, look for a
-        // lane in the transition range.
-        lane = pickArbitraryLane(TransitionLanes & ~wipLanes);
+  } else {
+    // Old behavior that uses wipLanes to shift interleaved updates into a
+    // separate lane. This is no longer needed because we put interleaved
+    // updates on a special queue.
+    switch (lanePriority) {
+      case NoLanePriority:
+        break;
+      case SyncLanePriority:
+        return SyncLane;
+      case SyncBatchedLanePriority:
+        return SyncBatchedLane;
+      case InputDiscreteLanePriority: {
+        const lane = pickArbitraryLane(InputDiscreteLanes & ~wipLanes);
         if (lane === NoLane) {
-          // All the transition lanes are taken, too. This should be very
-          // rare, but as a last resort, pick a default lane. This will have
-          // the effect of interrupting the current work-in-progress render.
-          lane = pickArbitraryLane(DefaultLanes);
+          // Shift to the next priority level
+          return findUpdateLane(InputContinuousLanePriority, wipLanes);
         }
+        return lane;
       }
-      return lane;
-    }
-    case TransitionPriority: // Should be handled by findTransitionLane instead
-    case RetryLanePriority: // Should be handled by findRetryLane instead
-      break;
-    case IdleLanePriority:
-      let lane = pickArbitraryLane(IdleLanes & ~wipLanes);
-      if (lane === NoLane) {
-        lane = pickArbitraryLane(IdleLanes);
+      case InputContinuousLanePriority: {
+        const lane = pickArbitraryLane(InputContinuousLanes & ~wipLanes);
+        if (lane === NoLane) {
+          // Shift to the next priority level
+          return findUpdateLane(DefaultLanePriority, wipLanes);
+        }
+        return lane;
       }
-      return lane;
-    default:
-      // The remaining priorities are not valid for updates
-      break;
+      case DefaultLanePriority: {
+        let lane = pickArbitraryLane(DefaultLanes & ~wipLanes);
+        if (lane === NoLane) {
+          // If all the default lanes are already being worked on, look for a
+          // lane in the transition range.
+          lane = pickArbitraryLane(TransitionLanes & ~wipLanes);
+          if (lane === NoLane) {
+            // All the transition lanes are taken, too. This should be very
+            // rare, but as a last resort, pick a default lane. This will have
+            // the effect of interrupting the current work-in-progress render.
+            lane = pickArbitraryLane(DefaultLanes);
+          }
+        }
+        return lane;
+      }
+      case TransitionPriority: // Should be handled by findTransitionLane instead
+      case RetryLanePriority: // Should be handled by findRetryLane instead
+        break;
+      case IdleLanePriority:
+        let lane = pickArbitraryLane(IdleLanes & ~wipLanes);
+        if (lane === NoLane) {
+          lane = pickArbitraryLane(IdleLanes);
+        }
+        return lane;
+      default:
+        // The remaining priorities are not valid for updates
+        break;
+    }
   }
   invariant(
     false,

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

@@ -34,6 +34,7 @@ import {
   disableSchedulerTimeoutInWorkLoop,
   enableDoubleInvokingEffects,
   skipUnmountedBoundaries,
+  enableTransitionEntanglement,
 } from 'shared/ReactFeatureFlags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import invariant from 'shared/invariant';
@@ -809,6 +810,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
   let exitStatus = renderRootConcurrent(root, lanes);
 
   if (
+    !enableTransitionEntanglement &&
     includesSomeLane(
       workInProgressRootIncludedLanes,
       workInProgressRootUpdatedLanes,
@@ -1014,6 +1016,7 @@ function performSyncWorkOnRoot(root) {
     lanes = workInProgressRootRenderLanes;
     exitStatus = renderRootSync(root, lanes);
     if (
+      !enableTransitionEntanglement &&
       includesSomeLane(
         workInProgressRootIncludedLanes,
         workInProgressRootUpdatedLanes,

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

@@ -327,6 +327,9 @@ describe('DebugTracing', () => {
     ]);
   });
 
+  // This test is coupled to lane implementation details, so I'm disabling it in
+  // the new fork until it stabilizes so we don't have to repeatedly update it.
+  // @gate !enableParallelTransitions
   // @gate experimental && build === 'development' && enableDebugTracing
   it('should log cascading passive updates', () => {
     function Example() {

packages/react-reconciler/src/__tests__/ReactExpiration-test.js

@@ -14,6 +14,7 @@ let ReactNoop;
 let Scheduler;
 let readText;
 let resolveText;
+let startTransition;
 
 describe('ReactExpiration', () => {
   beforeEach(() => {
@@ -22,6 +23,7 @@ describe('ReactExpiration', () => {
     React = require('react');
     ReactNoop = require('react-noop-renderer');
     Scheduler = require('scheduler');
+    startTransition = React.unstable_startTransition;
 
     const textCache = new Map();
 
@@ -610,6 +612,7 @@ describe('ReactExpiration', () => {
     expect(root).toMatchRenderedOutput('Sync pri: 2, Idle pri: 2');
   });
 
+  // @gate experimental
   it('a single update can expire without forcing all other updates to expire', async () => {
     const {useState} = React;
 
@@ -648,12 +651,18 @@ describe('ReactExpiration', () => {
 
     await ReactNoop.act(async () => {
       // Partially render an update
-      updateNormalPri();
+      startTransition(() => {
+        updateNormalPri();
+      });
       expect(Scheduler).toFlushAndYieldThrough(['High pri: 0']);
-      // Some time goes by. In an interleaved event, schedule another update.
+
+      // Some time goes by. Schedule another update.
       // This will be placed into a separate batch.
       Scheduler.unstable_advanceTime(4000);
-      updateNormalPri();
+
+      startTransition(() => {
+        updateNormalPri();
+      });
       // Keep rendering the first update
       expect(Scheduler).toFlushAndYieldThrough(['Normal pri: 1']);
       // More time goes by. Enough to expire the first batch, but not the
@@ -662,20 +671,30 @@ describe('ReactExpiration', () => {
       // Attempt to interrupt with a high pri update.
       updateHighPri();
 
-      // The first update expired, so first will finish it without interrupting.
-      // But not the second update, which hasn't expired yet.
-      expect(Scheduler).toFlushExpired(['Sibling']);
+      if (gate(flags => flags.enableParallelTransitions)) {
+        // The first update expired, so first will finish it without
+        // interrupting. But not the second update, which hasn't expired yet.
+        expect(Scheduler).toFlushExpired(['Sibling']);
+        expect(Scheduler).toFlushAndYield([
+          // Then render the high pri update
+          'High pri: 1',
+          'Normal pri: 1',
+          'Sibling',
+          // Then the second normal pri update
+          'High pri: 1',
+          'Normal pri: 2',
+          'Sibling',
+        ]);
+      } else {
+        // In the old implementation, all updates get entangled together.
+        expect(Scheduler).toFlushExpired([
+          'High pri: 1',
+          'Normal pri: 2',
+          'Sibling',
+        ]);
+        expect(Scheduler).toFlushAndYield([]);
+      }
     });
-    expect(Scheduler).toHaveYielded([
-      // Then render the high pri update
-      'High pri: 1',
-      'Normal pri: 1',
-      'Sibling',
-      // Then the second normal pri update
-      'High pri: 1',
-      'Normal pri: 2',
-      'Sibling',
-    ]);
   });
 
   it('detects starvation in multiple batches', async () => {

packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js

@@ -19,6 +19,7 @@ let NormalPriority;
 let LowPriority;
 let IdlePriority;
 let runWithPriority;
+let startTransition;
 
 describe('ReactSchedulerIntegration', () => {
   beforeEach(() => {
@@ -33,6 +34,7 @@ describe('ReactSchedulerIntegration', () => {
     LowPriority = Scheduler.unstable_LowPriority;
     IdlePriority = Scheduler.unstable_IdlePriority;
     runWithPriority = Scheduler.unstable_runWithPriority;
+    startTransition = React.unstable_startTransition;
   });
 
   function getCurrentPriorityAsString() {
@@ -446,6 +448,7 @@ describe(
       React = require('react');
       ReactNoop = require('react-noop-renderer');
       Scheduler = require('scheduler');
+      startTransition = React.unstable_startTransition;
     });
 
     afterEach(() => {
@@ -494,6 +497,7 @@ describe(
       });
     });
 
+    // @gate experimental
     it('mock Scheduler module to check if `shouldYield` is called', async () => {
       // This test reproduces a bug where React's Scheduler task timed out but
       // the `shouldYield` method returned true. Usually we try not to mock
@@ -518,7 +522,9 @@ describe(
 
       await ReactNoop.act(async () => {
         // Partially render the tree, then yield
-        ReactNoop.render(<App />);
+        startTransition(() => {
+          ReactNoop.render(<App />);
+        });
         expect(Scheduler).toFlushAndYieldThrough(['A']);
 
         // Start logging whenever shouldYield is called
@@ -535,11 +541,17 @@ describe(
         // We only check before yielding to the main thread (to avoid starvation
         // by other main thread work) or when receiving an update (to avoid
         // starvation by incoming updates).
-        ReactNoop.render(<App />);
+        startTransition(() => {
+          ReactNoop.render(<App />);
+        });
 
         // Because the render expired, React should finish the tree without
         // consulting `shouldYield` again
-        expect(Scheduler).toFlushExpired(['B', 'C']);
+        if (gate(flags => flags.enableParallelTransitions)) {
+          expect(Scheduler).toFlushExpired(['B', 'C']);
+        } else {
+          expect(Scheduler).toFlushExpired(['B', 'C', 'A', 'B', 'C']);
+        }
       });
     });
   },