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

Add dedicated LanePriority for Suspense retries #19287

Merged
merged 1 commit into from
Jul 9, 2020
Merged

Add dedicated LanePriority for Suspense retries #19287

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
88 changes: 62 additions & 26 deletions packages/react-reconciler/src/ReactFiberLane.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ export opaque type LanePriority =
| 13
| 14
| 15
| 16;
| 16
| 17;
export opaque type Lanes = number;
export opaque type Lane = number;
export opaque type LaneMap<T> = Array<T>;
Expand All @@ -42,23 +43,25 @@ import {
NoPriority as NoSchedulerPriority,
} from './SchedulerWithReactIntegration.new';

export const SyncLanePriority: LanePriority = 16;
const SyncBatchedLanePriority: LanePriority = 15;
export const SyncLanePriority: LanePriority = 17;
const SyncBatchedLanePriority: LanePriority = 16;

const InputDiscreteHydrationLanePriority: LanePriority = 14;
export const InputDiscreteLanePriority: LanePriority = 13;
const InputDiscreteHydrationLanePriority: LanePriority = 15;
export const InputDiscreteLanePriority: LanePriority = 14;

const InputContinuousHydrationLanePriority: LanePriority = 12;
export const InputContinuousLanePriority: LanePriority = 11;
const InputContinuousHydrationLanePriority: LanePriority = 13;
export const InputContinuousLanePriority: LanePriority = 12;

const DefaultHydrationLanePriority: LanePriority = 10;
export const DefaultLanePriority: LanePriority = 9;
const DefaultHydrationLanePriority: LanePriority = 11;
export const DefaultLanePriority: LanePriority = 10;

const TransitionShortHydrationLanePriority: LanePriority = 8;
export const TransitionShortLanePriority: LanePriority = 7;
const TransitionShortHydrationLanePriority: LanePriority = 9;
export const TransitionShortLanePriority: LanePriority = 8;

const TransitionLongHydrationLanePriority: LanePriority = 6;
export const TransitionLongLanePriority: LanePriority = 5;
const TransitionLongHydrationLanePriority: LanePriority = 7;
export const TransitionLongLanePriority: LanePriority = 6;

const RetryLanePriority: LanePriority = 5;

const SelectiveHydrationLanePriority: LanePriority = 4;

Expand Down Expand Up @@ -90,25 +93,30 @@ const InputContinuousUpdateRangeStart = 6;
const InputContinuousUpdateRangeEnd = 8;

export const DefaultHydrationLane: Lane = /* */ 0b0000000000000000000000100000000;
const DefaultLanes: Lanes = /* */ 0b0000000000000000011111100000000;
export const DefaultLanes: Lanes = /* */ 0b0000000000000000000111100000000;
const DefaultUpdateRangeStart = 9;
const DefaultUpdateRangeEnd = 14;
const DefaultUpdateRangeEnd = 12;

const TransitionShortHydrationLane: Lane = /* */ 0b0000000000000000001000000000000;
const TransitionShortLanes: Lanes = /* */ 0b0000000000000011111000000000000;
const TransitionShortUpdateRangeStart = 13;
const TransitionShortUpdateRangeEnd = 17;

const TransitionShortHydrationLane: Lane = /* */ 0b0000000000000000100000000000000;
const TransitionShortLanes: Lanes = /* */ 0b0000000000011111100000000000000;
const TransitionShortUpdateRangeStart = 15;
const TransitionShortUpdateRangeEnd = 20;
const TransitionLongHydrationLane: Lane = /* */ 0b0000000000000100000000000000000;
const TransitionLongLanes: Lanes = /* */ 0b0000000001111100000000000000000;
const TransitionLongUpdateRangeStart = 18;
const TransitionLongUpdateRangeEnd = 22;

const TransitionLongHydrationLane: Lane = /* */ 0b0000000000100000000000000000000;
const TransitionLongLanes: Lanes = /* */ 0b0000011111100000000000000000000;
const TransitionLongUpdateRangeStart = 21;
const TransitionLongUpdateRangeEnd = 26;
// Includes all updates. Except Idle updates, which have special semantics.
const UpdateRangeEnd = TransitionLongUpdateRangeEnd;

