@samitouri / QOS-React-1 / commits / defffdbba4

[Fiber] Don't work on scheduled tasks while we're in an async commit but flush it eagerly if we're sync (#31987)

This is a follow up to #31930 and a prerequisite for #31975. With View Transitions, the commit phase becomes async which means that other work can sneak in between. We need to be resilient to that. This PR first refactors the flushMutationEffects and flushLayoutEffects to use module scope variables to track its arguments so we can defer them. It shares these with how we were already doing it for flushPendingEffects. We also track how far along the commit phase we are so we know what we have left to flush. Then callers of flushPassiveEffects become flushPendingEffects. That helper synchronously flushes any remaining phases we've yet to commit. That ensure that things are at least consistent if that happens. Finally, when we are using a scheduled task, we don't do any work. This ensures that we're not flushing any work too early if we could've deferred it. This still ensures that we always do flush it before starting any new work on any root so new roots observe the committed state. There are some unfortunate effects that could happen from allowing things to flush eagerly. Such as if a flushSync sneaks in before startViewTransition, it'll skip the animation. If it's during a suspensey font it'll start the transition before the font has loaded which might be better than breaking flushSync. It'll also potentially flush passive effects inside the startViewTransition which should typically be ok.

Sebastian Markbåge committed Jan 6, 2025 at 11:30 UTC defffdbba43f89b95d9f67a4fb0fa146c1211734

4 files changed +150 -105

packages/react-reconciler/src/ReactFiberHotReloading.js

+2 -2

@@ -16,7 +16,7 @@ import type {ReactNodeList} from 'shared/ReactTypes';

16 import {

17 flushSyncWork,

18 scheduleUpdateOnFiber,

19 - flushPassiveEffects,

19 + flushPendingEffects,

20 } from './ReactFiberWorkLoop';

21 import {enqueueConcurrentRenderForLane} from './ReactFiberConcurrentUpdates';

22 import {updateContainerSync} from './ReactFiberReconciler';

@@ -229,7 +229,7 @@ export const scheduleRefresh: ScheduleRefresh = (

229 return;

230 }

231 const {staleFamilies, updatedFamilies} = update;

232 - flushPassiveEffects();

232 + flushPendingEffects();

