Skip to content

Commit

Permalink
Don't shift interleaved updates to separate lane
Browse files Browse the repository at this point in the history
Now that interleaved updates are added to a special queue, we no longer
need to shift them into their own lane. We can add to a lane that's
already in the middle of rendering without risk of tearing.

See #20615 for more background.

I've only changed this in the new fork, and only behind the
enableTransitionEntanglements flag.

Most of this commit involves updating tests. The "shift-to-a-new" lane
trick was intentionally used in a handful of tests where two or more
updates need to be scheduled in different lanes. Most of these tests
were written before `startTransition` existed, and all of them were
written before transitions were assigned arbitrary lanes.

So I ported these tests to use `startTransition` instead, which is the
idiomatic way to mark an update as parallel.

I didn't change the old fork at all. Writing these tests in such a way
that they also pass in the old fork actually revealed a few flaws in the
current implementation regarding interrupting a suspended refresh
transition early, which is a good reminder that we should be writing our
tests using idiomatic patterns as much as we possibly can.
  • Loading branch information
acdlite committed Feb 8, 2021
1 parent 261cf29 commit 62ae188
Show file tree
Hide file tree
Showing 11 changed files with 465 additions and 334 deletions.
123 changes: 79 additions & 44 deletions packages/react-reconciler/src/ReactFiberLane.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ export type Lane = number;
export type LaneMap<T> = Array<T>;

import invariant from 'shared/invariant';
import {enableCache} from 'shared/ReactFeatureFlags';
import {
enableCache,
enableTransitionEntanglement,
} from 'shared/ReactFeatureFlags';

