From 4e8616770890435da8bc9e6a93d0d54d1697a0cf Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 12 Feb 2021 16:08:30 -0600 Subject: [PATCH] Bailout in sync task if work is not sync 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. --- .../plugins/__tests__/SimpleEventPlugin-test.js | 4 ++-- .../react-reconciler/src/ReactFiberWorkLoop.new.js | 14 ++++++++++++++ .../react-reconciler/src/ReactFiberWorkLoop.old.js | 14 ++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js b/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js index 377fa61e507d7..795eda90c83cc 100644 --- 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 index 6935bb4b1f18d..918261a2e3562 100644 --- 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 index 2b57becf8bd79..12ef384a47527 100644 --- 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); }