From 4c71025d8d1bd46344ad793e7ed3049d24f7395a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 24 Sep 2024 10:03:15 -0700 Subject: [PATCH] Make prerendering always non-blocking When a synchronous update suspends, and we prerender the siblings, the prerendering should be non-blocking so that we can immediately restart once the data arrives. This happens automatically when there's a Suspense boundary, because we immediately commit the boundary and then proceed to a Retry render, which are always concurrent. When there's not a Suspense boundary, there is no Retry, so we need to take care to switch from the synchronous work loop to the concurrent one, to enable time slicing. --- .../src/__tests__/ReactDOMFiberAsync-test.js | 2 +- .../react-reconciler/src/ReactFiberLane.js | 6 +- .../src/ReactFiberRootScheduler.js | 20 +- .../src/ReactFiberWorkLoop.js | 293 +++++++++++------- .../src/__tests__/ReactDeferredValue-test.js | 12 + .../ReactSiblingPrerendering-test.js | 71 +++++ 6 files changed, 278 insertions(+), 126 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js index ee843996bef1c..027099d54707c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js @@ -744,7 +744,7 @@ describe('ReactDOMFiberAsync', () => { // Because it suspended, it remains on the current path expect(div.textContent).toBe('/path/a'); }); - assertLog([]); + assertLog(gate('enableSiblingPrerendering') ? ['Suspend! [/path/b]'] : []); await act(async () => { resolvePromise(); diff --git a/packages/react-reconciler/src/ReactFiberLane.js b/packages/react-reconciler/src/ReactFiberLane.js index 7d6bfd16e8aa0..b98019046e3c2 100644 --- a/packages/react-reconciler/src/ReactFiberLane.js +++ b/packages/react-reconciler/src/ReactFiberLane.js @@ -765,12 +765,14 @@ export function markRootSuspended( root: FiberRoot, suspendedLanes: Lanes, spawnedLane: Lane, - didSkipSuspendedSiblings: boolean, + didAttemptEntireTree: boolean, ) { + // TODO: Split this into separate functions for marking the root at the end of + // a render attempt versus suspending while the root is still in progress. root.suspendedLanes |= suspendedLanes; root.pingedLanes &= ~suspendedLanes; - if (enableSiblingPrerendering && !didSkipSuspendedSiblings) { + if (enableSiblingPrerendering && didAttemptEntireTree) { // Mark these lanes as warm so we know there's nothing else to work on. root.warmLanes |= suspendedLanes; } else { diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js index 9f267e8345e39..39f6f466ed983 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -18,6 +18,7 @@ import { disableSchedulerTimeoutInWorkLoop, enableProfilerTimer, enableProfilerNestedUpdatePhase, + enableSiblingPrerendering, } from 'shared/ReactFeatureFlags'; import { NoLane, @@ -29,6 +30,7 @@ import { markStarvedLanesAsExpired, claimNextTransitionLane, getNextLanesToFlushSync, + checkIfRootIsPrerendering, } from './ReactFiberLane'; import { CommitContext, @@ -206,7 +208,10 @@ function flushSyncWorkAcrossRoots_impl( ? workInProgressRootRenderLanes : NoLanes, ); - if (includesSyncLane(nextLanes)) { + if ( + includesSyncLane(nextLanes) && + !checkIfRootIsPrerendering(root, nextLanes) + ) { // This root has pending sync work. Flush it now. didPerformSomeWork = true; performSyncWorkOnRoot(root, nextLanes); @@ -341,7 +346,13 @@ function scheduleTaskForRootDuringMicrotask( } // Schedule a new callback in the host environment. - if (includesSyncLane(nextLanes)) { + if ( + includesSyncLane(nextLanes) && + // If we're prerendering, then we should use the concurrent work loop + // even if the lanes are synchronous, so that prerendering never blocks + // the main thread. + !(enableSiblingPrerendering && checkIfRootIsPrerendering(root, nextLanes)) + ) { // Synchronous work is always flushed at the end of the microtask, so we // don't need to schedule an additional task. if (existingCallbackNode !== null) { @@ -375,9 +386,10 @@ function scheduleTaskForRootDuringMicrotask( let schedulerPriorityLevel; switch (lanesToEventPriority(nextLanes)) { + // Scheduler does have an "ImmediatePriority", but now that we use + // microtasks for sync work we no longer use that. Any sync work that + // reaches this path is meant to be time sliced. case DiscreteEventPriority: - schedulerPriorityLevel = ImmediateSchedulerPriority; - break; case ContinuousEventPriority: schedulerPriorityLevel = UserBlockingSchedulerPriority; break; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index ba3270214c6a5..5206caa5c0f76 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -765,11 +765,12 @@ export function scheduleUpdateOnFiber( // The incoming update might unblock the current render. Interrupt the // current attempt and restart from the top. prepareFreshStack(root, NoLanes); + const didAttemptEntireTree = false; markRootSuspended( root, workInProgressRootRenderLanes, workInProgressDeferredLane, - workInProgressRootDidSkipSuspendedSiblings, + didAttemptEntireTree, ); } @@ -832,11 +833,12 @@ export function scheduleUpdateOnFiber( // effect of interrupting the current render and switching to the update. // TODO: Make sure this doesn't override pings that happen while we've // already started rendering. + const didAttemptEntireTree = false; markRootSuspended( root, workInProgressRootRenderLanes, workInProgressDeferredLane, - workInProgressRootDidSkipSuspendedSiblings, + didAttemptEntireTree, ); } } @@ -898,100 +900,120 @@ export function performWorkOnRoot( // for too long ("expired" work, to prevent starvation), or we're in // sync-updates-by-default mode. const shouldTimeSlice = - !forceSync && - !includesBlockingLane(lanes) && - !includesExpiredLane(root, lanes); + (!forceSync && + !includesBlockingLane(lanes) && + !includesExpiredLane(root, lanes)) || + // If we're prerendering, then we should use the concurrent work loop + // even if the lanes are synchronous, so that prerendering never blocks + // the main thread. + // TODO: We should consider doing this whenever a sync lane is suspended, + // even for regular pings. + (enableSiblingPrerendering && checkIfRootIsPrerendering(root, lanes)); + let exitStatus = shouldTimeSlice ? renderRootConcurrent(root, lanes) - : renderRootSync(root, lanes); + : renderRootSync(root, lanes, true); - if (exitStatus !== RootInProgress) { + do { let renderWasConcurrent = shouldTimeSlice; - do { - 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( + if (exitStatus === RootInProgress) { + // Render phase is still in progress. + if ( + enableSiblingPrerendering && + workInProgressRootIsPrerendering && + !shouldTimeSlice + ) { + // We're in prerendering mode, but time slicing is not enabled. This + // happens when something suspends during a synchronous update. Exit the + // the work loop. When we resume, we'll use the concurrent work loop so + // that prerendering is non-blocking. + // + // Mark the root as suspended. Usually we do this at the end of the + // render phase, but we do it here so that we resume in + // prerendering mode. + // TODO: Consider always calling markRootSuspended immediately. + // Needs to be *after* we attach a ping listener, though. + const didAttemptEntireTree = false; + markRootSuspended(root, lanes, NoLane, didAttemptEntireTree); + } + break; + } else 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. + const didAttemptEntireTree = !workInProgressRootDidSkipSuspendedSiblings; + markRootSuspended(root, lanes, NoLane, didAttemptEntireTree); + } else { + // The render completed. + + // Check if this render may have yielded to a concurrent event, and if so, + // confirm that any newly rendered stores are consistent. + // TODO: It's possible that even a concurrent render may never have yielded + // to the main thread, if it was fast enough, or if it expired. We could + // skip the consistency check in that case, too. + const finishedWork: Fiber = (root.current.alternate: any); + if ( + renderWasConcurrent && + !isRenderConsistentWithExternalStores(finishedWork) + ) { + // A store was mutated in an interleaved event. Render again, + // synchronously, to block further mutations. + exitStatus = renderRootSync(root, lanes, false); + // We assume the tree is now consistent because we didn't yield to any + // concurrent events. + renderWasConcurrent = false; + // Need to check the exit status again. + continue; + } + + // Check if something threw + if ( + (disableLegacyMode || root.tag !== LegacyRoot) && + exitStatus === RootErrored + ) { + const lanesThatJustErrored = lanes; + const errorRetryLanes = getLanesToRetrySynchronouslyOnError( root, - lanes, - NoLane, - workInProgressRootDidSkipSuspendedSiblings, + lanesThatJustErrored, ); - } else { - // The render completed. - - // Check if this render may have yielded to a concurrent event, and if so, - // confirm that any newly rendered stores are consistent. - // TODO: It's possible that even a concurrent render may never have yielded - // to the main thread, if it was fast enough, or if it expired. We could - // skip the consistency check in that case, too. - const finishedWork: Fiber = (root.current.alternate: any); - if ( - renderWasConcurrent && - !isRenderConsistentWithExternalStores(finishedWork) - ) { - // A store was mutated in an interleaved event. Render again, - // synchronously, to block further mutations. - exitStatus = renderRootSync(root, lanes); - // We assume the tree is now consistent because we didn't yield to any - // concurrent events. - renderWasConcurrent = false; - // Need to check the exit status again. - continue; - } - - // Check if something threw - if ( - (disableLegacyMode || root.tag !== LegacyRoot) && - exitStatus === RootErrored - ) { - const lanesThatJustErrored = lanes; - const errorRetryLanes = getLanesToRetrySynchronouslyOnError( + if (errorRetryLanes !== NoLanes) { + lanes = errorRetryLanes; + exitStatus = recoverFromConcurrentError( root, lanesThatJustErrored, + errorRetryLanes, ); - if (errorRetryLanes !== NoLanes) { - lanes = errorRetryLanes; - exitStatus = recoverFromConcurrentError( - root, - lanesThatJustErrored, - errorRetryLanes, - ); - renderWasConcurrent = false; - // Need to check the exit status again. - if (exitStatus !== RootErrored) { - // The root did not error this time. Restart the exit algorithm - // from the beginning. - // TODO: Refactor the exit algorithm to be less confusing. Maybe - // more branches + recursion instead of a loop. I think the only - // thing that causes it to be a loop is the RootDidNotComplete - // check. If that's true, then we don't need a loop/recursion - // at all. - continue; - } else { - // The root errored yet again. Proceed to commit the tree. - } + renderWasConcurrent = false; + // Need to check the exit status again. + if (exitStatus !== RootErrored) { + // The root did not error this time. Restart the exit algorithm + // from the beginning. + // TODO: Refactor the exit algorithm to be less confusing. Maybe + // more branches + recursion instead of a loop. I think the only + // thing that causes it to be a loop is the RootDidNotComplete + // check. If that's true, then we don't need a loop/recursion + // at all. + continue; + } else { + // The root errored yet again. Proceed to commit the tree. } } - if (exitStatus === RootFatalErrored) { - prepareFreshStack(root, NoLanes); - markRootSuspended( - root, - lanes, - NoLane, - workInProgressRootDidSkipSuspendedSiblings, - ); - break; - } - - // We now have a consistent tree. The next step is either to commit it, - // or, if something suspended, wait to commit it after a timeout. - finishConcurrentRender(root, exitStatus, finishedWork, lanes); } - break; - } while (true); - } + if (exitStatus === RootFatalErrored) { + prepareFreshStack(root, NoLanes); + // Since this is a fatal error, we're going to pretend we attempted + // the entire tree, to avoid scheduling a prerender. + const didAttemptEntireTree = true; + markRootSuspended(root, lanes, NoLane, didAttemptEntireTree); + break; + } + + // We now have a consistent tree. The next step is either to commit it, + // or, if something suspended, wait to commit it after a timeout. + finishConcurrentRender(root, exitStatus, finishedWork, lanes); + } + break; + } while (true); ensureRootIsScheduled(root); } @@ -1024,7 +1046,7 @@ function recoverFromConcurrentError( rootWorkInProgress.flags |= ForceClientRender; } - const exitStatus = renderRootSync(root, errorRetryLanes); + const exitStatus = renderRootSync(root, errorRetryLanes, false); if (exitStatus !== RootErrored) { // Successfully finished rendering on retry @@ -1108,11 +1130,13 @@ function finishConcurrentRender( // This is a transition, so we should exit without committing a // placeholder and without scheduling a timeout. Delay indefinitely // until we receive more data. + const didAttemptEntireTree = + !workInProgressRootDidSkipSuspendedSiblings; markRootSuspended( root, lanes, workInProgressDeferredLane, - workInProgressRootDidSkipSuspendedSiblings, + didAttemptEntireTree, ); return; } @@ -1168,11 +1192,13 @@ function finishConcurrentRender( // Don't bother with a very short suspense time. if (msUntilTimeout > 10) { + const didAttemptEntireTree = + !workInProgressRootDidSkipSuspendedSiblings; markRootSuspended( root, lanes, workInProgressDeferredLane, - workInProgressRootDidSkipSuspendedSiblings, + didAttemptEntireTree, ); const nextLanes = getNextLanes(root, NoLanes); @@ -1284,7 +1310,8 @@ function commitRootWhenReady( SUSPENDED_COMMIT, ), ); - markRootSuspended(root, lanes, spawnedLane, didSkipSuspendedSiblings); + const didAttemptEntireTree = !didSkipSuspendedSiblings; + markRootSuspended(root, lanes, spawnedLane, didAttemptEntireTree); return; } } @@ -1407,7 +1434,7 @@ function markRootSuspended( root: FiberRoot, suspendedLanes: Lanes, spawnedLane: Lane, - didSkipSuspendedSiblings: boolean, + didAttemptEntireTree: boolean, ) { // When suspending, we should always exclude lanes that were pinged or (more // rarely, since we try to avoid it) updated during the render phase. @@ -1416,12 +1443,7 @@ function markRootSuspended( suspendedLanes, workInProgressRootInterleavedUpdatedLanes, ); - _markRootSuspended( - root, - suspendedLanes, - spawnedLane, - didSkipSuspendedSiblings, - ); + _markRootSuspended(root, suspendedLanes, spawnedLane, didAttemptEntireTree); } export function flushRoot(root: FiberRoot, lanes: Lanes) { @@ -1963,7 +1985,12 @@ export function renderDidSuspendDelayIfPossible(): void { if ( !workInProgressRootDidSkipSuspendedSiblings && - !includesBlockingLane(workInProgressRootRenderLanes) + // Check if the root will be blocked from committing. + // TODO: Consider aligning this better with the rest of the logic. Maybe + // we should only set the exit status to RootSuspendedWithDelay if this + // condition is true? And remove the equivalent checks elsewhere. + (includesOnlyTransitions(workInProgressRootRenderLanes) || + getSuspenseHandler() === null) ) { // This render may not have originally been scheduled as a prerender, but // something suspended inside the visible part of the tree, which means we @@ -1989,11 +2016,12 @@ export function renderDidSuspendDelayIfPossible(): void { // pinged or updated while we were rendering. // TODO: Consider unwinding immediately, using the // SuspendedOnHydration mechanism. + const didAttemptEntireTree = false; markRootSuspended( workInProgressRoot, workInProgressRootRenderLanes, workInProgressDeferredLane, - workInProgressRootDidSkipSuspendedSiblings, + didAttemptEntireTree, ); } } @@ -2023,7 +2051,11 @@ export function renderHasNotSuspendedYet(): boolean { // TODO: Over time, this function and renderRootConcurrent have become more // and more similar. Not sure it makes sense to maintain forked paths. Consider // unifying them again. -function renderRootSync(root: FiberRoot, lanes: Lanes) { +function renderRootSync( + root: FiberRoot, + lanes: Lanes, + shouldYieldForPrerendering: boolean, +): RootExitStatus { const prevExecutionContext = executionContext; executionContext |= RenderContext; const prevDispatcher = pushDispatcher(root.containerInfo); @@ -2063,6 +2095,7 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { } let didSuspendInShell = false; + let exitStatus = workInProgressRootExitStatus; outer: do { try { if ( @@ -2084,16 +2117,37 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { // Selective hydration. An update flowed into a dehydrated tree. // Interrupt the current render so the work loop can switch to the // hydration lane. + // TODO: I think we might not need to reset the stack here; we can + // just yield and reset the stack when we re-enter the work loop, + // like normal. resetWorkInProgressStack(); - workInProgressRootExitStatus = RootDidNotComplete; + exitStatus = RootDidNotComplete; break outer; } case SuspendedOnImmediate: - case SuspendedOnData: { - if (!didSuspendInShell && getSuspenseHandler() === null) { + case SuspendedOnData: + case SuspendedOnDeprecatedThrowPromise: { + if (getSuspenseHandler() === null) { didSuspendInShell = true; } - // Intentional fallthrough + const reason = workInProgressSuspendedReason; + workInProgressSuspendedReason = NotSuspended; + workInProgressThrownValue = null; + throwAndUnwindWorkLoop(root, unitOfWork, thrownValue, reason); + if ( + enableSiblingPrerendering && + shouldYieldForPrerendering && + workInProgressRootIsPrerendering + ) { + // We've switched into prerendering mode. This implies that we + // suspended outside of a Suspense boundary, which means this + // render will be blocked from committing. Yield to the main + // thread so we can switch to prerendering using the concurrent + // work loop. + exitStatus = RootInProgress; + break outer; + } + break; } default: { // Unwind then continue with the normal work loop. @@ -2106,6 +2160,7 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { } } workLoopSync(); + exitStatus = workInProgressRootExitStatus; break; } catch (thrownValue) { handleThrow(root, thrownValue); @@ -2128,14 +2183,6 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { popDispatcher(prevDispatcher); popAsyncDispatcher(prevAsyncDispatcher); - if (workInProgress !== null) { - // This is a sync render, so we should have finished the whole tree. - throw new Error( - 'Cannot commit an incomplete root. This error is likely caused by a ' + - 'bug in React. Please file an issue.', - ); - } - if (__DEV__) { if (enableDebugTracing) { logRenderStopped(); @@ -2146,14 +2193,21 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { markRenderStopped(); } - // Set this to null to indicate there's no in-progress render. - workInProgressRoot = null; - workInProgressRootRenderLanes = NoLanes; + if (workInProgress !== null) { + // Did not complete the tree. This can happen if something suspended in + // the shell. + } else { + // Normal case. We completed the whole tree. + + // Set this to null to indicate there's no in-progress render. + workInProgressRoot = null; + workInProgressRootRenderLanes = NoLanes; - // It's safe to process the queue now that the render phase is complete. - finishQueueingConcurrentUpdates(); + // It's safe to process the queue now that the render phase is complete. + finishQueueingConcurrentUpdates(); + } - return workInProgressRootExitStatus; + return exitStatus; } // The work loop is an extremely hot path. Tell Closure not to inline it. @@ -2199,9 +2253,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { // // If we were previously in prerendering mode, check if we received any new // data during an interleaved event. - if (workInProgressRootIsPrerendering) { - workInProgressRootIsPrerendering = checkIfRootIsPrerendering(root, lanes); - } + workInProgressRootIsPrerendering = checkIfRootIsPrerendering(root, lanes); } if (__DEV__) { @@ -3753,6 +3805,9 @@ function pingSuspendedRoot( // the logic of whether or not a root suspends once it completes. // TODO: If we're rendering sync either due to Sync, Batched or expired, // we should probably never restart. + // TODO: Attach different listeners depending on whether the listener was + // attached during prerendering. Prerender pings should not interrupt + // normal renders. // If we're suspended with delay, or if it's a retry, we'll always suspend // so we can always restart. diff --git a/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js b/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js index fd03ba7310f83..8ca142cb50517 100644 --- a/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js +++ b/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js @@ -420,6 +420,10 @@ describe('ReactDeferredValue', () => { // The initial value suspended, so we attempt the final value, which // also suspends. 'Suspend! [Final]', + + ...(gate('enableSiblingPrerendering') + ? ['Suspend! [Loading...]', 'Suspend! [Final]'] + : []), ]); expect(root).toMatchRenderedOutput(null); @@ -459,6 +463,10 @@ describe('ReactDeferredValue', () => { // The initial value suspended, so we attempt the final value, which // also suspends. 'Suspend! [Final]', + + ...(gate('enableSiblingPrerendering') + ? ['Suspend! [Loading...]', 'Suspend! [Final]'] + : []), ]); expect(root).toMatchRenderedOutput(null); @@ -533,6 +541,10 @@ describe('ReactDeferredValue', () => { // The initial value suspended, so we attempt the final value, which // also suspends. 'Suspend! [Final]', + + ...(gate('enableSiblingPrerendering') + ? ['Suspend! [Loading...]', 'Suspend! [Final]'] + : []), ]); expect(root).toMatchRenderedOutput(null); diff --git a/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js b/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js index 37d907b0fdc65..235e8df2201de 100644 --- a/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js @@ -479,4 +479,75 @@ describe('ReactSiblingPrerendering', () => { assertLog([]); }, ); + + it( + 'when a synchronous update suspends outside a boundary, the resulting' + + 'prerender is concurrent', + async () => { + function App() { + return ( + <> + + + + + + + ); + } + + const root = ReactNoop.createRoot(); + // Mount the root synchronously + ReactNoop.flushSync(() => root.render()); + + // Synchronously render everything until we suspend in the shell + assertLog(['A', 'B', 'Suspend! [Async]']); + + if (gate('enableSiblingPrerendering')) { + // The rest of the siblings begin to prerender concurrently. Notice + // that we don't unwind here; we pick up where we left off above. + await waitFor(['C']); + await waitFor(['D']); + } + + assertLog([]); + expect(root).toMatchRenderedOutput(null); + + await resolveText('Async'); + assertLog(['A', 'B', 'Async', 'C', 'D']); + expect(root).toMatchRenderedOutput('ABAsyncCD'); + }, + ); + + it('restart a suspended sync render if something suspends while prerendering the siblings', async () => { + function App() { + return ( + <> + + + + + + + ); + } + + const root = ReactNoop.createRoot(); + // Mount the root synchronously + ReactNoop.flushSync(() => root.render()); + + // Synchronously render everything until we suspend in the shell + assertLog(['A', 'B', 'Suspend! [Async]']); + + if (gate('enableSiblingPrerendering')) { + // The rest of the siblings begin to prerender concurrently + await waitFor(['C']); + } + + // While we're prerendering, Async resolves. We should unwind and + // start over, rather than continue prerendering D. + await resolveText('Async'); + assertLog(['A', 'B', 'Async', 'C', 'D']); + expect(root).toMatchRenderedOutput('ABAsyncCD'); + }); });