Skip to content

Commit

Permalink
Adjust SuspenseList CPU bound heuristic (facebook#17455)
Browse files Browse the repository at this point in the history
* 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.

* Comment
  • Loading branch information
sebmarkbage authored and trueadm committed Dec 4, 2019
1 parent cda2bf2 commit 4ba8038
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 7 deletions.
2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -2358,6 +2358,7 @@ function initSuspenseListRenderState(
workInProgress.memoizedState = ({
isBackwards: isBackwards,
rendering: null,
renderingStartTime: 0,
last: lastContentRow,
tail: tail,
tailExpiration: 0,
Expand All @@ -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;
Expand Down
12 changes: 11 additions & 1 deletion packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1114,7 +1114,10 @@ function completeWork(
return null;
}
} else if (
now() > renderState.tailExpiration &&
// The time it took to render last row is greater than time until
// the expiration.
now() * 2 - renderState.renderingStartTime >
renderState.tailExpiration &&
renderExpirationTime > Never
) {
// We have now passed our CPU deadline and we'll just give up further
Expand Down Expand Up @@ -1164,12 +1167,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.
Expand Down
2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactFiberSuspenseComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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']);

Expand Down Expand Up @@ -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']);

Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/__tests__/ReactDOMTracing-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -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']);

Expand Down

0 comments on commit 4ba8038

Please sign in to comment.