Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use performConcurrentWorkOnRoot for "sync default" #21322

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiberLane.new.js
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
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
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
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
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
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
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
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
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