@samitouri / QOS-React / commits / 3c7667a694

Unify perform{Sync,Concurrent}WorkOnRoot implementation (#31029)

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.)

Andrew Clark committed Sep 25, 2024 at 14:33 UTC 3c7667a694face1827356a7c90ee6f86a9c0baa0

4 files changed +115 -176

packages/react-reconciler/src/ReactFiberRootScheduler.js

+85 -10

@@ -15,6 +15,9 @@ import type {BatchConfigTransition} from './ReactFiberTracingMarkerComponent';

15 import {

16 disableLegacyMode,

17 enableDeferRootSchedulingToMicrotask,

18 + disableSchedulerTimeoutInWorkLoop,

19 + enableProfilerTimer,

20 + enableProfilerNestedUpdatePhase,

21 } from 'shared/ReactFeatureFlags';

22 import {

23 NoLane,

@@ -31,12 +34,12 @@ import {

34 CommitContext,

35 NoContext,

36 RenderContext,

37 + flushPassiveEffects,

38 getExecutionContext,

39 getWorkInProgressRoot,

40 getWorkInProgressRootRenderLanes,

41 isWorkLoopSuspendedOnData,

38 - performConcurrentWorkOnRoot,

39 - performSyncWorkOnRoot,

42 + performWorkOnRoot,

43 } from './ReactFiberWorkLoop';

44 import {LegacyRoot} from './ReactRootTags';

45 import {

@@ -62,6 +65,10 @@ import {

65 } from './ReactFiberConfig';

67 import ReactSharedInternals from 'shared/ReactSharedInternals';

68 +import {

69 + resetNestedUpdateFlag,

70 + syncNestedUpdateFlag,

71 +} from './ReactProfilerTimer';

73 // A linked list of all the roots with pending work. In an idiomatic app,

74 // there's only a single root, but we do support multi root apps, hence this

@@ -387,7 +394,7 @@ function scheduleTaskForRootDuringMicrotask(

394

395 const newCallbackNode = scheduleCallback(

396 schedulerPriorityLevel,

390 - performConcurrentWorkOnRoot.bind(null, root),

397 + performWorkOnRootViaSchedulerTask.bind(null, root),

398 );

399

400 root.callbackPriority = newCallbackPriority;

@@ -396,15 +403,67 @@ function scheduleTaskForRootDuringMicrotask(

403 }

404 }

405

399 -export type RenderTaskFn = (didTimeout: boolean) => RenderTaskFn | null;

406 +type RenderTaskFn = (didTimeout: boolean) => RenderTaskFn | null;

407

401 -export function getContinuationForRoot(

408 +function performWorkOnRootViaSchedulerTask(

409 root: FiberRoot,

403 - originalCallbackNode: mixed,

410 + didTimeout: boolean,

411 ): RenderTaskFn | null {

405 - // This is called at the end of `performConcurrentWorkOnRoot` to determine

406 - // if we need to schedule a continuation task.

407 - //

412 + // This is the entry point for concurrent tasks scheduled via Scheduler (and

413 + // postTask, in the future).

414 +

415 + if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {

416 + resetNestedUpdateFlag();

417 + }

418 +

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

420 + // in case they schedule additional work.

421 + const originalCallbackNode = root.callbackNode;

422 + const didFlushPassiveEffects = flushPassiveEffects();

423 + if (didFlushPassiveEffects) {

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

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

426 + if (root.callbackNode !== originalCallbackNode) {

427 + // The current task was canceled. Exit. We don't need to call

428 + // `ensureRootIsScheduled` because the check above implies either that

429 + // there's a new task, or that there's no remaining work on this root.

430 + return null;

431 + } else {

432 + // Current task was not canceled. Continue.

433 + }

434 + }

435 +

436 + // Determine the next lanes to work on, using the fields stored on the root.

437 + // TODO: We already called getNextLanes when we scheduled the callback; we

438 + // should be able to avoid calling it again by stashing the result on the

439 + // root object. However, because we always schedule the callback during

440 + // a microtask (scheduleTaskForRootDuringMicrotask), it's possible that

441 + // an update was scheduled earlier during this same browser task (and

442 + // therefore before the microtasks have run). That's because Scheduler batches

443 + // together multiple callbacks into a single browser macrotask, without

444 + // yielding to microtasks in between. We should probably change this to align

445 + // with the postTask behavior (and literally use postTask when

446 + // it's available).

447 + const workInProgressRoot = getWorkInProgressRoot();

448 + const workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes();

449 + const lanes = getNextLanes(

450 + root,

451 + root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,

452 + );

453 + if (lanes === NoLanes) {

454 + // No more work on this root.

455 + return null;

456 + }

457 +

458 + // Enter the work loop.

459 + // TODO: We only check `didTimeout` defensively, to account for a Scheduler

460 + // bug we're still investigating. Once the bug in Scheduler is fixed,

461 + // we can remove this, since we track expiration ourselves.

462 + const forceSync = !disableSchedulerTimeoutInWorkLoop && didTimeout;

463 + performWorkOnRoot(root, lanes, forceSync);

464 +

465 + // The work loop yielded, but there may or may not be work left at the current

466 + // priority. Need to determine whether we need to schedule a continuation.

467 // Usually `scheduleTaskForRootDuringMicrotask` only runs inside a microtask;

468 // however, since most of the logic for determining if we need a continuation

469 // versus a new task is the same, we cheat a bit and call it here. This is

@@ -414,11 +473,27 @@ export function getContinuationForRoot(

473 if (root.callbackNode === originalCallbackNode) {

474 // The task node scheduled for this root is the same one that's

475 // currently executed. Need to return a continuation.

417 - return performConcurrentWorkOnRoot.bind(null, root);

476 + return performWorkOnRootViaSchedulerTask.bind(null, root);

477 }

478 return null;

479 }

480

481 +function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes) {

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

483 + // through Scheduler.

484 + const didFlushPassiveEffects = flushPassiveEffects();

485 + if (didFlushPassiveEffects) {

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

487 + // scheduler, so we can recompute the priority.

488 + return null;

489 + }

490 + if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {

491 + syncNestedUpdateFlag();

492 + }

493 + const forceSync = true;

494 + performWorkOnRoot(root, lanes, forceSync);

495 +}

496 +

497 const fakeActCallbackNode = {};

498

499 function scheduleCallback(

packages/react-reconciler/src/ReactFiberWorkLoop.js

+10 -148

@@ -22,7 +22,6 @@ import type {

22 TransitionAbort,

23 } from './ReactFiberTracingMarkerComponent';

24 import type {OffscreenInstance} from './ReactFiberActivityComponent';

25 -import type {RenderTaskFn} from './ReactFiberRootScheduler';

25 import type {Resource} from './ReactFiberConfig';

27 import {

@@ -32,7 +31,6 @@ import {

31 enableProfilerNestedUpdatePhase,

32 enableDebugTracing,

33 enableSchedulingProfiler,

35 - disableSchedulerTimeoutInWorkLoop,

34 enableUpdaterTracking,

35 enableCache,

36 enableTransitionTracing,

@@ -250,11 +248,9 @@ import {

248 recordRenderTime,

249 recordCommitTime,

250 recordCommitEndTime,

253 - resetNestedUpdateFlag,

251 startProfilerTimer,

252 stopProfilerTimerIfRunningAndRecordDuration,

253 stopProfilerTimerIfRunningAndRecordIncompleteDuration,

257 - syncNestedUpdateFlag,

254 } from './ReactProfilerTimer';

255 import {setCurrentTrackFromLanes} from './ReactFiberPerformanceTrack';

256

@@ -308,7 +304,6 @@ import {

304 ensureRootIsScheduled,

305 flushSyncWorkOnAllRoots,

306 flushSyncWorkOnLegacyRootsOnly,

311 - getContinuationForRoot,

307 requestTransitionLane,

308 } from './ReactFiberRootScheduler';

309 import {getMaskedContext, getUnmaskedContext} from './ReactFiberContext';

@@ -890,59 +885,22 @@ export function isUnsafeClassRenderPhaseUpdate(fiber: Fiber): boolean {

885 return (executionContext & RenderContext) !== NoContext;

886 }

887

893 -// This is the entry point for every concurrent task, i.e. anything that

894 -// goes through Scheduler.

895 -export function performConcurrentWorkOnRoot(

888 +export function performWorkOnRoot(

889 root: FiberRoot,

897 - didTimeout: boolean,

898 -): RenderTaskFn | null {

899 - if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {

900 - resetNestedUpdateFlag();

901 - }

902 -

890 + lanes: Lanes,

891 + forceSync: boolean,

892 +): void {

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

894 throw new Error('Should not already be working.');

895 }

896

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

908 - // in case they schedule additional work.

909 - const originalCallbackNode = root.callbackNode;

910 - const didFlushPassiveEffects = flushPassiveEffects();

911 - if (didFlushPassiveEffects) {

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

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

914 - if (root.callbackNode !== originalCallbackNode) {

915 - // The current task was canceled. Exit. We don't need to call

916 - // `ensureRootIsScheduled` because the check above implies either that

917 - // there's a new task, or that there's no remaining work on this root.

918 - return null;

919 - } else {

920 - // Current task was not canceled. Continue.

921 - }

922 - }

923 -

924 - // Determine the next lanes to work on, using the fields stored

925 - // on the root.

926 - // TODO: This was already computed in the caller. Pass it as an argument.

927 - let lanes = getNextLanes(

928 - root,

929 - root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,

930 - );

931 - if (lanes === NoLanes) {

932 - // Defensive coding. This is never expected to happen.

933 - return null;

934 - }

935 -

897 // We disable time-slicing in some cases: if the work has been CPU-bound

898 // for too long ("expired" work, to prevent starvation), or we're in

899 // sync-updates-by-default mode.

939 - // TODO: We only check `didTimeout` defensively, to account for a Scheduler

940 - // bug we're still investigating. Once the bug in Scheduler is fixed,

941 - // we can remove this, since we track expiration ourselves.

900 const shouldTimeSlice =

901 + !forceSync &&

902 !includesBlockingLane(lanes) &&

944 - !includesExpiredLane(root, lanes) &&

945 - (disableSchedulerTimeoutInWorkLoop || !didTimeout);

903 + !includesExpiredLane(root, lanes);

904 let exitStatus = shouldTimeSlice

905 ? renderRootConcurrent(root, lanes)

906 : renderRootSync(root, lanes);

@@ -984,7 +942,10 @@ export function performConcurrentWorkOnRoot(

942 }

943

944 // Check if something threw

987 - if (exitStatus === RootErrored) {

945 + if (

946 + (disableLegacyMode || root.tag !== LegacyRoot) &&

947 + exitStatus === RootErrored

948 + ) {

949 const lanesThatJustErrored = lanes;

950 const errorRetryLanes = getLanesToRetrySynchronouslyOnError(

951 root,

@@ -1033,7 +994,6 @@ export function performConcurrentWorkOnRoot(

994 }

995

996 ensureRootIsScheduled(root);

1036 - return getContinuationForRoot(root, originalCallbackNode);

997 }

998

999 function recoverFromConcurrentError(

@@ -1464,104 +1424,6 @@ function markRootSuspended(

1424 );

1425 }

1426

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

1468 -// through Scheduler

1469 -export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null {

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

1471 - throw new Error('Should not already be working.');

1472 - }

1473 -

1474 - const didFlushPassiveEffects = flushPassiveEffects();

1475 - if (didFlushPassiveEffects) {

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

1477 - // scheduler, so we can recompute the priority.

1478 - // TODO: We don't actually need this `ensureRootIsScheduled` call because

1479 - // this path is only reachable if the root is already part of the schedule.

1480 - // I'm including it only for consistency with the other exit points from

1481 - // this function. Can address in a subsequent refactor.

1482 - ensureRootIsScheduled(root);

1483 - return null;

1484 - }

1485 -

1486 - if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {

1487 - syncNestedUpdateFlag();

1488 - }

1489 -

1490 - let exitStatus = renderRootSync(root, lanes);

1491 - if (

1492 - (disableLegacyMode || root.tag !== LegacyRoot) &&

1493 - exitStatus === RootErrored

1494 - ) {

1495 - // If something threw an error, try rendering one more time. We'll render

1496 - // synchronously to block concurrent data mutations, and we'll includes

1497 - // all pending updates are included. If it still fails after the second

1498 - // attempt, we'll give up and commit the resulting tree.

1499 - const originallyAttemptedLanes = lanes;

1500 - const errorRetryLanes = getLanesToRetrySynchronouslyOnError(

1501 - root,

1502 - originallyAttemptedLanes,

1503 - );

1504 - if (errorRetryLanes !== NoLanes) {

1505 - lanes = errorRetryLanes;

1506 - exitStatus = recoverFromConcurrentError(

1507 - root,

1508 - originallyAttemptedLanes,

1509 - errorRetryLanes,

1510 - );

1511 - }

1512 - }

1513 -

1514 - if (exitStatus === RootFatalErrored) {

1515 - prepareFreshStack(root, NoLanes);

1516 - markRootSuspended(root, lanes, NoLane, false);

1517 - ensureRootIsScheduled(root);

1518 - return null;

1519 - }

1520 -

1521 - if (exitStatus === RootDidNotComplete) {

1522 - // The render unwound without completing the tree. This happens in special

1523 - // cases where need to exit the current render without producing a

1524 - // consistent tree or committing.

1525 - markRootSuspended(

1526 - root,

1527 - lanes,

1528 - workInProgressDeferredLane,

1529 - workInProgressRootDidSkipSuspendedSiblings,

1530 - );

1531 - ensureRootIsScheduled(root);

1532 - return null;

1533 - }

1534 -

1535 - let renderEndTime = 0;

1536 - if (enableProfilerTimer && enableComponentPerformanceTrack) {

1537 - renderEndTime = now();

1538 - }

1539 -

1540 - // We now have a consistent tree. Because this is a sync render, we

1541 - // will commit it even if something suspended.

1542 - const finishedWork: Fiber = (root.current.alternate: any);

1543 - root.finishedWork = finishedWork;

1544 - root.finishedLanes = lanes;

1545 - commitRoot(

1546 - root,

1547 - workInProgressRootRecoverableErrors,

1548 - workInProgressTransitions,

1549 - workInProgressRootDidIncludeRecursiveRenderUpdate,

1550 - workInProgressDeferredLane,

1551 - workInProgressRootInterleavedUpdatedLanes,

1552 - workInProgressSuspendedRetryLanes,

1553 - IMMEDIATE_COMMIT,

1554 - renderStartTime,

1555 - renderEndTime,

1556 - );

1557 -

1558 - // Before exiting, make sure there's a callback scheduled for the next

1559 - // pending level.

1560 - ensureRootIsScheduled(root);

1561 -

1562 - return null;

1563 -}

1564 -

1427 export function flushRoot(root: FiberRoot, lanes: Lanes) {

1428 if (lanes !== NoLanes) {

1429 upgradePendingLanesToSync(root, lanes);

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

+14 -4

@@ -349,6 +349,7 @@ describe('ReactSiblingPrerendering', () => {

349 <div>

350 <Suspense fallback={<Text text="Loading inner..." />}>

351 <AsyncText text="B" />

352 + <AsyncText text="C" />

353 </Suspense>

354 </div>

355 </Suspense>

@@ -370,10 +371,17 @@ describe('ReactSiblingPrerendering', () => {

371 // is throttled because it's been less than a Just Noticeable Difference

372 // since the outer fallback was committed.

373 //

373 - // In the meantime, we could choose to start prerendering B, but instead

374 + // In the meantime, we could choose to start prerendering C, but instead

375 // we wait for a JND to elapse and the commit to finish — it's not

376 // worth discarding the work we've already done.

376 - await waitForAll(['A', 'Suspend! [B]', 'Loading inner...']);

377 + await waitForAll([

378 + 'A',

379 + 'Suspend! [B]',

380 +

381 + // C is skipped because we're no longer in prerendering mode; there's

382 + // a new fallback we can show.

383 + 'Loading inner...',

384 + ]);

385 expect(root).toMatchRenderedOutput(<div>Loading outer...</div>);

386

387 // Fire the timer to commit the outer fallback.

@@ -385,8 +393,10 @@ describe('ReactSiblingPrerendering', () => {

393 </div>,

394 );

395 });

388 - // Once the outer fallback is committed, we can start prerendering B.

389 - assertLog(gate('enableSiblingPrerendering') ? ['Suspend! [B]'] : []);

396 + // Once the inner fallback is committed, we can start prerendering C.

397 + assertLog(

398 + gate('enableSiblingPrerendering') ? ['Suspend! [B]', 'Suspend! [C]'] : [],

399 + );

400 });

401

402 it(

packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js

+6 -14

@@ -239,11 +239,7 @@ describe('ReactSuspenseyCommitPhase', () => {

239 expect(root).toMatchRenderedOutput(<suspensey-thing src="B" />);

240 });

241

242 - // @TODO This isn't actually ideal behavior. We would really want the commit to suspend

243 - // even if it is forced to be sync because we don't want to FOUC but refactoring the sync

244 - // pathway is too risky to land right now so we just accept that we can still FOUC in this

245 - // very specific case.

246 - it('does not suspend commit during urgent initial mount at the root when sync rendering', async () => {

242 + it('does suspend commit during urgent initial mount at the root when sync rendering', async () => {

243 const root = ReactNoop.createRoot();

244 await act(async () => {

245 ReactNoop.flushSync(() => {

@@ -252,19 +248,15 @@ describe('ReactSuspenseyCommitPhase', () => {

248 });

249 assertLog(['Image requested [A]']);

250 expect(getSuspenseyThingStatus('A')).toBe('pending');

255 - // We would expect this to be null if we did in fact suspend this commit

256 - expect(root).toMatchRenderedOutput(<suspensey-thing src="A" />);

251 + // Suspend the initial mount

252 + expect(root).toMatchRenderedOutput(null);

253

254 resolveSuspenseyThing('A');

255 expect(getSuspenseyThingStatus('A')).toBe('fulfilled');

256 expect(root).toMatchRenderedOutput(<suspensey-thing src="A" />);

257 });

258

263 - // @TODO This isn't actually ideal behavior. We would really want the commit to suspend

264 - // even if it is forced to be sync because we don't want to FOUC but refactoring the sync

265 - // pathway is too risky to land right now so we just accept that we can still FOUC in this

266 - // very specific case.

267 - it('does not suspend commit during urgent update at the root when sync rendering', async () => {

259 + it('does suspend commit during urgent update at the root when sync rendering', async () => {

260 const root = ReactNoop.createRoot();

261 await act(() => resolveSuspenseyThing('A'));

262 expect(getSuspenseyThingStatus('A')).toBe('fulfilled');

@@ -283,8 +275,8 @@ describe('ReactSuspenseyCommitPhase', () => {

275 });

276 assertLog(['Image requested [B]']);

277 expect(getSuspenseyThingStatus('B')).toBe('pending');

286 - // We would expect this to be hidden if we did in fact suspend this commit

287 - expect(root).toMatchRenderedOutput(<suspensey-thing src="B" />);

278 + // Suspend and remain on previous screen

279 + expect(root).toMatchRenderedOutput(<suspensey-thing src="A" />);

280

281 resolveSuspenseyThing('B');

282 expect(getSuspenseyThingStatus('B')).toBe('fulfilled');