From cc31b2bb1d7fa0eebc38afa04ff9cfe4415db6a1 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 25 Nov 2019 18:48:56 -0800 Subject: [PATCH] Adjust SuspenseList CPU bound heuristic In SuspenseList we switch to rendering fallbacks (or stop rendering further rows in the case of tail="collapsed/hidden") if it takes more than 500ms to render the list. The limit of 500ms is similar to the train model and designed to be short enough to be in the not noticeable range. This works well if each row is small because we time the 500ms range well. However, if we have a few large rows then we're likely to exceed the limit by a lot. E.g. two 480ms rows hits almost a second instead of 500ms. This PR adjusts the heuristic to instead compute whether something has expired based on the render time of the last row. I.e. if we think rendering one more row would exceed the timeout, then we don't attempt. This still works well for small rows and bails earlier for large rows. The expiration is still based on the start of the list rather than the start of the render. It should probably be based on the start of the render but that's a bigger change and needs some thought. --- packages/react-reconciler/src/ReactFiberBeginWork.js | 2 ++ .../react-reconciler/src/ReactFiberCompleteWork.js | 10 +++++++++- .../src/ReactFiberSuspenseComponent.js | 2 ++ .../src/__tests__/ReactSuspenseList-test.internal.js | 8 ++++---- .../src/__tests__/ReactDOMTracing-test.internal.js | 4 ++-- 5 files changed, 19 insertions(+), 7 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 65bab16650151..1df6aa55bf7bf 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -2358,6 +2358,7 @@ function initSuspenseListRenderState( workInProgress.memoizedState = ({ isBackwards: isBackwards, rendering: null, + renderingStartTime: 0, last: lastContentRow, tail: tail, tailExpiration: 0, @@ -2368,6 +2369,7 @@ function initSuspenseListRenderState( // We can reuse the existing object from previous renders. renderState.isBackwards = isBackwards; renderState.rendering = null; + renderState.renderingStartTime = 0; renderState.last = lastContentRow; renderState.tail = tail; renderState.tailExpiration = 0; diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 8f002d9f1cfaa..84e3293bd05a1 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -1114,7 +1114,8 @@ function completeWork( return null; } } else if ( - now() > renderState.tailExpiration && + now() * 2 - renderState.renderingStartTime > + renderState.tailExpiration && renderExpirationTime > Never ) { // We have now passed our CPU deadline and we'll just give up further @@ -1164,12 +1165,19 @@ function completeWork( // until we just give up and show what we have so far. const TAIL_EXPIRATION_TIMEOUT_MS = 500; renderState.tailExpiration = now() + TAIL_EXPIRATION_TIMEOUT_MS; + // TODO: This is meant to mimic the train model or JND but this + // is a per component value. It should really be since the start + // of the total render or last commit. Consider using something like + // globalMostRecentFallbackTime. That doesn't account for being + // suspended for part of the time or when it's a new render. + // It should probably use a global start time value instead. } // Pop a row. let next = renderState.tail; renderState.rendering = next; renderState.tail = next.sibling; renderState.lastEffect = workInProgress.lastEffect; + renderState.renderingStartTime = now(); next.sibling = null; // Restore the context. diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js index 4d29159d270cb..d988c8d1b1f77 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js @@ -47,6 +47,8 @@ export type SuspenseListRenderState = {| isBackwards: boolean, // The currently rendering tail row. rendering: null | Fiber, + // The absolute time when we started rendering the tail row. + renderingStartTime: number, // The last of the already rendered children. last: null | Fiber, // Remaining rows on the tail of the list. diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js index 506737b7f4f6f..16a43a6bf31c9 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js @@ -1233,8 +1233,8 @@ describe('ReactSuspenseList', () => { expect(Scheduler).toFlushAndYieldThrough(['A']); - Scheduler.unstable_advanceTime(300); - jest.advanceTimersByTime(300); + Scheduler.unstable_advanceTime(200); + jest.advanceTimersByTime(200); expect(Scheduler).toFlushAndYieldThrough(['B']); @@ -1407,8 +1407,8 @@ describe('ReactSuspenseList', () => { expect(Scheduler).toFlushAndYieldThrough(['A']); - Scheduler.unstable_advanceTime(300); - jest.advanceTimersByTime(300); + Scheduler.unstable_advanceTime(200); + jest.advanceTimersByTime(200); expect(Scheduler).toFlushAndYieldThrough(['B']); diff --git a/packages/react/src/__tests__/ReactDOMTracing-test.internal.js b/packages/react/src/__tests__/ReactDOMTracing-test.internal.js index 75e25205de18e..020b2b580a24f 100644 --- a/packages/react/src/__tests__/ReactDOMTracing-test.internal.js +++ b/packages/react/src/__tests__/ReactDOMTracing-test.internal.js @@ -561,8 +561,8 @@ describe('ReactDOMTracing', () => { expect(Scheduler).toFlushAndYieldThrough(['A']); - Scheduler.unstable_advanceTime(300); - jest.advanceTimersByTime(300); + Scheduler.unstable_advanceTime(200); + jest.advanceTimersByTime(200); expect(Scheduler).toFlushAndYieldThrough(['B']);