Bailout in sync task if work is not sync (#20813)

acdlite · web-flow · commit e2fd460cca3b · 2021-02-16T12:38:11.000-08:00
Because we don't cancel synchronous tasks, sometimes more than one
synchronous task ends up being scheduled. This is an artifact of the
fact that we have two different lanes that schedule sync tasks: discrete
and sync. So what can happen is that a discrete update gets scheduled,
then a sync update right after that. Because sync is encoded as higher
priority than discrete, we schedule a second sync task. And since we
don't cancel the first one, there are now two separate sync tasks.

As a next step, what we should do is merge InputDiscreteLane with
SyncLane, then (I believe) this extra bailout wouldn't be necessary,
because there's nothing higher priority than sync that would cause us to
cancel it. Though we may want to add logging to be sure.
diff --git a/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js b/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js
@@ -477,8 +477,8 @@ describe('SimpleEventPlugin', function() {
         // At the end, both counters should equal the total number of clicks
         expect(Scheduler).toHaveYielded([
           'High-pri count: 8, Low-pri count: 0',
-
-          // TODO: with cancellation, this required another flush?
+        ]);
+        expect(Scheduler).toFlushAndYield([
           'High-pri count: 8, Low-pri count: 8',
         ]);
       } else {
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -1028,6 +1028,20 @@ function performSyncWorkOnRoot(root) {
     exitStatus = renderRootSync(root, lanes);
   } else {
     lanes = getNextLanes(root, NoLanes);
+    // Because we don't cancel synchronous tasks, sometimes more than one
+    // synchronous task ends up being scheduled. This is an artifact of the fact
+    // that we have two different lanes that schedule sync tasks: discrete and
+    // sync. If we had only one, then (I believe) this extra check wouldn't be
+    // necessary, because there's nothing higher priority than sync that would
+    // cause us to cancel it.
+    // TODO: Merge InputDiscreteLanePriority with SyncLanePriority, then delete
+    // this bailout.
+    if (supportsMicrotasks) {
+      const nextLanesPriority = returnNextLanesPriority();
+      if (nextLanesPriority < InputDiscreteLanePriority) {
+        return null;
+      }
+    }
     exitStatus = renderRootSync(root, lanes);
   }
 
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -1028,6 +1028,20 @@ function performSyncWorkOnRoot(root) {
     exitStatus = renderRootSync(root, lanes);
   } else {
     lanes = getNextLanes(root, NoLanes);
+    // Because we don't cancel synchronous tasks, sometimes more than one
+    // synchronous task ends up being scheduled. This is an artifact of the fact
+    // that we have two different lanes that schedule sync tasks: discrete and
+    // sync. If we had only one, then (I believe) this extra check wouldn't be
+    // necessary, because there's nothing higher priority than sync that would
+    // cause us to cancel it.
+    // TODO: Merge InputDiscreteLanePriority with SyncLanePriority, then delete
+    // this bailout.
+    if (supportsMicrotasks) {
+      const nextLanesPriority = returnNextLanesPriority();
+      if (nextLanesPriority < InputDiscreteLanePriority) {
+        return null;
+      }
+    }
     exitStatus = renderRootSync(root, lanes);
   }
 
