From 0b931f90e8964183f08ac328e7350d847abb08f9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 11 Apr 2023 00:19:49 -0400 Subject: [PATCH] Remove JND delay for non-transition updates (#26597) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updates that are marked as part of a transition are allowed to block a render from committing. Generally, other updates cannot — however, there's one exception that's leftover from a previous iteration of our Suspense architecture. If an update is not the result of a known urgent event type — known as "Default" updates — then we allow it to suspend briefly, as long as the delay is short enough that the user won't notice. We refer to this delay as a "Just Noticable Difference" (JND) delay. To illustrate, if the user has already waited 400ms for an update to be reflected on the screen, the theory is that they won't notice if you wait an additional 100ms. So React can suspend for a bit longer in case more data comes in. The longer the user has already waited, the longer the JND. While we still believe this theory is sound from a UX perspective, we no longer think the implementation complexity is worth it. The main thing that's changed is how we handle Default updates. We used to render Default updates concurrently (i.e. they were time sliced, and were scheduled with postTask), but now they are blocking. Soon, they will also be scheduled with rAF, too, which means by the end of the next rAF, they will have either finished rendering or the main thread will be blocked until they do. There are various motivations for this but part of the rationale is that anything that can be made non-blocking should be marked as a Transition, anyway, so it's not worth adding implementation complexity to Default. This commit removes the JND delay for Default updates. They will now commit immediately once the render phase is complete, even if a component suspends. --- .../__tests__/ReactCacheOld-test.internal.js | 4 +- .../src/ReactFiberWorkLoop.js | 60 --- .../src/__tests__/ReactExpiration-test.js | 8 +- .../ReactHooksWithNoopRenderer-test.js | 41 +- .../src/__tests__/ReactLazy-test.internal.js | 18 +- .../__tests__/ReactOffscreenSuspense-test.js | 72 +-- .../__tests__/ReactSuspense-test.internal.js | 8 +- .../ReactSuspenseEffectsSemantics-test.js | 129 +---- .../ReactSuspensePlaceholder-test.internal.js | 15 +- .../ReactSuspenseWithNoopRenderer-test.js | 502 ++---------------- 10 files changed, 114 insertions(+), 743 deletions(-) diff --git a/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js b/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js index 117c0b488b..579c835878 100644 --- a/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js +++ b/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js @@ -236,7 +236,7 @@ describe('ReactCache', () => { jest.advanceTimersByTime(100); assertLog(['Promise resolved [4]']); - await waitForAll([1, 4, 'Suspend! [5]', 'Loading...']); + await waitForAll([1, 4, 'Suspend! [5]']); jest.advanceTimersByTime(100); assertLog(['Promise resolved [5]']); @@ -264,7 +264,7 @@ describe('ReactCache', () => { ]); jest.advanceTimersByTime(100); assertLog(['Promise resolved [2]']); - await waitForAll([1, 2, 'Suspend! [3]', 'Loading...']); + await waitForAll([1, 2, 'Suspend! [3]']); jest.advanceTimersByTime(100); assertLog(['Promise resolved [3]']); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 94af7e44d5..d75cc2c47f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -145,7 +145,6 @@ import { includesExpiredLane, getNextLanes, getLanesToRetrySynchronouslyOnError, - getMostRecentEventTime, markRootUpdated, markRootSuspended as markRootSuspended_dontCallThisOneDirectly, markRootPinged, @@ -284,8 +283,6 @@ import { } from './ReactFiberRootScheduler'; import {getMaskedContext, getUnmaskedContext} from './ReactFiberContext'; -const ceil = Math.ceil; - const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map; const { @@ -1193,38 +1190,6 @@ function finishConcurrentRender( break; } - if (!shouldForceFlushFallbacksInDEV()) { - // This is not a transition, but we did trigger an avoided state. - // Schedule a placeholder to display after a short delay, using the Just - // Noticeable Difference. - // TODO: Is the JND optimization worth the added complexity? If this is - // the only reason we track the event time, then probably not. - // Consider removing. - - const mostRecentEventTime = getMostRecentEventTime(root, lanes); - const eventTimeMs = mostRecentEventTime; - const timeElapsedMs = now() - eventTimeMs; - const msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs; - - // Don't bother with a very short suspense time. - if (msUntilTimeout > 10) { - // Instead of committing the fallback immediately, wait for more data - // to arrive. - root.timeoutHandle = scheduleTimeout( - commitRootWhenReady.bind( - null, - root, - finishedWork, - workInProgressRootRecoverableErrors, - workInProgressTransitions, - lanes, - ), - msUntilTimeout, - ); - break; - } - } - // Commit the placeholder. commitRootWhenReady( root, @@ -3580,31 +3545,6 @@ export function resolveRetryWakeable(boundaryFiber: Fiber, wakeable: Wakeable) { retryTimedOutBoundary(boundaryFiber, retryLane); } -// Computes the next Just Noticeable Difference (JND) boundary. -// The theory is that a person can't tell the difference between small differences in time. -// Therefore, if we wait a bit longer than necessary that won't translate to a noticeable -// difference in the experience. However, waiting for longer might mean that we can avoid -// showing an intermediate loading state. The longer we have already waited, the harder it -// is to tell small differences in time. Therefore, the longer we've already waited, -// the longer we can wait additionally. At some point we have to give up though. -// We pick a train model where the next boundary commits at a consistent schedule. -// These particular numbers are vague estimates. We expect to adjust them based on research. -function jnd(timeElapsed: number) { - return timeElapsed < 120 - ? 120 - : timeElapsed < 480 - ? 480 - : timeElapsed < 1080 - ? 1080 - : timeElapsed < 1920 - ? 1920 - : timeElapsed < 3000 - ? 3000 - : timeElapsed < 4320 - ? 4320 - : ceil(timeElapsed / 1960) * 1960; -} - export function throwIfInfiniteUpdateLoopDetected() { if (nestedUpdateCount > NESTED_UPDATE_LIMIT) { nestedUpdateCount = 0; diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js index 2c38a05168..653bc30460 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js @@ -732,13 +732,9 @@ describe('ReactExpiration', () => { expect(root).toMatchRenderedOutput('A0BC'); await act(async () => { - if (gate(flags => flags.enableSyncDefaultUpdates)) { - React.startTransition(() => { - root.render(); - }); - } else { + React.startTransition(() => { root.render(); - } + }); await waitForAll(['Suspend! [A1]', 'Loading...']); // Lots of time elapses before the promise resolves diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 627dc4e856..1c7b25a2c9 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -692,24 +692,16 @@ describe('ReactHooksWithNoopRenderer', () => { await waitForAll([0]); expect(root).toMatchRenderedOutput(); - if (gate(flags => flags.enableSyncDefaultUpdates)) { - React.startTransition(() => { - root.render(); - }); - } else { + React.startTransition(() => { root.render(); - } + }); await waitForAll(['Suspend!']); expect(root).toMatchRenderedOutput(); // Rendering again should suspend again. - if (gate(flags => flags.enableSyncDefaultUpdates)) { - React.startTransition(() => { - root.render(); - }); - } else { + React.startTransition(() => { root.render(); - } + }); await waitForAll(['Suspend!']); }); @@ -755,38 +747,25 @@ describe('ReactHooksWithNoopRenderer', () => { expect(root).toMatchRenderedOutput(); await act(async () => { - if (gate(flags => flags.enableSyncDefaultUpdates)) { - React.startTransition(() => { - root.render(); - setLabel('B'); - }); - } else { + React.startTransition(() => { root.render(); setLabel('B'); - } + }); await waitForAll(['Suspend!']); expect(root).toMatchRenderedOutput(); // Rendering again should suspend again. - if (gate(flags => flags.enableSyncDefaultUpdates)) { - React.startTransition(() => { - root.render(); - }); - } else { + React.startTransition(() => { root.render(); - } + }); await waitForAll(['Suspend!']); // Flip the signal back to "cancel" the update. However, the update to // label should still proceed. It shouldn't have been dropped. - if (gate(flags => flags.enableSyncDefaultUpdates)) { - React.startTransition(() => { - root.render(); - }); - } else { + React.startTransition(() => { root.render(); - } + }); await waitForAll(['B:0']); expect(root).toMatchRenderedOutput(); }); diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index 0ebf8f2d53..bd4c534a78 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -1414,10 +1414,12 @@ describe('ReactLazy', () => { // Swap the position of A and B root.update(); - await waitForAll(['Init B2', 'Loading...']); - jest.runAllTimers(); - - assertLog(['Did unmount: A', 'Did unmount: B']); + await waitForAll([ + 'Init B2', + 'Loading...', + 'Did unmount: A', + 'Did unmount: B', + ]); // The suspense boundary should've triggered now. expect(root).toMatchRenderedOutput('Loading...'); @@ -1559,13 +1561,9 @@ describe('ReactLazy', () => { expect(root).toMatchRenderedOutput('AB'); // Swap the position of A and B - if (gate(flags => flags.enableSyncDefaultUpdates)) { - React.startTransition(() => { - root.update(); - }); - } else { + React.startTransition(() => { root.update(); - } + }); await waitForAll(['Init B2', 'Loading...']); await resolveFakeImport(ChildB2); // We need to flush to trigger the second one to load. diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js index 3a09abd9b1..c73e3caf62 100644 --- a/packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js +++ b/packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js @@ -9,6 +9,7 @@ let useState; let useEffect; let startTransition; let textCache; +let waitFor; let waitForPaint; let assertLog; @@ -28,6 +29,7 @@ describe('ReactOffscreen', () => { startTransition = React.startTransition; const InternalTestUtils = require('internal-test-utils'); + waitFor = InternalTestUtils.waitFor; waitForPaint = InternalTestUtils.waitForPaint; assertLog = InternalTestUtils.assertLog; @@ -407,7 +409,6 @@ describe('ReactOffscreen', () => { expect(root).toMatchRenderedOutput(); }); - // Only works in new reconciler // @gate enableOffscreen test('detect updates to a hidden tree during a concurrent event', async () => { // This is a pretty complex test case. It relates to how we detect if an @@ -442,17 +443,17 @@ describe('ReactOffscreen', () => { setOuter = _setOuter; return ( <> - - - + + + }> - + @@ -466,50 +467,41 @@ describe('ReactOffscreen', () => { root.render(); }); assertLog([ - 'Outer: 0', 'Inner: 0', - 'Async: 0', + 'Outer: 0', + 'Sibling: 0', 'Inner and outer are consistent', ]); expect(root).toMatchRenderedOutput( <> - Outer: 0 Inner: 0 - Async: 0 + Outer: 0 + Sibling: 0 , ); await act(async () => { // Update a value both inside and outside the hidden tree. These values // must always be consistent. - setOuter(1); - setInner(1); - // In the same render, also hide the offscreen tree. - root.render(); + startTransition(() => { + setOuter(1); + setInner(1); + // In the same render, also hide the offscreen tree. + root.render(); + }); - await waitForPaint([ + await waitFor([ // The outer update will commit, but the inner update is deferred until // a later render. 'Outer: 1', - - // Something suspended. This means we won't commit immediately; there - // will be an async gap between render and commit. In this test, we will - // use this property to schedule a concurrent update. The fact that - // we're using Suspense to schedule a concurrent update is not directly - // relevant to the test — we could also use time slicing, but I've - // chosen to use Suspense the because implementation details of time - // slicing are more volatile. - 'Suspend! [Async: 1]', - - 'Loading...', ]); // Assert that we haven't committed quite yet expect(root).toMatchRenderedOutput( <> - Outer: 0 Inner: 0 - Async: 0 + Outer: 0 + Sibling: 0 , ); @@ -520,14 +512,13 @@ describe('ReactOffscreen', () => { setInner(2); }); - // Commit the previous render. - jest.runAllTimers(); + // Finish rendering and commit the in-progress render. + await waitForPaint(['Sibling: 1']); expect(root).toMatchRenderedOutput( <> - Outer: 1 - - Loading... + Outer: 1 + Sibling: 1 , ); @@ -536,32 +527,27 @@ describe('ReactOffscreen', () => { root.render(); }); assertLog([ - 'Outer: 1', - // There are two pending updates on Inner, but only the first one // is processed, even though they share the same lane. If the second // update were erroneously processed, then Inner would be inconsistent // with Outer. 'Inner: 1', - - 'Suspend! [Async: 1]', - 'Loading...', + 'Outer: 1', + 'Sibling: 1', 'Inner and outer are consistent', ]); }); assertLog([ - 'Outer: 2', 'Inner: 2', - 'Suspend! [Async: 2]', - 'Loading...', + 'Outer: 2', + 'Sibling: 2', 'Inner and outer are consistent', ]); expect(root).toMatchRenderedOutput( <> - Outer: 2 Inner: 2 - - Loading... + Outer: 2 + Sibling: 2 , ); }); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 5723e0039e..75827aa409 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -125,13 +125,9 @@ describe('ReactSuspense', () => { // Navigate the shell to now render the child content. // This should suspend. - if (gate(flags => flags.enableSyncDefaultUpdates)) { - React.startTransition(() => { - root.update(); - }); - } else { + React.startTransition(() => { root.update(); - } + }); await waitForAll([ 'Foo', diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js index a684f6ab4d..5200635b6f 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js @@ -576,7 +576,7 @@ describe('ReactSuspenseEffectsSemantics', () => { ]); }); - // @gate enableLegacyCache && enableSyncDefaultUpdates + // @gate enableLegacyCache it('should be destroyed and recreated for function components', async () => { function App({children = null}) { Scheduler.log('App render'); @@ -642,19 +642,6 @@ describe('ReactSuspenseEffectsSemantics', () => { 'Suspend:Async', 'Text:Fallback render', 'Text:Outside render', - ]); - expect(ReactNoop).toMatchRenderedOutput( - <> - - - - , - ); - - await jest.runAllTimers(); - - // Timing out should commit the fallback and destroy inner layout effects. - assertLog([ 'Text:Inside:Before destroy layout', 'Text:Inside:After destroy layout', 'Text:Fallback create layout', @@ -711,7 +698,7 @@ describe('ReactSuspenseEffectsSemantics', () => { ]); }); - // @gate enableLegacyCache && enableSyncDefaultUpdates + // @gate enableLegacyCache it('should be destroyed and recreated for class components', async () => { class ClassText extends React.Component { componentDidMount() { @@ -796,19 +783,6 @@ describe('ReactSuspenseEffectsSemantics', () => { 'Suspend:Async', 'ClassText:Fallback render', 'ClassText:Outside render', - ]); - expect(ReactNoop).toMatchRenderedOutput( - <> - - - - , - ); - - await jest.runAllTimers(); - - // Timing out should commit the fallback and destroy inner layout effects. - assertLog([ 'ClassText:Inside:Before componentWillUnmount', 'ClassText:Inside:After componentWillUnmount', 'ClassText:Fallback componentDidMount', @@ -860,7 +834,7 @@ describe('ReactSuspenseEffectsSemantics', () => { ]); }); - // @gate enableLegacyCache && enableSyncDefaultUpdates + // @gate enableLegacyCache it('should be destroyed and recreated when nested below host components', async () => { function App({children = null}) { Scheduler.log('App render'); @@ -914,17 +888,10 @@ describe('ReactSuspenseEffectsSemantics', () => { , ); - await waitFor(['App render', 'Suspend:Async', 'Text:Fallback render']); - expect(ReactNoop).toMatchRenderedOutput( - - - , - ); - - await jest.runAllTimers(); - - // Timing out should commit the fallback and destroy inner layout effects. - assertLog([ + await waitFor([ + 'App render', + 'Suspend:Async', + 'Text:Fallback render', 'Text:Outer destroy layout', 'Text:Inner destroy layout', 'Text:Fallback create layout', @@ -979,7 +946,7 @@ describe('ReactSuspenseEffectsSemantics', () => { ]); }); - // @gate enableLegacyCache && enableSyncDefaultUpdates + // @gate enableLegacyCache it('should be destroyed and recreated even if there is a bailout because of memoization', async () => { const MemoizedText = React.memo(Text, () => true); @@ -1040,18 +1007,6 @@ describe('ReactSuspenseEffectsSemantics', () => { 'Suspend:Async', // Text:MemoizedInner is memoized 'Text:Fallback render', - ]); - expect(ReactNoop).toMatchRenderedOutput( - - - , - ); - - await jest.runAllTimers(); - - // Timing out should commit the fallback and destroy inner layout effects. - // Even though the innermost layout effects are beneath a hidden HostComponent. - assertLog([ 'Text:Outer destroy layout', 'Text:MemoizedInner destroy layout', 'Text:Fallback create layout', @@ -1448,7 +1403,7 @@ describe('ReactSuspenseEffectsSemantics', () => { ); }); - // @gate enableLegacyCache && enableSyncDefaultUpdates + // @gate enableLegacyCache it('should be cleaned up inside of a fallback that suspends', async () => { function App({fallbackChildren = null, outerChildren = null}) { return ( @@ -1501,17 +1456,6 @@ describe('ReactSuspenseEffectsSemantics', () => { 'Text:Fallback:Inside render', 'Text:Fallback:Outside render', 'Text:Outside render', - ]); - expect(ReactNoop).toMatchRenderedOutput( - <> - - - , - ); - - // Timing out should commit the fallback and destroy inner layout effects. - await jest.runAllTimers(); - assertLog([ 'Text:Inside destroy layout', 'Text:Fallback:Inside create layout', 'Text:Fallback:Outside create layout', @@ -1546,19 +1490,6 @@ describe('ReactSuspenseEffectsSemantics', () => { 'Text:Fallback:Fallback render', 'Text:Fallback:Outside render', 'Text:Outside render', - ]); - expect(ReactNoop).toMatchRenderedOutput( - <> -