From 53e8639dce86d84428bbdd61b8655ddd418b3a6d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 12 Dec 2018 19:41:12 -0800 Subject: [PATCH] Memoize promise listeners to prevent exponential growth Previously, React would attach a new listener every time a promise is thrown, regardless of whether the same listener was already attached during a previous render. Because React attempts to render every time a promise resolves, the number of listeners grows quickly. This was especially bad in synchronous mode because the renders that happen when the promise pings are not batched together. So if a single promise has multiple listeners for the same root, there will be multiple renders, which in turn results in more listeners being added to the remaining unresolved promises. This results in exponential growth in the number of listeners with respect to the number of IO-bound components in a single render. Fixes #14220 --- .../src/ReactFiberCommitWork.js | 35 +++++++ .../src/ReactFiberPendingPriority.js | 8 +- .../src/ReactFiberScheduler.js | 93 +++++++------------ .../src/ReactFiberSuspenseComponent.js | 5 +- .../src/ReactFiberUnwindWork.js | 75 ++++++++++----- .../__tests__/ReactSuspense-test.internal.js | 71 +++++++++++--- .../ReactSuspensePlaceholder-test.internal.js | 10 +- .../__tests__/ReactProfiler-test.internal.js | 11 +-- 8 files changed, 186 insertions(+), 122 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index f525b9ecf0049..1aeb92f1c6c4f 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -20,7 +20,9 @@ import type {ExpirationTime} from './ReactFiberExpirationTime'; import type {CapturedValue, CapturedError} from './ReactCapturedValue'; import type {SuspenseState} from './ReactFiberSuspenseComponent'; import type {FunctionComponentUpdateQueue} from './ReactFiberHooks'; +import type {Thenable} from './ReactFiberScheduler'; +import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing'; import { enableHooks, enableSchedulerTracing, @@ -88,6 +90,7 @@ import { import { captureCommitPhaseError, requestCurrentTime, + retryTimedOutBoundary, } from './ReactFiberScheduler'; import { NoEffect as NoHookEffect, @@ -1180,6 +1183,38 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { if (primaryChildParent !== null) { hideOrUnhideAllChildren(primaryChildParent, newDidTimeout); } + + // If this boundary just timed out, then it will have a set of thenables. + // For each thenable, attach a listener so that when it resolves, React + // attempts to re-render the boundary in the primary (pre-timeout) state. + const thenables: Set | null = (finishedWork.updateQueue: any); + if (thenables !== null) { + finishedWork.updateQueue = null; + let retry = retryTimedOutBoundary.bind(null, finishedWork); + + if (enableSchedulerTracing) { + retry = Schedule_tracing_wrap(retry); + } + + thenables.forEach(thenable => { + // Memoize using the boundary fiber to prevent redundant listeners. + let retryCache: Set | void = thenable._reactRetryCache; + if (retryCache === undefined) { + retryCache = thenable._reactRetryCache = new Set(); + } else if ( + // Check both the fiber and its alternate. Only a single listener + // is needed per fiber pair. + retryCache.has(finishedWork) || + retryCache.has((finishedWork.alternate: any)) + ) { + // Already attached a retry listener to this promise. + return; + } + retryCache.add(finishedWork); + thenable.then(retry, retry); + }); + } + return; } case IncompleteClassComponent: { diff --git a/packages/react-reconciler/src/ReactFiberPendingPriority.js b/packages/react-reconciler/src/ReactFiberPendingPriority.js index 6f2fcaa2c395d..3e3a038ef58ef 100644 --- a/packages/react-reconciler/src/ReactFiberPendingPriority.js +++ b/packages/react-reconciler/src/ReactFiberPendingPriority.js @@ -62,6 +62,10 @@ export function markCommittedPriorityLevels( return; } + if (earliestRemainingTime < root.latestPingedTime) { + root.latestPingedTime = NoWork; + } + // Let's see if the previous latest known pending level was just flushed. const latestPendingTime = root.latestPendingTime; if (latestPendingTime !== NoWork) { @@ -209,10 +213,8 @@ export function markPingedPriorityLevel( } function clearPing(root, completedTime) { - // TODO: Track whether the root was pinged during the render phase. If so, - // we need to make sure we don't lose track of it. const latestPingedTime = root.latestPingedTime; - if (latestPingedTime !== NoWork && latestPingedTime >= completedTime) { + if (latestPingedTime >= completedTime) { root.latestPingedTime = NoWork; } } diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 135c015d6b52c..701a8980a148d 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -125,13 +125,8 @@ import { computeAsyncExpiration, computeInteractiveExpiration, } from './ReactFiberExpirationTime'; -import {ConcurrentMode, ProfileMode, NoContext} from './ReactTypeOfMode'; -import { - enqueueUpdate, - resetCurrentlyProcessingQueue, - ForceUpdate, - createUpdate, -} from './ReactUpdateQueue'; +import {ConcurrentMode, ProfileMode} from './ReactTypeOfMode'; +import {enqueueUpdate, resetCurrentlyProcessingQueue} from './ReactUpdateQueue'; import {createCapturedValue} from './ReactCapturedValue'; import { isContextProvider as isLegacyContextProvider, @@ -174,6 +169,8 @@ import {Dispatcher, DispatcherWithoutHooks} from './ReactFiberDispatcher'; export type Thenable = { then(resolve: () => mixed, reject?: () => mixed): mixed, + _reactPingCache: Map> | void, + _reactRetryCache: Set | void, }; const {ReactCurrentOwner} = ReactSharedInternals; @@ -1646,62 +1643,41 @@ function renderDidError() { nextRenderDidError = true; } -function retrySuspendedRoot( - root: FiberRoot, - boundaryFiber: Fiber, - sourceFiber: Fiber, - suspendedTime: ExpirationTime, -) { - let retryTime; - - if (isPriorityLevelSuspended(root, suspendedTime)) { - // Ping at the original level - retryTime = suspendedTime; - - markPingedPriorityLevel(root, retryTime); +function pingSuspendedRoot(root: FiberRoot, pingTime: ExpirationTime) { + // A promise that previously suspended React from committing has resolved. + // If React is still suspended, try again at the previous level (pingTime). + if (nextRoot !== null && nextRenderExpirationTime === pingTime) { + // Received a ping at the same priority level at which we're currently + // rendering. Restart from the root. + nextRoot = null; } else { - // Suspense already timed out. Compute a new expiration time - const currentTime = requestCurrentTime(); - retryTime = computeExpirationForFiber(currentTime, boundaryFiber); - markPendingPriorityLevel(root, retryTime); - } - - // TODO: If the suspense fiber has already rendered the primary children - // without suspending (that is, all of the promises have already resolved), - // we should not trigger another update here. One case this happens is when - // we are in sync mode and a single promise is thrown both on initial render - // and on update; we attach two .then(retrySuspendedRoot) callbacks and each - // one performs Sync work, rerendering the Suspense. - - if ((boundaryFiber.mode & ConcurrentMode) !== NoContext) { - if (root === nextRoot && nextRenderExpirationTime === suspendedTime) { - // Received a ping at the same priority level at which we're currently - // rendering. Restart from the root. - nextRoot = null; + // Confirm that the root is still suspended at this level. Otherwise exit. + if (isPriorityLevelSuspended(root, pingTime)) { + // Ping at the original level + markPingedPriorityLevel(root, pingTime); + const rootExpirationTime = root.expirationTime; + if (rootExpirationTime !== NoWork) { + requestWork(root, rootExpirationTime); + } } } +} - scheduleWorkToRoot(boundaryFiber, retryTime); - if ((boundaryFiber.mode & ConcurrentMode) === NoContext) { - // Outside of concurrent mode, we must schedule an update on the source - // fiber, too, since it already committed in an inconsistent state and - // therefore does not have any pending work. - scheduleWorkToRoot(sourceFiber, retryTime); - const sourceTag = sourceFiber.tag; - if (sourceTag === ClassComponent && sourceFiber.stateNode !== null) { - // When we try rendering again, we should not reuse the current fiber, - // since it's known to be in an inconsistent state. Use a force updte to - // prevent a bail out. - const update = createUpdate(retryTime); - update.tag = ForceUpdate; - enqueueUpdate(sourceFiber, update); +function retryTimedOutBoundary(boundaryFiber: Fiber) { + // The boundary fiber (a Suspense component) previously timed out and 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(); + const retryTime = computeExpirationForFiber(currentTime, boundaryFiber); + const root = scheduleWorkToRoot(boundaryFiber, retryTime); + if (root !== null) { + markPendingPriorityLevel(root, retryTime); + const rootExpirationTime = root.expirationTime; + if (rootExpirationTime !== NoWork) { + requestWork(root, rootExpirationTime); } } - - const rootExpirationTime = root.expirationTime; - if (rootExpirationTime !== NoWork) { - requestWork(root, rootExpirationTime); - } } function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { @@ -2550,7 +2526,8 @@ export { onUncaughtError, renderDidSuspend, renderDidError, - retrySuspendedRoot, + pingSuspendedRoot, + retryTimedOutBoundary, markLegacyErrorBoundaryAsFailed, isAlreadyFailedLegacyErrorBoundary, scheduleWork, diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js index c31cf93473928..33c19e5ac1e5a 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js @@ -14,10 +14,7 @@ export type SuspenseState = {| timedOutAt: ExpirationTime, |}; -export function shouldCaptureSuspense( - current: Fiber | null, - workInProgress: Fiber, -): boolean { +export function shouldCaptureSuspense(workInProgress: Fiber): boolean { // In order to capture, the Suspense component must have a fallback prop. if (workInProgress.memoizedProps.fallback === undefined) { return false; diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 4ddd5db8a9d28..3d4f059317be1 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -43,6 +43,8 @@ import { enqueueCapturedUpdate, createUpdate, CaptureUpdate, + ForceUpdate, + enqueueUpdate, } from './ReactUpdateQueue'; import {logError} from './ReactFiberCommitWork'; import {getStackByFiberInDevAndProd} from './ReactCurrentFiber'; @@ -59,7 +61,7 @@ import { onUncaughtError, markLegacyErrorBoundaryAsFailed, isAlreadyFailedLegacyErrorBoundary, - retrySuspendedRoot, + pingSuspendedRoot, } from './ReactFiberScheduler'; import invariant from 'shared/invariant'; @@ -202,29 +204,18 @@ function throwException( do { if ( workInProgress.tag === SuspenseComponent && - shouldCaptureSuspense(workInProgress.alternate, workInProgress) + shouldCaptureSuspense(workInProgress) ) { // Found the nearest boundary. - // If the boundary is not in concurrent mode, we should not suspend, and - // likewise, when the promise resolves, we should ping synchronously. - const pingTime = - (workInProgress.mode & ConcurrentMode) === NoEffect - ? Sync - : renderExpirationTime; - - // Attach a listener to the promise to "ping" the root and retry. - let onResolveOrReject = retrySuspendedRoot.bind( - null, - root, - workInProgress, - sourceFiber, - pingTime, - ); - if (enableSchedulerTracing) { - onResolveOrReject = Schedule_tracing_wrap(onResolveOrReject); + // Stash the promise on the boundary fiber. If the boundary times out, we'll + // attach another listener to flip the boundary back to its normal state. + const thenables: Set = (workInProgress.updateQueue: any); + if (thenables === null) { + workInProgress.updateQueue = (new Set([thenable]): any); + } else { + thenables.add(thenable); } - thenable.then(onResolveOrReject, onResolveOrReject); // If the boundary is outside of concurrent mode, we should *not* // suspend the commit. Pretend as if the suspended component rendered @@ -243,18 +234,25 @@ function throwException( sourceFiber.effectTag &= ~(LifecycleEffectMask | Incomplete); if (sourceFiber.tag === ClassComponent) { - const current = sourceFiber.alternate; - if (current === null) { + const currentSourceFiber = sourceFiber.alternate; + if (currentSourceFiber === null) { // This is a new mount. Change the tag so it's not mistaken for a // completed class component. For example, we should not call // componentWillUnmount if it is deleted. sourceFiber.tag = IncompleteClassComponent; + } else { + // When we try rendering again, we should not reuse the current fiber, + // since it's known to be in an inconsistent state. Use a force updte to + // prevent a bail out. + const update = createUpdate(Sync); + update.tag = ForceUpdate; + enqueueUpdate(sourceFiber, update); } } - // The source fiber did not complete. Mark it with the current - // render priority to indicate that it still has pending work. - sourceFiber.expirationTime = renderExpirationTime; + // The source fiber did not complete. Mark it with Sync priority to + // indicate that it still has pending work. + sourceFiber.expirationTime = Sync; // Exit without suspending. return; @@ -263,6 +261,33 @@ function throwException( // Confirmed that the boundary is in a concurrent mode tree. Continue // with the normal suspend path. + // Attach a listener to the promise to "ping" the root and retry. But + // only if one does not already exist for the current render expiration + // time (which acts like a "thread ID" here). + let pingCache: Map> | void = + thenable._reactPingCache; + let threadIDs; + if (pingCache === undefined) { + pingCache = thenable._reactPingCache = new Map(); + threadIDs = new Set(); + pingCache.set(root, threadIDs); + } else { + threadIDs = pingCache.get(root); + if (threadIDs === undefined) { + threadIDs = new Set(); + pingCache.set(root, threadIDs); + } + } + if (!threadIDs.has(renderExpirationTime)) { + // Memoize using the thread ID to prevent redundant listeners. + threadIDs.add(renderExpirationTime); + let ping = pingSuspendedRoot.bind(null, root, renderExpirationTime); + if (enableSchedulerTracing) { + ping = Schedule_tracing_wrap(ping); + } + thenable.then(ping, ping); + } + let absoluteTimeoutMs; if (earliestTimeoutMs === -1) { // If no explicit threshold is given, default to an abitrarily large diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 5b4e111c169d0..b6ca5beba1172 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -485,11 +485,7 @@ describe('ReactSuspense', () => { expect(root).toMatchRenderedOutput('Loading...'); jest.advanceTimersByTime(1000); - expect(ReactTestRenderer).toHaveYielded([ - 'Promise resolved [B]', - 'B', - 'B', - ]); + expect(ReactTestRenderer).toHaveYielded(['Promise resolved [B]', 'B']); expect(root).toMatchRenderedOutput('Stateful: 2B'); }); @@ -552,11 +548,7 @@ describe('ReactSuspense', () => { expect(root).toMatchRenderedOutput('Loading...'); jest.advanceTimersByTime(1000); - expect(ReactTestRenderer).toHaveYielded([ - 'Promise resolved [B]', - 'B', - 'B', - ]); + expect(ReactTestRenderer).toHaveYielded(['Promise resolved [B]', 'B']); expect(root).toMatchRenderedOutput('Stateful: 2B'); }); @@ -736,10 +728,6 @@ describe('ReactSuspense', () => { 'Promise resolved [Child 2]', 'Child 2', 'Suspend! [Child 3]', - // TODO: This suspends twice because there were two pings, once for each - // time Child 2 suspended. This is only an issue in sync mode because - // pings aren't batched. - 'Suspend! [Child 3]', ]); jest.advanceTimersByTime(1000); expect(ReactTestRenderer).toHaveYielded([ @@ -886,6 +874,61 @@ describe('ReactSuspense', () => { jest.advanceTimersByTime(1000); }); + it('memoizes promise listeners per thread ID to prevent redundant renders', () => { + function App() { + return ( + }> + + + + + ); + } + + const root = ReactTestRenderer.create(null); + + root.update(); + + expect(ReactTestRenderer).toHaveYielded([ + 'Suspend! [A]', + 'Suspend! [B]', + 'Suspend! [C]', + 'Loading...', + ]); + + // Resolve A + jest.advanceTimersByTime(1000); + + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [A]', + 'A', + // The promises for B and C have now been thrown twice + 'Suspend! [B]', + 'Suspend! [C]', + ]); + + // Resolve B + jest.advanceTimersByTime(1000); + + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [B]', + // Even though the promise for B was thrown twice, we should only + // re-render once. + 'B', + // The promise for C has now been thrown three times + 'Suspend! [C]', + ]); + + // Resolve C + jest.advanceTimersByTime(1000); + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [C]', + // Even though the promise for C was thrown three times, we should only + // re-render once. + 'C', + ]); + }); + it('#14162', () => { const {lazy} = React; diff --git a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js index 4765b32704d6d..8389cdb34b709 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js @@ -366,22 +366,14 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { jest.advanceTimersByTime(1000); - // TODO Change expected onRender count to 4. - // At the moment, every time we suspended while rendering will cause a commit. - // This will probably change in the future, but that's why there are two new ones. expect(root.toJSON()).toEqual(['Loaded', 'New']); - expect(onRender).toHaveBeenCalledTimes(5); + expect(onRender).toHaveBeenCalledTimes(4); // When the suspending data is resolved and our final UI is rendered, // the baseDuration should only include the 1ms re-rendering AsyncText, // but the treeBaseDuration should include the full 9ms spent in the tree. expect(onRender.mock.calls[3][2]).toBe(1); expect(onRender.mock.calls[3][3]).toBe(9); - - // TODO Remove these assertions once this commit is gone. - // For now, there was no actual work done during this commit; see above comment. - expect(onRender.mock.calls[4][2]).toBe(0); - expect(onRender.mock.calls[4][3]).toBe(9); }); it('properly accounts for base durations when a suspended times out in a concurrent tree', () => { diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index a811cec410358..f98c7fbfa337f 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -2548,22 +2548,15 @@ describe('Profiler', () => { await originalPromise; expect(renderer.toJSON()).toEqual(['loaded', 'updated']); - // TODO: Bug. This *should* just be one render tied to both interactions. - expect(onRender).toHaveBeenCalledTimes(2); + expect(onRender).toHaveBeenCalledTimes(1); expect(onRender.mock.calls[0][6]).toMatchInteractions([ initialRenderInteraction, ]); - expect(onRender.mock.calls[1][6]).toMatchInteractions([ - highPriUpdateInteraction, - ]); - expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(2); + expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(1); expect( onInteractionScheduledWorkCompleted.mock.calls[0][0], ).toMatchInteraction(initialRenderInteraction); - expect( - onInteractionScheduledWorkCompleted.mock.calls[1][0], - ).toMatchInteraction(highPriUpdateInteraction); }); it('handles high-pri renderers between suspended and resolved (async) trees', async () => {