From ac17c6900a94fd64206b98a828c6f5a5b97f8606 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 20 Aug 2024 14:39:28 -0400 Subject: [PATCH 1/2] Fix: Synchronous popstate transitions This is a refactor of the fix in #27505. When a transition update is scheduled by a popstate event, (i.e. a back/ forward navigation) we attempt to render it synchronously even though it's a transition, since it's likely the previous page's data is cached. In #27505, I changed the implementation so that it only "upgrades" the priority of the transition for a single attempt. If the attempt suspends, say because the data is not cached after all, from then on it should be treated as a normal transition. But it turns out #27505 did not work as intended, because it relied on marking the root with pending synchronous work (root.pendingLanes), which was never cleared until the popstate update completed. The test scenarios I wrote accidentally worked for a different reason related to suspending the work loop, which I'm currently in the middle of refactoring. --- .../src/__tests__/ReactDOMFiberAsync-test.js | 2 +- .../react-reconciler/src/ReactFiberLane.js | 72 +++++++++++++--- .../src/ReactFiberRootScheduler.js | 85 ++++++++++++------- 3 files changed, 116 insertions(+), 43 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js index e161eb85aa194..ee843996bef1c 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(['Suspend! [/path/b]']); + assertLog([]); await act(async () => { resolvePromise(); diff --git a/packages/react-reconciler/src/ReactFiberLane.js b/packages/react-reconciler/src/ReactFiberLane.js index f34f1aeccd85a..6ed4caae70974 100644 --- a/packages/react-reconciler/src/ReactFiberLane.js +++ b/packages/react-reconciler/src/ReactFiberLane.js @@ -92,6 +92,14 @@ export const DeferredLane: Lane = /* */ 0b1000000000000000000 export const UpdateLanes: Lanes = SyncLane | InputContinuousLane | DefaultLane | TransitionLanes; +export const HydrationLanes = + SyncHydrationLane | + InputContinuousHydrationLane | + DefaultHydrationLane | + TransitionHydrationLane | + SelectiveHydrationLane | + IdleHydrationLane; + // This function is used for the experimental timeline (react-devtools-timeline) // It should be kept in sync with the Lanes values above. export function getLabelForLane(lane: Lane): string | void { @@ -282,6 +290,51 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes { return nextLanes; } +export function getNextLanesToFlushSync( + root: FiberRoot, + extraLanesToForceSync: Lane | Lanes, +): Lanes { + // Similar to getNextLanes, except instead of choosing the next lanes to work + // on based on their priority, it selects all the lanes that have equal or + // higher priority than those are given. That way they can be synchronously + // rendered in a single batch. + // + // The main use case is updates scheduled by popstate events, which are + // flushed synchronously even though they are transitions. + const lanesToFlush = SyncUpdateLanes | extraLanesToForceSync; + + // Early bailout if there's no pending work left. + const pendingLanes = root.pendingLanes; + if (pendingLanes === NoLanes) { + return NoLanes; + } + + const suspendedLanes = root.suspendedLanes; + const pingedLanes = root.pingedLanes; + + // Remove lanes that are suspended (but not pinged) + const unblockedLanes = pendingLanes & ~(suspendedLanes & ~pingedLanes); + const unblockedLanesWithMatchingPriority = + unblockedLanes & getLanesOfEqualOrHigherPriority(lanesToFlush); + + // If there are matching hydration lanes, we should do those by themselves. + // Hydration lanes must never include updates. + if (unblockedLanesWithMatchingPriority & HydrationLanes) { + return ( + (unblockedLanesWithMatchingPriority & HydrationLanes) | SyncHydrationLane + ); + } + + if (unblockedLanesWithMatchingPriority) { + // Always include the SyncLane as part of the result, even if there's no + // pending sync work, to indicate the priority of the entire batch of work + // is considered Sync. + return unblockedLanesWithMatchingPriority | SyncLane; + } + + return NoLanes; +} + export function getEntangledLanes(root: FiberRoot, renderLanes: Lanes): Lanes { let entangledLanes = renderLanes; @@ -534,6 +587,14 @@ export function getHighestPriorityLane(lanes: Lanes): Lane { return lanes & -lanes; } +function getLanesOfEqualOrHigherPriority(lanes: Lane | Lanes): Lanes { + // Create a mask with all bits to the right or same as the highest bit. + // So if lanes is 0b100, the result would be 0b111. + // If lanes is 0b101, the result would be 0b111. + const lowestPriorityLaneIndex = 31 - clz32(lanes); + return (1 << (lowestPriorityLaneIndex + 1)) - 1; +} + export function pickArbitraryLane(lanes: Lanes): Lane { // This wrapper function gets inlined. Only exists so to communicate that it // doesn't matter which bit is selected; you can pick any bit without @@ -757,17 +818,6 @@ export function markRootEntangled(root: FiberRoot, entangledLanes: Lanes) { } } -export function upgradePendingLaneToSync(root: FiberRoot, lane: Lane) { - // Since we're upgrading the priority of the given lane, there is now pending - // sync work. - root.pendingLanes |= SyncLane; - - // Entangle the sync lane with the lane we're upgrading. This means SyncLane - // will not be allowed to finish without also finishing the given lane. - root.entangledLanes |= SyncLane; - root.entanglements[SyncLaneIndex] |= lane; -} - export function upgradePendingLanesToSync( root: FiberRoot, lanesToUpgrade: Lanes, diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js index dc3dd366b8da9..15730d3e5b624 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -8,7 +8,7 @@ */ import type {FiberRoot} from './ReactInternalTypes'; -import type {Lane} from './ReactFiberLane'; +import type {Lane, Lanes} from './ReactFiberLane'; import type {PriorityLevel} from 'scheduler/src/SchedulerPriorities'; import type {BatchConfigTransition} from './ReactFiberTracingMarkerComponent'; @@ -24,8 +24,8 @@ import { getNextLanes, includesSyncLane, markStarvedLanesAsExpired, - upgradePendingLaneToSync, claimNextTransitionLane, + getNextLanesToFlushSync, } from './ReactFiberLane'; import { CommitContext, @@ -145,18 +145,21 @@ export function ensureRootIsScheduled(root: FiberRoot): void { export function flushSyncWorkOnAllRoots() { // This is allowed to be called synchronously, but the caller should check // the execution context first. - flushSyncWorkAcrossRoots_impl(false); + flushSyncWorkAcrossRoots_impl(NoLanes, false); } export function flushSyncWorkOnLegacyRootsOnly() { // This is allowed to be called synchronously, but the caller should check // the execution context first. if (!disableLegacyMode) { - flushSyncWorkAcrossRoots_impl(true); + flushSyncWorkAcrossRoots_impl(NoLanes, true); } } -function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) { +function flushSyncWorkAcrossRoots_impl( + syncTransitionLanes: Lanes | Lane, + onlyLegacy: boolean, +) { if (isFlushingWork) { // Prevent reentrancy. // TODO: Is this overly defensive? The callers must check the execution @@ -179,17 +182,28 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) { if (onlyLegacy && (disableLegacyMode || root.tag !== LegacyRoot)) { // Skip non-legacy roots. } else { - const workInProgressRoot = getWorkInProgressRoot(); - const workInProgressRootRenderLanes = - getWorkInProgressRootRenderLanes(); - const nextLanes = getNextLanes( - root, - root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes, - ); - if (includesSyncLane(nextLanes)) { - // This root has pending sync work. Flush it now. - didPerformSomeWork = true; - performSyncWorkOnRoot(root, nextLanes); + if (syncTransitionLanes !== NoLanes) { + const nextLanes = getNextLanesToFlushSync(root, syncTransitionLanes); + if (nextLanes !== NoLanes) { + // This root has pending sync work. Flush it now. + didPerformSomeWork = true; + performSyncWorkOnRoot(root, nextLanes); + } + } else { + const workInProgressRoot = getWorkInProgressRoot(); + const workInProgressRootRenderLanes = + getWorkInProgressRootRenderLanes(); + const nextLanes = getNextLanes( + root, + root === workInProgressRoot + ? workInProgressRootRenderLanes + : NoLanes, + ); + if (includesSyncLane(nextLanes)) { + // This root has pending sync work. Flush it now. + didPerformSomeWork = true; + performSyncWorkOnRoot(root, nextLanes); + } } } root = root.next; @@ -209,23 +223,23 @@ function processRootScheduleInMicrotask() { // We'll recompute this as we iterate through all the roots and schedule them. mightHavePendingSyncWork = false; + let syncTransitionLanes = NoLanes; + if (currentEventTransitionLane !== NoLane) { + if (shouldAttemptEagerTransition()) { + // A transition was scheduled during an event, but we're going to try to + // render it synchronously anyway. We do this during a popstate event to + // preserve the scroll position of the previous page. + syncTransitionLanes = currentEventTransitionLane; + } + currentEventTransitionLane = NoLane; + } + const currentTime = now(); let prev = null; let root = firstScheduledRoot; while (root !== null) { const next = root.next; - - if ( - currentEventTransitionLane !== NoLane && - shouldAttemptEagerTransition() - ) { - // A transition was scheduled during an event, but we're going to try to - // render it synchronously anyway. We do this during a popstate event to - // preserve the scroll position of the previous page. - upgradePendingLaneToSync(root, currentEventTransitionLane); - } - const nextLanes = scheduleTaskForRootDuringMicrotask(root, currentTime); if (nextLanes === NoLane) { // This root has no more pending work. Remove it from the schedule. To @@ -248,18 +262,27 @@ function processRootScheduleInMicrotask() { } else { // This root still has work. Keep it in the list. prev = root; - if (includesSyncLane(nextLanes)) { + + // This is a fast-path optimization to early exit from + // flushSyncWorkOnAllRoots if we can be certain that there is no remaining + // synchronous work to perform. Set this to true if there might be sync + // work left. + if ( + // Skip the optimization if syncTransitionLanes is set + syncTransitionLanes !== NoLanes || + // Common case: we're not treating any extra lanes as synchronous, so we + // can just check if the next lanes are sync. + includesSyncLane(nextLanes) + ) { mightHavePendingSyncWork = true; } } root = next; } - currentEventTransitionLane = NoLane; - // At the end of the microtask, flush any pending synchronous work. This has // to come at the end, because it does actual rendering work that might throw. - flushSyncWorkOnAllRoots(); + flushSyncWorkAcrossRoots_impl(syncTransitionLanes, false); } function scheduleTaskForRootDuringMicrotask( From f72bdceb2d7c283f3f3330c34ef529e2ee74a19a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 20 Aug 2024 15:05:24 -0400 Subject: [PATCH 2/2] Temporarily disable suspending during work loop `use` has an optimization where in some cases it can suspend the work loop during the render phase until the data has resolved, rather than unwind the stack and lose context. However, the current implementation is not compatible with sibling prerendering. So I've temporarily disabled it until the sibling prerendering has been refactored. We will add it back in a later step. --- packages/react-reconciler/src/ReactFiberWorkLoop.js | 5 +++++ .../react-reconciler/src/__tests__/ReactUse-test.js | 12 ++++++++++++ scripts/jest/TestFlags.js | 4 ++++ 3 files changed, 21 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 40eed7a71325a..3190cd0035b33 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -43,6 +43,7 @@ import { disableLegacyMode, disableDefaultPropsExceptForClasses, disableStringRefs, + enableSiblingPrerendering, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import is from 'shared/objectIs'; @@ -1700,6 +1701,10 @@ function handleThrow(root: FiberRoot, thrownValue: any): void { // deprecate the old API in favor of `use`. thrownValue = getSuspendedThenable(); workInProgressSuspendedReason = + // TODO: Suspending the work loop during the render phase is + // currently not compatible with sibling prerendering. We will add + // this optimization back in a later step. + !enableSiblingPrerendering && shouldRemainOnPreviousScreen() && // Check if there are other pending updates that might possibly unblock this // component from suspending. This mirrors the check in diff --git a/packages/react-reconciler/src/__tests__/ReactUse-test.js b/packages/react-reconciler/src/__tests__/ReactUse-test.js index 1dae8a56dd25f..0abb47d516a3d 100644 --- a/packages/react-reconciler/src/__tests__/ReactUse-test.js +++ b/packages/react-reconciler/src/__tests__/ReactUse-test.js @@ -558,6 +558,7 @@ describe('ReactUse', () => { } }); + // @gate enableSuspendingDuringWorkLoop it('during a transition, can unwrap async operations even if nothing is cached', async () => { function App() { return ; @@ -593,6 +594,7 @@ describe('ReactUse', () => { expect(root).toMatchRenderedOutput('Async'); }); + // @gate enableSuspendingDuringWorkLoop it("does not prevent a Suspense fallback from showing if it's a new boundary, even during a transition", async () => { function App() { return ; @@ -635,6 +637,7 @@ describe('ReactUse', () => { expect(root).toMatchRenderedOutput('Async'); }); + // @gate enableSuspendingDuringWorkLoop it('when waiting for data to resolve, a fresh update will trigger a restart', async () => { function App() { return ; @@ -666,6 +669,7 @@ describe('ReactUse', () => { assertLog(['Something different']); }); + // @gate enableSuspendingDuringWorkLoop it('when waiting for data to resolve, an update on a different root does not cause work to be dropped', async () => { const promise = getAsyncText('Hi'); @@ -708,6 +712,7 @@ describe('ReactUse', () => { expect(root1).toMatchRenderedOutput('Hi'); }); + // @gate enableSuspendingDuringWorkLoop it('while suspended, hooks cannot be called (i.e. current dispatcher is unset correctly)', async () => { function App() { return ; @@ -845,6 +850,7 @@ describe('ReactUse', () => { expect(root).toMatchRenderedOutput('(empty)'); }); + // @gate enableSuspendingDuringWorkLoop it('when replaying a suspended component, reuses the hooks computed during the previous attempt (Memo)', async () => { function ExcitingText({text}) { // This computes the uppercased version of some text. Pretend it's an @@ -894,6 +900,7 @@ describe('ReactUse', () => { ]); }); + // @gate enableSuspendingDuringWorkLoop it('when replaying a suspended component, reuses the hooks computed during the previous attempt (State)', async () => { let _setFruit; let _setVegetable; @@ -950,6 +957,7 @@ describe('ReactUse', () => { expect(root).toMatchRenderedOutput('banana dill'); }); + // @gate enableSuspendingDuringWorkLoop it('when replaying a suspended component, reuses the hooks computed during the previous attempt (DebugValue+State)', async () => { // Make sure we don't get a Hook mismatch warning on updates if there were non-stateful Hooks before the use(). let _setLawyer; @@ -991,6 +999,7 @@ describe('ReactUse', () => { expect(root).toMatchRenderedOutput('aguacate avocat'); }); + // @gate enableSuspendingDuringWorkLoop it( 'wrap an async function with useMemo to skip running the function ' + 'twice when loading new data', @@ -1073,6 +1082,7 @@ describe('ReactUse', () => { expect(root).toMatchRenderedOutput('ABC'); }); + // @gate enableSuspendingDuringWorkLoop it('load multiple nested Suspense boundaries (uncached requests)', async () => { // This the same as the previous test, except the requests are not cached. // The tree should still eventually resolve, despite the @@ -1196,6 +1206,7 @@ describe('ReactUse', () => { expect(root).toMatchRenderedOutput('Hi'); }); + // @gate enableSuspendingDuringWorkLoop it('basic async component', async () => { async function App() { await getAsyncText('Hi'); @@ -1220,6 +1231,7 @@ describe('ReactUse', () => { expect(root).toMatchRenderedOutput('Hi'); }); + // @gate enableSuspendingDuringWorkLoop it('async child of a non-function component (e.g. a class)', async () => { class App extends React.Component { async render() { diff --git a/scripts/jest/TestFlags.js b/scripts/jest/TestFlags.js index 6ced58a32be3c..b970955491aae 100644 --- a/scripts/jest/TestFlags.js +++ b/scripts/jest/TestFlags.js @@ -83,6 +83,10 @@ function getTestFlags() { enableActivity: releaseChannel === 'experimental' || www || xplat, enableSuspenseList: releaseChannel === 'experimental' || www || xplat, enableLegacyHidden: www, + // TODO: Suspending the work loop during the render phase is currently + // not compatible with sibling prerendering. We will add this optimization + // back in a later step. + enableSuspendingDuringWorkLoop: !featureFlags.enableSiblingPrerendering, // This flag is used to determine whether we should run Fizz tests using // the external runtime or the inline script runtime.