@samitouri / QOS-React-2 / commits / 79572e34d1

Adjust SuspenseList CPU bound heuristic (#17455)

* 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

Sebastian Markbåge committed Dec 2, 2019 at 17:53 UTC 79572e34d18c67768c93b1a4d60703a5929363a3
5 files changed +21 -7
packages/react-reconciler/src/ReactFiberBeginWork.js
+2
@@ -2358,6 +2358,7 @@ function initSuspenseListRenderState(
2358 workInProgress.memoizedState = ({
2359 isBackwards: isBackwards,
2360 rendering: null,
2361 + renderingStartTime: 0,
2362 last: lastContentRow,
2363 tail: tail,
2364 tailExpiration: 0,
@@ -2368,6 +2369,7 @@ function initSuspenseListRenderState(
2369 // We can reuse the existing object from previous renders.
2370 renderState.isBackwards = isBackwards;
2371 renderState.rendering = null;
2372 + renderState.renderingStartTime = 0;
2373 renderState.last = lastContentRow;
2374 renderState.tail = tail;
2375 renderState.tailExpiration = 0;
packages/react-reconciler/src/ReactFiberCompleteWork.js
+11 -1
@@ -1114,7 +1114,10 @@ function completeWork(
1114 return null;
1115 }
1116 } else if (
1117 - now() > renderState.tailExpiration &&
1117 + // The time it took to render last row is greater than time until
1118 + // the expiration.
1119 + now() * 2 - renderState.renderingStartTime >
1120 + renderState.tailExpiration &&
1121 renderExpirationTime > Never
1122 ) {
1123 // We have now passed our CPU deadline and we'll just give up further
@@ -1164,12 +1167,19 @@ function completeWork(
1167 // until we just give up and show what we have so far.
1168 const TAIL_EXPIRATION_TIMEOUT_MS = 500;
1169 renderState.tailExpiration = now() + TAIL_EXPIRATION_TIMEOUT_MS;
1170 + // TODO: This is meant to mimic the train model or JND but this
1171 + // is a per component value. It should really be since the start
1172 + // of the total render or last commit. Consider using something like
1173 + // globalMostRecentFallbackTime. That doesn't account for being
1174 + // suspended for part of the time or when it's a new render.
1175 + // It should probably use a global start time value instead.
1176 }
1177 // Pop a row.
1178 let next = renderState.tail;
1179 renderState.rendering = next;
1180 renderState.tail = next.sibling;
1181 renderState.lastEffect = workInProgress.lastEffect;
1182 + renderState.renderingStartTime = now();
1183 next.sibling = null;
1184
1185 // Restore the context.
packages/react-reconciler/src/ReactFiberSuspenseComponent.js
+2
@@ -47,6 +47,8 @@ export type SuspenseListRenderState = {|
47 isBackwards: boolean,
48 // The currently rendering tail row.
49 rendering: null | Fiber,
50 + // The absolute time when we started rendering the tail row.
51 + renderingStartTime: number,
52 // The last of the already rendered children.
53 last: null | Fiber,
54 // Remaining rows on the tail of the list.
packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js
+4 -4
@@ -1233,8 +1233,8 @@ describe('ReactSuspenseList', () => {
1233
1234 expect(Scheduler).toFlushAndYieldThrough(['A']);
1235
1236 - Scheduler.unstable_advanceTime(300);
1237 - jest.advanceTimersByTime(300);
1236 + Scheduler.unstable_advanceTime(200);
1237 + jest.advanceTimersByTime(200);
1238
1239 expect(Scheduler).toFlushAndYieldThrough(['B']);
1240
@@ -1407,8 +1407,8 @@ describe('ReactSuspenseList', () => {
1407
1408 expect(Scheduler).toFlushAndYieldThrough(['A']);
1409
1410 - Scheduler.unstable_advanceTime(300);
1411 - jest.advanceTimersByTime(300);
1410 + Scheduler.unstable_advanceTime(200);
1411 + jest.advanceTimersByTime(200);
1412
1413 expect(Scheduler).toFlushAndYieldThrough(['B']);
1414
packages/react/src/__tests__/ReactDOMTracing-test.internal.js
+2 -2
@@ -561,8 +561,8 @@ describe('ReactDOMTracing', () => {
561
562 expect(Scheduler).toFlushAndYieldThrough(['A']);
563
564 - Scheduler.unstable_advanceTime(300);
565 - jest.advanceTimersByTime(300);
564 + Scheduler.unstable_advanceTime(200);
565 + jest.advanceTimersByTime(200);
566
567 expect(Scheduler).toFlushAndYieldThrough(['B']);
568