Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove JND delay for non-transition updates #26597

Merged
merged 1 commit into from
Apr 11, 2023
Merged

Remove JND delay for non-transition updates #26597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]']);
Expand Down Expand Up @@ -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]']);
Expand Down
60 changes: 0 additions & 60 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ import {
includesExpiredLane,
getNextLanes,
getLanesToRetrySynchronouslyOnError,
getMostRecentEventTime,
markRootUpdated,
markRootSuspended as markRootSuspended_dontCallThisOneDirectly,
markRootPinged,
Expand Down Expand Up @@ -284,8 +283,6 @@ import {
} from './ReactFiberRootScheduler';
import {getMaskedContext, getUnmaskedContext} from './ReactFiberContext';

const ceil = Math.ceil;

const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map;

const {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down
8 changes: 2 additions & 6 deletions packages/react-reconciler/src/__tests__/ReactExpiration-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,13 +732,9 @@ describe('ReactExpiration', () => {
expect(root).toMatchRenderedOutput('A0BC');

await act(async () => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<App step={1} />);
});
} else {
React.startTransition(() => {
root.render(<App step={1} />);
}
});
await waitForAll(['Suspend! [A1]', 'Loading...']);

// Lots of time elapses before the promise resolves
Expand Down
41 changes: 10 additions & 31 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,24 +692,16 @@ describe('ReactHooksWithNoopRenderer', () => {
await waitForAll([0]);
expect(root).toMatchRenderedOutput(<span prop={0} />);

if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<Foo signal={false} />);
});
} else {
React.startTransition(() => {
root.render(<Foo signal={false} />);
}
});
await waitForAll(['Suspend!']);
expect(root).toMatchRenderedOutput(<span prop={0} />);

// Rendering again should suspend again.
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<Foo signal={false} />);
});
} else {
React.startTransition(() => {
root.render(<Foo signal={false} />);
}
});
await waitForAll(['Suspend!']);
});

Expand Down Expand Up @@ -755,38 +747,25 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(root).toMatchRenderedOutput(<span prop="A:0" />);

await act(async () => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<Foo signal={false} />);
setLabel('B');
});
} else {
React.startTransition(() => {
root.render(<Foo signal={false} />);
setLabel('B');
}
});

await waitForAll(['Suspend!']);
expect(root).toMatchRenderedOutput(<span prop="A:0" />);

// Rendering again should suspend again.
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<Foo signal={false} />);
});
} else {
React.startTransition(() => {
root.render(<Foo signal={false} />);
}
});
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(<Foo signal={true} />);
});
} else {
React.startTransition(() => {
root.render(<Foo signal={true} />);
}
});
await waitForAll(['B:0']);
expect(root).toMatchRenderedOutput(<span prop="B:0" />);
});
Expand Down
18 changes: 8 additions & 10 deletions packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1414,10 +1414,12 @@ describe('ReactLazy', () => {

// Swap the position of A and B
root.update(<Parent swap={true} />);
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...');
Expand Down Expand Up @@ -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(<Parent swap={true} />);
});
} else {
React.startTransition(() => {
root.update(<Parent swap={true} />);
}
});
await waitForAll(['Init B2', 'Loading...']);
await resolveFakeImport(ChildB2);
// We need to flush to trigger the second one to load.
Expand Down
72 changes: 29 additions & 43 deletions packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ let useState;
let useEffect;
let startTransition;
let textCache;
let waitFor;
let waitForPaint;
let assertLog;

Expand All @@ -28,6 +29,7 @@ describe('ReactOffscreen', () => {
startTransition = React.startTransition;

const InternalTestUtils = require('internal-test-utils');
waitFor = InternalTestUtils.waitFor;
waitForPaint = InternalTestUtils.waitForPaint;
assertLog = InternalTestUtils.assertLog;

Expand Down Expand Up @@ -407,7 +409,6 @@ describe('ReactOffscreen', () => {
expect(root).toMatchRenderedOutput(<span hidden={true}>B1</span>);
});

// 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
Expand Down Expand Up @@ -442,17 +443,17 @@ describe('ReactOffscreen', () => {
setOuter = _setOuter;
return (
<>
<span>
<Text text={'Outer: ' + outer} />
</span>
<Offscreen mode={show ? 'visible' : 'hidden'}>
<span>
<Child outer={outer} />
</span>
</Offscreen>
<span>
<Text text={'Outer: ' + outer} />
</span>
<Suspense fallback={<Text text="Loading..." />}>
<span>
<AsyncText text={'Async: ' + outer} />
<Text text={'Sibling: ' + outer} />
</span>
</Suspense>
</>
Expand All @@ -466,50 +467,41 @@ describe('ReactOffscreen', () => {
root.render(<App show={true} />);
});
assertLog([
'Outer: 0',
'Inner: 0',
'Async: 0',
'Outer: 0',
'Sibling: 0',
'Inner and outer are consistent',
]);
expect(root).toMatchRenderedOutput(
<>
<span>Outer: 0</span>
<span>Inner: 0</span>
<span>Async: 0</span>
<span>Outer: 0</span>
<span>Sibling: 0</span>
</>,
);

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(<App show={false} />);
startTransition(() => {
setOuter(1);
setInner(1);
// In the same render, also hide the offscreen tree.
root.render(<App show={false} />);
});

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(
<>
<span>Outer: 0</span>
<span>Inner: 0</span>
<span>Async: 0</span>
<span>Outer: 0</span>
<span>Sibling: 0</span>
</>,
);

Expand All @@ -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(
<>
<span>Outer: 1</span>
<span hidden={true}>Inner: 0</span>
<span hidden={true}>Async: 0</span>
Loading...
<span>Outer: 1</span>
<span>Sibling: 1</span>
</>,
);

Expand All @@ -536,32 +527,27 @@ describe('ReactOffscreen', () => {
root.render(<App show={true} />);
});
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(
<>
<span>Outer: 2</span>
<span>Inner: 2</span>
<span hidden={true}>Async: 0</span>
Loading...
<span>Outer: 2</span>
<span>Sibling: 2</span>
</>,
);
});
Expand Down
Loading