233 scheduleFibersWithFamiliesRecursively(

234 root.current,

235 updatedFamilies,

packages/react-reconciler/src/ReactFiberReconciler.js

+5 -4

@@ -41,6 +41,7 @@ import isArray from 'shared/isArray';

41 import {

42 enableSchedulingProfiler,

43 enableHydrationLaneScheduling,

44 + disableLegacyMode,

45 } from 'shared/ReactFeatureFlags';

46 import ReactSharedInternals from 'shared/ReactSharedInternals';

47 import {

@@ -75,7 +76,7 @@ import {

76 isAlreadyRendering,

77 deferredUpdates,

78 discreteUpdates,

78 - flushPassiveEffects,

79 + flushPendingEffects,

80 } from './ReactFiberWorkLoop';

81 import {enqueueConcurrentRenderForLane} from './ReactFiberConcurrentUpdates';

82 import {

@@ -364,8 +365,8 @@ export function updateContainerSync(

365 parentComponent: ?React$Component<any, any>,

366 callback: ?Function,

367 ): Lane {

367 - if (container.tag === LegacyRoot) {

368 - flushPassiveEffects();

368 + if (!disableLegacyMode && container.tag === LegacyRoot) {

369 + flushPendingEffects();

370 }

371 const current = container.current;

372 updateContainerImpl(

@@ -453,7 +454,7 @@ export {

454 flushSyncFromReconciler,

455 flushSyncWork,

456 isAlreadyRendering,

456 - flushPassiveEffects,

457 + flushPendingEffects as flushPassiveEffects,

458 };

459

460 export function getPublicRootInstance(

packages/react-reconciler/src/ReactFiberRootScheduler.js

+17 -3

@@ -38,12 +38,13 @@ import {

38 CommitContext,

39 NoContext,

40 RenderContext,

41 - flushPassiveEffects,

41 + flushPendingEffects,

42 getExecutionContext,

43 getWorkInProgressRoot,

44 getWorkInProgressRootRenderLanes,

45 getRootWithPendingPassiveEffects,

46 getPendingPassiveEffectsLanes,

47 + hasPendingCommitEffects,

48 isWorkLoopSuspendedOnData,

49 performWorkOnRoot,

50 } from './ReactFiberWorkLoop';

@@ -466,10 +467,23 @@ function performWorkOnRootViaSchedulerTask(

467 trackSchedulerEvent();

468 }

469

470 + if (hasPendingCommitEffects()) {

471 + // We are currently in the middle of an async committing (such as a View Transition).

472 + // We could force these to flush eagerly but it's better to defer any work until

473 + // it finishes. This may not be the same root as we're waiting on.

474 + // TODO: This relies on the commit eventually calling ensureRootIsScheduled which

475 + // always calls processRootScheduleInMicrotask which in turn always loops through

476 + // all the roots to figure out. This is all a bit inefficient and if optimized

477 + // it'll need to consider rescheduling a task for any skipped roots.

478 + root.callbackNode = null;

479 + root.callbackPriority = NoLane;

480 + return null;

481 + }

482 +

483 // Flush any pending passive effects before deciding which lanes to work on,

484 // in case they schedule additional work.

485 const originalCallbackNode = root.callbackNode;

472 - const didFlushPassiveEffects = flushPassiveEffects();

486 + const didFlushPassiveEffects = flushPendingEffects(true);

487 if (didFlushPassiveEffects) {

488 // Something in the passive effect phase may have canceled the current task.

489 // Check if the task node for this root was changed.

@@ -534,7 +548,7 @@ function performWorkOnRootViaSchedulerTask(

548 function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes) {

549 // This is the entry point for synchronous tasks that don't go

550 // through Scheduler.

537 - const didFlushPassiveEffects = flushPassiveEffects();

551 + const didFlushPassiveEffects = flushPendingEffects();

552 if (didFlushPassiveEffects) {

553 // If passive effects were flushed, exit to the outer work loop in the root

554 // scheduler, so we can recompute the priority.

packages/react-reconciler/src/ReactFiberWorkLoop.js

+126 -96

@@ -426,7 +426,6 @@ let workInProgressRootDidIncludeRecursiveRenderUpdate: boolean = false;

426 // variable from the one for renders because the commit phase may run

427 // concurrently to a render phase.

428 let didIncludeCommitPhaseUpdate: boolean = false;

429 -

429 // The most recent time we either committed a fallback, or when a fallback was

430 // filled in with the resolved UI. This lets us throttle the appearance of new

431 // content as it streams in, to minimize jank.

@@ -617,11 +616,25 @@ export function getRenderTargetTime(): number {

616

617 let legacyErrorBoundariesThatAlreadyFailed: Set<mixed> | null = null;

618

620 -let rootWithPendingPassiveEffects: FiberRoot | null = null;

621 -let pendingPassiveEffectsLanes: Lanes = NoLanes;

622 -let pendingPassiveEffectsRemainingLanes: Lanes = NoLanes;

623 -let pendingPassiveEffectsRenderEndTime: number = -0; // Profiling-only

619 +type SuspendedCommitReason = 0 | 1 | 2;

620 +const IMMEDIATE_COMMIT = 0;

621 +const SUSPENDED_COMMIT = 1;

622 +const THROTTLED_COMMIT = 2;

623 +

624 +const NO_PENDING_EFFECTS = 0;

625 +const PENDING_MUTATION_PHASE = 1;

626 +const PENDING_LAYOUT_PHASE = 2;

627 +const PENDING_PASSIVE_PHASE = 3;

628 +let pendingEffectsStatus: 0 | 1 | 2 | 3 = 0;

629 +let pendingEffectsRoot: FiberRoot = (null: any);

630 +let pendingFinishedWork: Fiber = (null: any);

631 +let pendingEffectsLanes: Lanes = NoLanes;

632 +let pendingEffectsRemainingLanes: Lanes = NoLanes;

633 +let pendingEffectsRenderEndTime: number = -0; // Profiling-only

634 let pendingPassiveTransitions: Array<Transition> | null = null;

635 +let pendingRecoverableErrors: null | Array<CapturedValue<mixed>> = null;

636 +let pendingDidIncludeRenderPhaseUpdate: boolean = false;

637 +let pendingSuspendedCommitReason: SuspendedCommitReason = IMMEDIATE_COMMIT; // Profiling-only

638

639 // Use these to prevent an infinite loop of nested updates

640 const NESTED_UPDATE_LIMIT = 50;

@@ -644,12 +657,21 @@ export function getWorkInProgressRootRenderLanes(): Lanes {

657 return workInProgressRootRenderLanes;

658 }

659

660 +export function hasPendingCommitEffects(): boolean {

661 + return (

662 + pendingEffectsStatus !== NO_PENDING_EFFECTS &&

663 + pendingEffectsStatus !== PENDING_PASSIVE_PHASE

664 + );

665 +}

666 +

667 export function getRootWithPendingPassiveEffects(): FiberRoot | null {

648 - return rootWithPendingPassiveEffects;

668 + return pendingEffectsStatus === PENDING_PASSIVE_PHASE

669 + ? pendingEffectsRoot

670 + : null;

671 }

672

673 export function getPendingPassiveEffectsLanes(): Lanes {

652 - return pendingPassiveEffectsLanes;

674 + return pendingEffectsLanes;

675 }

676

677 export function isWorkLoopSuspendedOnData(): boolean {

@@ -1622,12 +1644,12 @@ export function flushSyncFromReconciler<R>(fn: (() => R) | void): R | void {

1644 // In legacy mode, we flush pending passive effects at the beginning of the

1645 // next event, not at the end of the previous one.

1646 if (

1625 - rootWithPendingPassiveEffects !== null &&

1647 + pendingEffectsStatus !== NO_PENDING_EFFECTS &&

1648 !disableLegacyMode &&

1627 - rootWithPendingPassiveEffects.tag === LegacyRoot &&

1649 + pendingEffectsRoot.tag === LegacyRoot &&

1650 (executionContext & (RenderContext | CommitContext)) === NoContext

1651 ) {

1630 - flushPassiveEffects();

1652 + flushPendingEffects();

1653 }

1654

1655 const prevExecutionContext = executionContext;

@@ -3120,11 +3142,6 @@ function unwindUnitOfWork(unitOfWork: Fiber, skipSiblings: boolean): void {

3142 workInProgress = null;

3143 }

3144

3123 -type SuspendedCommitReason = 0 | 1 | 2;

3124 -const IMMEDIATE_COMMIT = 0;

3125 -const SUSPENDED_COMMIT = 1;

3126 -const THROTTLED_COMMIT = 2;

3127 -

3145 function commitRoot(

3146 root: FiberRoot,

3147 finishedWork: null | Fiber,

@@ -3149,8 +3166,8 @@ function commitRoot(

3166 // no more pending effects.

3167 // TODO: Might be better if `flushPassiveEffects` did not automatically

3168 // flush synchronous work at the end, to avoid factoring hazards like this.

3152 - flushPassiveEffects();

3153 - } while (rootWithPendingPassiveEffects !== null);

3169 + flushPendingEffects();

3170 + } while (pendingEffectsStatus !== NO_PENDING_EFFECTS);

3171 flushRenderPhaseStrictModeWarningsInDEV();

3172

3173 if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {

@@ -3243,6 +3260,24 @@ function commitRoot(

3260 // times out.

3261 }

3262

3263 + // workInProgressX might be overwritten, so we want

3264 + // to store it in pendingPassiveX until they get processed

3265 + // We need to pass this through as an argument to commitRoot

3266 + // because workInProgressX might have changed between

3267 + // the previous render and commit if we throttle the commit

3268 + // with setTimeout

3269 + pendingFinishedWork = finishedWork;

3270 + pendingEffectsRoot = root;

3271 + pendingEffectsLanes = lanes;

3272 + pendingEffectsRemainingLanes = remainingLanes;

3273 + pendingPassiveTransitions = transitions;

3274 + pendingRecoverableErrors = recoverableErrors;

3275 + pendingDidIncludeRenderPhaseUpdate = didIncludeRenderPhaseUpdate;

3276 + if (enableProfilerTimer) {

3277 + pendingEffectsRenderEndTime = completedRenderEndTime;

3278 + pendingSuspendedCommitReason = suspendedCommitReason;

3279 + }

3280 +

3281 // If there are pending passive effects, schedule a callback to process them.

3282 // Do this as early as possible, so it is queued before anything else that

3283 // might get scheduled in the commit phase. (See #16714.)

@@ -3256,15 +3291,6 @@ function commitRoot(

3291 (finishedWork.subtreeFlags & PassiveMask) !== NoFlags ||

3292 (finishedWork.flags & PassiveMask) !== NoFlags

3293 ) {

3259 - pendingPassiveEffectsRemainingLanes = remainingLanes;

3260 - pendingPassiveEffectsRenderEndTime = completedRenderEndTime;

3261 - // workInProgressTransitions might be overwritten, so we want

3262 - // to store it in pendingPassiveTransitions until they get processed

3263 - // We need to pass this through as an argument to commitRoot

3264 - // because workInProgressTransitions might have changed between

3265 - // the previous render and commit if we throttle the commit

3266 - // with setTimeout

3267 - pendingPassiveTransitions = transitions;

3294 if (enableYieldingBeforePassive) {

3295 // We don't schedule a separate task for flushing passive effects.

3296 // Instead, we just rely on ensureRootIsScheduled below to schedule

@@ -3341,23 +3367,20 @@ function commitRoot(

3367 ReactSharedInternals.T = prevTransition;

3368 }

3369 }

3344 - flushMutationEffects(root, finishedWork, lanes);

3345 - flushLayoutEffects(

3346 - root,

3347 - finishedWork,

3348 - lanes,

3349 - recoverableErrors,

3350 - didIncludeRenderPhaseUpdate,

3351 - suspendedCommitReason,

3352 - completedRenderEndTime,

3353 - );

3370 + pendingEffectsStatus = PENDING_MUTATION_PHASE;

3371 + flushMutationEffects();

3372 + flushLayoutEffects();

3373 }

3374

3356 -function flushMutationEffects(

3357 - root: FiberRoot,

3358 - finishedWork: Fiber,

3359 - lanes: Lanes,

3360 -): void {

3375 +function flushMutationEffects(): void {

3376 + if (pendingEffectsStatus !== PENDING_MUTATION_PHASE) {

3377 + return;

3378 + }

3379 + pendingEffectsStatus = NO_PENDING_EFFECTS;

3380 +

3381 + const root = pendingEffectsRoot;

3382 + const finishedWork = pendingFinishedWork;

3383 + const lanes = pendingEffectsLanes;

3384 const subtreeMutationHasEffects =

3385 (finishedWork.subtreeFlags & MutationMask) !== NoFlags;

3386 const rootMutationHasEffect = (finishedWork.flags & MutationMask) !== NoFlags;

@@ -3392,17 +3415,23 @@ function flushMutationEffects(

3415 // componentWillUnmount, but before the layout phase, so that the finished

3416 // work is current during componentDidMount/Update.

3417 root.current = finishedWork;

3418 + pendingEffectsStatus = PENDING_LAYOUT_PHASE;

3419 }

3420

3397 -function flushLayoutEffects(

3398 - root: FiberRoot,

3399 - finishedWork: Fiber,

3400 - lanes: Lanes,

3401 - recoverableErrors: null | Array<CapturedValue<mixed>>,

3402 - didIncludeRenderPhaseUpdate: boolean,

3403 - suspendedCommitReason: SuspendedCommitReason, // Profiling-only

3404 - completedRenderEndTime: number, // Profiling-only

3405 -): void {

3421 +function flushLayoutEffects(): void {

3422 + if (pendingEffectsStatus !== PENDING_LAYOUT_PHASE) {

3423 + return;

3424 + }

3425 + pendingEffectsStatus = NO_PENDING_EFFECTS;

3426 +

3427 + const root = pendingEffectsRoot;

3428 + const finishedWork = pendingFinishedWork;

3429 + const lanes = pendingEffectsLanes;

3430 + const completedRenderEndTime = pendingEffectsRenderEndTime;

3431 + const recoverableErrors = pendingRecoverableErrors;

3432 + const didIncludeRenderPhaseUpdate = pendingDidIncludeRenderPhaseUpdate;

3433 + const suspendedCommitReason = pendingSuspendedCommitReason;

3434 +

3435 const subtreeHasLayoutEffects =

3436 (finishedWork.subtreeFlags & LayoutMask) !== NoFlags;

3437 const rootHasLayoutEffect = (finishedWork.flags & LayoutMask) !== NoFlags;

@@ -3456,11 +3485,10 @@ function flushLayoutEffects(

3485 (finishedWork.flags & PassiveMask) !== NoFlags;

3486

3487 if (rootDidHavePassiveEffects) {

3459 - // This commit has passive effects. Stash a reference to them. But don't

3460 - // schedule a callback until after flushing layout work.

3461 - rootWithPendingPassiveEffects = root;

3462 - pendingPassiveEffectsLanes = lanes;

3488 + pendingEffectsStatus = PENDING_PASSIVE_PHASE;

3489 } else {

3490 + pendingEffectsStatus = NO_PENDING_EFFECTS;

3491 + pendingEffectsRoot = (null: any); // Clear for GC purposes.

3492 // There were no passive effects, so we can immediately release the cache

3493 // pool for this render.

3494 releaseRootPooledCache(root, root.pendingLanes);

@@ -3546,10 +3574,10 @@ function flushLayoutEffects(

3574 // currently schedule the callback in multiple places, will wait until those

3575 // are consolidated.

3576 if (

3549 - includesSyncLane(pendingPassiveEffectsLanes) &&

3577 + includesSyncLane(pendingEffectsLanes) &&

3578 (disableLegacyMode || root.tag !== LegacyRoot)

3579 ) {

3552 - flushPassiveEffects();

3580 + flushPendingEffects();

3581 }

3582

3583 // Always call this before exiting `commitRoot`, to ensure that any

@@ -3666,61 +3694,63 @@ function releaseRootPooledCache(root: FiberRoot, remainingLanes: Lanes) {

3694 }

3695 }

3696

3669 -export function flushPassiveEffects(wasDelayedCommit?: boolean): boolean {

3697 +export function flushPendingEffects(wasDelayedCommit?: boolean): boolean {

3698 // Returns whether passive effects were flushed.

3671 - // TODO: Combine this check with the one in flushPassiveEFfectsImpl. We should

3672 - // probably just combine the two functions. I believe they were only separate

3699 + flushMutationEffects();

3700 + flushLayoutEffects();

3701 + return flushPassiveEffects(wasDelayedCommit);

3702 +}

3703 +

3704 +function flushPassiveEffects(wasDelayedCommit?: boolean): boolean {

3705 + if (pendingEffectsStatus !== PENDING_PASSIVE_PHASE) {

3706 + return false;

3707 + }

3708 + // TODO: Merge flushPassiveEffectsImpl into this function. I believe they were only separate

3709 // in the first place because we used to wrap it with

3710 // `Scheduler.runWithPriority`, which accepts a function. But now we track the

3711 // priority within React itself, so we can mutate the variable directly.

3676 - if (rootWithPendingPassiveEffects !== null) {

3677 - // Cache the root since rootWithPendingPassiveEffects is cleared in

3678 - // flushPassiveEffectsImpl

3679 - const root = rootWithPendingPassiveEffects;

3680 - // Cache and clear the remaining lanes flag; it must be reset since this

3681 - // method can be called from various places, not always from commitRoot

3682 - // where the remaining lanes are known

3683 - const remainingLanes = pendingPassiveEffectsRemainingLanes;

3684 - pendingPassiveEffectsRemainingLanes = NoLanes;

3685 -

3686 - const renderPriority = lanesToEventPriority(pendingPassiveEffectsLanes);

3687 - const priority = lowerEventPriority(DefaultEventPriority, renderPriority);

3688 - const prevTransition = ReactSharedInternals.T;

3689 - const previousPriority = getCurrentUpdatePriority();

3712 + // Cache the root since pendingEffectsRoot is cleared in

3713 + // flushPassiveEffectsImpl

3714 + const root = pendingEffectsRoot;

3715 + // Cache and clear the remaining lanes flag; it must be reset since this

3716 + // method can be called from various places, not always from commitRoot

3717 + // where the remaining lanes are known

3718 + const remainingLanes = pendingEffectsRemainingLanes;

3719 + pendingEffectsRemainingLanes = NoLanes;

3720 +

3721 + const renderPriority = lanesToEventPriority(pendingEffectsLanes);

3722 + const priority = lowerEventPriority(DefaultEventPriority, renderPriority);

3723 + const prevTransition = ReactSharedInternals.T;

3724 + const previousPriority = getCurrentUpdatePriority();

3725

3691 - try {

3692 - setCurrentUpdatePriority(priority);

3693 - ReactSharedInternals.T = null;

3694 - return flushPassiveEffectsImpl(wasDelayedCommit);

3695 - } finally {

3696 - setCurrentUpdatePriority(previousPriority);

3697 - ReactSharedInternals.T = prevTransition;

3726 + try {

3727 + setCurrentUpdatePriority(priority);

3728 + ReactSharedInternals.T = null;

3729 + return flushPassiveEffectsImpl(wasDelayedCommit);

3730 + } finally {

3731 + setCurrentUpdatePriority(previousPriority);

3732 + ReactSharedInternals.T = prevTransition;

3733

3699 - // Once passive effects have run for the tree - giving components a

3700 - // chance to retain cache instances they use - release the pooled

3701 - // cache at the root (if there is one)

3702 - releaseRootPooledCache(root, remainingLanes);

3703 - }

3734 + // Once passive effects have run for the tree - giving components a

3735 + // chance to retain cache instances they use - release the pooled

3736 + // cache at the root (if there is one)

3737 + releaseRootPooledCache(root, remainingLanes);

3738 }

3705 - return false;

3739 }

3740

3741 function flushPassiveEffectsImpl(wasDelayedCommit: void | boolean) {

3709 - if (rootWithPendingPassiveEffects === null) {

3710 - return false;

3711 - }

3712 -

3742 // Cache and clear the transitions flag

3743 const transitions = pendingPassiveTransitions;

3744 pendingPassiveTransitions = null;

3745

3717 - const root = rootWithPendingPassiveEffects;

3718 - const lanes = pendingPassiveEffectsLanes;

3719 - rootWithPendingPassiveEffects = null;

3720 - // TODO: This is sometimes out of sync with rootWithPendingPassiveEffects.

3746 + const root = pendingEffectsRoot;

3747 + const lanes = pendingEffectsLanes;

3748 + pendingEffectsStatus = NO_PENDING_EFFECTS;

3749 + pendingEffectsRoot = (null: any); // Clear for GC purposes.

3750 + // TODO: This is sometimes out of sync with pendingEffectsRoot.

3751 // Figure out why and fix it. It's not causing any known issues (probably

3752 // because it's only used for profiling), but it's a refactor hazard.

3723 - pendingPassiveEffectsLanes = NoLanes;

3753 + pendingEffectsLanes = NoLanes;

3754

3755 if (enableYieldingBeforePassive) {

3756 // We've finished our work for this render pass.

@@ -3767,7 +3797,7 @@ function flushPassiveEffectsImpl(wasDelayedCommit: void | boolean) {

3797 root.current,

3798 lanes,

3799 transitions,

3770 - pendingPassiveEffectsRenderEndTime,

3800 + pendingEffectsRenderEndTime,

3801 );

3802

3803 if (enableSchedulingProfiler) {