export const SelectiveHydrationLane: Lane = /* */ 0b0000110000000000000000000000000;
const RetryLanes: Lanes = /* */ 0b0000011110000000000000000000000;
const RetryRangeStart = 22;
const RetryRangeEnd = 26;

export const SelectiveHydrationLane: Lane = /* */ 0b0000100000000000000000000000000;
const SelectiveHydrationRangeEnd = 27;

// Includes all non-Idle updates
const UpdateRangeEnd = 27;
const NonIdleLanes = /* */ 0b0000111111111111111111111111111;

export const IdleHydrationLane: Lane = /* */ 0b0001000000000000000000000000000;
Expand Down Expand Up @@ -206,6 +214,12 @@ function getHighestPriorityLanes(lanes: Lanes | Lane): Lanes {
return transitionLongLanes;
}
}
const retryLanes = RetryLanes & lanes;
if (retryLanes !== NoLanes) {
return_highestLanePriority = RetryLanePriority;
return_updateRangeEnd = RetryRangeEnd;
return retryLanes;
}
if (lanes & SelectiveHydrationLane) {
return_highestLanePriority = SelectiveHydrationLanePriority;
return_updateRangeEnd = SelectiveHydrationRangeEnd;
Expand Down Expand Up @@ -275,6 +289,7 @@ export function lanePriorityToSchedulerPriority(
case TransitionLongHydrationLanePriority:
case TransitionLongLanePriority:
case SelectiveHydrationLanePriority:
case RetryLanePriority:
return NormalSchedulerPriority;
case IdleHydrationLanePriority:
case IdleLanePriority:
Expand Down Expand Up @@ -537,6 +552,9 @@ export function findUpdateLane(
case TransitionLongLanePriority:
// Should be handled by findTransitionLane instead
break;
case RetryLanePriority:
// Should be handled by findRetryLane instead
break;
case IdleLanePriority:
let lane = findLane(IdleUpdateRangeStart, IdleUpdateRangeEnd, wipLanes);
if (lane === NoLane) {
Expand Down Expand Up @@ -604,6 +622,19 @@ export function findTransitionLane(
);
}

// To ensure consistency across multiple updates in the same event, this should
// be pure function, so that it always returns the same lane for given inputs.
export function findRetryLane(wipLanes: Lanes): Lane {
// This is a fork of `findUpdateLane` designed specifically for Suspense
// "retries" — a special update that attempts to flip a Suspense boundary
// from its placeholder state to its primary/resolved state.
let lane = findLane(RetryRangeStart, RetryRangeEnd, wipLanes);
if (lane === NoLane) {
lane = pickArbitraryLane(RetryLanes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late to the party here (sorry, oncall this week and a bit behind) but catching up now, I had a small thought.

The naming of pickArbitraryLane suggests (to me) that it could return a different value each time it's called. Its current implementation always happens to return a predictable/consistent value, but do we think this might ever change? If it did, the usage of it would be at conflict with the above comment:

This should be pure function, so that it always returns the same lane for given inputs.

...in which case maybe calling getLowestPriorityLane directly would be "better"

Copy link
Contributor Author

@acdlite acdlite Jul 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“arbitrary” in this case means it doesn’t matter if it’s the lowest priority lane or highest priority lane or anything in between.

For example, it communicates if there’s a refactor where the most significant lane is available for some other reason, you could use that instead without affecting the algorithm.

}
return lane;
}

function findLane(start, end, skipLanes) {
// This finds the first bit between the `start` and `end` positions that isn't
// in `skipLanes`.
Expand Down Expand Up @@ -808,6 +839,11 @@ export function getBumpedLaneForHydration(
case TransitionLongLanePriority:
lane = TransitionLongHydrationLane;
break;
case RetryLanePriority:
// Shouldn't be reachable under normal circumstances, so there's no
// dedicated lane for retry priority. Use the one for long transitions.
lane = TransitionLongHydrationLane;
break;
case SelectiveHydrationLanePriority:
lane = SelectiveHydrationLane;
break;
Expand Down
28 changes: 24 additions & 4 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ import {
NoTimestamp,
findUpdateLane,
findTransitionLane,
findRetryLane,
includesSomeLane,
isSubsetOfLanes,
mergeLanes,
Expand Down Expand Up @@ -470,6 +471,28 @@ export function requestUpdateLane(
return lane;
}

function requestRetryLane(fiber: Fiber) {
// This is a fork of `requestUpdateLane` designed specifically for Suspense
// "retries" — a special update that attempts to flip a Suspense boundary
// from its placeholder state to its primary/resolved state.

// Special cases
const mode = fiber.mode;
if ((mode & BlockingMode) === NoMode) {
return (SyncLane: Lane);
} else if ((mode & ConcurrentMode) === NoMode) {
return getCurrentPriorityLevel() === ImmediateSchedulerPriority
? (SyncLane: Lane)
: (SyncBatchedLane: Lane);
}

// See `requestUpdateLane` for explanation of `currentEventWipLanes`
if (currentEventWipLanes === NoLanes) {
currentEventWipLanes = workInProgressRootIncludedLanes;
}
return findRetryLane(currentEventWipLanes);
}

export function scheduleUpdateOnFiber(
fiber: Fiber,
lane: Lane,
Expand Down Expand Up @@ -2691,10 +2714,7 @@ function retryTimedOutBoundary(boundaryFiber: Fiber, retryLane: Lane) {
// suspended it has resolved, which means at least part of the tree was
// likely unblocked. Try rendering again, at a new expiration time.
if (retryLane === NoLane) {
const suspenseConfig = null; // Retries don't carry over the already committed update.
// TODO: Should retries get their own lane? Maybe it could share with
// transitions.
retryLane = requestUpdateLane(boundaryFiber, suspenseConfig);
retryLane = requestRetryLane(boundaryFiber);
}
// TODO: Special case idle priority?
const eventTime = requestEventTime();
Expand Down
28 changes: 24 additions & 4 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ import {
NoTimestamp,
findUpdateLane,
findTransitionLane,
findRetryLane,
includesSomeLane,
isSubsetOfLanes,
mergeLanes,
Expand Down Expand Up @@ -493,6 +494,28 @@ export function requestUpdateLane(
return lane;
}

function requestRetryLane(fiber: Fiber) {
// This is a fork of `requestUpdateLane` designed specifically for Suspense
// "retries" — a special update that attempts to flip a Suspense boundary
// from its placeholder state to its primary/resolved state.

// Special cases
const mode = fiber.mode;
if ((mode & BlockingMode) === NoMode) {
return (SyncLane: Lane);
} else if ((mode & ConcurrentMode) === NoMode) {
return getCurrentPriorityLevel() === ImmediateSchedulerPriority
? (SyncLane: Lane)
: (SyncBatchedLane: Lane);
}

// See `requestUpdateLane` for explanation of `currentEventWipLanes`
if (currentEventWipLanes === NoLanes) {
currentEventWipLanes = workInProgressRootIncludedLanes;
}
return findRetryLane(currentEventWipLanes);
}

export function scheduleUpdateOnFiber(
fiber: Fiber,
lane: Lane,
Expand Down Expand Up @@ -2838,10 +2861,7 @@ function retryTimedOutBoundary(boundaryFiber: Fiber, retryLane: Lane) {
// suspended it has resolved, which means at least part of the tree was
// likely unblocked. Try rendering again, at a new expiration time.
if (retryLane === NoLane) {
const suspenseConfig = null; // Retries don't carry over the already committed update.
// TODO: Should retries get their own lane? Maybe it could share with
// transitions.
retryLane = requestUpdateLane(boundaryFiber, suspenseConfig);
retryLane = requestRetryLane(boundaryFiber);
}
// TODO: Special case idle priority?
const eventTime = requestEventTime();
Expand Down
5 changes: 5 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,11 @@ describe('ReactSuspense', () => {
await resolve1();
ReactNoop.render(element);
expect(Scheduler).toFlushWithoutYielding();

// Force fallback to commit.
// TODO: Should be able to use `act` here.
rickhanlonii marked this conversation as resolved.
Show resolved Hide resolved
jest.runAllTimers();

expect(ReactNoop.getChildren()).toEqual([
text('Waiting Tier 2'),
text('Done'),
Expand Down
66 changes: 66 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,17 @@ describe('ReactSuspenseWithNoopRenderer', () => {
// The new reconciler batches everything together, so it finishes without
// suspending again.
'Sibling',

// NOTE: The final of the update got pushed into a lower priority range of
// lanes, leading to the extra intermediate render. This is because when
// we schedule the fourth update, we're already in the middle of rendering
// the three others. Since there are only three lanes in the default
// range, the fourth lane is shifted to slightly lower priority. This
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fourth lane is shifted to slightly lower priority

Could you expand in this more? I would think that the "priority" doesn't change, the work just gets pushed out further?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "lane priority" of a bit is determined by which range of bits it belongs to. We can't assign a new update to a lane that's already in the middle of rendering. In this (fairly contrived) test scenario, we ran out of lanes in the Default range so we had to pick from the next available lane in a lower priority range. Another option would be to throw out the in-progress work and start over — then we're free to pick any lane we want. tl;dr this is a batching heuristic that doesn't matter that much in most scenarios and probably isn't the last time this will change. It's only observable in tests if you do clever stuff like partial rendering, which we don't officially support and is only used in our own test suite.

Copy link
Member

@rickhanlonii rickhanlonii Jul 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what's confusing to me in "the fourth lane is shifted to slightly lower priority" is that the lane isn't what changes priorities? Do you mean "the fourth update is shifted to a (lane in a) slightly lower priority"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. Yeah I meant fourth update :D

// could easily change when we tweak our batching heuristics. Ideally,
// they'd all have default priority and render in a single batch.
'Suspend! [Step 3]',
'Sibling',

'Step 4',
]);
});
Expand Down Expand Up @@ -3896,4 +3907,59 @@ describe('ReactSuspenseWithNoopRenderer', () => {
expect(Scheduler).toHaveYielded(['Promise resolved [B]', 'B']);
expect(root).toMatchRenderedOutput(<span prop="B" />);
});

it('retries have lower priority than normal updates', async () => {
const {useState} = React;

let setText;
function UpdatingText() {
const [text, _setText] = useState('A');
setText = _setText;
return <Text text={text} />;
}

const root = ReactNoop.createRoot();
await ReactNoop.act(async () => {
root.render(
<>
<UpdatingText />
<Suspense fallback={<Text text="Loading..." />}>
<AsyncText text="Async" />
</Suspense>
</>,
);
});
expect(Scheduler).toHaveYielded(['A', 'Suspend! [Async]', 'Loading...']);
expect(root).toMatchRenderedOutput(
<>
<span prop="A" />
<span prop="Loading..." />
</>,
);

await ReactNoop.act(async () => {
// Resolve the promise. This will trigger a retry.
await resolveText('Async');
expect(Scheduler).toHaveYielded(['Promise resolved [Async]']);
// Before the retry happens, schedule a new update.
setText('B');

// The update should be allowed to finish before the retry is attempted.
expect(Scheduler).toFlushUntilNextPaint(['B']);
expect(root).toMatchRenderedOutput(
<>
<span prop="B" />
<span prop="Loading..." />
</>,
);
});
// Then do the retry.
expect(Scheduler).toHaveYielded(['Async']);
expect(root).toMatchRenderedOutput(
<>
<span prop="B" />
<span prop="Async" />
</>,
);
});
});
1 change: 0 additions & 1 deletion packages/react/src/__tests__/ReactProfiler-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3923,7 +3923,6 @@ describe('Profiler', () => {

expect(onPostCommit).toHaveBeenCalledTimes(1);
expect(onPostCommit.mock.calls[0][4]).toMatchInteractions([
initialRenderInteraction,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just gone?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl;dr the initial render is no longer entangled with the Suspense retry. See rationale from previous comment.

highPriUpdateInteraction,
]);

Expand Down