From 53e8639dce86d84428bbdd61b8655ddd418b3a6d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 12 Dec 2018 19:41:12 -0800 Subject: [PATCH 1/3] 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 f525b9ecf00..1aeb92f1c6c 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 6f2fcaa2c39..3e3a038ef58 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 135c015d6b5..701a8980a14 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 c31cf934739..33c19e5ac1e 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 4ddd5db8a9d..3d4f059317b 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 5b4e111c169..b6ca5beba11 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 4765b32704d..8389cdb34b7 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 a811cec4103..f98c7fbfa33 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 () => { From 4c1bedc12545acbd5ab5dda401ceb65d0e5e8201 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 13 Dec 2018 18:09:47 -0800 Subject: [PATCH 2/3] Memoize on the root and Suspense fiber instead of on the promise --- .../src/ReactFiberBeginWork.js | 1 + .../src/ReactFiberCommitWork.js | 30 ++++++++----------- .../react-reconciler/src/ReactFiberRoot.js | 10 +++++++ .../src/ReactFiberScheduler.js | 27 ++++++++++++++--- .../src/ReactFiberUnwindWork.js | 22 +++++++++----- .../ReactSuspensePlaceholder-test.internal.js | 6 ++-- scripts/rollup/validate/eslintrc.cjs.js | 1 + scripts/rollup/validate/eslintrc.fb.js | 1 + scripts/rollup/validate/eslintrc.rn.js | 1 + scripts/rollup/validate/eslintrc.umd.js | 1 + 10 files changed, 67 insertions(+), 33 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 23df1d25bd7..988ee30f5fc 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1474,6 +1474,7 @@ function updateSuspenseComponent( ); } } + workInProgress.stateNode = current.stateNode; } workInProgress.memoizedState = nextState; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 1aeb92f1c6c..85cd0a5791b 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -109,6 +109,8 @@ if (__DEV__) { didWarnAboutUndefinedSnapshotBeforeUpdate = new Set(); } +const PossiblyWeakSet = typeof WeakSet === 'function' ? WeakSet : Set; + export function logError(boundary: Fiber, errorInfo: CapturedValue) { const source = errorInfo.source; let stack = errorInfo.stack; @@ -1190,28 +1192,20 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { 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); + let retryCache = finishedWork.stateNode; + if (retryCache === null) { + retryCache = finishedWork.stateNode = new PossiblyWeakSet(); } - 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; + let retry = retryTimedOutBoundary.bind(null, finishedWork, thenable); + if (enableSchedulerTracing) { + retry = Schedule_tracing_wrap(retry); + } + if (!retryCache.has(thenable)) { + retryCache.add(thenable); + thenable.then(retry, retry); } - retryCache.add(finishedWork); - thenable.then(retry, retry); }); } diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index d257f3c93ab..e3e445b46ef 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -10,6 +10,7 @@ import type {Fiber} from './ReactFiber'; import type {ExpirationTime} from './ReactFiberExpirationTime'; import type {TimeoutHandle, NoTimeout} from './ReactFiberHostConfig'; +import type {Thenable} from './ReactFiberScheduler'; import type {Interaction} from 'scheduler/src/Tracing'; import {noTimeout} from './ReactFiberHostConfig'; @@ -51,6 +52,11 @@ type BaseFiberRootProperties = {| // be retried. latestPingedTime: ExpirationTime, + pingCache: + | WeakMap> + | Map> + | null, + // If an error is thrown, and there are no more updates in the queue, we try // rendering from the root one more time, synchronously, before handling // the error. @@ -121,6 +127,8 @@ export function createFiberRoot( latestSuspendedTime: NoWork, latestPingedTime: NoWork, + pingCache: null, + didError: false, pendingCommitExpirationTime: NoWork, @@ -144,6 +152,8 @@ export function createFiberRoot( containerInfo: containerInfo, pendingChildren: null, + pingCache: null, + earliestPendingTime: NoWork, latestPendingTime: NoWork, earliestSuspendedTime: NoWork, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 701a8980a14..a63bfd56b79 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -169,8 +169,6 @@ import {Dispatcher, DispatcherWithoutHooks} from './ReactFiberDispatcher'; export type Thenable = { then(resolve: () => mixed, reject?: () => mixed): mixed, - _reactPingCache: Map> | void, - _reactRetryCache: Set | void, }; const {ReactCurrentOwner} = ReactSharedInternals; @@ -1643,9 +1641,21 @@ function renderDidError() { nextRenderDidError = true; } -function pingSuspendedRoot(root: FiberRoot, pingTime: ExpirationTime) { +function pingSuspendedRoot( + root: FiberRoot, + thenable: Thenable, + pingTime: ExpirationTime, +) { // A promise that previously suspended React from committing has resolved. // If React is still suspended, try again at the previous level (pingTime). + + const pingCache = root.pingCache; + if (pingCache !== null) { + // The thenable resolved, so we no longer need to memoize, because it will + // never be thrown again. + pingCache.delete(thenable); + } + if (nextRoot !== null && nextRenderExpirationTime === pingTime) { // Received a ping at the same priority level at which we're currently // rendering. Restart from the root. @@ -1663,11 +1673,20 @@ function pingSuspendedRoot(root: FiberRoot, pingTime: ExpirationTime) { } } -function retryTimedOutBoundary(boundaryFiber: Fiber) { +function retryTimedOutBoundary(boundaryFiber: Fiber, thenable: Thenable) { // 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 retryCache: WeakSet | Set | null = + boundaryFiber.stateNode; + if (retryCache !== null) { + // The thenable resolved, so we no longer need to memoize, because it will + // never be thrown again. + retryCache.delete(thenable); + } + const currentTime = requestCurrentTime(); const retryTime = computeExpirationForFiber(currentTime, boundaryFiber); const root = scheduleWorkToRoot(boundaryFiber, retryTime); diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 3d4f059317b..86e2b1a964a 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -73,6 +73,8 @@ import { } from './ReactFiberExpirationTime'; import {findEarliestOutstandingPriorityLevel} from './ReactFiberPendingPriority'; +const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map; + function createRootErrorUpdate( fiber: Fiber, errorInfo: CapturedValue, @@ -264,24 +266,28 @@ function throwException( // 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 pingCache = root.pingCache; let threadIDs; - if (pingCache === undefined) { - pingCache = thenable._reactPingCache = new Map(); + if (pingCache === null) { + pingCache = root.pingCache = new PossiblyWeakMap(); threadIDs = new Set(); - pingCache.set(root, threadIDs); + pingCache.set(thenable, threadIDs); } else { - threadIDs = pingCache.get(root); + threadIDs = pingCache.get(thenable); if (threadIDs === undefined) { threadIDs = new Set(); - pingCache.set(root, threadIDs); + pingCache.set(thenable, threadIDs); } } if (!threadIDs.has(renderExpirationTime)) { // Memoize using the thread ID to prevent redundant listeners. threadIDs.add(renderExpirationTime); - let ping = pingSuspendedRoot.bind(null, root, renderExpirationTime); + let ping = pingSuspendedRoot.bind( + null, + root, + thenable, + renderExpirationTime, + ); if (enableSchedulerTracing) { ping = Schedule_tracing_wrap(ping); } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js index 8389cdb34b7..eed868549d3 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js @@ -8,9 +8,9 @@ * @jest-environment node */ -runPlaceholderTests('ReactSuspensePlaceholder (mutation)', () => - require('react-noop-renderer'), -); +// runPlaceholderTests('ReactSuspensePlaceholder (mutation)', () => +// require('react-noop-renderer'), +// ); runPlaceholderTests('ReactSuspensePlaceholder (persistence)', () => require('react-noop-renderer/persistent'), ); diff --git a/scripts/rollup/validate/eslintrc.cjs.js b/scripts/rollup/validate/eslintrc.cjs.js index e23b7c68fce..ff6cf584e4e 100644 --- a/scripts/rollup/validate/eslintrc.cjs.js +++ b/scripts/rollup/validate/eslintrc.cjs.js @@ -12,6 +12,7 @@ module.exports = { Proxy: true, Symbol: true, WeakMap: true, + WeakSet: true, Uint16Array: true, // Vendor specific MSApp: true, diff --git a/scripts/rollup/validate/eslintrc.fb.js b/scripts/rollup/validate/eslintrc.fb.js index 878a170a31b..d48c9caa931 100644 --- a/scripts/rollup/validate/eslintrc.fb.js +++ b/scripts/rollup/validate/eslintrc.fb.js @@ -12,6 +12,7 @@ module.exports = { Symbol: true, Proxy: true, WeakMap: true, + WeakSet: true, Uint16Array: true, // Vendor specific MSApp: true, diff --git a/scripts/rollup/validate/eslintrc.rn.js b/scripts/rollup/validate/eslintrc.rn.js index 527fd0a473c..66c244d0fe5 100644 --- a/scripts/rollup/validate/eslintrc.rn.js +++ b/scripts/rollup/validate/eslintrc.rn.js @@ -12,6 +12,7 @@ module.exports = { Symbol: true, Proxy: true, WeakMap: true, + WeakSet: true, // Vendor specific MSApp: true, __REACT_DEVTOOLS_GLOBAL_HOOK__: true, diff --git a/scripts/rollup/validate/eslintrc.umd.js b/scripts/rollup/validate/eslintrc.umd.js index a3d5fadf2c5..59fb02e93e1 100644 --- a/scripts/rollup/validate/eslintrc.umd.js +++ b/scripts/rollup/validate/eslintrc.umd.js @@ -11,6 +11,7 @@ module.exports = { Symbol: true, Proxy: true, WeakMap: true, + WeakSet: true, Uint16Array: true, // Vendor specific MSApp: true, From 3b60eefa4e3633516e2883da9ff857350fbc02e3 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 13 Dec 2018 19:40:32 -0800 Subject: [PATCH 3/3] Add TODO to fix persistent mode tests --- .../src/__tests__/ReactSuspensePlaceholder-test.internal.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js index eed868549d3..6c291de7d16 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js @@ -8,6 +8,9 @@ * @jest-environment node */ +// TODO: This does nothing since it was migrated from noop renderer to test +// renderer! Switch back to noop renderer, or add persistent mode to test +// renderer, or merge the two renderers into one somehow. // runPlaceholderTests('ReactSuspensePlaceholder (mutation)', () => // require('react-noop-renderer'), // );