Skip to content

Commit

Permalink
Use performConcurrentWorkOnRoot for "sync default" (facebook#21322)
Browse files Browse the repository at this point in the history 
Instead of `performSyncWorkOnRoot`.

The conceptual model is that the only difference between sync default
updates (in React 18) and concurrent default updates (in a future major
release) is time slicing. All other behavior should be the same
(i.e. the stuff in `finishConcurrentRender`).

Given this, I think it makes more sense to model the implementation this
way, too. This exposed a quirk in the previous implementation where
non-sync work was sometimes mistaken for sync work and flushed too
early. In the new implementation, `performSyncWorkOnRoot` is only used
for truly synchronous renders (i.e. `SyncLane`), which should make these
mistakes less common.

Fixes most of the tests marked with TODOs from facebook#21072.
  • Loading branch information
@acdlite @koto
acdlite authored and koto committed Jun 15, 2021
1 parent c711806 commit 826ce2f
Show file tree
Hide file tree
Showing 9 changed files with 45 additions and 99 deletions.
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiberLane.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,9 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
// Default priority updates should not interrupt transition updates. The
// only difference between default updates and transition updates is that
// default updates do not support refresh transitions.
// TODO: This applies to sync default updates, too. Which is probably what
// we want for default priority events, but not for continuous priority
// events like hover.
(nextLane === DefaultLane && (wipLane & TransitionLanes) !== NoLanes)
) {
// Keep working on the existing in-progress tree. Do not interrupt.
Expand Down
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiberLane.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,9 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
// Default priority updates should not interrupt transition updates. The
// only difference between default updates and transition updates is that
// default updates do not support refresh transitions.
// TODO: This applies to sync default updates, too. Which is probably what
// we want for default priority events, but not for continuous priority
// events like hover.
(nextLane === DefaultLane && (wipLane & TransitionLanes) !== NoLanes)
) {
// Keep working on the existing in-progress tree. Do not interrupt.
Expand Down
37 changes: 10 additions & 27 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -713,16 +713,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {

// Schedule a new callback.
let newCallbackNode;
if (
enableSyncDefaultUpdates &&
(newCallbackPriority === DefaultLane ||
newCallbackPriority === DefaultHydrationLane)
) {
newCallbackNode = scheduleCallback(
ImmediateSchedulerPriority,
performSyncWorkOnRoot.bind(null, root),
);
} else if (newCallbackPriority === SyncLane) {
if (newCallbackPriority === SyncLane) {
// Special case: Sync React callbacks are scheduled on a special
// internal queue
scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root));
Expand Down Expand Up @@ -820,7 +811,13 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
return null;
}

let exitStatus = renderRootConcurrent(root, lanes);
let exitStatus =
enableSyncDefaultUpdates &&
(includesSomeLane(lanes, DefaultLane) ||
includesSomeLane(lanes, DefaultHydrationLane))
? // Time slicing is disabled for default updates in this root.
renderRootSync(root, lanes)
: renderRootConcurrent(root, lanes);
if (exitStatus !== RootIncomplete) {
if (exitStatus === RootErrored) {
executionContext |= RetryAfterError;
Expand Down Expand Up @@ -1017,13 +1014,7 @@ function performSyncWorkOnRoot(root) {
// rendering it before rendering the rest of the expired work.
lanes = workInProgressRootRenderLanes;
}
} else if (
!(
enableSyncDefaultUpdates &&
(includesSomeLane(lanes, DefaultLane) ||
includesSomeLane(lanes, DefaultHydrationLane))
)
) {
} else {
// There's no remaining sync work left.
ensureRootIsScheduled(root, now());
return null;
Expand Down Expand Up @@ -1067,15 +1058,7 @@ function performSyncWorkOnRoot(root) {
const finishedWork: Fiber = (root.current.alternate: any);
root.finishedWork = finishedWork;
root.finishedLanes = lanes;
if (
enableSyncDefaultUpdates &&
(includesSomeLane(lanes, DefaultLane) ||
includesSomeLane(lanes, DefaultHydrationLane))
) {
finishConcurrentRender(root, exitStatus, lanes);
} else {
commitRoot(root);
}
commitRoot(root);

// Before exiting, make sure there's a callback scheduled for the next
// pending level.
Expand Down
37 changes: 10 additions & 27 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -713,16 +713,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {

// Schedule a new callback.
let newCallbackNode;
if (
enableSyncDefaultUpdates &&
(newCallbackPriority === DefaultLane ||
newCallbackPriority === DefaultHydrationLane)
) {
newCallbackNode = scheduleCallback(
ImmediateSchedulerPriority,
performSyncWorkOnRoot.bind(null, root),
);
} else if (newCallbackPriority === SyncLane) {
if (newCallbackPriority === SyncLane) {
// Special case: Sync React callbacks are scheduled on a special
// internal queue
scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root));
Expand Down Expand Up @@ -820,7 +811,13 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
return null;
}

let exitStatus = renderRootConcurrent(root, lanes);
let exitStatus =
enableSyncDefaultUpdates &&
(includesSomeLane(lanes, DefaultLane) ||
includesSomeLane(lanes, DefaultHydrationLane))
? // Time slicing is disabled for default updates in this root.
renderRootSync(root, lanes)
: renderRootConcurrent(root, lanes);
if (exitStatus !== RootIncomplete) {
if (exitStatus === RootErrored) {
executionContext |= RetryAfterError;
Expand Down Expand Up @@ -1017,13 +1014,7 @@ function performSyncWorkOnRoot(root) {
// rendering it before rendering the rest of the expired work.
lanes = workInProgressRootRenderLanes;
}
} else if (
!(
enableSyncDefaultUpdates &&
(includesSomeLane(lanes, DefaultLane) ||
includesSomeLane(lanes, DefaultHydrationLane))
)
) {
} else {
// There's no remaining sync work left.
ensureRootIsScheduled(root, now());
return null;
Expand Down Expand Up @@ -1067,15 +1058,7 @@ function performSyncWorkOnRoot(root) {
const finishedWork: Fiber = (root.current.alternate: any);
root.finishedWork = finishedWork;
root.finishedLanes = lanes;
if (
enableSyncDefaultUpdates &&
(includesSomeLane(lanes, DefaultLane) ||
includesSomeLane(lanes, DefaultHydrationLane))
) {
finishConcurrentRender(root, exitStatus, lanes);
} else {
commitRoot(root);
}
commitRoot(root);

// Before exiting, make sure there's a callback scheduled for the next
// pending level.
Expand Down
9 changes: 1 addition & 8 deletions packages/react-reconciler/src/__tests__/ReactExpiration-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -440,12 +440,7 @@ describe('ReactExpiration', () => {
flushNextRenderIfExpired();
expect(Scheduler).toHaveYielded([]);

if (gate(flags => flags.enableSyncDefaultUpdates)) {
// TODO: Why is this flushed?
expect(ReactNoop).toMatchRenderedOutput('Hi');
} else {
expect(ReactNoop).toMatchRenderedOutput(null);
}
expect(ReactNoop).toMatchRenderedOutput(null);

// Advance the time some more to expire the update.
Scheduler.unstable_advanceTime(10000);
Expand Down Expand Up @@ -477,8 +472,6 @@ describe('ReactExpiration', () => {
// Advancing by ~5 seconds should be sufficient to expire the update. (I
// used a slightly larger number to allow for possible rounding.)
Scheduler.unstable_advanceTime(6000);

ReactNoop.render('Hi');
flushNextRenderIfExpired();
expect(Scheduler).toHaveYielded([]);
expect(ReactNoop).toMatchRenderedOutput('Hi');
Expand Down
13 changes: 2 additions & 11 deletions packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,24 +51,15 @@ describe('ReactFlushSync', () => {
// The passive effect will schedule a sync update and a normal update.
// They should commit in two separate batches. First the sync one.
expect(() => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
expect(Scheduler).toFlushUntilNextPaint(['1, 0', '1, 1']);
} else {
expect(Scheduler).toFlushUntilNextPaint(['1, 0']);
}
expect(Scheduler).toFlushUntilNextPaint(['1, 0']);
}).toErrorDev('flushSync was called from inside a lifecycle method');

// The remaining update is not sync
ReactNoop.flushSync();
expect(Scheduler).toHaveYielded([]);

// Now flush it.
if (gate(flags => flags.enableSyncDefaultUpdates)) {
// With sync default updates, passive effects are synchronously flushed.
expect(Scheduler).toHaveYielded([]);
} else {
expect(Scheduler).toFlushUntilNextPaint(['1, 1']);
}
expect(Scheduler).toFlushUntilNextPaint(['1, 1']);
});
expect(root).toMatchRenderedOutput('1, 1');
});
Expand Down
17 changes: 9 additions & 8 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1406,20 +1406,21 @@ describe('ReactHooksWithNoopRenderer', () => {
setParentState(false);
});
if (gate(flags => flags.enableSyncDefaultUpdates)) {
// TODO: Default updates do not interrupt transition updates, to
// prevent starvation. However, when sync default updates are enabled,
// continuous updates are treated like default updates. In this case,
// we probably don't want this behavior; continuous should be allowed
// to interrupt.
expect(Scheduler).toFlushUntilNextPaint([
// TODO: why do the children render and fire effects?
'Child two render',
'Child one commit',
'Child two commit',
'Parent false render',
'Parent false commit',
]);
} else {
expect(Scheduler).toFlushUntilNextPaint([
'Parent false render',
'Parent false commit',
]);
}
expect(Scheduler).toFlushUntilNextPaint([
'Parent false render',
'Parent false commit',
]);

// Schedule updates for children too (which should be ignored)
setChildStates.forEach(setChildState => setChildState(2));
Expand Down
12 changes: 3 additions & 9 deletions packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,9 @@ describe('ReactIncrementalUpdates', () => {
ReactNoop.render(<Foo />);
expect(Scheduler).toFlushAndYieldThrough(['commit']);

if (gate(flags => flags.enableSyncDefaultUpdates)) {
// TODO: should deferredUpdates flush sync with the default update?
expect(state).toEqual({a: 'a', b: 'b', c: 'c'});
expect(Scheduler).toFlushWithoutYielding();
} else {
expect(state).toEqual({a: 'a'});
expect(Scheduler).toFlushWithoutYielding();
expect(state).toEqual({a: 'a', b: 'b', c: 'c'});
}
expect(state).toEqual({a: 'a'});
expect(Scheduler).toFlushWithoutYielding();
expect(state).toEqual({a: 'a', b: 'b', c: 'c'});
});

it('applies updates with equal priority in insertion order', () => {
Expand Down
13 changes: 4 additions & 9 deletions packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1569,15 +1569,10 @@ describe('useMutableSource', () => {
mutateB('b0');
});
// Finish the current render
if (gate(flags => flags.enableSyncDefaultUpdates)) {
// Default sync will flush both without yielding
expect(Scheduler).toFlushUntilNextPaint(['c', 'a0']);
} else {
expect(Scheduler).toFlushUntilNextPaint(['c']);
// a0 will re-render because of the mutation update. But it should show
// the latest value, not the intermediate one, to avoid tearing with b.
expect(Scheduler).toFlushUntilNextPaint(['a0']);
}
expect(Scheduler).toFlushUntilNextPaint(['c']);
// a0 will re-render because of the mutation update. But it should show
// the latest value, not the intermediate one, to avoid tearing with b.
expect(Scheduler).toFlushUntilNextPaint(['a0']);

expect(root).toMatchRenderedOutput('a0b0c');
// We should be done.
Expand Down

0 comments on commit 826ce2f

Please sign in to comment.