Skip to content

Commit facec3e

Browse files
sebmarkbagesebmarkbage
authored
[Fiber] Schedule passive effects using the regular ensureRootIsScheduled flow (facebook#31785)
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" />
1 parent f5077bc commit facec3e

18 files changed

+161
-48
lines changed

‎packages/react-dom/src/__tests__/ReactDOMSelect-test.js‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -911,9 +911,7 @@ describe('ReactDOMSelect', () => {
911911
const container = document.createElement('div');
912912
const root = ReactDOMClient.createRoot(container);
913913
async function changeView() {
914-
await act(() => {
915-
root.unmount();
916-
});
914+
root.unmount();
917915
}
918916

919917
const stub = (

‎packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3743,6 +3743,10 @@ describe('ReactDOMServerPartialHydration', () => {
37433743
await waitForPaint(['App']);
37443744
expect(visibleRef.current).toBe(visibleSpan);
37453745

3746+
if (gate(flags => flags.enableYieldingBeforePassive)) {
3747+
// Passive effects.
3748+
await waitForPaint([]);
3749+
}
37463750
// Subsequently, the hidden child is prerendered on the client
37473751
await waitForPaint(['HiddenChild']);
37483752
expect(container).toMatchInlineSnapshot(`

‎packages/react-reconciler/src/ReactFiberRootScheduler.js‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
enableProfilerNestedUpdatePhase,
2121
enableComponentPerformanceTrack,
2222
enableSiblingPrerendering,
23+
enableYieldingBeforePassive,
2324
} from 'shared/ReactFeatureFlags';
2425
import {
2526
NoLane,
@@ -41,6 +42,8 @@ import {
4142
getExecutionContext,
4243
getWorkInProgressRoot,
4344
getWorkInProgressRootRenderLanes,
45+
getRootWithPendingPassiveEffects,
46+
getPendingPassiveEffectsLanes,
4447
isWorkLoopSuspendedOnData,
4548
performWorkOnRoot,
4649
} from './ReactFiberWorkLoop';
@@ -324,12 +327,21 @@ function scheduleTaskForRootDuringMicrotask(
324327
markStarvedLanesAsExpired(root, currentTime);
325328

326329
// Determine the next lanes to work on, and their priority.
330+
const rootWithPendingPassiveEffects = getRootWithPendingPassiveEffects();
331+
const pendingPassiveEffectsLanes = getPendingPassiveEffectsLanes();
327332
const workInProgressRoot = getWorkInProgressRoot();
328333
const workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes();
329-
const nextLanes = getNextLanes(
330-
root,
331-
root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
332-
);
334+
const nextLanes =
335+
enableYieldingBeforePassive && root === rootWithPendingPassiveEffects
336+
? // This will schedule the callback at the priority of the lane but we used to
337+
// always schedule it at NormalPriority. Discrete will flush it sync anyway.
338+
// So the only difference is Idle and it doesn't seem necessarily right for that
339+
// to get upgraded beyond something important just because we're past commit.
340+
pendingPassiveEffectsLanes
341+
: getNextLanes(
342+
root,
343+
root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
344+
);
333345

334346
const existingCallbackNode = root.callbackNode;
335347
if (

‎packages/react-reconciler/src/ReactFiberWorkLoop.js‎

Lines changed: 54 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import {
4040
disableDefaultPropsExceptForClasses,
4141
enableSiblingPrerendering,
4242
enableComponentPerformanceTrack,
43+
enableYieldingBeforePassive,
4344
} from 'shared/ReactFeatureFlags';
4445
import ReactSharedInternals from 'shared/ReactSharedInternals';
4546
import is from 'shared/objectIs';
@@ -610,7 +611,6 @@ export function getRenderTargetTime(): number {
610611

611612
let legacyErrorBoundariesThatAlreadyFailed: Set<mixed> | null = null;
612613

613-
let rootDoesHavePassiveEffects: boolean = false;
614614
let rootWithPendingPassiveEffects: FiberRoot | null = null;
615615
let pendingPassiveEffectsLanes: Lanes = NoLanes;
616616
let pendingPassiveEffectsRemainingLanes: Lanes = NoLanes;
@@ -638,6 +638,14 @@ export function getWorkInProgressRootRenderLanes(): Lanes {
638638
return workInProgressRootRenderLanes;
639639
}
640640

641+
export function getRootWithPendingPassiveEffects(): FiberRoot | null {
642+
return rootWithPendingPassiveEffects;
643+
}
644+
645+
export function getPendingPassiveEffectsLanes(): Lanes {
646+
return pendingPassiveEffectsLanes;
647+
}
648+
641649
export function isWorkLoopSuspendedOnData(): boolean {
642650
return (
643651
workInProgressSuspendedReason === SuspendedOnData ||
@@ -3210,12 +3218,6 @@ function commitRootImpl(
32103218
);
32113219
}
32123220

3213-
// commitRoot never returns a continuation; it always finishes synchronously.
3214-
// So we can clear these now to allow a new callback to be scheduled.
3215-
root.callbackNode = null;
3216-
root.callbackPriority = NoLane;
3217-
root.cancelPendingCommit = null;
3218-
32193221
// Check which lanes no longer have any work scheduled on them, and mark
32203222
// those as finished.
32213223
let remainingLanes = mergeLanes(finishedWork.lanes, finishedWork.childLanes);
@@ -3253,6 +3255,7 @@ function commitRootImpl(
32533255
// might get scheduled in the commit phase. (See #16714.)
32543256
// TODO: Delete all other places that schedule the passive effect callback
32553257
// They're redundant.
3258+
let rootDoesHavePassiveEffects: boolean = false;
32563259
if (
32573260
// If this subtree rendered with profiling this commit, we need to visit it to log it.
32583261
(enableProfilerTimer &&
@@ -3261,17 +3264,25 @@ function commitRootImpl(
32613264
(finishedWork.subtreeFlags & PassiveMask) !== NoFlags ||
32623265
(finishedWork.flags & PassiveMask) !== NoFlags
32633266
) {
3264-
if (!rootDoesHavePassiveEffects) {
3265-
rootDoesHavePassiveEffects = true;
3266-
pendingPassiveEffectsRemainingLanes = remainingLanes;
3267-
pendingPassiveEffectsRenderEndTime = completedRenderEndTime;
3268-
// workInProgressTransitions might be overwritten, so we want
3269-
// to store it in pendingPassiveTransitions until they get processed
3270-
// We need to pass this through as an argument to commitRoot
3271-
// because workInProgressTransitions might have changed between
3272-
// the previous render and commit if we throttle the commit
3273-
// with setTimeout
3274-
pendingPassiveTransitions = transitions;
3267+
rootDoesHavePassiveEffects = true;
3268+
pendingPassiveEffectsRemainingLanes = remainingLanes;
3269+
pendingPassiveEffectsRenderEndTime = completedRenderEndTime;
3270+
// workInProgressTransitions might be overwritten, so we want
3271+
// to store it in pendingPassiveTransitions until they get processed
3272+
// We need to pass this through as an argument to commitRoot
3273+
// because workInProgressTransitions might have changed between
3274+
// the previous render and commit if we throttle the commit
3275+
// with setTimeout
3276+
pendingPassiveTransitions = transitions;
3277+
if (enableYieldingBeforePassive) {
3278+
// We don't schedule a separate task for flushing passive effects.
3279+
// Instead, we just rely on ensureRootIsScheduled below to schedule
3280+
// a callback for us to flush the passive effects.
3281+
} else {
3282+
// So we can clear these now to allow a new callback to be scheduled.
3283+
root.callbackNode = null;
3284+
root.callbackPriority = NoLane;
3285+
root.cancelPendingCommit = null;
32753286
scheduleCallback(NormalSchedulerPriority, () => {
32763287
if (enableProfilerTimer && enableComponentPerformanceTrack) {
32773288
// Track the currently executing event if there is one so we can ignore this
@@ -3285,6 +3296,12 @@ function commitRootImpl(
32853296
return null;
32863297
});
32873298
}
3299+
} else {
3300+
// If we don't have passive effects, we're not going to need to perform more work
3301+
// so we can clear the callback now.
3302+
root.callbackNode = null;
3303+
root.callbackPriority = NoLane;
3304+
root.cancelPendingCommit = null;
32883305
}
32893306

32903307
if (enableProfilerTimer) {
@@ -3441,10 +3458,6 @@ function commitRootImpl(
34413458
onCommitRootTestSelector();
34423459
}
34433460

3444-
// Always call this before exiting `commitRoot`, to ensure that any
3445-
// additional work on this root is scheduled.
3446-
ensureRootIsScheduled(root);
3447-
34483461
if (recoverableErrors !== null) {
34493462
// There were errors during this render, but recovered from them without
34503463
// needing to surface it to the UI. We log them here.
@@ -3480,6 +3493,10 @@ function commitRootImpl(
34803493
flushPassiveEffects();
34813494
}
34823495

3496+
// Always call this before exiting `commitRoot`, to ensure that any
3497+
// additional work on this root is scheduled.
3498+
ensureRootIsScheduled(root);
3499+
34833500
// Read this again, since a passive effect might have updated it
34843501
remainingLanes = root.pendingLanes;
34853502

@@ -3648,6 +3665,13 @@ function flushPassiveEffectsImpl(wasDelayedCommit: void | boolean) {
36483665
// because it's only used for profiling), but it's a refactor hazard.
36493666
pendingPassiveEffectsLanes = NoLanes;
36503667

3668+
if (enableYieldingBeforePassive) {
3669+
// We've finished our work for this render pass.
3670+
root.callbackNode = null;
3671+
root.callbackPriority = NoLane;
3672+
root.cancelPendingCommit = null;
3673+
}
3674+
36513675
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
36523676
throw new Error('Cannot flush passive effects while already rendering.');
36533677
}
@@ -3745,6 +3769,14 @@ function flushPassiveEffectsImpl(wasDelayedCommit: void | boolean) {
37453769
didScheduleUpdateDuringPassiveEffects = false;
37463770
}
37473771

3772+
if (enableYieldingBeforePassive) {
3773+
// Next, we reschedule any remaining work in a new task since it's a new
3774+
// sequence of work. We wait until the end to do this in case the passive
3775+
// effect schedules higher priority work than we had remaining. That way
3776+
// we don't schedule an early callback that gets cancelled anyway.
3777+
ensureRootIsScheduled(root);
3778+
}
3779+
37483780
// TODO: Move to commitPassiveMountEffects
37493781
onPostCommitRootDevTools(root);
37503782
if (enableProfilerTimer && enableProfilerCommitHooks) {

‎packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -753,6 +753,10 @@ describe('ReactDeferredValue', () => {
753753
revealContent();
754754
// Because the preview state was already prerendered, we can reveal it
755755
// without any addditional work.
756+
if (gate(flags => flags.enableYieldingBeforePassive)) {
757+
// Passive effects.
758+
await waitForPaint([]);
759+
}
756760
await waitForPaint([]);
757761
expect(root).toMatchRenderedOutput(<div>Preview [B]</div>);
758762
});

‎packages/react-reconciler/src/__tests__/ReactExpiration-test.js‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -755,10 +755,16 @@ describe('ReactExpiration', () => {
755755

756756
// The update finishes without yielding. But it does not flush the effect.
757757
await waitFor(['B1'], {
758-
additionalLogsAfterAttemptingToYield: ['C1'],
758+
additionalLogsAfterAttemptingToYield: gate(
759+
flags => flags.enableYieldingBeforePassive,
760+
)
761+
? ['C1', 'Effect: 1']
762+
: ['C1'],
759763
});
760764
});
761-
// The effect flushes after paint.
762-
assertLog(['Effect: 1']);
765+
if (!gate(flags => flags.enableYieldingBeforePassive)) {
766+
// The effect flushes after paint.
767+
assertLog(['Effect: 1']);
768+
}
763769
});
764770
});

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2695,13 +2695,23 @@ describe('ReactHooksWithNoopRenderer', () => {
26952695
React.startTransition(() => {
26962696
ReactNoop.render(<Counter count={1} />);
26972697
});
2698-
await waitForPaint([
2699-
'Create passive [current: 0]',
2700-
'Destroy insertion [current: 0]',
2701-
'Create insertion [current: 0]',
2702-
'Destroy layout [current: 1]',
2703-
'Create layout [current: 1]',
2704-
]);
2698+
if (gate(flags => flags.enableYieldingBeforePassive)) {
2699+
await waitForPaint(['Create passive [current: 0]']);
2700+
await waitForPaint([
2701+
'Destroy insertion [current: 0]',
2702+
'Create insertion [current: 0]',
2703+
'Destroy layout [current: 1]',
2704+
'Create layout [current: 1]',
2705+
]);
2706+
} else {
2707+
await waitForPaint([
2708+
'Create passive [current: 0]',
2709+
'Destroy insertion [current: 0]',
2710+
'Create insertion [current: 0]',
2711+
'Destroy layout [current: 1]',
2712+
'Create layout [current: 1]',
2713+
]);
2714+
}
27052715
expect(committedText).toEqual('1');
27062716
});
27072717
assertLog([

‎packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,10 @@ describe('ReactSiblingPrerendering', () => {
200200
await waitForPaint(['A']);
201201
expect(root).toMatchRenderedOutput('A');
202202

203+
if (gate(flags => flags.enableYieldingBeforePassive)) {
204+
// Passive effects.
205+
await waitForPaint([]);
206+
}
203207
// The second render is a prerender of the hidden content.
204208
await waitForPaint([
205209
'Suspend! [B]',
@@ -237,6 +241,10 @@ describe('ReactSiblingPrerendering', () => {
237241
// Immediately after the fallback commits, retry the boundary again. This
238242
// time we include B, since we're not blocking the fallback from showing.
239243
if (gate('enableSiblingPrerendering')) {
244+
if (gate(flags => flags.enableYieldingBeforePassive)) {
245+
// Passive effects.
246+
await waitForPaint([]);
247+
}
240248
await waitForPaint(['Suspend! [A]', 'Suspend! [B]']);
241249
}
242250
});
@@ -452,6 +460,10 @@ describe('ReactSiblingPrerendering', () => {
452460
</>,
453461
);
454462

463+
if (gate(flags => flags.enableYieldingBeforePassive)) {
464+
// Passive effects.
465+
await waitForPaint([]);
466+
}
455467
// Immediately after the fallback commits, retry the boundary again.
456468
// Because the promise for A resolved, this is a normal render, _not_
457469
// a prerender. So when we proceed to B, and B suspends, we unwind again
@@ -471,6 +483,10 @@ describe('ReactSiblingPrerendering', () => {
471483
</>,
472484
);
473485

486+
if (gate(flags => flags.enableYieldingBeforePassive)) {
487+
// Passive effects.
488+
await waitForPaint([]);
489+
}
474490
// Now we can proceed to prerendering C.
475491
if (gate('enableSiblingPrerendering')) {
476492
await waitForPaint(['Suspend! [B]', 'Suspend! [C]']);

‎packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1901,6 +1901,10 @@ describe('ReactSuspenseWithNoopRenderer', () => {
19011901
// be throttled because the fallback would have appeared too recently.
19021902
Scheduler.unstable_advanceTime(10000);
19031903
jest.advanceTimersByTime(10000);
1904+
if (gate(flags => flags.enableYieldingBeforePassive)) {
1905+
// Passive effects.
1906+
await waitForPaint([]);
1907+
}
19041908
await waitForPaint(['A']);
19051909
expect(ReactNoop).toMatchRenderedOutput(
19061910
<>

‎packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -81,14 +81,26 @@ describe('ReactUpdatePriority', () => {
8181
// Schedule another update at default priority
8282
setDefaultState(2);
8383

84-
// The default update flushes first, because
85-
await waitForPaint([
86-
// Idle update is scheduled
87-
'Idle update',
88-
89-
// The default update flushes first, without including the idle update
90-
'Idle: 1, Default: 2',
91-
]);
84+
if (gate(flags => flags.enableYieldingBeforePassive)) {
85+
// The default update flushes first, because
86+
await waitForPaint([
87+
// Idle update is scheduled
88+
'Idle update',
89+
]);
90+
await waitForPaint([
91+
// The default update flushes first, without including the idle update
92+
'Idle: 1, Default: 2',
93+
]);
94+
} else {
95+
// The default update flushes first, because
96+
await waitForPaint([
97+
// Idle update is scheduled
98+
'Idle update',
99+
100+
// The default update flushes first, without including the idle update
101+
'Idle: 1, Default: 2',
102+
]);
103+
}
92104
});
93105
// Now the idle update has flushed
94106
assertLog(['Idle: 2, Default: 2']);

0 commit comments

Comments
 (0)