Repository: facebook/react
Pull request: Unify perform{Sync,Concurrent}WorkOnRoot implementation

Over time the behavior of these two paths has converged to be essentially the same. So this merges them back into one function. This should save some code size and also make it harder for the behavior to accidentally diverge. (For the same reason, rolling out this change might expose some areas where we had already accidentally diverged.)

Changed files:
filename: packages/react-reconciler/src/ReactFiberRootScheduler.js
patch: @@ -15,6 +15,9 @@ import type {BatchConfigTransition} from './ReactFiberTracingMarkerComponent';
 import {
   disableLegacyMode,
   enableDeferRootSchedulingToMicrotask,
+  disableSchedulerTimeoutInWorkLoop,
+  enableProfilerTimer,
+  enableProfilerNestedUpdatePhase,
 } from 'shared/ReactFeatureFlags';
 import {
   NoLane,
@@ -31,12 +34,12 @@ import {
   CommitContext,
   NoContext,
   RenderContext,
+  flushPassiveEffects,
   getExecutionContext,
   getWorkInProgressRoot,
   getWorkInProgressRootRenderLanes,
   isWorkLoopSuspendedOnData,
-  performConcurrentWorkOnRoot,
-  performSyncWorkOnRoot,
+  performWorkOnRoot,
 } from './ReactFiberWorkLoop';
 import {LegacyRoot} from './ReactRootTags';
 import {
@@ -62,6 +65,10 @@ import {
 } from './ReactFiberConfig';
 
 import ReactSharedInternals from 'shared/ReactSharedInternals';
+import {
+  resetNestedUpdateFlag,
+  syncNestedUpdateFlag,
+} from './ReactProfilerTimer';
 
 // A linked list of all the roots with pending work. In an idiomatic app,
 // there's only a single root, but we do support multi root apps, hence this
@@ -387,7 +394,7 @@ function scheduleTaskForRootDuringMicrotask(
 
     const newCallbackNode = scheduleCallback(
       schedulerPriorityLevel,
-      performConcurrentWorkOnRoot.bind(null, root),
+      performWorkOnRootViaSchedulerTask.bind(null, root),
     );
 
     root.callbackPriority = newCallbackPriority;
@@ -396,15 +403,67 @@ function scheduleTaskForRootDuringMicrotask(
   }
 }
 
-export type RenderTaskFn = (didTimeout: boolean) => RenderTaskFn | null;
+type RenderTaskFn = (didTimeout: boolean) => RenderTaskFn | null;
 
-export function getContinuationForRoot(
+function performWorkOnRootViaSchedulerTask(
   root: FiberRoot,
-  originalCallbackNode: mixed,
+  didTimeout: boolean,
 ): RenderTaskFn | null {
-  // This is called at the end of `performConcurrentWorkOnRoot` to determine
-  // if we need to schedule a continuation task.
-  //
+  // This is the entry point for concurrent tasks scheduled via Scheduler (and
+  // postTask, in the future).
+
+  if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
+    resetNestedUpdateFlag();
+  }
+
+  // Flush any pending passive effects before deciding which lanes to work on,
+  // in case they schedule additional work.
+  const originalCallbackNode = root.callbackNode;
+  const didFlushPassiveEffects = flushPassiveEffects();
+  if (didFlushPassiveEffects) {
+    // Something in the passive effect phase may have canceled the current task.
+    // Check if the task node for this root was changed.
+    if (root.callbackNode !== originalCallbackNode) {
+      // The current task was canceled. Exit. We don't need to call
+      // `ensureRootIsScheduled` because the check above implies either that
+      // there's a new task, or that there's no remaining work on this root.
+      return null;
+    } else {
+      // Current task was not canceled. Continue.
+    }
+  }
+
+  // Determine the next lanes to work on, using the fields stored on the root.
+  // TODO: We already called getNextLanes when we scheduled the callback; we
+  // should be able to avoid calling it again by stashing the result on the
+  // root object. However, because we always schedule the callback during
+  // a microtask (scheduleTaskForRootDuringMicrotask), it's possible that
+  // an update was scheduled earlier during this same browser task (and
+  // therefore before the microtasks have run). That's because Scheduler batches
+  // together multiple callbacks into a single browser macrotask, without
+  // yielding to microtasks in between. We should probably change this to align
+  // with the postTask behavior (and literally use postTask when
+  // it's available).
+  const workInProgressRoot = getWorkInProgressRoot();
+  const workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes();
+  const lanes = getNextLanes(
+    root,
+    root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
+  );
+  if (lanes === NoLanes) {
+    // No more work on this root.
+    return null;
+  }
+
+  // Enter the work loop.
+  // TODO: We only check `didTimeout` defensively, to account for a Scheduler
+  // bug we're still investigating. Once the bug in Scheduler is fixed,
+  // we can remove this, since we track expiration ourselves.
+  const forceSync = !disableSchedulerTimeoutInWorkLoop && didTimeout;
+  performWorkOnRoot(root, lanes, forceSync);
+
+  // The work loop yielded, but there may or may not be work left at the current
+  // priority. Need to determine whether we need to schedule a continuation.
   // Usually `scheduleTaskForRootDuringMicrotask` only runs inside a microtask;
   // however, since most of the logic for determining if we need a continuation
   // versus a new task is the same, we cheat a bit and call it here. This is
@@ -414,11 +473,27 @@ export function getContinuationForRoot(
   if (root.callbackNode === originalCallbackNode) {
     // The task node scheduled for this root is the same one that's
     // currently executed. Need to return a continuation.
-    return performConcurrentWorkOnRoot.bind(null, root);
+    return performWorkOnRootViaSchedulerTask.bind(null, root);
   }
   return null;
 }
 
+function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes) {
+  // This is the entry point for synchronous tasks that don't go
+  // through Scheduler.
+  const didFlushPassiveEffects = flushPassiveEffects();
+  if (didFlushPassiveEffects) {
+    // If passive effects were flushed, exit to the outer work loop in the root
+    // scheduler, so we can recompute the priority.
+    return null;
+  }
+  if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
+    syncNestedUpdateFlag();
+  }
+  const forceSync = true;
+  performWorkOnRoot(root, lanes, forceSync);
+}
+
 const fakeActCallbackNode = {};
 
 function scheduleCallback(

filename: packages/react-reconciler/src/ReactFiberWorkLoop.js
patch: @@ -22,7 +22,6 @@ import type {
   TransitionAbort,
 } from './ReactFiberTracingMarkerComponent';
 import type {OffscreenInstance} from './ReactFiberActivityComponent';
-import type {RenderTaskFn} from './ReactFiberRootScheduler';
 import type {Resource} from './ReactFiberConfig';
 
 import {
@@ -32,7 +31,6 @@ import {
   enableProfilerNestedUpdatePhase,
   enableDebugTracing,
   enableSchedulingProfiler,
-  disableSchedulerTimeoutInWorkLoop,
   enableUpdaterTracking,
   enableCache,
   enableTransitionTracing,
@@ -250,11 +248,9 @@ import {
   recordRenderTime,
   recordCommitTime,
   recordCommitEndTime,
-  resetNestedUpdateFlag,
   startProfilerTimer,
   stopProfilerTimerIfRunningAndRecordDuration,
   stopProfilerTimerIfRunningAndRecordIncompleteDuration,
-  syncNestedUpdateFlag,
 } from './ReactProfilerTimer';
 import {setCurrentTrackFromLanes} from './ReactFiberPerformanceTrack';
 
@@ -308,7 +304,6 @@ import {
   ensureRootIsScheduled,
   flushSyncWorkOnAllRoots,
   flushSyncWorkOnLegacyRootsOnly,
-  getContinuationForRoot,
   requestTransitionLane,
 } from './ReactFiberRootScheduler';
 import {getMaskedContext, getUnmaskedContext} from './ReactFiberContext';
@@ -890,59 +885,22 @@ export function isUnsafeClassRenderPhaseUpdate(fiber: Fiber): boolean {
   return (executionContext & RenderContext) !== NoContext;
 }
 
-// This is the entry point for every concurrent task, i.e. anything that
-// goes through Scheduler.
-export function performConcurrentWorkOnRoot(
+export function performWorkOnRoot(
   root: FiberRoot,
-  didTimeout: boolean,
-): RenderTaskFn | null {
-  if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
-    resetNestedUpdateFlag();
-  }
-
+  lanes: Lanes,
+  forceSync: boolean,
+): void {
   if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
     throw new Error('Should not already be working.');
   }
 
-  // Flush any pending passive effects before deciding which lanes to work on,
-  // in case they schedule additional work.
-  const originalCallbackNode = root.callbackNode;
-  const didFlushPassiveEffects = flushPassiveEffects();
-  if (didFlushPassiveEffects) {
-    // Something in the passive effect phase may have canceled the current task.
-    // Check if the task node for this root was changed.
-    if (root.callbackNode !== originalCallbackNode) {
-      // The current task was canceled. Exit. We don't need to call
-      // `ensureRootIsScheduled` because the check above implies either that
-      // there's a new task, or that there's no remaining work on this root.
-      return null;
-    } else {
-      // Current task was not canceled. Continue.
-    }
-  }
-
-  // Determine the next lanes to work on, using the fields stored
-  // on the root.
-  // TODO: This was already computed in the caller. Pass it as an argument.
-  let lanes = getNextLanes(
-    root,
-    root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
-  );
-  if (lanes === NoLanes) {
-    // Defensive coding. This is never expected to happen.
-    return null;
-  }
-
   // We disable time-slicing in some cases: if the work has been CPU-bound
   // for too long ("expired" work, to prevent starvation), or we're in
   // sync-updates-by-default mode.
-  // TODO: We only check `didTimeout` defensively, to account for a Scheduler
-  // bug we're still investigating. Once the bug in Scheduler is fixed,
-  // we can remove this, since we track expiration ourselves.
   const shouldTimeSlice =
+    !forceSync &&
     !includesBlockingLane(lanes) &&
-    !includesExpiredLane(root, lanes) &&
-    (disableSchedulerTimeoutInWorkLoop || !didTimeout);
+    !includesExpiredLane(root, lanes);
   let exitStatus = shouldTimeSlice
     ? renderRootConcurrent(root, lanes)
     : renderRootSync(root, lanes);
@@ -984,7 +942,10 @@ export function performConcurrentWorkOnRoot(
         }
 
         // Check if something threw
-        if (exitStatus === RootErrored) {
+        if (
+          (disableLegacyMode || root.tag !== LegacyRoot) &&
+          exitStatus === RootErrored
+        ) {
           const lanesThatJustErrored = lanes;
           const errorRetryLanes = getLanesToRetrySynchronouslyOnError(
             root,
@@ -1033,7 +994,6 @@ export function performConcurrentWorkOnRoot(
   }
 
   ensureRootIsScheduled(root);
-  return getContinuationForRoot(root, originalCallbackNode);
 }
 
 function recoverFromConcurrentError(
@@ -1464,104 +1424,6 @@ function markRootSuspended(
   );
 }
 
-// This is the entry point for synchronous tasks that don't go
-// through Scheduler
-export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null {
-  if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
-    throw new Error('Should not already be working.');
-  }
-
-  const didFlushPassiveEffects = flushPassiveEffects();
-  if (didFlushPassiveEffects) {
-    // If passive effects were flushed, exit to the outer work loop in the root
-    // scheduler, so we can recompute the priority.
-    // TODO: We don't actually need this `ensureRootIsScheduled` call because
-    // this path is only reachable if the root is already part of the schedule.
-    // I'm including it only for consistency with the other exit points from
-    // this function. Can address in a subsequent refactor.
-    ensureRootIsScheduled(root);
-    return null;
-  }
-
-  if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
-    syncNestedUpdateFlag();
-  }
-
-  let exitStatus = renderRootSync(root, lanes);
-  if (
-    (disableLegacyMode || root.tag !== LegacyRoot) &&
-    exitStatus === RootErrored
-  ) {
-    // If something threw an error, try rendering one more time. We'll render
-    // synchronously to block concurrent data mutations, and we'll includes
-    // all pending updates are included. If it still fails after the second
-    // attempt, we'll give up and commit the resulting tree.
-    const originallyAttemptedLanes = lanes;
-    const errorRetryLanes = getLanesToRetrySynchronouslyOnError(
-      root,
-      originallyAttemptedLanes,
-    );
-    if (errorRetryLanes !== NoLanes) {
-      lanes = errorRetryLanes;
-      exitStatus = recoverFromConcurrentError(
-        root,
-        originallyAttemptedLanes,
-        errorRetryLanes,
-      );
-    }
-  }
-
-  if (exitStatus === RootFatalErrored) {
-    prepareFreshStack(root, NoLanes);
-    markRootSuspended(root, lanes, NoLane, false);
-    ensureRootIsScheduled(root);
-    return null;
-  }
-
-  if (exitStatus === RootDidNotComplete) {
-    // The render unwound without completing the tree. This happens in special
-    // cases where need to exit the current render without producing a
-    // consistent tree or committing.
-    markRootSuspended(
-      root,
-      lanes,
-      workInProgressDeferredLane,
-      workInProgressRootDidSkipSuspendedSiblings,
-    );
-    ensureRootIsScheduled(root);
-    return null;
-  }
-
-  let renderEndTime = 0;
-  if (enableProfilerTimer && enableComponentPerformanceTrack) {
-    renderEndTime = now();
-  }
-
-  // We now have a consistent tree. Because this is a sync render, we
-  // will commit it even if something suspended.
-  const finishedWork: Fiber = (root.current.alternate: any);
-  root.finishedWork = finishedWork;
-  root.finishedLanes = lanes;
-  commitRoot(
-    root,
-    workInProgressRootRecoverableErrors,
-    workInProgressTransitions,
-    workInProgressRootDidIncludeRecursiveRenderUpdate,
-    workInProgressDeferredLane,
-    workInProgressRootInterleavedUpdatedLanes,
-    workInProgressSuspendedRetryLanes,
-    IMMEDIATE_COMMIT,
-    renderStartTime,
-    renderEndTime,
-  );
-
-  // Before exiting, make sure there's a callback scheduled for the next
-  // pending level.
-  ensureRootIsScheduled(root);
-
-  return null;
-}
-
 export function flushRoot(root: FiberRoot, lanes: Lanes) {
   if (lanes !== NoLanes) {
     upgradePendingLanesToSync(root, lanes);

filename: packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js
patch: @@ -349,6 +349,7 @@ describe('ReactSiblingPrerendering', () => {
             <div>
               <Suspense fallback={<Text text="Loading inner..." />}>
                 <AsyncText text="B" />
+                <AsyncText text="C" />
               </Suspense>
             </div>
           </Suspense>
@@ -370,10 +371,17 @@ describe('ReactSiblingPrerendering', () => {
       // is throttled because it's been less than a Just Noticeable Difference
       // since the outer fallback was committed.
       //
-      // In the meantime, we could choose to start prerendering B, but instead
+      // In the meantime, we could choose to start prerendering C, but instead
       // we wait for a JND to elapse and the commit to finish — it's not
       // worth discarding the work we've already done.
-      await waitForAll(['A', 'Suspend! [B]', 'Loading inner...']);
+      await waitForAll([
+        'A',
+        'Suspend! [B]',
+
+        // C is skipped because we're no longer in prerendering mode; there's
+        // a new fallback we can show.
+        'Loading inner...',
+      ]);
       expect(root).toMatchRenderedOutput(<div>Loading outer...</div>);
 
       // Fire the timer to commit the outer fallback.
@@ -385,8 +393,10 @@ describe('ReactSiblingPrerendering', () => {
         </div>,
       );
     });
-    // Once the outer fallback is committed, we can start prerendering B.
-    assertLog(gate('enableSiblingPrerendering') ? ['Suspend! [B]'] : []);
+    // Once the inner fallback is committed, we can start prerendering C.
+    assertLog(
+      gate('enableSiblingPrerendering') ? ['Suspend! [B]', 'Suspend! [C]'] : [],
+    );
   });
 
   it(

filename: packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js
patch: @@ -239,11 +239,7 @@ describe('ReactSuspenseyCommitPhase', () => {
     expect(root).toMatchRenderedOutput(<suspensey-thing src="B" />);
   });
 
-  // @TODO This isn't actually ideal behavior. We would really want the commit to suspend
-  // even if it is forced to be sync because we don't want to FOUC but refactoring the sync
-  // pathway is too risky to land right now so we just accept that we can still FOUC in this
-  // very specific case.
-  it('does not suspend commit during urgent initial mount at the root when sync rendering', async () => {
+  it('does suspend commit during urgent initial mount at the root when sync rendering', async () => {
     const root = ReactNoop.createRoot();
     await act(async () => {
       ReactNoop.flushSync(() => {
@@ -252,19 +248,15 @@ describe('ReactSuspenseyCommitPhase', () => {
     });
     assertLog(['Image requested [A]']);
     expect(getSuspenseyThingStatus('A')).toBe('pending');
-    // We would expect this to be null if we did in fact suspend this commit
-    expect(root).toMatchRenderedOutput(<suspensey-thing src="A" />);
+    // Suspend the initial mount
+    expect(root).toMatchRenderedOutput(null);
 
     resolveSuspenseyThing('A');
     expect(getSuspenseyThingStatus('A')).toBe('fulfilled');
     expect(root).toMatchRenderedOutput(<suspensey-thing src="A" />);
   });
 
-  // @TODO This isn't actually ideal behavior. We would really want the commit to suspend
-  // even if it is forced to be sync because we don't want to FOUC but refactoring the sync
-  // pathway is too risky to land right now so we just accept that we can still FOUC in this
-  // very specific case.
-  it('does not suspend commit during urgent update at the root when sync rendering', async () => {
+  it('does suspend commit during urgent update at the root when sync rendering', async () => {
     const root = ReactNoop.createRoot();
     await act(() => resolveSuspenseyThing('A'));
     expect(getSuspenseyThingStatus('A')).toBe('fulfilled');
@@ -283,8 +275,8 @@ describe('ReactSuspenseyCommitPhase', () => {
     });
     assertLog(['Image requested [B]']);
     expect(getSuspenseyThingStatus('B')).toBe('pending');
-    // We would expect this to be hidden if we did in fact suspend this commit
-    expect(root).toMatchRenderedOutput(<suspensey-thing src="B" />);
+    // Suspend and remain on previous screen
+    expect(root).toMatchRenderedOutput(<suspensey-thing src="A" />);
 
     resolveSuspenseyThing('B');
     expect(getSuspenseyThingStatus('B')).toBe('fulfilled');