From 4c78ac0b9df88edec73492f92f09d5438dd74c4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 9 Apr 2019 18:59:39 -0700 Subject: [PATCH] Track Event Time as the Start Time for Suspense (#15358) * Track the earliest event time in this render Rebase * Track the time of the fallback being shown as an event time When we switch back from fallback to content, we made progress and we track the time from when we showed the fallback in the first place as the last time we made progress. * Don't retry if synchronous * Only suspend when we switch to fallback mode This ensures that we don't resuspend unnecessarily if we're just retrying the same exact boundary again. We can still unnecessarily suspend for nested boundaries. * Rename timedOutAt to fallbackExpirationTime * Account for suspense in devtools suspense test --- .../ReactDevToolsHooksIntegration-test.js | 3 + .../src/ReactFiberBeginWork.js | 3 +- .../src/ReactFiberCommitWork.js | 13 +- .../src/ReactFiberCompleteWork.js | 66 ++++++++--- .../src/ReactFiberExpirationTime.js | 8 ++ .../react-reconciler/src/ReactFiberHooks.js | 11 ++ .../src/ReactFiberPendingPriority.js | 17 --- .../src/ReactFiberScheduler.js | 10 +- .../src/ReactFiberScheduler.new.js | 111 ++++++++---------- .../src/ReactFiberScheduler.old.js | 78 +++++------- .../src/ReactFiberSuspenseComponent.js | 2 +- .../src/ReactFiberUnwindWork.js | 75 +----------- .../react-reconciler/src/ReactUpdateQueue.js | 14 ++- .../ReactSuspensePlaceholder-test.internal.js | 17 ++- 14 files changed, 200 insertions(+), 228 deletions(-) diff --git a/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js index f7ea7ffebc031..81d91e7455aff 100644 --- a/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js +++ b/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js @@ -251,7 +251,10 @@ describe('React hooks DevTools integration', () => { , {unstable_isConcurrent: true}, ); + expect(Scheduler).toFlushAndYield([]); + // Ensure we timeout any suspense time. + jest.advanceTimersByTime(1000); const fiber = renderer.root._currentFiber().child; if (__DEV__) { // First render was locked diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 3c66c38e229d8..8659e010a9847 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1413,7 +1413,8 @@ function updateSuspenseComponent( // Something in this boundary's subtree already suspended. Switch to // rendering the fallback children. nextState = { - timedOutAt: nextState !== null ? nextState.timedOutAt : NoWork, + fallbackExpirationTime: + nextState !== null ? nextState.fallbackExpirationTime : NoWork, }; nextDidTimeout = true; workInProgress.effectTag &= ~DidCapture; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 1c0ed5b82cebd..3bf04ea9c11a7 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -63,7 +63,10 @@ import invariant from 'shared/invariant'; import warningWithoutStack from 'shared/warningWithoutStack'; import warning from 'shared/warning'; -import {NoWork} from './ReactFiberExpirationTime'; +import { + NoWork, + computeAsyncExpirationNoBucket, +} from './ReactFiberExpirationTime'; import {onCommitUnmount} from './ReactFiberDevToolsHook'; import {startPhaseTimer, stopPhaseTimer} from './ReactDebugFiberPerf'; import {getStackByFiberInDevAndProd} from './ReactCurrentFiber'; @@ -1272,11 +1275,15 @@ function commitSuspenseComponent(finishedWork: Fiber) { } else { newDidTimeout = true; primaryChildParent = finishedWork.child; - if (newState.timedOutAt === NoWork) { + if (newState.fallbackExpirationTime === NoWork) { // If the children had not already timed out, record the time. // This is used to compute the elapsed time during subsequent // attempts to render the children. - newState.timedOutAt = requestCurrentTime(); + // We model this as a normal pri expiration time since that's + // how we infer start time for updates. + newState.fallbackExpirationTime = computeAsyncExpirationNoBucket( + requestCurrentTime(), + ); } } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 62915dd7a6a18..41b4ba71c4dfc 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -18,6 +18,7 @@ import type { ChildSet, } from './ReactFiberHostConfig'; import type {ReactEventComponentInstance} from 'shared/ReactTypes'; +import type {SuspenseState} from './ReactFiberSuspenseComponent'; import { IndeterminateComponent, @@ -42,6 +43,7 @@ import { EventComponent, EventTarget, } from 'shared/ReactWorkTags'; +import {ConcurrentMode, NoContext} from './ReactTypeOfMode'; import { Placement, Ref, @@ -92,6 +94,7 @@ import { enableSuspenseServerRenderer, enableEventAPI, } from 'shared/ReactFeatureFlags'; +import {markRenderEventTime, renderDidSuspend} from './ReactFiberScheduler'; function markUpdate(workInProgress: Fiber) { // Tag the fiber with an update effect. This turns a Placement into @@ -665,7 +668,7 @@ function completeWork( case ForwardRef: break; case SuspenseComponent: { - const nextState = workInProgress.memoizedState; + const nextState: null | SuspenseState = workInProgress.memoizedState; if ((workInProgress.effectTag & DidCapture) !== NoEffect) { // Something suspended. Re-render with the fallback children. workInProgress.expirationTime = renderExpirationTime; @@ -674,34 +677,58 @@ function completeWork( } const nextDidTimeout = nextState !== null; - const prevDidTimeout = current !== null && current.memoizedState !== null; - + let prevDidTimeout = false; if (current === null) { // In cases where we didn't find a suitable hydration boundary we never // downgraded this to a DehydratedSuspenseComponent, but we still need to // pop the hydration state since we might be inside the insertion tree. popHydrationState(workInProgress); - } else if (!nextDidTimeout && prevDidTimeout) { - // We just switched from the fallback to the normal children. Delete - // the fallback. - // TODO: Would it be better to store the fallback fragment on - // the stateNode during the begin phase? - const currentFallbackChild: Fiber | null = (current.child: any).sibling; - if (currentFallbackChild !== null) { - // Deletions go at the beginning of the return fiber's effect list - const first = workInProgress.firstEffect; - if (first !== null) { - workInProgress.firstEffect = currentFallbackChild; - currentFallbackChild.nextEffect = first; - } else { - workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChild; - currentFallbackChild.nextEffect = null; + } else { + const prevState: null | SuspenseState = current.memoizedState; + prevDidTimeout = prevState !== null; + if (!nextDidTimeout && prevState !== null) { + // We just switched from the fallback to the normal children. + + // Mark the event time of the switching from fallback to normal children, + // based on the start of when we first showed the fallback. This time + // was given a normal pri expiration time at the time it was shown. + const fallbackExpirationTimeExpTime: ExpirationTime = + prevState.fallbackExpirationTime; + markRenderEventTime(fallbackExpirationTimeExpTime); + + // Delete the fallback. + // TODO: Would it be better to store the fallback fragment on + // the stateNode during the begin phase? + const currentFallbackChild: Fiber | null = (current.child: any) + .sibling; + if (currentFallbackChild !== null) { + // Deletions go at the beginning of the return fiber's effect list + const first = workInProgress.firstEffect; + if (first !== null) { + workInProgress.firstEffect = currentFallbackChild; + currentFallbackChild.nextEffect = first; + } else { + workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChild; + currentFallbackChild.nextEffect = null; + } + currentFallbackChild.effectTag = Deletion; } - currentFallbackChild.effectTag = Deletion; + } + } + + if (nextDidTimeout && !prevDidTimeout) { + // If this subtreee is running in concurrent mode we can suspend, + // otherwise we won't suspend. + // TODO: This will still suspend a synchronous tree if anything + // in the concurrent tree already suspended during this render. + // This is a known bug. + if ((workInProgress.mode & ConcurrentMode) !== NoContext) { + renderDidSuspend(); } } if (supportsPersistence) { + // TODO: Only schedule updates if not prevDidTimeout. if (nextDidTimeout) { // If this boundary just timed out, schedule an effect to attach a // retry listener to the proimse. This flag is also used to hide the @@ -710,6 +737,7 @@ function completeWork( } } if (supportsMutation) { + // TODO: Only schedule updates if these values are non equal, i.e. it changed. if (nextDidTimeout || prevDidTimeout) { // If this boundary just timed out, schedule an effect to attach a // retry listener to the proimse. This flag is also used to hide the diff --git a/packages/react-reconciler/src/ReactFiberExpirationTime.js b/packages/react-reconciler/src/ReactFiberExpirationTime.js index f75ca42a74a57..e28e888545a29 100644 --- a/packages/react-reconciler/src/ReactFiberExpirationTime.js +++ b/packages/react-reconciler/src/ReactFiberExpirationTime.js @@ -70,6 +70,14 @@ export function computeAsyncExpiration( ); } +// Same as computeAsyncExpiration but without the bucketing logic. This is +// used to compute timestamps instead of actual expiration times. +export function computeAsyncExpirationNoBucket( + currentTime: ExpirationTime, +): ExpirationTime { + return currentTime - LOW_PRIORITY_EXPIRATION / UNIT_SIZE; +} + // We intentionally set a higher expiration time for interactive updates in // dev than in production. // diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 975d4ada3f7df..b5b7f9dd79f7a 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -34,6 +34,7 @@ import { flushPassiveEffects, requestCurrentTime, warnIfNotCurrentlyActingUpdatesInDev, + markRenderEventTime, } from './ReactFiberScheduler'; import invariant from 'shared/invariant'; @@ -718,6 +719,16 @@ function updateReducer( remainingExpirationTime = updateExpirationTime; } } else { + // This update does have sufficient priority. + + // Mark the event time of this update as relevant to this render pass. + // TODO: This should ideally use the true event time of this update rather than + // its priority which is a derived and not reverseable value. + // TODO: We should skip this update if it was already committed but currently + // we have no way of detecting the difference between a committed and suspended + // update here. + markRenderEventTime(updateExpirationTime); + // Process this update. if (update.eagerReducer === reducer) { // If this update was processed eagerly, and its reducer matches the diff --git a/packages/react-reconciler/src/ReactFiberPendingPriority.js b/packages/react-reconciler/src/ReactFiberPendingPriority.js index 3e3a038ef58ef..43e241d3a2683 100644 --- a/packages/react-reconciler/src/ReactFiberPendingPriority.js +++ b/packages/react-reconciler/src/ReactFiberPendingPriority.js @@ -219,23 +219,6 @@ function clearPing(root, completedTime) { } } -export function findEarliestOutstandingPriorityLevel( - root: FiberRoot, - renderExpirationTime: ExpirationTime, -): ExpirationTime { - let earliestExpirationTime = renderExpirationTime; - - const earliestPendingTime = root.earliestPendingTime; - const earliestSuspendedTime = root.earliestSuspendedTime; - if (earliestPendingTime > earliestExpirationTime) { - earliestExpirationTime = earliestPendingTime; - } - if (earliestSuspendedTime > earliestExpirationTime) { - earliestExpirationTime = earliestSuspendedTime; - } - return earliestExpirationTime; -} - export function didExpireAtExpirationTime( root: FiberRoot, currentTime: ExpirationTime, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index fb94b012ef904..f58aaa7913c9a 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -14,6 +14,7 @@ import { computeExpirationForFiber as computeExpirationForFiber_old, captureCommitPhaseError as captureCommitPhaseError_old, onUncaughtError as onUncaughtError_old, + markRenderEventTime as markRenderEventTime_old, renderDidSuspend as renderDidSuspend_old, renderDidError as renderDidError_old, pingSuspendedRoot as pingSuspendedRoot_old, @@ -34,7 +35,6 @@ import { computeUniqueAsyncExpiration as computeUniqueAsyncExpiration_old, flushPassiveEffects as flushPassiveEffects_old, warnIfNotCurrentlyActingUpdatesInDev as warnIfNotCurrentlyActingUpdatesInDev_old, - inferStartTimeFromExpirationTime as inferStartTimeFromExpirationTime_old, } from './ReactFiberScheduler.old'; import { @@ -42,6 +42,7 @@ import { computeExpirationForFiber as computeExpirationForFiber_new, captureCommitPhaseError as captureCommitPhaseError_new, onUncaughtError as onUncaughtError_new, + markRenderEventTime as markRenderEventTime_new, renderDidSuspend as renderDidSuspend_new, renderDidError as renderDidError_new, pingSuspendedRoot as pingSuspendedRoot_new, @@ -62,7 +63,6 @@ import { computeUniqueAsyncExpiration as computeUniqueAsyncExpiration_new, flushPassiveEffects as flushPassiveEffects_new, warnIfNotCurrentlyActingUpdatesInDev as warnIfNotCurrentlyActingUpdatesInDev_new, - inferStartTimeFromExpirationTime as inferStartTimeFromExpirationTime_new, } from './ReactFiberScheduler.new'; export const requestCurrentTime = enableNewScheduler @@ -77,6 +77,9 @@ export const captureCommitPhaseError = enableNewScheduler export const onUncaughtError = enableNewScheduler ? onUncaughtError_new : onUncaughtError_old; +export const markRenderEventTime = enableNewScheduler + ? markRenderEventTime_new + : markRenderEventTime_old; export const renderDidSuspend = enableNewScheduler ? renderDidSuspend_new : renderDidSuspend_old; @@ -133,9 +136,6 @@ export const flushPassiveEffects = enableNewScheduler export const warnIfNotCurrentlyActingUpdatesInDev = enableNewScheduler ? warnIfNotCurrentlyActingUpdatesInDev_new : warnIfNotCurrentlyActingUpdatesInDev_old; -export const inferStartTimeFromExpirationTime = enableNewScheduler - ? inferStartTimeFromExpirationTime_new - : inferStartTimeFromExpirationTime_old; export type Thenable = { then(resolve: () => mixed, reject?: () => mixed): void | Thenable, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.new.js b/packages/react-reconciler/src/ReactFiberScheduler.new.js index 26a2e12ed1d90..669f121c5433f 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.new.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.new.js @@ -194,7 +194,11 @@ let workInProgress: Fiber | null = null; let renderExpirationTime: ExpirationTime = NoWork; // Whether to root completed, errored, suspended, etc. let workInProgressRootExitStatus: RootExitStatus = RootIncomplete; -let workInProgressRootAbsoluteTimeoutMs: number = -1; +// Most recent event time among processed updates during this render. +// This is conceptually a time stamp but expressed in terms of an ExpirationTime +// because we deal mostly with expiration times in the hot path, so this avoids +// the conversion happening in the hot path. +let workInProgressRootMostRecentEventTime: ExpirationTime = Sync; let nextEffect: Fiber | null = null; let hasUncaughtError = false; @@ -678,7 +682,7 @@ function prepareFreshStack(root, expirationTime) { workInProgress = createWorkInProgress(root.current, null, expirationTime); renderExpirationTime = expirationTime; workInProgressRootExitStatus = RootIncomplete; - workInProgressRootAbsoluteTimeoutMs = -1; + workInProgressRootMostRecentEventTime = Sync; if (__DEV__) { ReactStrictModeWarnings.discardPendingWarnings(); @@ -878,26 +882,30 @@ function renderRoot( return commitRoot.bind(null, root, expirationTime); } case RootSuspended: { - const lastPendingTime = root.lastPendingTime; - if (root.lastPendingTime < expirationTime) { - // There's lower priority work. It might be unsuspended. Try rendering - // at that level. - return renderRoot.bind(null, root, lastPendingTime); - } if (!isSync) { - const msUntilTimeout = computeMsUntilTimeout( - root, - workInProgressRootAbsoluteTimeoutMs, - ); - if (msUntilTimeout > 0) { - // 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. - root.timeoutHandle = scheduleTimeout( - commitRoot.bind(null, root, expirationTime), - msUntilTimeout, + const lastPendingTime = root.lastPendingTime; + if (root.lastPendingTime < expirationTime) { + // There's lower priority work. It might be unsuspended. Try rendering + // at that level. + return renderRoot.bind(null, root, lastPendingTime); + } + // If workInProgressRootMostRecentEventTime is Sync, that means we didn't + // track any event times. That can happen if we retried but nothing switched + // from fallback to content. There's no reason to delay doing no work. + if (workInProgressRootMostRecentEventTime !== Sync) { + const msUntilTimeout = computeMsUntilTimeout( + workInProgressRootMostRecentEventTime, ); - return null; + if (msUntilTimeout > 0) { + // 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. + root.timeoutHandle = scheduleTimeout( + commitRoot.bind(null, root, expirationTime), + msUntilTimeout, + ); + return null; + } } } // The work expired. Commit immediately. @@ -913,20 +921,15 @@ function renderRoot( } } -export function renderDidSuspend( - root: FiberRoot, - absoluteTimeoutMs: number, - // TODO: Don't need this argument anymore - suspendedTime: ExpirationTime, -) { - if ( - absoluteTimeoutMs >= 0 && - workInProgressRootAbsoluteTimeoutMs < absoluteTimeoutMs - ) { - workInProgressRootAbsoluteTimeoutMs = absoluteTimeoutMs; - if (workInProgressRootExitStatus === RootIncomplete) { - workInProgressRootExitStatus = RootSuspended; - } +export function markRenderEventTime(expirationTime: ExpirationTime): void { + if (expirationTime < workInProgressRootMostRecentEventTime) { + workInProgressRootMostRecentEventTime = expirationTime; + } +} + +export function renderDidSuspend(): void { + if (workInProgressRootExitStatus === RootIncomplete) { + workInProgressRootExitStatus = RootSuspended; } } @@ -939,6 +942,13 @@ export function renderDidError() { } } +function inferTimeFromExpirationTime(expirationTime: ExpirationTime): number { + // We don't know exactly when the update was scheduled, but we can infer an + // approximate start time from the expiration time. + const earliestExpirationTimeMs = expirationTimeToMs(expirationTime); + return earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION + initialTimeMs; +} + function workLoopSync() { // Already timed out, so perform work without checking if we need to yield. while (workInProgress !== null) { @@ -1805,37 +1815,20 @@ export function resolveRetryThenable(boundaryFiber: Fiber, thenable: Thenable) { retryTimedOutBoundary(boundaryFiber); } -export function inferStartTimeFromExpirationTime( - root: FiberRoot, - expirationTime: ExpirationTime, -) { - // We don't know exactly when the update was scheduled, but we can infer an - // approximate start time from the expiration time. - const earliestExpirationTimeMs = expirationTimeToMs(root.firstPendingTime); - // TODO: Track this on the root instead. It's more accurate, doesn't rely on - // assumptions about priority, and isn't coupled to Scheduler details. - return earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION; -} - -function computeMsUntilTimeout(root, absoluteTimeoutMs) { +function computeMsUntilTimeout(mostRecentEventTime: ExpirationTime) { if (disableYielding) { // Timeout immediately when yielding is disabled. return 0; } - // Find the earliest uncommitted expiration time in the tree, including - // work that is suspended. The timeout threshold cannot be longer than - // the overall expiration. - const earliestExpirationTimeMs = expirationTimeToMs(root.firstPendingTime); - if (earliestExpirationTimeMs < absoluteTimeoutMs) { - absoluteTimeoutMs = earliestExpirationTimeMs; - } + const eventTimeMs: number = inferTimeFromExpirationTime(mostRecentEventTime); + const currentTimeMs: number = now(); + const timeElapsed = currentTimeMs - eventTimeMs; - // Subtract the current time from the absolute timeout to get the number - // of milliseconds until the timeout. In other words, convert an absolute - // timestamp to a relative time. This is the value that is passed - // to `setTimeout`. - let msUntilTimeout = absoluteTimeoutMs - now(); + // TODO: Account for the Just Noticeable Difference + const timeoutMs = 150; + const msUntilTimeout = timeoutMs - timeElapsed; + // This is the value that is passed to `setTimeout`. return msUntilTimeout < 0 ? 0 : msUntilTimeout; } diff --git a/packages/react-reconciler/src/ReactFiberScheduler.old.js b/packages/react-reconciler/src/ReactFiberScheduler.old.js index 965eaa1cdc62c..8614c2eb15de7 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.old.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.old.js @@ -91,7 +91,6 @@ import { markPingedPriorityLevel, hasLowerPriorityWork, isPriorityLevelSuspended, - findEarliestOutstandingPriorityLevel, didExpireAtExpirationTime, } from './ReactFiberPendingPriority'; import { @@ -272,7 +271,8 @@ let nextUnitOfWork: Fiber | null = null; let nextRoot: FiberRoot | null = null; // The time at which we're currently rendering work. let nextRenderExpirationTime: ExpirationTime = NoWork; -let nextLatestAbsoluteTimeoutMs: number = -1; +let mostRecentEventTime: ExpirationTime = Sync; +let nextRenderDidSuspend: boolean = false; let nextRenderDidError: boolean = false; // The next fiber with an effect that we're currently committing. @@ -399,7 +399,8 @@ function resetStack() { nextRoot = null; nextRenderExpirationTime = NoWork; - nextLatestAbsoluteTimeoutMs = -1; + mostRecentEventTime = Sync; + nextRenderDidSuspend = false; nextRenderDidError = false; nextUnitOfWork = null; } @@ -1502,32 +1503,25 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void { } } - if (isYieldy && nextLatestAbsoluteTimeoutMs !== -1) { + // Check if we should suspend this commit. + // If mostRecentEventTime is Sync, that means we didn't track any event + // times. That can happen if we retried but nothing switched from fallback + // to content. There's no reason to delay doing no work. + if (isYieldy && nextRenderDidSuspend && mostRecentEventTime !== Sync) { // The tree was suspended. const suspendedExpirationTime = expirationTime; markSuspendedPriorityLevel(root, suspendedExpirationTime); - // Find the earliest uncommitted expiration time in the tree, including - // work that is suspended. The timeout threshold cannot be longer than - // the overall expiration. - const earliestExpirationTime = findEarliestOutstandingPriorityLevel( - root, - expirationTime, + const eventTimeMs: number = inferTimeFromExpirationTime( + mostRecentEventTime, ); - const earliestExpirationTimeMs = expirationTimeToMs(earliestExpirationTime); - if (earliestExpirationTimeMs < nextLatestAbsoluteTimeoutMs) { - nextLatestAbsoluteTimeoutMs = earliestExpirationTimeMs; - } - - // Subtract the current time from the absolute timeout to get the number - // of milliseconds until the timeout. In other words, convert an absolute - // timestamp to a relative time. This is the value that is passed - // to `setTimeout`. - const currentTimeMs = expirationTimeToMs(requestCurrentTime()); - let msUntilTimeout = nextLatestAbsoluteTimeoutMs - currentTimeMs; - msUntilTimeout = msUntilTimeout < 0 ? 0 : msUntilTimeout; + const currentTimeMs: number = now(); + const timeElapsed = currentTimeMs - eventTimeMs; // TODO: Account for the Just Noticeable Difference + const timeoutMs = 150; + let msUntilTimeout = timeoutMs - timeElapsed; + msUntilTimeout = msUntilTimeout < 0 ? 0 : msUntilTimeout; const rootExpirationTime = root.expirationTime; onSuspend( @@ -1662,41 +1656,27 @@ function computeExpirationForFiber(currentTime: ExpirationTime, fiber: Fiber) { return expirationTime; } -function renderDidSuspend( - root: FiberRoot, - absoluteTimeoutMs: number, - suspendedTime: ExpirationTime, -) { - // Schedule the timeout. - if ( - absoluteTimeoutMs >= 0 && - nextLatestAbsoluteTimeoutMs < absoluteTimeoutMs - ) { - nextLatestAbsoluteTimeoutMs = absoluteTimeoutMs; +function markRenderEventTime(expirationTime: ExpirationTime): void { + if (expirationTime < mostRecentEventTime) { + mostRecentEventTime = expirationTime; } } +function renderDidSuspend() { + nextRenderDidSuspend = true; +} + function renderDidError() { nextRenderDidError = true; } -function inferStartTimeFromExpirationTime( - root: FiberRoot, - expirationTime: ExpirationTime, -) { +function inferTimeFromExpirationTime(expirationTime: ExpirationTime) { // We don't know exactly when the update was scheduled, but we can infer an - // approximate start time from the expiration time. First, find the earliest - // uncommitted expiration time in the tree, including work that is suspended. - // Then subtract the offset used to compute an async update's expiration time. - // This will cause high priority (interactive) work to expire earlier than - // necessary, but we can account for this by adjusting for the Just - // Noticeable Difference. - const earliestExpirationTime = findEarliestOutstandingPriorityLevel( - root, - expirationTime, + // approximate start time from the expiration time. + const earliestExpirationTimeMs = expirationTimeToMs(expirationTime); + return ( + earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION + originalStartTimeMs ); - const earliestExpirationTimeMs = expirationTimeToMs(earliestExpirationTime); - return earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION; } function pingSuspendedRoot( @@ -2692,6 +2672,7 @@ export { computeExpirationForFiber, captureCommitPhaseError, onUncaughtError, + markRenderEventTime, renderDidSuspend, renderDidError, pingSuspendedRoot, @@ -2711,5 +2692,4 @@ export { flushInteractiveUpdates, computeUniqueAsyncExpiration, flushPassiveEffects, - inferStartTimeFromExpirationTime, }; diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js index 33c19e5ac1e5a..c3d9cfecb7950 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js @@ -11,7 +11,7 @@ import type {Fiber} from './ReactFiber'; import type {ExpirationTime} from './ReactFiberExpirationTime'; export type SuspenseState = {| - timedOutAt: ExpirationTime, + fallbackExpirationTime: ExpirationTime, |}; export function shouldCaptureSuspense(workInProgress: Fiber): boolean { diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 4f5340857552a..c6c8276733037 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -13,7 +13,6 @@ import type {ExpirationTime} from './ReactFiberExpirationTime'; import type {CapturedValue} from './ReactCapturedValue'; import type {Update} from './ReactUpdateQueue'; import type {Thenable} from './ReactFiberScheduler'; -import type {SuspenseState} from './ReactFiberSuspenseComponent'; import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing'; import getComponentName from 'shared/getComponentName'; @@ -42,7 +41,7 @@ import { enableSuspenseServerRenderer, enableEventAPI, } from 'shared/ReactFeatureFlags'; -import {ConcurrentMode} from './ReactTypeOfMode'; +import {ConcurrentMode, NoContext} from './ReactTypeOfMode'; import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent'; import {createCapturedValue} from './ReactCapturedValue'; @@ -63,19 +62,17 @@ import { } from './ReactFiberContext'; import {popProvider} from './ReactFiberNewContext'; import { - renderDidSuspend, renderDidError, onUncaughtError, markLegacyErrorBoundaryAsFailed, isAlreadyFailedLegacyErrorBoundary, pingSuspendedRoot, resolveRetryThenable, - inferStartTimeFromExpirationTime, } from './ReactFiberScheduler'; import invariant from 'shared/invariant'; -import maxSigned31BitInt from './maxSigned31BitInt'; -import {Sync, expirationTimeToMs} from './ReactFiberExpirationTime'; + +import {Sync} from './ReactFiberExpirationTime'; const PossiblyWeakSet = typeof WeakSet === 'function' ? WeakSet : Set; const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map; @@ -206,44 +203,8 @@ function throwException( // This is a thenable. const thenable: Thenable = (value: any); - // Find the earliest timeout threshold of all the placeholders in the - // ancestor path. We could avoid this traversal by storing the thresholds on - // the stack, but we choose not to because we only hit this path if we're - // IO-bound (i.e. if something suspends). Whereas the stack is used even in - // the non-IO- bound case. - let workInProgress = returnFiber; - let earliestTimeoutMs = -1; - let startTimeMs = -1; - do { - if (workInProgress.tag === SuspenseComponent) { - const current = workInProgress.alternate; - if (current !== null) { - const currentState: SuspenseState | null = current.memoizedState; - if (currentState !== null) { - // Reached a boundary that already timed out. Do not search - // any further. - const timedOutAt = currentState.timedOutAt; - startTimeMs = expirationTimeToMs(timedOutAt); - // Do not search any further. - break; - } - } - const defaultSuspenseTimeout = 150; - if ( - earliestTimeoutMs === -1 || - defaultSuspenseTimeout < earliestTimeoutMs - ) { - earliestTimeoutMs = defaultSuspenseTimeout; - } - } - // If there is a DehydratedSuspenseComponent we don't have to do anything because - // if something suspends inside it, we will simply leave that as dehydrated. It - // will never timeout. - workInProgress = workInProgress.return; - } while (workInProgress !== null); - // Schedule the nearest Suspense to re-render the timed out view. - workInProgress = returnFiber; + let workInProgress = returnFiber; do { if ( workInProgress.tag === SuspenseComponent && @@ -270,7 +231,7 @@ function throwException( // Note: It doesn't matter whether the component that suspended was // inside a concurrent mode tree. If the Suspense is outside of it, we // should *not* suspend the commit. - if ((workInProgress.mode & ConcurrentMode) === NoEffect) { + if ((workInProgress.mode & ConcurrentMode) === NoContext) { workInProgress.effectTag |= DidCapture; // We're going to commit this fiber even though it didn't complete. @@ -308,32 +269,6 @@ function throwException( attachPingListener(root, renderExpirationTime, thenable); - let absoluteTimeoutMs; - if (earliestTimeoutMs === -1) { - // If no explicit threshold is given, default to an arbitrarily large - // value. The actual size doesn't matter because the threshold for the - // whole tree will be clamped to the expiration time. - absoluteTimeoutMs = maxSigned31BitInt; - } else { - if (startTimeMs === -1) { - // This suspend happened outside of any already timed-out - // placeholders. We don't know exactly when the update was - // scheduled, but we can infer an approximate start time based on - // the expiration time and the priority. - startTimeMs = inferStartTimeFromExpirationTime( - root, - renderExpirationTime, - ); - } - absoluteTimeoutMs = startTimeMs + earliestTimeoutMs; - } - - // Mark the earliest timeout in the suspended fiber's ancestor path. - // After completing the root, we'll take the largest of all the - // suspended fiber's timeouts and use it to compute a timeout for the - // whole tree. - renderDidSuspend(root, absoluteTimeoutMs, renderExpirationTime); - workInProgress.effectTag |= ShouldCapture; workInProgress.expirationTime = renderExpirationTime; return; diff --git a/packages/react-reconciler/src/ReactUpdateQueue.js b/packages/react-reconciler/src/ReactUpdateQueue.js index 64fc6a8a5840c..aecbc4f678823 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.js @@ -101,6 +101,7 @@ import { } from 'shared/ReactFeatureFlags'; import {StrictMode} from './ReactTypeOfMode'; +import {markRenderEventTime} from './ReactFiberScheduler'; import invariant from 'shared/invariant'; import warningWithoutStack from 'shared/warningWithoutStack'; @@ -454,8 +455,17 @@ export function processUpdateQueue( newExpirationTime = updateExpirationTime; } } else { - // This update does have sufficient priority. Process it and compute - // a new result. + // This update does have sufficient priority. + + // Mark the event time of this update as relevant to this render pass. + // TODO: This should ideally use the true event time of this update rather than + // its priority which is a derived and not reverseable value. + // TODO: We should skip this update if it was already committed but currently + // we have no way of detecting the difference between a committed and suspended + // update here. + markRenderEventTime(updateExpirationTime); + + // Process it and compute a new result. resultState = getStateFromUpdate( workInProgress, queue, diff --git a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js index 0711e244f2dce..529bb33fc93f5 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js @@ -490,8 +490,16 @@ describe('ReactSuspensePlaceholder', () => { expect(onRender.mock.calls[1][3]).toBe(15); // Update again while timed out. + // Since this test was originally written we added an optimization to avoid + // suspending in the case that we already timed out. To simulate the old + // behavior, we add a different suspending boundary as a sibling. ReactNoop.render( - , + + + + + + , ); expect(Scheduler).toFlushAndYield([ 'App', @@ -499,18 +507,23 @@ describe('ReactSuspensePlaceholder', () => { 'Suspend! [Loaded]', 'New', 'Fallback', + 'Suspend! [Sibling]', ]); expect(ReactNoop).toMatchRenderedOutput('Loading...'); expect(onRender).toHaveBeenCalledTimes(2); // Resolve the pending promise. jest.advanceTimersByTime(250); - expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']); + expect(Scheduler).toHaveYielded([ + 'Promise resolved [Loaded]', + 'Promise resolved [Sibling]', + ]); expect(Scheduler).toFlushAndYield([ 'App', 'Suspending', 'Loaded', 'New', + 'Sibling', ]); expect(onRender).toHaveBeenCalledTimes(3);