import {
ImmediatePriority as ImmediateSchedulerPriority,
Expand Down Expand Up @@ -498,56 +501,88 @@ export function findUpdateLane(
lanePriority: LanePriority,
wipLanes: Lanes,
): Lane {
switch (lanePriority) {
case NoLanePriority:
break;
case SyncLanePriority:
return SyncLane;
case SyncBatchedLanePriority:
return SyncBatchedLane;
case InputDiscreteLanePriority: {
const lane = pickArbitraryLane(InputDiscreteLanes & ~wipLanes);
if (lane === NoLane) {
// Shift to the next priority level
return findUpdateLane(InputContinuousLanePriority, wipLanes);
if (enableTransitionEntanglement) {
// Ignore wipLanes. Always assign to the same bit per priority.
switch (lanePriority) {
case NoLanePriority:
break;
case SyncLanePriority:
return SyncLane;
case SyncBatchedLanePriority:
return SyncBatchedLane;
case InputDiscreteLanePriority: {
return pickArbitraryLane(InputDiscreteLanes);
}
return lane;
}
case InputContinuousLanePriority: {
const lane = pickArbitraryLane(InputContinuousLanes & ~wipLanes);
if (lane === NoLane) {
// Shift to the next priority level
return findUpdateLane(DefaultLanePriority, wipLanes);
case InputContinuousLanePriority: {
return pickArbitraryLane(InputContinuousLanes);
}
case DefaultLanePriority: {
return pickArbitraryLane(DefaultLanes);
}
return lane;
case TransitionPriority: // Should be handled by findTransitionLane instead
case RetryLanePriority: // Should be handled by findRetryLane instead
break;
case IdleLanePriority:
return pickArbitraryLane(IdleLanes);
default:
// The remaining priorities are not valid for updates
break;
}
case DefaultLanePriority: {
let lane = pickArbitraryLane(DefaultLanes & ~wipLanes);
if (lane === NoLane) {
// If all the default lanes are already being worked on, look for a
// lane in the transition range.
lane = pickArbitraryLane(TransitionLanes & ~wipLanes);
} else {
// Old behavior that uses wipLanes to shift interleaved updates into a
// separate lane. This is no longer needed because we put interleaved
// updates on a special queue.
switch (lanePriority) {
case NoLanePriority:
break;
case SyncLanePriority:
return SyncLane;
case SyncBatchedLanePriority:
return SyncBatchedLane;
case InputDiscreteLanePriority: {
const lane = pickArbitraryLane(InputDiscreteLanes & ~wipLanes);
if (lane === NoLane) {
// All the transition lanes are taken, too. This should be very
// rare, but as a last resort, pick a default lane. This will have
// the effect of interrupting the current work-in-progress render.
lane = pickArbitraryLane(DefaultLanes);
// Shift to the next priority level
return findUpdateLane(InputContinuousLanePriority, wipLanes);
}
return lane;
}
return lane;
}
case TransitionPriority: // Should be handled by findTransitionLane instead
case RetryLanePriority: // Should be handled by findRetryLane instead
break;
case IdleLanePriority:
let lane = pickArbitraryLane(IdleLanes & ~wipLanes);
if (lane === NoLane) {
lane = pickArbitraryLane(IdleLanes);
case InputContinuousLanePriority: {
const lane = pickArbitraryLane(InputContinuousLanes & ~wipLanes);
if (lane === NoLane) {
// Shift to the next priority level
return findUpdateLane(DefaultLanePriority, wipLanes);
}
return lane;
}
return lane;
default:
// The remaining priorities are not valid for updates
break;
case DefaultLanePriority: {
let lane = pickArbitraryLane(DefaultLanes & ~wipLanes);
if (lane === NoLane) {
// If all the default lanes are already being worked on, look for a
// lane in the transition range.
lane = pickArbitraryLane(TransitionLanes & ~wipLanes);
if (lane === NoLane) {
// All the transition lanes are taken, too. This should be very
// rare, but as a last resort, pick a default lane. This will have
// the effect of interrupting the current work-in-progress render.
lane = pickArbitraryLane(DefaultLanes);
}
}
return lane;
}
case TransitionPriority: // Should be handled by findTransitionLane instead
case RetryLanePriority: // Should be handled by findRetryLane instead
break;
case IdleLanePriority:
let lane = pickArbitraryLane(IdleLanes & ~wipLanes);
if (lane === NoLane) {
lane = pickArbitraryLane(IdleLanes);
}
return lane;
default:
// The remaining priorities are not valid for updates
break;
}
}
invariant(
false,
Expand Down
123 changes: 79 additions & 44 deletions packages/react-reconciler/src/ReactFiberLane.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ export type Lane = number;
export type LaneMap<T> = Array<T>;

import invariant from 'shared/invariant';
import {enableCache} from 'shared/ReactFeatureFlags';
import {
enableCache,
enableTransitionEntanglement,
} from 'shared/ReactFeatureFlags';

import {
ImmediatePriority as ImmediateSchedulerPriority,
Expand Down Expand Up @@ -498,56 +501,88 @@ export function findUpdateLane(
lanePriority: LanePriority,
wipLanes: Lanes,
): Lane {
switch (lanePriority) {
case NoLanePriority:
break;
case SyncLanePriority:
return SyncLane;
case SyncBatchedLanePriority:
return SyncBatchedLane;
case InputDiscreteLanePriority: {
const lane = pickArbitraryLane(InputDiscreteLanes & ~wipLanes);
if (lane === NoLane) {
// Shift to the next priority level
return findUpdateLane(InputContinuousLanePriority, wipLanes);
if (enableTransitionEntanglement) {
// Ignore wipLanes. Always assign to the same bit per priority.
switch (lanePriority) {
case NoLanePriority:
break;
case SyncLanePriority:
return SyncLane;
case SyncBatchedLanePriority:
return SyncBatchedLane;
case InputDiscreteLanePriority: {
return pickArbitraryLane(InputDiscreteLanes);
}
return lane;
}
case InputContinuousLanePriority: {
const lane = pickArbitraryLane(InputContinuousLanes & ~wipLanes);
if (lane === NoLane) {
// Shift to the next priority level
return findUpdateLane(DefaultLanePriority, wipLanes);
case InputContinuousLanePriority: {
return pickArbitraryLane(InputContinuousLanes);
}
case DefaultLanePriority: {
return pickArbitraryLane(DefaultLanes);
}
return lane;
case TransitionPriority: // Should be handled by findTransitionLane instead
case RetryLanePriority: // Should be handled by findRetryLane instead
break;
case IdleLanePriority:
return pickArbitraryLane(IdleLanes);
default:
// The remaining priorities are not valid for updates
break;
}
case DefaultLanePriority: {
let lane = pickArbitraryLane(DefaultLanes & ~wipLanes);
if (lane === NoLane) {
// If all the default lanes are already being worked on, look for a
// lane in the transition range.
lane = pickArbitraryLane(TransitionLanes & ~wipLanes);
} else {
// Old behavior that uses wipLanes to shift interleaved updates into a
// separate lane. This is no longer needed because we put interleaved
// updates on a special queue.
switch (lanePriority) {
case NoLanePriority:
break;
case SyncLanePriority:
return SyncLane;
case SyncBatchedLanePriority:
return SyncBatchedLane;
case InputDiscreteLanePriority: {
const lane = pickArbitraryLane(InputDiscreteLanes & ~wipLanes);
if (lane === NoLane) {
// All the transition lanes are taken, too. This should be very
// rare, but as a last resort, pick a default lane. This will have
// the effect of interrupting the current work-in-progress render.
lane = pickArbitraryLane(DefaultLanes);
// Shift to the next priority level
return findUpdateLane(InputContinuousLanePriority, wipLanes);
}
return lane;
}
return lane;
}
case TransitionPriority: // Should be handled by findTransitionLane instead
case RetryLanePriority: // Should be handled by findRetryLane instead
break;
case IdleLanePriority:
let lane = pickArbitraryLane(IdleLanes & ~wipLanes);
if (lane === NoLane) {
lane = pickArbitraryLane(IdleLanes);
case InputContinuousLanePriority: {
const lane = pickArbitraryLane(InputContinuousLanes & ~wipLanes);
if (lane === NoLane) {
// Shift to the next priority level
return findUpdateLane(DefaultLanePriority, wipLanes);
}
return lane;
}
return lane;
default:
// The remaining priorities are not valid for updates
break;
case DefaultLanePriority: {
let lane = pickArbitraryLane(DefaultLanes & ~wipLanes);
if (lane === NoLane) {
// If all the default lanes are already being worked on, look for a
// lane in the transition range.
lane = pickArbitraryLane(TransitionLanes & ~wipLanes);
if (lane === NoLane) {
// All the transition lanes are taken, too. This should be very
// rare, but as a last resort, pick a default lane. This will have
// the effect of interrupting the current work-in-progress render.
lane = pickArbitraryLane(DefaultLanes);
}
}
return lane;
}
case TransitionPriority: // Should be handled by findTransitionLane instead
case RetryLanePriority: // Should be handled by findRetryLane instead
break;
case IdleLanePriority:
let lane = pickArbitraryLane(IdleLanes & ~wipLanes);
if (lane === NoLane) {
lane = pickArbitraryLane(IdleLanes);
}
return lane;
default:
// The remaining priorities are not valid for updates
break;
}
}
invariant(
false,
Expand Down
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
disableSchedulerTimeoutInWorkLoop,
enableDoubleInvokingEffects,
skipUnmountedBoundaries,
enableTransitionEntanglement,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -832,6 +833,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
let exitStatus = renderRootConcurrent(root, lanes);

if (
!enableTransitionEntanglement &&
includesSomeLane(
workInProgressRootIncludedLanes,
workInProgressRootUpdatedLanes,
Expand Down Expand Up @@ -1037,6 +1039,7 @@ function performSyncWorkOnRoot(root) {
lanes = workInProgressRootRenderLanes;
exitStatus = renderRootSync(root, lanes);
if (
!enableTransitionEntanglement &&
includesSomeLane(
workInProgressRootIncludedLanes,
workInProgressRootUpdatedLanes,
Expand Down
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
disableSchedulerTimeoutInWorkLoop,
enableDoubleInvokingEffects,
skipUnmountedBoundaries,
enableTransitionEntanglement,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -832,6 +833,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
let exitStatus = renderRootConcurrent(root, lanes);

if (
!enableTransitionEntanglement &&
includesSomeLane(
workInProgressRootIncludedLanes,
workInProgressRootUpdatedLanes,
Expand Down Expand Up @@ -1037,6 +1039,7 @@ function performSyncWorkOnRoot(root) {
lanes = workInProgressRootRenderLanes;
exitStatus = renderRootSync(root, lanes);
if (
!enableTransitionEntanglement &&
includesSomeLane(
workInProgressRootIncludedLanes,
workInProgressRootUpdatedLanes,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,9 @@ describe('DebugTracing', () => {
]);
});

// This test is coupled to lane implementation details, so I'm disabling it in
// the new fork until it stabilizes so we don't have to repeatedly update it.
// @gate !enableTransitionEntanglement
// @gate experimental && build === 'development' && enableDebugTracing
it('should log cascading passive updates', () => {
function Example() {
Expand Down
Loading

0 comments on commit 62ae188

Please sign in to comment.