[Fiber] Schedule passive effects using the regular ensureRootIsScheduled flow (facebook#31785)

sebmarkbage · web-flow · commit facec3ee71ff · 2024-12-17T17:01:31.000-05:00
This treats workInProgressRoot work and rootWithPendingPassiveEffects the same way. Basically as long as there's some work on the root, yield the current task. Including passive effects. This means that passive effects are now a continuation instead of a separate callback. This can mean they're earlier or later than before. Later for Idle in case there's other non-React work. Earlier for same Default if there's other Default priority work. This makes sense since increasing priority of the passive effects beyond Idle doesn't really make sense for an Idle render. However, for any given render at same priority it's more important to complete this work than start something new. Since we special case continuations to always yield to the browser, this has the same effect as facebook#31784 without implementing `requestPaint`. At least assuming nothing else calls `requestPaint`. <img width="587" alt="Screenshot 2024-12-14 at 5 37 37 PM" src="https://github.com/user-attachments/assets/8641b172-8842-4191-8bf0-50cbe263a30c" />
diff --git a/packages/react-dom/src/__tests__/ReactDOMSelect-test.js b/packages/react-dom/src/__tests__/ReactDOMSelect-test.js
@@ -911,9 +911,7 @@ describe('ReactDOMSelect', () => {
     const container = document.createElement('div');
     const root = ReactDOMClient.createRoot(container);
     async function changeView() {
-      await act(() => {
-        root.unmount();
-      });
+      root.unmount();
     }
 
     const stub = (
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
@@ -3743,6 +3743,10 @@ describe('ReactDOMServerPartialHydration', () => {
     await waitForPaint(['App']);
     expect(visibleRef.current).toBe(visibleSpan);
 
+    if (gate(flags => flags.enableYieldingBeforePassive)) {
+      // Passive effects.
+      await waitForPaint([]);
+    }
     // Subsequently, the hidden child is prerendered on the client
     await waitForPaint(['HiddenChild']);
     expect(container).toMatchInlineSnapshot(`
diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js
@@ -20,6 +20,7 @@ import {
   enableProfilerNestedUpdatePhase,
   enableComponentPerformanceTrack,
   enableSiblingPrerendering,
+  enableYieldingBeforePassive,
 } from 'shared/ReactFeatureFlags';
 import {
   NoLane,
@@ -41,6 +42,8 @@ import {
   getExecutionContext,
   getWorkInProgressRoot,
   getWorkInProgressRootRenderLanes,
+  getRootWithPendingPassiveEffects,
+  getPendingPassiveEffectsLanes,
   isWorkLoopSuspendedOnData,
   performWorkOnRoot,
 } from './ReactFiberWorkLoop';
@@ -324,12 +327,21 @@ function scheduleTaskForRootDuringMicrotask(
   markStarvedLanesAsExpired(root, currentTime);
 
   // Determine the next lanes to work on, and their priority.
+  const rootWithPendingPassiveEffects = getRootWithPendingPassiveEffects();
+  const pendingPassiveEffectsLanes = getPendingPassiveEffectsLanes();
   const workInProgressRoot = getWorkInProgressRoot();
   const workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes();
-  const nextLanes = getNextLanes(
-    root,
-    root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
-  );
+  const nextLanes =
+    enableYieldingBeforePassive && root === rootWithPendingPassiveEffects
+      ? // This will schedule the callback at the priority of the lane but we used to
+        // always schedule it at NormalPriority. Discrete will flush it sync anyway.
+        // So the only difference is Idle and it doesn't seem necessarily right for that
+        // to get upgraded beyond something important just because we're past commit.
+        pendingPassiveEffectsLanes
+      : getNextLanes(
+          root,
+          root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
+        );
 
   const existingCallbackNode = root.callbackNode;
   if (
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -40,6 +40,7 @@ import {
   disableDefaultPropsExceptForClasses,
   enableSiblingPrerendering,
   enableComponentPerformanceTrack,
+  enableYieldingBeforePassive,
 } from 'shared/ReactFeatureFlags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import is from 'shared/objectIs';
@@ -610,7 +611,6 @@ export function getRenderTargetTime(): number {
 
 let legacyErrorBoundariesThatAlreadyFailed: Set<mixed> | null = null;
 
-let rootDoesHavePassiveEffects: boolean = false;
 let rootWithPendingPassiveEffects: FiberRoot | null = null;
 let pendingPassiveEffectsLanes: Lanes = NoLanes;
 let pendingPassiveEffectsRemainingLanes: Lanes = NoLanes;
@@ -638,6 +638,14 @@ export function getWorkInProgressRootRenderLanes(): Lanes {
   return workInProgressRootRenderLanes;
 }
 
+export function getRootWithPendingPassiveEffects(): FiberRoot | null {
+  return rootWithPendingPassiveEffects;
+}
+
+export function getPendingPassiveEffectsLanes(): Lanes {
+  return pendingPassiveEffectsLanes;
+}
+
 export function isWorkLoopSuspendedOnData(): boolean {
   return (
     workInProgressSuspendedReason === SuspendedOnData ||
@@ -3210,12 +3218,6 @@ function commitRootImpl(
     );
   }
 
-  // commitRoot never returns a continuation; it always finishes synchronously.
-  // So we can clear these now to allow a new callback to be scheduled.
-  root.callbackNode = null;
-  root.callbackPriority = NoLane;
-  root.cancelPendingCommit = null;
-
   // Check which lanes no longer have any work scheduled on them, and mark
   // those as finished.
   let remainingLanes = mergeLanes(finishedWork.lanes, finishedWork.childLanes);
@@ -3253,6 +3255,7 @@ function commitRootImpl(
   // might get scheduled in the commit phase. (See #16714.)
   // TODO: Delete all other places that schedule the passive effect callback
   // They're redundant.
+  let rootDoesHavePassiveEffects: boolean = false;
   if (
     // If this subtree rendered with profiling this commit, we need to visit it to log it.
     (enableProfilerTimer &&
@@ -3261,17 +3264,25 @@ function commitRootImpl(
     (finishedWork.subtreeFlags & PassiveMask) !== NoFlags ||
     (finishedWork.flags & PassiveMask) !== NoFlags
   ) {
-    if (!rootDoesHavePassiveEffects) {
-      rootDoesHavePassiveEffects = true;
-      pendingPassiveEffectsRemainingLanes = remainingLanes;
-      pendingPassiveEffectsRenderEndTime = completedRenderEndTime;
-      // workInProgressTransitions might be overwritten, so we want
-      // to store it in pendingPassiveTransitions until they get processed
-      // We need to pass this through as an argument to commitRoot
-      // because workInProgressTransitions might have changed between
-      // the previous render and commit if we throttle the commit
-      // with setTimeout
-      pendingPassiveTransitions = transitions;
+    rootDoesHavePassiveEffects = true;
+    pendingPassiveEffectsRemainingLanes = remainingLanes;
+    pendingPassiveEffectsRenderEndTime = completedRenderEndTime;
+    // workInProgressTransitions might be overwritten, so we want
+    // to store it in pendingPassiveTransitions until they get processed
+    // We need to pass this through as an argument to commitRoot
+    // because workInProgressTransitions might have changed between
+    // the previous render and commit if we throttle the commit
+    // with setTimeout
+    pendingPassiveTransitions = transitions;
+    if (enableYieldingBeforePassive) {
+      // We don't schedule a separate task for flushing passive effects.
+      // Instead, we just rely on ensureRootIsScheduled below to schedule
+      // a callback for us to flush the passive effects.
+    } else {
+      // So we can clear these now to allow a new callback to be scheduled.
+      root.callbackNode = null;
+      root.callbackPriority = NoLane;
+      root.cancelPendingCommit = null;
       scheduleCallback(NormalSchedulerPriority, () => {
         if (enableProfilerTimer && enableComponentPerformanceTrack) {
           // Track the currently executing event if there is one so we can ignore this
@@ -3285,6 +3296,12 @@ function commitRootImpl(
         return null;
       });
     }
+  } else {
+    // If we don't have passive effects, we're not going to need to perform more work
+    // so we can clear the callback now.
+    root.callbackNode = null;
+    root.callbackPriority = NoLane;
+    root.cancelPendingCommit = null;
   }
 
   if (enableProfilerTimer) {
@@ -3441,10 +3458,6 @@ function commitRootImpl(
     onCommitRootTestSelector();
   }
 
-  // Always call this before exiting `commitRoot`, to ensure that any
-  // additional work on this root is scheduled.
-  ensureRootIsScheduled(root);
-
   if (recoverableErrors !== null) {
     // There were errors during this render, but recovered from them without
     // needing to surface it to the UI. We log them here.
@@ -3480,6 +3493,10 @@ function commitRootImpl(
     flushPassiveEffects();
   }
 
+  // Always call this before exiting `commitRoot`, to ensure that any
+  // additional work on this root is scheduled.
+  ensureRootIsScheduled(root);
+
   // Read this again, since a passive effect might have updated it
   remainingLanes = root.pendingLanes;
 
@@ -3648,6 +3665,13 @@ function flushPassiveEffectsImpl(wasDelayedCommit: void | boolean) {
   // because it's only used for profiling), but it's a refactor hazard.
   pendingPassiveEffectsLanes = NoLanes;
 
+  if (enableYieldingBeforePassive) {
+    // We've finished our work for this render pass.
+    root.callbackNode = null;
+    root.callbackPriority = NoLane;
+    root.cancelPendingCommit = null;
+  }
+
   if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
     throw new Error('Cannot flush passive effects while already rendering.');
   }
@@ -3745,6 +3769,14 @@ function flushPassiveEffectsImpl(wasDelayedCommit: void | boolean) {
     didScheduleUpdateDuringPassiveEffects = false;
   }
 
+  if (enableYieldingBeforePassive) {
+    // Next, we reschedule any remaining work in a new task since it's a new
+    // sequence of work. We wait until the end to do this in case the passive
+    // effect schedules higher priority work than we had remaining. That way
+    // we don't schedule an early callback that gets cancelled anyway.
+    ensureRootIsScheduled(root);
+  }
+
   // TODO: Move to commitPassiveMountEffects
   onPostCommitRootDevTools(root);
   if (enableProfilerTimer && enableProfilerCommitHooks) {
diff --git a/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js b/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js
@@ -753,6 +753,10 @@ describe('ReactDeferredValue', () => {
       revealContent();
       // Because the preview state was already prerendered, we can reveal it
       // without any addditional work.
+      if (gate(flags => flags.enableYieldingBeforePassive)) {
+        // Passive effects.
+        await waitForPaint([]);
+      }
       await waitForPaint([]);
       expect(root).toMatchRenderedOutput(<div>Preview [B]</div>);
     });
diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js
@@ -755,10 +755,16 @@ describe('ReactExpiration', () => {
 
       // The update finishes without yielding. But it does not flush the effect.
       await waitFor(['B1'], {
-        additionalLogsAfterAttemptingToYield: ['C1'],
+        additionalLogsAfterAttemptingToYield: gate(
+          flags => flags.enableYieldingBeforePassive,
+        )
+          ? ['C1', 'Effect: 1']
+          : ['C1'],
       });
     });
-    // The effect flushes after paint.
-    assertLog(['Effect: 1']);
+    if (!gate(flags => flags.enableYieldingBeforePassive)) {
+      // The effect flushes after paint.
+      assertLog(['Effect: 1']);
+    }
   });
 });
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -2695,13 +2695,23 @@ describe('ReactHooksWithNoopRenderer', () => {
         React.startTransition(() => {
           ReactNoop.render(<Counter count={1} />);
         });
-        await waitForPaint([
-          'Create passive [current: 0]',
-          'Destroy insertion [current: 0]',
-          'Create insertion [current: 0]',
-          'Destroy layout [current: 1]',
-          'Create layout [current: 1]',
-        ]);
+        if (gate(flags => flags.enableYieldingBeforePassive)) {
+          await waitForPaint(['Create passive [current: 0]']);
+          await waitForPaint([
+            'Destroy insertion [current: 0]',
+            'Create insertion [current: 0]',
+            'Destroy layout [current: 1]',
+            'Create layout [current: 1]',
+          ]);
+        } else {
+          await waitForPaint([
+            'Create passive [current: 0]',
+            'Destroy insertion [current: 0]',
+            'Create insertion [current: 0]',
+            'Destroy layout [current: 1]',
+            'Create layout [current: 1]',
+          ]);
+        }
         expect(committedText).toEqual('1');
       });
       assertLog([
diff --git a/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js b/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js
@@ -200,6 +200,10 @@ describe('ReactSiblingPrerendering', () => {
       await waitForPaint(['A']);
       expect(root).toMatchRenderedOutput('A');
 
+      if (gate(flags => flags.enableYieldingBeforePassive)) {
+        // Passive effects.
+        await waitForPaint([]);
+      }
       // The second render is a prerender of the hidden content.
       await waitForPaint([
         'Suspend! [B]',
@@ -237,6 +241,10 @@ describe('ReactSiblingPrerendering', () => {
       // Immediately after the fallback commits, retry the boundary again. This
       // time we include B, since we're not blocking the fallback from showing.
       if (gate('enableSiblingPrerendering')) {
+        if (gate(flags => flags.enableYieldingBeforePassive)) {
+          // Passive effects.
+          await waitForPaint([]);
+        }
         await waitForPaint(['Suspend! [A]', 'Suspend! [B]']);
       }
     });
@@ -452,6 +460,10 @@ describe('ReactSiblingPrerendering', () => {
           </>,
         );
 
+        if (gate(flags => flags.enableYieldingBeforePassive)) {
+          // Passive effects.
+          await waitForPaint([]);
+        }
         // Immediately after the fallback commits, retry the boundary again.
         // Because the promise for A resolved, this is a normal render, _not_
         // a prerender. So when we proceed to B, and B suspends, we unwind again
@@ -471,6 +483,10 @@ describe('ReactSiblingPrerendering', () => {
           </>,
         );
 
+        if (gate(flags => flags.enableYieldingBeforePassive)) {
+          // Passive effects.
+          await waitForPaint([]);
+        }
         // Now we can proceed to prerendering C.
         if (gate('enableSiblingPrerendering')) {
           await waitForPaint(['Suspend! [B]', 'Suspend! [C]']);
diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
@@ -1901,6 +1901,10 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       // be throttled because the fallback would have appeared too recently.
       Scheduler.unstable_advanceTime(10000);
       jest.advanceTimersByTime(10000);
+      if (gate(flags => flags.enableYieldingBeforePassive)) {
+        // Passive effects.
+        await waitForPaint([]);
+      }
       await waitForPaint(['A']);
       expect(ReactNoop).toMatchRenderedOutput(
         <>
diff --git a/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js b/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js
@@ -81,14 +81,26 @@ describe('ReactUpdatePriority', () => {
       // Schedule another update at default priority
       setDefaultState(2);
 
-      // The default update flushes first, because
-      await waitForPaint([
-        // Idle update is scheduled
-        'Idle update',
-
-        // The default update flushes first, without including the idle update
-        'Idle: 1, Default: 2',
-      ]);
+      if (gate(flags => flags.enableYieldingBeforePassive)) {
+        // The default update flushes first, because
+        await waitForPaint([
+          // Idle update is scheduled
+          'Idle update',
+        ]);
+        await waitForPaint([
+          // The default update flushes first, without including the idle update
+          'Idle: 1, Default: 2',
+        ]);
+      } else {
+        // The default update flushes first, because
+        await waitForPaint([
+          // Idle update is scheduled
+          'Idle update',
+
+          // The default update flushes first, without including the idle update
+          'Idle: 1, Default: 2',
+        ]);
+      }
     });
     // Now the idle update has flushed
     assertLog(['Idle: 2, Default: 2']);
diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js
@@ -77,6 +77,9 @@ export const enableLegacyFBSupport = false;
 // likely to include in an upcoming release.
 // -----------------------------------------------------------------------------
 
+// Yield to the browser event loop and not just the scheduler event loop before passive effects.
+export const enableYieldingBeforePassive = __EXPERIMENTAL__;
+
 export const enableLegacyCache = __EXPERIMENTAL__;
 
 export const enableAsyncIterableChildren = __EXPERIMENTAL__;
diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js
@@ -82,6 +82,7 @@ export const syncLaneExpirationMs = 250;
 export const transitionLaneExpirationMs = 5000;
 export const useModernStrictMode = true;
 export const enableHydrationLaneScheduling = true;
+export const enableYieldingBeforePassive = false;
 
 // Flow magic to verify the exports of this file match the original version.
 ((((null: any): ExportsType): FeatureFlagsType): ExportsType);
diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js
@@ -76,6 +76,8 @@ export const enableUseResourceEffectHook = false;
 
 export const enableHydrationLaneScheduling = true;
 
+export const enableYieldingBeforePassive = false;
+
 // Profiling Only
 export const enableProfilerTimer = __PROFILE__;
 export const enableProfilerCommitHooks = __PROFILE__;
diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js
diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js
diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js
diff --git a/packages/use-subscription/src/__tests__/useSubscription-test.js b/packages/use-subscription/src/__tests__/useSubscription-test.js
