Skip to content

Commit

Permalink
Bailout in sync task if work is not sync (facebook#20813)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite authored and koto committed Jun 15, 2021
1 parent 884d11c commit 946f588
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
14 changes: 14 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
14 changes: 14 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down

0 comments on commit 946f588

Please sign in to comment.