From 1210c37bd8c9d0182b4cb0c6f5d488df668a28ab Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 20 Apr 2021 15:16:33 -0500 Subject: [PATCH] Use performConcurrentWorkOnRoot for "sync default" 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 #21072. --- .../src/ReactFiberLane.new.js | 3 ++ .../src/ReactFiberLane.old.js | 3 ++ .../src/ReactFiberWorkLoop.new.js | 37 +++++-------------- .../src/ReactFiberWorkLoop.old.js | 37 +++++-------------- .../src/__tests__/ReactExpiration-test.js | 9 +---- .../src/__tests__/ReactFlushSync-test.js | 13 +------ .../ReactHooksWithNoopRenderer-test.js | 17 +++++---- .../__tests__/ReactIncrementalUpdates-test.js | 12 ++---- .../useMutableSource-test.internal.js | 13 ++----- 9 files changed, 45 insertions(+), 99 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js index c4332d1b148d7..ced32541ee8df 100644 --- a/packages/react-reconciler/src/ReactFiberLane.new.js +++ b/packages/react-reconciler/src/ReactFiberLane.new.js @@ -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. diff --git a/packages/react-reconciler/src/ReactFiberLane.old.js b/packages/react-reconciler/src/ReactFiberLane.old.js index 05626e7a7376f..5af142edbbae9 100644 --- a/packages/react-reconciler/src/ReactFiberLane.old.js +++ b/packages/react-reconciler/src/ReactFiberLane.old.js @@ -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. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index bcfb82da57f82..665df80b13347 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -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)); @@ -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; @@ -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; @@ -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. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 95066815c4d50..baebee3dc0490 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -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)); @@ -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; @@ -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; @@ -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. diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js index d251bb04f1115..4c89c341c649f 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js @@ -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); @@ -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'); diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js index a4c9fef937797..a3df1effd7b35 100644 --- a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js @@ -51,11 +51,7 @@ 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 @@ -63,12 +59,7 @@ describe('ReactFlushSync', () => { 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'); }); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 95e6a55c7835c..485d477ed7e8e 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -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)); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js index 9eaa95f93edf5..991937ee37fa0 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js @@ -62,15 +62,9 @@ describe('ReactIncrementalUpdates', () => { ReactNoop.render(); 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', () => { diff --git a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js index fd7ebbeb72c78..b8c47e93d6bfe 100644 --- a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js +++ b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js @@ -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.