From 100945ba8f3fe3b6bc8f1488300b770c89173fd7 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 4 Sep 2019 19:33:36 -0700 Subject: [PATCH 1/8] Track "pending" and "suspended" ranges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A FiberRoot can have pending work at many distinct priorities. (Note: we refer to these levels as "expiration times" to distinguish the concept from Scheduler's notion of priority levels, which represent broad categories of work. React expiration times are more granualar. They're more like a concurrent thread ID, which also happens to correspond to a moment on a timeline. It's an overloaded concept and I'm handwaving over some of the details.) Given a root, there's no convenient way to read all the pending levels in the entire tree, i.e. there's no single queue-like structure that tracks all the levels, because that granularity of information is not needed by our algorithms. Instead we track the subset of information that we actually need — most importantly, the highest priority level that exists in the entire tree. Aside from that, the other information we track includes the range of pending levels that are known to be suspended, and therefore should not be worked on. This is a refactor of how that information is tracked, and what each field represents: - A *pending* level is work that is unfinished, or not yet committed. This includes work that is suspended from committing. `firstPendingTime` and `lastPendingTime` represent the range of pending work. (Previously, "pending" was the same as "not suspended.") - A *suspended* level is work that did not complete because data was missing. `firstSuspendedTime` and `lastSuspendedTime` represent the range of suspended work. It is a subset of the pending range. (These fields are new to this commit.) - `nextAfterSuspendedTime` represents the next known level that comes after the suspended range. This commit doesn't change much in terms of observable behavior. The one change is that, when a level is suspended, React will continue working on the next known level instead of jumping straight to the last pending level. Subsequent commits will use this new structure for a more substantial refactor for how tasks are scheduled per root. --- .../react-reconciler/src/ReactFiberRoot.js | 66 +++++++++- .../src/ReactFiberWorkLoop.js | 117 ++++++++++++------ ...tSuspenseWithNoopRenderer-test.internal.js | 65 ++++++++++ 3 files changed, 206 insertions(+), 42 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index 7ef93739039e0..c7d8d07916dac 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -73,8 +73,15 @@ type BaseFiberRootProperties = {| firstPendingTime: ExpirationTime, // The latest pending expiration time that exists in the tree lastPendingTime: ExpirationTime, - // The time at which a suspended component pinged the root to render again - pingTime: ExpirationTime, + // The earliest suspended expiration time that exists in the tree + firstSuspendedTime: ExpirationTime, + // The latest suspended expiration time that exists in the tree + lastSuspendedTime: ExpirationTime, + // The next known expiration time after the suspended range + nextAfterSuspendedTime: ExpirationTime, + // The latest time at which a suspended component pinged the root to + // render again + lastPingedTime: ExpirationTime, |}; // The following attributes are only used by interaction tracing builds. @@ -120,7 +127,10 @@ function FiberRootNode(containerInfo, tag, hydrate) { this.callbackExpirationTime = NoWork; this.firstPendingTime = NoWork; this.lastPendingTime = NoWork; - this.pingTime = NoWork; + this.firstSuspendedTime = NoWork; + this.lastSuspendedTime = NoWork; + this.nextAfterSuspendedTime = NoWork; + this.lastPingedTime = NoWork; if (enableSchedulerTracing) { this.interactionThreadID = unstable_getThreadID(); @@ -151,3 +161,53 @@ export function createFiberRoot( return root; } + +export function isRootSuspendedAtTime( + root: FiberRoot, + expirationTime: ExpirationTime, +): boolean { + const firstSuspendedTime = root.firstSuspendedTime; + const lastSuspendedTime = root.lastSuspendedTime; + return ( + firstSuspendedTime !== NoWork && + (firstSuspendedTime >= expirationTime && + lastSuspendedTime <= expirationTime) + ); +} + +export function markRootSuspendedAtTime( + root: FiberRoot, + expirationTime: ExpirationTime, +): void { + const firstSuspendedTime = root.firstSuspendedTime; + const lastSuspendedTime = root.lastSuspendedTime; + if (firstSuspendedTime < expirationTime) { + root.firstSuspendedTime = expirationTime; + } + if (lastSuspendedTime > expirationTime || firstSuspendedTime === NoWork) { + root.lastSuspendedTime = expirationTime; + } + + if (expirationTime <= root.lastPingedTime) { + root.lastPingedTime = NoWork; + } +} + +export function markRootUnsuspendedAtTime( + root: FiberRoot, + expirationTime: ExpirationTime, +): void { + if (expirationTime <= root.lastSuspendedTime) { + // The entire suspended range is now unsuspended. + root.firstSuspendedTime = root.lastSuspendedTime = root.nextAfterSuspendedTime = NoWork; + } else if (expirationTime <= root.firstSuspendedTime) { + // Part of the suspended range is now unsuspended. Narrow the range to + // include everything between the unsuspended time (non-inclusive) and the + // last suspended time. + root.firstSuspendedTime = expirationTime - 1; + } + + if (expirationTime <= root.lastPingedTime) { + root.lastPingedTime = NoWork; + } +} diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 9d9dad28a41aa..f47913100b326 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -66,6 +66,11 @@ import { } from './ReactFiberHostConfig'; import {createWorkInProgress, assignFiberPropertiesInDEV} from './ReactFiber'; +import { + isRootSuspendedAtTime, + markRootSuspendedAtTime, + markRootUnsuspendedAtTime, +} from './ReactFiberRoot'; import { NoMode, StrictMode, @@ -377,8 +382,6 @@ export function scheduleUpdateOnFiber( return; } - root.pingTime = NoWork; - checkForInterruption(fiber, expirationTime); recordScheduleUpdate(); @@ -492,6 +495,9 @@ function markUpdateTimeFromFiberToRoot(fiber, expirationTime) { if (lastPendingTime === NoWork || expirationTime < lastPendingTime) { root.lastPendingTime = expirationTime; } + + // Mark that the root is no longer suspended at this time. + markRootUnsuspendedAtTime(root, expirationTime); } return root; @@ -807,13 +813,6 @@ function renderRoot( 'Should not already be working.', ); - if (root.firstPendingTime < expirationTime) { - // If there's no work left at this expiration time, exit immediately. This - // happens when multiple callbacks are scheduled for a single root, but an - // earlier callback flushes the work of a later one. - return null; - } - if (isSync && root.finishedExpirationTime === expirationTime) { // There's already a pending commit at this expiration time. // TODO: This is poorly factored. This case only exists for the @@ -831,8 +830,9 @@ function renderRoot( } else if (workInProgressRootExitStatus === RootSuspendedWithDelay) { // We could've received an update at a lower priority while we yielded. // We're suspended in a delayed state. Once we complete this render we're - // just going to try to recover at the last pending time anyway so we might - // as well start doing that eagerly. + // just going to try to recover at the pending time anyway so we might as + // well start doing that eagerly. + // // Ideally we should be able to do this even for retries but we don't yet // know if we're going to process an update which wants to commit earlier, // and this path happens very early so it would happen too often. Instead, @@ -840,12 +840,15 @@ function renderRoot( if (workInProgressRootHasPendingPing) { // We have a ping at this expiration. Let's restart to see if we get unblocked. prepareFreshStack(root, expirationTime); - } else { - const lastPendingTime = root.lastPendingTime; - if (lastPendingTime < expirationTime) { - // There's lower priority work. It might be unsuspended. Try rendering - // at that level immediately, while preserving the position in the queue. - return renderRoot.bind(null, root, lastPendingTime); + } else if (!isSync) { + // Check if there's work that isn't in the suspended range + const firstPendingTime = root.firstPendingTime; + if (!isRootSuspendedAtTime(root, firstPendingTime)) { + // There's a pending update that falls outside the range of + // suspended work. + if (firstPendingTime > expirationTime) { + return renderRoot.bind(null, root, firstPendingTime); + } } } } @@ -958,7 +961,8 @@ function renderRoot( // something suspended, wait to commit it after a timeout. stopFinishedWorkLoopTimer(); - root.finishedWork = root.current.alternate; + const finishedWork: Fiber = ((root.finishedWork = + root.current.alternate): any); root.finishedExpirationTime = expirationTime; const isLocked = resolveLocksOnRoot(root, expirationTime); @@ -1002,6 +1006,11 @@ function renderRoot( return commitRoot.bind(null, root); } case RootSuspended: { + markRootSuspendedAtTime(root, expirationTime); + const lastSuspendedTime = root.lastSuspendedTime; + if (expirationTime === lastSuspendedTime) { + root.nextAfterSuspendedTime = getRemainingExpirationTime(finishedWork); + } flushSuspensePriorityWarningInDEV(); // We have an acceptable loading state. We need to figure out if we should @@ -1038,11 +1047,20 @@ function renderRoot( prepareFreshStack(root, expirationTime); return renderRoot.bind(null, root, expirationTime); } - const lastPendingTime = root.lastPendingTime; - if (lastPendingTime < expirationTime) { + + const nextAfterSuspendedTime = root.nextAfterSuspendedTime; + if (nextAfterSuspendedTime !== NoWork) { // There's lower priority work. It might be unsuspended. Try rendering // at that level. - return renderRoot.bind(null, root, lastPendingTime); + return renderRoot.bind(null, root, nextAfterSuspendedTime); + } + if ( + lastSuspendedTime !== NoWork && + lastSuspendedTime !== expirationTime + ) { + // We should prefer to render the fallback of at the last + // suspended level. + return renderRoot.bind(null, root, lastSuspendedTime); } // The render is suspended, it hasn't timed out, and there's no lower // priority work to do. Instead of committing the fallback @@ -1058,6 +1076,11 @@ function renderRoot( return commitRoot.bind(null, root); } case RootSuspendedWithDelay: { + markRootSuspendedAtTime(root, expirationTime); + const lastSuspendedTime = root.lastSuspendedTime; + if (expirationTime === lastSuspendedTime) { + root.nextAfterSuspendedTime = getRemainingExpirationTime(finishedWork); + } flushSuspensePriorityWarningInDEV(); if ( @@ -1077,11 +1100,20 @@ function renderRoot( prepareFreshStack(root, expirationTime); return renderRoot.bind(null, root, expirationTime); } - const lastPendingTime = root.lastPendingTime; - if (lastPendingTime < expirationTime) { + + const nextAfterSuspendedTime = root.nextAfterSuspendedTime; + if (nextAfterSuspendedTime !== NoWork) { // There's lower priority work. It might be unsuspended. Try rendering - // at that level immediately. - return renderRoot.bind(null, root, lastPendingTime); + // at that level. + return renderRoot.bind(null, root, nextAfterSuspendedTime); + } + if ( + lastSuspendedTime !== NoWork && + lastSuspendedTime !== expirationTime + ) { + // We should prefer to render the fallback of at the last + // suspended level. + return renderRoot.bind(null, root, lastSuspendedTime); } let msUntilTimeout; @@ -1425,6 +1457,14 @@ function completeUnitOfWork(unitOfWork: Fiber): Fiber | null { return null; } +function getRemainingExpirationTime(fiber: Fiber) { + const updateExpirationTime = fiber.expirationTime; + const childExpirationTime = fiber.childExpirationTime; + return updateExpirationTime > childExpirationTime + ? updateExpirationTime + : childExpirationTime; +} + function resetChildExpirationTime(completedWork: Fiber) { if ( renderExpirationTime !== Never && @@ -1540,19 +1580,19 @@ function commitRootImpl(root, renderPriorityLevel) { // Update the first and last pending times on this root. The new first // pending time is whatever is left on the root fiber. - const updateExpirationTimeBeforeCommit = finishedWork.expirationTime; - const childExpirationTimeBeforeCommit = finishedWork.childExpirationTime; - const firstPendingTimeBeforeCommit = - childExpirationTimeBeforeCommit > updateExpirationTimeBeforeCommit - ? childExpirationTimeBeforeCommit - : updateExpirationTimeBeforeCommit; - root.firstPendingTime = firstPendingTimeBeforeCommit; - if (firstPendingTimeBeforeCommit < root.lastPendingTime) { + const remainingExpirationTimeBeforeCommit = getRemainingExpirationTime( + finishedWork, + ); + root.firstPendingTime = remainingExpirationTimeBeforeCommit; + if (remainingExpirationTimeBeforeCommit < root.lastPendingTime) { // This usually means we've finished all the work, but it can also happen // when something gets downprioritized during render, like a hidden tree. - root.lastPendingTime = firstPendingTimeBeforeCommit; + root.lastPendingTime = remainingExpirationTimeBeforeCommit; } + // Mark that the root is no longer suspended at the finished time + markRootUnsuspendedAtTime(root, expirationTime); + if (root === workInProgressRoot) { // We can reset these now that they are finished. workInProgressRoot = null; @@ -2148,20 +2188,19 @@ export function pingSuspendedRoot( return; } - const lastPendingTime = root.lastPendingTime; - if (lastPendingTime < suspendedTime) { + if (!isRootSuspendedAtTime(root, suspendedTime)) { // The root is no longer suspended at this time. return; } - const pingTime = root.pingTime; - if (pingTime !== NoWork && pingTime < suspendedTime) { + const lastPingedTime = root.lastPingedTime; + if (lastPingedTime !== NoWork && lastPingedTime < suspendedTime) { // There's already a lower priority ping scheduled. return; } // Mark the time at which this ping was scheduled. - root.pingTime = suspendedTime; + root.lastPingedTime = suspendedTime; if (root.finishedExpirationTime === suspendedTime) { // If there's a pending fallback waiting to commit, throw it away. diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 65e672d87dceb..60dfcb0a55983 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -518,6 +518,71 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('(empty)')]); }); + it('tries each subsequent level after suspending', async () => { + const root = ReactNoop.createRoot(); + + function App({step, shouldSuspend}) { + return ( + + + {shouldSuspend ? ( + + ) : ( + + )} + + ); + } + + function interrupt() { + // React has a heuristic to batch all updates that occur within the same + // event. This is a trick to circumvent that heuristic. + ReactNoop.flushSync(() => { + ReactNoop.renderToRootWithID(null, 'other-root'); + }); + } + + // Mount the Suspense boundary without suspending, so that the subsequent + // updates suspend with a delay. + await ReactNoop.act(async () => { + root.render(); + }); + await advanceTimers(1000); + expect(Scheduler).toHaveYielded(['Sibling', 'Step 0']); + + // Schedule an update at several distinct expiration times + await ReactNoop.act(async () => { + root.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['Sibling']); + interrupt(); + + root.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['Sibling']); + interrupt(); + + root.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['Sibling']); + interrupt(); + + root.render(); + }); + + // Should suspend at each distinct level + expect(Scheduler).toHaveYielded([ + 'Sibling', + 'Suspend! [Step 1]', + 'Sibling', + 'Suspend! [Step 2]', + 'Sibling', + 'Suspend! [Step 3]', + 'Sibling', + 'Step 4', + ]); + }); + it('forces an expiration after an update times out', async () => { ReactNoop.render( From 6ace553825626774d9d9325be84389fe7fe5e33a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 5 Sep 2019 20:11:42 -0700 Subject: [PATCH 2/8] Get next expiration time from FiberRoot Given a FiberRoot, we should be able to determine the next expiration time that needs to be worked on, taking into account the levels that are pending, suspended, pinged, and so on. This removes the `expirationTime` argument from `scheduleCallbackForRoot`, and renames it to `ensureRootIsScheduled` to reflect the new signature. The expiration time is instead read from the root using a new function, `getNextExpirationTimeToWorkOn`. The next step will be to remove the `expirationTime` argument from `renderRoot`, too. --- .../react-reconciler/src/ReactFiberRoot.js | 67 ++++- .../src/ReactFiberWorkLoop.js | 249 +++++++++--------- 2 files changed, 188 insertions(+), 128 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index c7d8d07916dac..facc33f2f4ba9 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -14,6 +14,7 @@ import type {TimeoutHandle, NoTimeout} from './ReactFiberHostConfig'; import type {Thenable} from './ReactFiberWorkLoop'; import type {Interaction} from 'scheduler/src/Tracing'; import type {SuspenseHydrationCallbacks} from './ReactFiberSuspenseComponent'; +import type {ReactPriorityLevel} from './SchedulerWithReactIntegration'; import {noTimeout} from './ReactFiberHostConfig'; import {createHostRootFiber} from './ReactFiber'; @@ -23,6 +24,7 @@ import { enableSuspenseCallback, } from 'shared/ReactFeatureFlags'; import {unstable_getThreadID} from 'scheduler/tracing'; +import {NoPriority} from './SchedulerWithReactIntegration'; // TODO: This should be lifted into the renderer. export type Batch = { @@ -69,6 +71,8 @@ type BaseFiberRootProperties = {| callbackNode: *, // Expiration of the callback associated with this root callbackExpirationTime: ExpirationTime, + // Priority of the callback associated with this root + callbackPriority: ReactPriorityLevel, // The earliest pending expiration time that exists in the tree firstPendingTime: ExpirationTime, // The latest pending expiration time that exists in the tree @@ -78,7 +82,7 @@ type BaseFiberRootProperties = {| // The latest suspended expiration time that exists in the tree lastSuspendedTime: ExpirationTime, // The next known expiration time after the suspended range - nextAfterSuspendedTime: ExpirationTime, + nextKnownPendingLevel: ExpirationTime, // The latest time at which a suspended component pinged the root to // render again lastPingedTime: ExpirationTime, @@ -124,12 +128,12 @@ function FiberRootNode(containerInfo, tag, hydrate) { this.hydrate = hydrate; this.firstBatch = null; this.callbackNode = null; - this.callbackExpirationTime = NoWork; + this.callbackPriority = NoPriority; this.firstPendingTime = NoWork; this.lastPendingTime = NoWork; this.firstSuspendedTime = NoWork; this.lastSuspendedTime = NoWork; - this.nextAfterSuspendedTime = NoWork; + this.nextKnownPendingLevel = NoWork; this.lastPingedTime = NoWork; if (enableSchedulerTracing) { @@ -193,21 +197,66 @@ export function markRootSuspendedAtTime( } } -export function markRootUnsuspendedAtTime( +export function markRootUpdatedAtTime( root: FiberRoot, expirationTime: ExpirationTime, ): void { - if (expirationTime <= root.lastSuspendedTime) { + // Update the range of pending times + const firstPendingTime = root.firstPendingTime; + if (expirationTime > firstPendingTime) { + root.firstPendingTime = expirationTime; + } + const lastPendingTime = root.lastPendingTime; + if (lastPendingTime === NoWork || expirationTime < lastPendingTime) { + root.lastPendingTime = expirationTime; + } + + // Update the range of suspended times. Treat everything lower priority or + // equal to this update as unsuspended. + const firstSuspendedTime = root.firstSuspendedTime; + if (firstSuspendedTime !== NoWork) { + if (expirationTime >= firstSuspendedTime) { + // The entire suspended range is now unsuspended. + root.firstSuspendedTime = root.lastSuspendedTime = root.nextKnownPendingLevel = NoWork; + } else if (expirationTime >= root.lastSuspendedTime) { + root.lastSuspendedTime = expirationTime + 1; + } + + // This is a pending level. Check if it's higher priority than the next + // known pending level. + if (expirationTime > root.nextKnownPendingLevel) { + root.nextKnownPendingLevel = expirationTime; + } + } +} + +export function markRootFinishedAtTime( + root: FiberRoot, + finishedExpirationTime: ExpirationTime, + remainingExpirationTime: ExpirationTime, +): void { + // Update the range of pending times + root.firstPendingTime = remainingExpirationTime; + if (remainingExpirationTime < root.lastPendingTime) { + // This usually means we've finished all the work, but it can also happen + // when something gets downprioritized during render, like a hidden tree. + root.lastPendingTime = remainingExpirationTime; + } + + // Update the range of suspended times. Treat everything higher priority or + // equal to this update as unsuspended. + if (finishedExpirationTime <= root.lastSuspendedTime) { // The entire suspended range is now unsuspended. - root.firstSuspendedTime = root.lastSuspendedTime = root.nextAfterSuspendedTime = NoWork; - } else if (expirationTime <= root.firstSuspendedTime) { + root.firstSuspendedTime = root.lastSuspendedTime = root.nextKnownPendingLevel = NoWork; + } else if (finishedExpirationTime <= root.firstSuspendedTime) { // Part of the suspended range is now unsuspended. Narrow the range to // include everything between the unsuspended time (non-inclusive) and the // last suspended time. - root.firstSuspendedTime = expirationTime - 1; + root.firstSuspendedTime = finishedExpirationTime - 1; } - if (expirationTime <= root.lastPingedTime) { + if (finishedExpirationTime <= root.lastPingedTime) { + // Clear the pinged time root.lastPingedTime = NoWork; } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index f47913100b326..8f2915291845f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -69,7 +69,8 @@ import {createWorkInProgress, assignFiberPropertiesInDEV} from './ReactFiber'; import { isRootSuspendedAtTime, markRootSuspendedAtTime, - markRootUnsuspendedAtTime, + markRootFinishedAtTime, + markRootUpdatedAtTime, } from './ReactFiberRoot'; import { NoMode, @@ -407,7 +408,8 @@ export function scheduleUpdateOnFiber( callback = callback(true); } } else { - scheduleCallbackForRoot(root, ImmediatePriority, Sync); + ensureRootIsScheduled(root); + schedulePendingInteractions(root, expirationTime); if (executionContext === NoContext) { // Flush the synchronous work now, wnless we're already working or inside // a batch. This is intentionally inside scheduleUpdateOnFiber instead of @@ -418,7 +420,8 @@ export function scheduleUpdateOnFiber( } } } else { - scheduleCallbackForRoot(root, priorityLevel, expirationTime); + ensureRootIsScheduled(root); + schedulePendingInteractions(root, expirationTime); } if ( @@ -486,23 +489,36 @@ function markUpdateTimeFromFiberToRoot(fiber, expirationTime) { } if (root !== null) { - // Update the first and last pending expiration times in this root - const firstPendingTime = root.firstPendingTime; - if (expirationTime > firstPendingTime) { - root.firstPendingTime = expirationTime; - } - const lastPendingTime = root.lastPendingTime; - if (lastPendingTime === NoWork || expirationTime < lastPendingTime) { - root.lastPendingTime = expirationTime; - } - - // Mark that the root is no longer suspended at this time. - markRootUnsuspendedAtTime(root, expirationTime); + markRootUpdatedAtTime(root, expirationTime); } return root; } +function getNextRootExpirationTimeToWorkOn(root: FiberRoot): ExpirationTime { + // Determines the next expiration time that the root should render, taking + // into account levels that may be suspended, or levels that may have + // received a ping. + // + // "Pending" refers to any update that hasn't committed yet, including if it + // suspended. The "suspended" range is therefore a subset. + + const firstPendingTime = root.firstPendingTime; + if (!isRootSuspendedAtTime(root, firstPendingTime)) { + // The highest priority pending time is not suspended. Let's work on that. + return firstPendingTime; + } + + // If the first pending time is suspended, check if there's a lower priority + // pending level that we know about. Or check if we received a ping. Work + // on whichever is higher priority. + const lastPingedTime = root.lastPingedTime; + const nextKnownPendingLevel = root.nextKnownPendingLevel; + return lastPingedTime > nextKnownPendingLevel + ? lastPingedTime + : nextKnownPendingLevel; +} + // Use this function, along with runRootCallback, to ensure that only a single // callback per root is scheduled. It's still possible to call renderRoot // directly, but scheduling via this function helps avoid excessive callbacks. @@ -511,53 +527,79 @@ function markUpdateTimeFromFiberToRoot(fiber, expirationTime) { // should cancel the previous one. It also relies on commitRoot scheduling a // callback to render the next level, because that means we don't need a // separate callback per expiration time. -function scheduleCallbackForRoot( - root: FiberRoot, - priorityLevel: ReactPriorityLevel, - expirationTime: ExpirationTime, -) { - const existingCallbackExpirationTime = root.callbackExpirationTime; - if (existingCallbackExpirationTime < expirationTime) { - // New callback has higher priority than the existing one. - const existingCallbackNode = root.callbackNode; - if (existingCallbackNode !== null) { - cancelCallback(existingCallbackNode); - } - root.callbackExpirationTime = expirationTime; - - if (expirationTime === Sync) { - // Sync React callbacks are scheduled on a special internal queue - root.callbackNode = scheduleSyncCallback( - runRootCallback.bind( - null, - root, - renderRoot.bind(null, root, expirationTime), - ), - ); - } else { - let options = null; - if ( - !disableSchedulerTimeoutBasedOnReactExpirationTime && - expirationTime !== Never - ) { - let timeout = expirationTimeToMs(expirationTime) - now(); - options = {timeout}; - } +function ensureRootIsScheduled(root: FiberRoot) { + const expirationTime = getNextRootExpirationTimeToWorkOn(root); + if (expirationTime === NoWork) { + // Nothing to work on. + return; + } - root.callbackNode = scheduleCallback( - priorityLevel, - runRootCallback.bind( - null, - root, - renderRoot.bind(null, root, expirationTime), - ), - options, - ); + // TODO: If this is an update, we already read the current time. Pass the + // time as an argument. + const currentTime = requestCurrentTime(); + const priorityLevel = inferPriorityFromExpirationTime( + currentTime, + expirationTime, + ); + + // If there's an existing render task, confirm it has the correct priority and + // expiration time. Otherwise, we'll cancel it and schedule a new one. + const existingCallbackNode = root.callbackNode; + if (existingCallbackNode !== null) { + const existingCallbackPriority = root.callbackPriority; + const existingCallbackExpirationTime = root.callbackExpirationTime; + if ( + // Callback must have the exact same expiration time. + existingCallbackExpirationTime === expirationTime && + // Callback must have greater or equal priority. + existingCallbackPriority >= priorityLevel + ) { + // Existing callback is sufficient. + return; } + // Need to schedule a new task. + // TODO: Instead of scheduling a new task, we should be able to change the + // priority of the existing one. + cancelCallback(existingCallbackNode); } - // Associate the current interactions with this new root+priority. - schedulePendingInteractions(root, expirationTime); + root.callbackExpirationTime = expirationTime; + root.callbackPriority = priorityLevel; + + let callbackNode; + if (expirationTime === Sync) { + // Sync React callbacks are scheduled on a special internal queue + callbackNode = scheduleSyncCallback( + runRootCallback.bind( + null, + root, + renderRoot.bind(null, root, expirationTime), + ), + ); + } else if (disableSchedulerTimeoutBasedOnReactExpirationTime) { + callbackNode = scheduleCallback( + priorityLevel, + runRootCallback.bind( + null, + root, + renderRoot.bind(null, root, expirationTime), + ), + ); + } else { + callbackNode = scheduleCallback( + priorityLevel, + runRootCallback.bind( + null, + root, + renderRoot.bind(null, root, expirationTime), + ), + // Compute a task timeout based on the expiration time. This also affects + // ordering because tasks are processed in timeout order. + {timeout: expirationTimeToMs(expirationTime) - now()}, + ); + } + + root.callbackNode = callbackNode; } function runRootCallback(root, callback, isSync) { @@ -578,6 +620,7 @@ function runRootCallback(root, callback, isSync) { if (continuation === null && prevCallbackNode === root.callbackNode) { root.callbackNode = null; root.callbackExpirationTime = NoWork; + root.callbackPriority = NoPriority; } } } @@ -817,7 +860,8 @@ function renderRoot( // There's already a pending commit at this expiration time. // TODO: This is poorly factored. This case only exists for the // batch.commit() API. - return commitRoot.bind(null, root); + commitRoot(root); + return null; } flushPassiveEffects(); @@ -827,30 +871,6 @@ function renderRoot( if (root !== workInProgressRoot || expirationTime !== renderExpirationTime) { prepareFreshStack(root, expirationTime); startWorkOnPendingInteractions(root, expirationTime); - } else if (workInProgressRootExitStatus === RootSuspendedWithDelay) { - // We could've received an update at a lower priority while we yielded. - // We're suspended in a delayed state. Once we complete this render we're - // just going to try to recover at the pending time anyway so we might as - // well start doing that eagerly. - // - // Ideally we should be able to do this even for retries but we don't yet - // know if we're going to process an update which wants to commit earlier, - // and this path happens very early so it would happen too often. Instead, - // for that case, we'll wait until we complete. - if (workInProgressRootHasPendingPing) { - // We have a ping at this expiration. Let's restart to see if we get unblocked. - prepareFreshStack(root, expirationTime); - } else if (!isSync) { - // Check if there's work that isn't in the suspended range - const firstPendingTime = root.firstPendingTime; - if (!isRootSuspendedAtTime(root, firstPendingTime)) { - // There's a pending update that falls outside the range of - // suspended work. - if (firstPendingTime > expirationTime) { - return renderRoot.bind(null, root, firstPendingTime); - } - } - } } // If we have a work-in-progress fiber, it means there's still work to do @@ -921,6 +941,7 @@ function renderRoot( // boundary. prepareFreshStack(root, expirationTime); executionContext = prevExecutionContext; + markRootSuspendedAtTime(root, expirationTime); throw thrownValue; } @@ -1009,7 +1030,7 @@ function renderRoot( markRootSuspendedAtTime(root, expirationTime); const lastSuspendedTime = root.lastSuspendedTime; if (expirationTime === lastSuspendedTime) { - root.nextAfterSuspendedTime = getRemainingExpirationTime(finishedWork); + root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork); } flushSuspensePriorityWarningInDEV(); @@ -1048,11 +1069,11 @@ function renderRoot( return renderRoot.bind(null, root, expirationTime); } - const nextAfterSuspendedTime = root.nextAfterSuspendedTime; - if (nextAfterSuspendedTime !== NoWork) { + const nextKnownPendingLevel = root.nextKnownPendingLevel; + if (nextKnownPendingLevel !== NoWork) { // There's lower priority work. It might be unsuspended. Try rendering // at that level. - return renderRoot.bind(null, root, nextAfterSuspendedTime); + return renderRoot.bind(null, root, nextKnownPendingLevel); } if ( lastSuspendedTime !== NoWork && @@ -1079,7 +1100,7 @@ function renderRoot( markRootSuspendedAtTime(root, expirationTime); const lastSuspendedTime = root.lastSuspendedTime; if (expirationTime === lastSuspendedTime) { - root.nextAfterSuspendedTime = getRemainingExpirationTime(finishedWork); + root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork); } flushSuspensePriorityWarningInDEV(); @@ -1101,11 +1122,11 @@ function renderRoot( return renderRoot.bind(null, root, expirationTime); } - const nextAfterSuspendedTime = root.nextAfterSuspendedTime; - if (nextAfterSuspendedTime !== NoWork) { + const nextKnownPendingLevel = root.nextKnownPendingLevel; + if (nextKnownPendingLevel !== NoWork) { // There's lower priority work. It might be unsuspended. Try rendering // at that level. - return renderRoot.bind(null, root, nextAfterSuspendedTime); + return renderRoot.bind(null, root, nextKnownPendingLevel); } if ( lastSuspendedTime !== NoWork && @@ -1575,6 +1596,8 @@ function commitRootImpl(root, renderPriorityLevel) { // So we can clear these now to allow a new callback to be scheduled. root.callbackNode = null; root.callbackExpirationTime = NoWork; + root.callbackPriority = NoPriority; + root.nextKnownPendingLevel = NoWork; startCommitTimer(); @@ -1583,15 +1606,11 @@ function commitRootImpl(root, renderPriorityLevel) { const remainingExpirationTimeBeforeCommit = getRemainingExpirationTime( finishedWork, ); - root.firstPendingTime = remainingExpirationTimeBeforeCommit; - if (remainingExpirationTimeBeforeCommit < root.lastPendingTime) { - // This usually means we've finished all the work, but it can also happen - // when something gets downprioritized during render, like a hidden tree. - root.lastPendingTime = remainingExpirationTimeBeforeCommit; - } - - // Mark that the root is no longer suspended at the finished time - markRootUnsuspendedAtTime(root, expirationTime); + markRootFinishedAtTime( + root, + expirationTime, + remainingExpirationTimeBeforeCommit, + ); if (root === workInProgressRoot) { // We can reset these now that they are finished. @@ -1793,12 +1812,6 @@ function commitRootImpl(root, renderPriorityLevel) { // Check if there's remaining work on this root const remainingExpirationTime = root.firstPendingTime; if (remainingExpirationTime !== NoWork) { - const currentTime = requestCurrentTime(); - const priorityLevel = inferPriorityFromExpirationTime( - currentTime, - remainingExpirationTime, - ); - if (enableSchedulerTracing) { if (spawnedWorkDuringRender !== null) { const expirationTimes = spawnedWorkDuringRender; @@ -1812,8 +1825,8 @@ function commitRootImpl(root, renderPriorityLevel) { } } } - - scheduleCallbackForRoot(root, priorityLevel, remainingExpirationTime); + ensureRootIsScheduled(root); + schedulePendingInteractions(root, expirationTime); } else { // If there's no remaining work, we can clear the set of already failed // error boundaries. @@ -2101,7 +2114,8 @@ function captureCommitPhaseErrorOnRoot( enqueueUpdate(rootFiber, update); const root = markUpdateTimeFromFiberToRoot(rootFiber, Sync); if (root !== null) { - scheduleCallbackForRoot(root, ImmediatePriority, Sync); + ensureRootIsScheduled(root); + schedulePendingInteractions(root, Sync); } } @@ -2136,7 +2150,8 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) { enqueueUpdate(fiber, update); const root = markUpdateTimeFromFiberToRoot(fiber, Sync); if (root !== null) { - scheduleCallbackForRoot(root, ImmediatePriority, Sync); + ensureRootIsScheduled(root); + schedulePendingInteractions(root, Sync); } return; } @@ -2208,12 +2223,8 @@ export function pingSuspendedRoot( root.finishedWork = null; } - const currentTime = requestCurrentTime(); - const priorityLevel = inferPriorityFromExpirationTime( - currentTime, - suspendedTime, - ); - scheduleCallbackForRoot(root, priorityLevel, suspendedTime); + ensureRootIsScheduled(root); + schedulePendingInteractions(root, suspendedTime); } function retryTimedOutBoundary( @@ -2224,9 +2235,9 @@ function retryTimedOutBoundary( // previously was rendered in its fallback state. One of the promises that // suspended it has resolved, which means at least part of the tree was // likely unblocked. Try rendering again, at a new expiration time. - const currentTime = requestCurrentTime(); if (retryTime === Never) { const suspenseConfig = null; // Retries don't carry over the already committed update. + const currentTime = requestCurrentTime(); retryTime = computeExpirationForFiber( currentTime, boundaryFiber, @@ -2234,10 +2245,10 @@ function retryTimedOutBoundary( ); } // TODO: Special case idle priority? - const priorityLevel = inferPriorityFromExpirationTime(currentTime, retryTime); const root = markUpdateTimeFromFiberToRoot(boundaryFiber, retryTime); if (root !== null) { - scheduleCallbackForRoot(root, priorityLevel, retryTime); + ensureRootIsScheduled(root); + schedulePendingInteractions(root, retryTime); } } From fd253c9b0662c48f9b780c6d48bc884c99cace7f Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 9 Sep 2019 10:59:27 -0700 Subject: [PATCH 3/8] Don't bind expiration time to render callback This is a fragile pattern because there's only meant to be a single task per root, running at a single expiration time. Instead of binding the expiration time to the render task, or closing over it, we should determine the correct expiration time to work on using fields we store on the root object itself. This removes the "return a continuation" pattern from the `renderRoot` function. Continuation handling is now handled by the wrapper function, which I've renamed from `runRootCallback` to `performWorkOnRoot`. That function is merely an entry point to `renderRoot`, so I've also removed the callback argument. So to sum up, at at the beginning of each task, `performWorkOnRoot` determines which expiration time to work on, then calls `renderRoot`. And before exiting, it checks if it needs to schedule another task. --- .../src/ReactFiberExpirationTime.js | 3 + .../src/ReactFiberWorkLoop.js | 255 +++++++++--------- ...tIncrementalErrorHandling-test.internal.js | 50 ---- ...tSuspenseWithNoopRenderer-test.internal.js | 2 +- ...ReactIncrementalPerf-test.internal.js.snap | 5 +- 5 files changed, 128 insertions(+), 187 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberExpirationTime.js b/packages/react-reconciler/src/ReactFiberExpirationTime.js index 652ca93aed3be..e61ed56079e17 100644 --- a/packages/react-reconciler/src/ReactFiberExpirationTime.js +++ b/packages/react-reconciler/src/ReactFiberExpirationTime.js @@ -21,7 +21,10 @@ import { export type ExpirationTime = number; export const NoWork = 0; +// TODO: Think of a better name for Never. export const Never = 1; +// TODO: Use the Idle expiration time for idle state updates +export const Idle = 2; export const Sync = MAX_SIGNED_31_BIT_INT; export const Batched = Sync - 1; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 8f2915291845f..b39b7c36ee053 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -10,10 +10,7 @@ import type {Fiber} from './ReactFiber'; import type {FiberRoot} from './ReactFiberRoot'; import type {ExpirationTime} from './ReactFiberExpirationTime'; -import type { - ReactPriorityLevel, - SchedulerCallback, -} from './SchedulerWithReactIntegration'; +import type {ReactPriorityLevel} from './SchedulerWithReactIntegration'; import type {Interaction} from 'scheduler/src/Tracing'; import type {SuspenseConfig} from './ReactFiberSuspenseConfig'; import type {SuspenseState} from './ReactFiberSuspenseComponent'; @@ -117,6 +114,7 @@ import { inferPriorityFromExpirationTime, LOW_PRIORITY_EXPIRATION, Batched, + Idle, } from './ReactFiberExpirationTime'; import {beginWork as originalBeginWork} from './ReactFiberBeginWork'; import {completeWork} from './ReactFiberCompleteWork'; @@ -403,10 +401,7 @@ export function scheduleUpdateOnFiber( // This is a legacy edge case. The initial mount of a ReactDOM.render-ed // root inside of batchedUpdates should be synchronous, but layout updates // should be deferred until the end of the batch. - let callback = renderRoot(root, Sync, true); - while (callback !== null) { - callback = callback(true); - } + renderRoot(root, Sync, true); } else { ensureRootIsScheduled(root); schedulePendingInteractions(root, expirationTime); @@ -519,18 +514,21 @@ function getNextRootExpirationTimeToWorkOn(root: FiberRoot): ExpirationTime { : nextKnownPendingLevel; } -// Use this function, along with runRootCallback, to ensure that only a single -// callback per root is scheduled. It's still possible to call renderRoot -// directly, but scheduling via this function helps avoid excessive callbacks. -// It works by storing the callback node and expiration time on the root. When a -// new callback comes in, it compares the expiration time to determine if it -// should cancel the previous one. It also relies on commitRoot scheduling a -// callback to render the next level, because that means we don't need a -// separate callback per expiration time. +// Use this function to schedule a task for a root. There's only one task per +// root; if a task was already scheduled, we'll check to make sure the +// expiration time of the existing task is the same as the expiration time of +// the next level that the root has work on. This function is called on every +// update, and right before exiting a task. function ensureRootIsScheduled(root: FiberRoot) { const expirationTime = getNextRootExpirationTimeToWorkOn(root); + const existingCallbackNode = root.callbackNode; if (expirationTime === NoWork) { - // Nothing to work on. + // There's nothing to work on. + if (existingCallbackNode !== null) { + root.callbackNode = null; + root.callbackExpirationTime = NoWork; + root.callbackPriority = NoPriority; + } return; } @@ -544,7 +542,6 @@ function ensureRootIsScheduled(root: FiberRoot) { // If there's an existing render task, confirm it has the correct priority and // expiration time. Otherwise, we'll cancel it and schedule a new one. - const existingCallbackNode = root.callbackNode; if (existingCallbackNode !== null) { const existingCallbackPriority = root.callbackPriority; const existingCallbackExpirationTime = root.callbackExpirationTime; @@ -569,30 +566,16 @@ function ensureRootIsScheduled(root: FiberRoot) { let callbackNode; if (expirationTime === Sync) { // Sync React callbacks are scheduled on a special internal queue - callbackNode = scheduleSyncCallback( - runRootCallback.bind( - null, - root, - renderRoot.bind(null, root, expirationTime), - ), - ); + callbackNode = scheduleSyncCallback(performWorkOnRoot.bind(null, root)); } else if (disableSchedulerTimeoutBasedOnReactExpirationTime) { callbackNode = scheduleCallback( priorityLevel, - runRootCallback.bind( - null, - root, - renderRoot.bind(null, root, expirationTime), - ), + performWorkOnRoot.bind(null, root), ); } else { callbackNode = scheduleCallback( priorityLevel, - runRootCallback.bind( - null, - root, - renderRoot.bind(null, root, expirationTime), - ), + performWorkOnRoot.bind(null, root), // Compute a task timeout based on the expiration time. This also affects // ordering because tasks are processed in timeout order. {timeout: expirationTimeToMs(expirationTime) - now()}, @@ -602,27 +585,43 @@ function ensureRootIsScheduled(root: FiberRoot) { root.callbackNode = callbackNode; } -function runRootCallback(root, callback, isSync) { - const prevCallbackNode = root.callbackNode; - let continuation = null; - try { - continuation = callback(isSync); - if (continuation !== null) { - return runRootCallback.bind(null, root, continuation); - } else { - return null; +// This is the entry point for every concurrent task. +function performWorkOnRoot(root, isSync) { + // Determine the next expiration time to work on, using the fields stored + // on the root. + let expirationTime = getNextRootExpirationTimeToWorkOn(root); + if (expirationTime !== NoWork) { + if (expirationTime !== Sync) { + // Since we know we're in a React event, we can clear the current + // event time. The next update will compute a new event time. + currentEventTime = NoWork; + + // An async update expired. There may be other expired updates on + // this root. + if (isSync) { + const currentTime = requestCurrentTime(); + if (currentTime < expirationTime) { + // Render all the expired work in a single batch. + expirationTime = currentTime; + } + } } - } finally { - // If the callback exits without returning a continuation, remove the - // corresponding callback node from the root. Unless the callback node - // has changed, which implies that it was already cancelled by a high - // priority update. - if (continuation === null && prevCallbackNode === root.callbackNode) { - root.callbackNode = null; - root.callbackExpirationTime = NoWork; - root.callbackPriority = NoPriority; + + const originalCallbackNode = root.callbackNode; + try { + renderRoot(root, expirationTime, isSync); + } finally { + // Before exiting, make sure there's a callback scheduled for the + // pending level. + ensureRootIsScheduled(root); + if (root.callbackNode === originalCallbackNode) { + // The task node scheduled for this root is the same one that's + // currently executed. Need to return a continuation. + return performWorkOnRoot.bind(null, root); + } } } + return null; } export function flushRoot(root: FiberRoot, expirationTime: ExpirationTime) { @@ -633,7 +632,10 @@ export function flushRoot(root: FiberRoot, expirationTime: ExpirationTime) { 'means you attempted to commit from inside a lifecycle method.', ); } - scheduleSyncCallback(renderRoot.bind(null, root, expirationTime)); + scheduleSyncCallback(() => { + renderRoot(root, expirationTime, true); + return null; + }); flushSyncCallbackQueue(); } @@ -702,7 +704,10 @@ function flushPendingDiscreteUpdates() { const roots = rootsWithPendingDiscreteUpdates; rootsWithPendingDiscreteUpdates = null; roots.forEach((expirationTime, root) => { - scheduleSyncCallback(renderRoot.bind(null, root, expirationTime)); + scheduleSyncCallback(() => { + renderRoot(root, expirationTime, true); + return null; + }); }); // Now flush the immediate queue. flushSyncCallbackQueue(); @@ -850,7 +855,7 @@ function renderRoot( root: FiberRoot, expirationTime: ExpirationTime, isSync: boolean, -): SchedulerCallback | null { +): void { invariant( (executionContext & (RenderContext | CommitContext)) === NoContext, 'Should not already be working.', @@ -861,7 +866,7 @@ function renderRoot( // TODO: This is poorly factored. This case only exists for the // batch.commit() API. commitRoot(root); - return null; + return; } flushPassiveEffects(); @@ -894,32 +899,6 @@ function renderRoot( startWorkLoopTimer(workInProgress); - // TODO: Fork renderRoot into renderRootSync and renderRootAsync - if (isSync) { - if (expirationTime !== Sync) { - // An async update expired. There may be other expired updates on - // this root. We should render all the expired work in a - // single batch. - const currentTime = requestCurrentTime(); - if (currentTime < expirationTime) { - // Restart at the current time. - executionContext = prevExecutionContext; - resetContextDependencies(); - ReactCurrentDispatcher.current = prevDispatcher; - if (enableSchedulerTracing) { - __interactionsRef.current = ((prevInteractions: any): Set< - Interaction, - >); - } - return renderRoot.bind(null, root, currentTime); - } - } - } else { - // Since we know we're in a React event, we can clear the current - // event time. The next update will compute a new event time. - currentEventTime = NoWork; - } - do { try { if (isSync) { @@ -972,9 +951,9 @@ function renderRoot( } if (workInProgress !== null) { - // There's still work left over. Return a continuation. + // There's still work left over. Exit without committing. stopInterruptedWorkLoopTimer(); - return renderRoot.bind(null, root, expirationTime); + return; } } @@ -991,7 +970,8 @@ function renderRoot( // This root has a lock that prevents it from committing. Exit. If we begin // work on the root again, without any intervening updates, it will finish // without doing additional work. - return null; + markRootSuspendedAtTime(root, expirationTime); + return; } // Set this to null to indicate there's no in-progress render. @@ -1005,26 +985,18 @@ function renderRoot( // but eslint doesn't know about invariant, so it complains if I do. // eslint-disable-next-line no-fallthrough case RootErrored: { - // An error was thrown. First check if there is lower priority work - // scheduled on this root. - const lastPendingTime = root.lastPendingTime; - if (lastPendingTime < expirationTime) { - // There's lower priority work. Before raising the error, try rendering - // at the lower priority to see if it fixes it. Use a continuation to - // maintain the existing priority and position in the queue. - return renderRoot.bind(null, root, lastPendingTime); - } - if (!isSync) { - // If we're rendering asynchronously, it's possible the error was - // caused by tearing due to a mutation during an event. Try rendering - // one more time without yiedling to events. - prepareFreshStack(root, expirationTime); - scheduleSyncCallback(renderRoot.bind(null, root, expirationTime)); - return null; + if (!isSync && expirationTime !== Idle) { + // If this was an async render, the error may have happened due to + // a mutation in a concurrent event. Try rendering one more time, + // synchronously, to see if the error goes away. If there are lower + // priority updates, let's include those, too, in case they fix the + // inconsistency. Render at Idle to include all updates. + renderRoot(root, Idle, true); + return; } - // If we're already rendering synchronously, commit the root in its - // errored state. - return commitRoot.bind(null, root); + // Commit the root in its errored state. + commitRoot(root); + return; } case RootSuspended: { markRootSuspendedAtTime(root, expirationTime); @@ -1063,26 +1035,31 @@ function renderRoot( // Don't bother with a very short suspense time. if (msUntilTimeout > 10) { if (workInProgressRootHasPendingPing) { - // This render was pinged but we didn't get to restart earlier so try - // restarting now instead. - prepareFreshStack(root, expirationTime); - return renderRoot.bind(null, root, expirationTime); + const lastPingedTime = root.lastPingedTime; + if (lastPingedTime === NoWork || lastPingedTime >= expirationTime) { + // This render was pinged but we didn't get to restart earlier so + // try restarting now instead. + root.lastPingedTime = expirationTime; + prepareFreshStack(root, expirationTime); + return; + } } - const nextKnownPendingLevel = root.nextKnownPendingLevel; - if (nextKnownPendingLevel !== NoWork) { - // There's lower priority work. It might be unsuspended. Try rendering - // at that level. - return renderRoot.bind(null, root, nextKnownPendingLevel); + const nextTime = getNextRootExpirationTimeToWorkOn(root); + if (nextTime !== NoWork && nextTime !== expirationTime) { + // There's additional work on this root. + return; } if ( lastSuspendedTime !== NoWork && lastSuspendedTime !== expirationTime ) { - // We should prefer to render the fallback of at the last - // suspended level. - return renderRoot.bind(null, root, lastSuspendedTime); + // We should prefer to render the fallback of at the last suspended + // level. Ping the last suspended level to try rendering it again. + root.lastPingedTime = lastSuspendedTime; + return; } + // The render is suspended, it hasn't timed out, and there's no lower // priority work to do. Instead of committing the fallback // immediately, wait for more data to arrive. @@ -1090,11 +1067,12 @@ function renderRoot( commitRoot.bind(null, root), msUntilTimeout, ); - return null; + return; } } // The work expired. Commit immediately. - return commitRoot.bind(null, root); + commitRoot(root); + return; } case RootSuspendedWithDelay: { markRootSuspendedAtTime(root, expirationTime); @@ -1116,25 +1094,29 @@ function renderRoot( // We're suspended in a state that should be avoided. We'll try to avoid committing // it for as long as the timeouts let us. if (workInProgressRootHasPendingPing) { - // This render was pinged but we didn't get to restart earlier so try - // restarting now instead. - prepareFreshStack(root, expirationTime); - return renderRoot.bind(null, root, expirationTime); + const lastPingedTime = root.lastPingedTime; + if (lastPingedTime === NoWork || lastPingedTime >= expirationTime) { + // This render was pinged but we didn't get to restart earlier so + // try restarting now instead. + root.lastPingedTime = expirationTime; + prepareFreshStack(root, expirationTime); + return; + } } - const nextKnownPendingLevel = root.nextKnownPendingLevel; - if (nextKnownPendingLevel !== NoWork) { - // There's lower priority work. It might be unsuspended. Try rendering - // at that level. - return renderRoot.bind(null, root, nextKnownPendingLevel); + const nextTime = getNextRootExpirationTimeToWorkOn(root); + if (nextTime !== NoWork && nextTime !== expirationTime) { + // There's additional work on this root. + return; } if ( lastSuspendedTime !== NoWork && lastSuspendedTime !== expirationTime ) { - // We should prefer to render the fallback of at the last - // suspended level. - return renderRoot.bind(null, root, lastSuspendedTime); + // We should prefer to render the fallback of at the last suspended + // level. Ping the last suspended level to try rendering it again. + root.lastPingedTime = lastSuspendedTime; + return; } let msUntilTimeout; @@ -1182,11 +1164,12 @@ function renderRoot( commitRoot.bind(null, root), msUntilTimeout, ); - return null; + return; } } // The work expired. Commit immediately. - return commitRoot.bind(null, root); + commitRoot(root); + return; } case RootCompleted: { // The work completed. Ready to commit. @@ -1210,14 +1193,16 @@ function renderRoot( workInProgressRootCanSuspendUsingConfig, ); if (msUntilTimeout > 10) { + markRootSuspendedAtTime(root, expirationTime); root.timeoutHandle = scheduleTimeout( commitRoot.bind(null, root), msUntilTimeout, ); - return null; + return; } } - return commitRoot.bind(null, root); + commitRoot(root); + return; } default: { invariant(false, 'Unknown root exit status.'); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 9020e3d739cb9..27d6dca7858b8 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -256,56 +256,6 @@ describe('ReactIncrementalErrorHandling', () => { expect(ReactNoop.getChildren()).toEqual([span('Everything is fine.')]); }); - it('on error, retries at a lower priority using the expiration of higher priority', () => { - class Parent extends React.Component { - state = {hideChild: false}; - componentDidUpdate() { - Scheduler.unstable_yieldValue('commit: ' + this.state.hideChild); - } - render() { - if (this.state.hideChild) { - Scheduler.unstable_yieldValue('(empty)'); - return ; - } - return ; - } - } - - function Child(props) { - if (props.isBroken) { - Scheduler.unstable_yieldValue('Error!'); - throw new Error('Error!'); - } - Scheduler.unstable_yieldValue('Child'); - return ; - } - - // Initial mount - const parent = React.createRef(null); - ReactNoop.render(); - expect(Scheduler).toFlushAndYield(['Child']); - expect(ReactNoop.getChildren()).toEqual([span('Child')]); - - // Schedule a low priority update to hide the child - parent.current.setState({hideChild: true}); - - // Before the low priority update is flushed, synchronously trigger an - // error in the child. - ReactNoop.flushSync(() => { - ReactNoop.render(); - }); - expect(Scheduler).toHaveYielded([ - // First the sync update triggers an error - 'Error!', - // Because there's a pending low priority update, we restart at the - // lower priority. This hides the children, suppressing the error. - '(empty)', - // Now the tree can commit. - 'commit: true', - ]); - expect(ReactNoop.getChildren()).toEqual([span('(empty)')]); - }); - it('retries one more time before handling error', () => { function BadRender() { Scheduler.unstable_yieldValue('BadRender'); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 60dfcb0a55983..97f0f480fbeba 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -494,7 +494,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { return ; } return ( - + ); diff --git a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap index 8667688a0b8d8..b438a6f7e6789 100644 --- a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap +++ b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap @@ -422,7 +422,10 @@ exports[`ReactDebugFiberPerf warns if an in-progress update is interrupted 1`] = `; exports[`ReactDebugFiberPerf warns if async work expires (starvation) 1`] = ` -"⚛ (Committing Changes) +"⚛ (React Tree Reconciliation: Completed Root) + ⚛ Foo [mount] + +⚛ (Committing Changes) ⚛ (Committing Snapshot Effects: 0 Total) ⚛ (Committing Host Effects: 1 Total) ⚛ (Calling Lifecycle Methods: 0 Total) From 052ee96afa4d7cfc1b817074afe6f652a003369b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 9 Sep 2019 17:04:32 -0700 Subject: [PATCH 4/8] Update error recovery test to match new semantics --- ...tIncrementalErrorHandling-test.internal.js | 93 +++++++++++++++++-- 1 file changed, 87 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 27d6dca7858b8..094758594de45 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -243,11 +243,20 @@ describe('ReactIncrementalErrorHandling', () => { // This update is in a separate batch ReactNoop.render(, onCommit); - expect(Scheduler).toFlushAndYield([ + expect(Scheduler).toFlushAndYieldThrough([ // The first render fails. But because there's a lower priority pending // update, it doesn't throw. 'error', - // Now we retry at the lower priority. This time it succeeds. + ]); + + // React will try to recover by rendering all the pending updates in a + // single batch, synchronously. This time it succeeds. + // + // This tells Scheduler to render a single unit of work. Because the render + // to recover from the error is synchronous, this should be enough to + // finish the rest of the work. + Scheduler.unstable_flushNumberOfYields(1); + expect(Scheduler).toHaveYielded([ 'success', // Nothing commits until the second update completes. 'commit', @@ -256,6 +265,82 @@ describe('ReactIncrementalErrorHandling', () => { expect(ReactNoop.getChildren()).toEqual([span('Everything is fine.')]); }); + it('does not include offscreen work when retrying after an error', () => { + function App(props) { + if (props.isBroken) { + Scheduler.unstable_yieldValue('error'); + throw new Error('Oops!'); + } + Scheduler.unstable_yieldValue('success'); + return ( + <> + Everything is fine + + + ); + } + + function onCommit() { + Scheduler.unstable_yieldValue('commit'); + } + + function interrupt() { + ReactNoop.flushSync(() => { + ReactNoop.renderToRootWithID(null, 'other-root'); + }); + } + + ReactNoop.render(, onCommit); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['error']); + interrupt(); + + expect(ReactNoop).toMatchRenderedOutput(null); + + // This update is in a separate batch + ReactNoop.render(, onCommit); + + expect(Scheduler).toFlushAndYieldThrough([ + // The first render fails. But because there's a lower priority pending + // update, it doesn't throw. + 'error', + ]); + + // React will try to recover by rendering all the pending updates in a + // single batch, synchronously. This time it succeeds. + // + // This tells Scheduler to render a single unit of work. Because the render + // to recover from the error is synchronous, this should be enough to + // finish the rest of the work. + Scheduler.unstable_flushNumberOfYields(1); + expect(Scheduler).toHaveYielded([ + 'success', + // Nothing commits until the second update completes. + 'commit', + 'commit', + ]); + // This should not include the offscreen content + expect(ReactNoop).toMatchRenderedOutput( + <> + Everything is fine +