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.

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

Experiment: Unsuspend all lanes on update #20660

Merged
merged 1 commit into from
Jan 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 34 additions & 6 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 @@ -306,9 +309,15 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
return NoLanes;
}

// If there are higher priority lanes, we'll include them even if they
// are suspended.
nextLanes = pendingLanes & getEqualOrHigherPriorityLanes(nextLanes);
if (enableTransitionEntanglement) {
// We don't need to include higher priority lanes, because in this
// experiment we always unsuspend all transitions whenever we receive
// an update.
} else {
// If there are higher priority lanes, we'll include them even if they
// are suspended.
nextLanes = pendingLanes & getEqualOrHigherPriorityLanes(nextLanes);
}

// If we're already in the middle of a render, switching lanes will interrupt
// it and we'll lose our progress. We should only do this if the new lanes are
Expand Down Expand Up @@ -673,8 +682,27 @@ export function markRootUpdated(
// Unsuspend any update at equal or lower priority.
const higherPriorityLanes = updateLane - 1; // Turns 0b1000 into 0b0111

root.suspendedLanes &= higherPriorityLanes;
root.pingedLanes &= higherPriorityLanes;
if (enableTransitionEntanglement) {
// If there are any suspended transitions, it's possible this new update
// could unblock them. Clear the suspended lanes so that we can try rendering
// them again.
//
// TODO: We really only need to unsuspend only lanes that are in the
// `subtreeLanes` of the updated fiber, or the update lanes of the return
// path. This would exclude suspended updates in an unrelated sibling tree,
// since there's no way for this update to unblock it.
//
// We don't do this if the incoming update is idle, because we never process
// idle updates until after all the regular updates have finished; there's no
// way it could unblock a transition.
if ((updateLane & IdleLanes) === NoLanes) {
root.suspendedLanes = NoLanes;
root.pingedLanes = NoLanes;
}
} else {
root.suspendedLanes &= higherPriorityLanes;
root.pingedLanes &= higherPriorityLanes;
}

const eventTimes = root.eventTimes;
const index = laneToIndex(updateLane);
Expand Down
40 changes: 34 additions & 6 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 @@ -306,9 +309,15 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
return NoLanes;
}

// If there are higher priority lanes, we'll include them even if they
// are suspended.
nextLanes = pendingLanes & getEqualOrHigherPriorityLanes(nextLanes);
if (enableTransitionEntanglement) {
// We don't need to include higher priority lanes, because in this
// experiment we always unsuspend all transitions whenever we receive
// an update.
} else {
// If there are higher priority lanes, we'll include them even if they
// are suspended.
nextLanes = pendingLanes & getEqualOrHigherPriorityLanes(nextLanes);
}

// If we're already in the middle of a render, switching lanes will interrupt
// it and we'll lose our progress. We should only do this if the new lanes are
Expand Down Expand Up @@ -673,8 +682,27 @@ export function markRootUpdated(
// Unsuspend any update at equal or lower priority.
const higherPriorityLanes = updateLane - 1; // Turns 0b1000 into 0b0111

root.suspendedLanes &= higherPriorityLanes;
root.pingedLanes &= higherPriorityLanes;
if (enableTransitionEntanglement) {
// If there are any suspended transitions, it's possible this new update
// could unblock them. Clear the suspended lanes so that we can try rendering
// them again.
//
// TODO: We really only need to unsuspend only lanes that are in the
// `subtreeLanes` of the updated fiber, or the update lanes of the return
// path. This would exclude suspended updates in an unrelated sibling tree,
// since there's no way for this update to unblock it.
//
// We don't do this if the incoming update is idle, because we never process
// idle updates until after all the regular updates have finished; there's no
// way it could unblock a transition.
if ((updateLane & IdleLanes) === NoLanes) {
root.suspendedLanes = NoLanes;
root.pingedLanes = NoLanes;
}
} else {
root.suspendedLanes &= higherPriorityLanes;
root.pingedLanes &= higherPriorityLanes;
}

const eventTimes = root.eventTimes;
const index = laneToIndex(updateLane);
Expand Down
20 changes: 13 additions & 7 deletions packages/react-reconciler/src/__tests__/ReactExpiration-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -733,13 +733,19 @@ describe('ReactExpiration', () => {
// Both normal pri updates should have expired.
expect(Scheduler).toFlushExpired([
'Sibling',
// Note: we also flushed the high pri update here, because in the
// current implementation, once we pick the next lanes to work on, we
// entangle it with all pending at equal or higher priority. We could
// feasibly change this heuristic so that the high pri update doesn't
// render until after the expired updates have finished. But the
// important thing in this test is that the normal updates expired.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I love when a test fails and there's a comment that anticipates the exact reason. Thank you to the very smart, cool, and interesting person who wrote it! (me if that wasn't clear)

'High pri: 1',
gate(flags => flags.enableTransitionEntanglement)
? // Notice that the high pri update didn't flush yet. Expiring one lane
// doesn't affect other lanes. (Unless they are intentionally
// entangled, like we do for overlapping transitions that affect the
// same state.)
'High pri: 0'
: // In the current implementation, once we pick the next lanes to work
// on, we entangle it with all pending at equal or higher priority.
// We could feasibly change this heuristic so that the high pri
// update doesn't render until after the expired updates have
// finished. But the important thing in this test is that the normal
// updates expired.
'High pri: 1',
'Normal pri: 2',
'Sibling',
]);
Expand Down
37 changes: 13 additions & 24 deletions packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2443,41 +2443,30 @@ describe('ReactSuspenseList', () => {

expect(ReactNoop).toMatchRenderedOutput(null);

ReactNoop.act(() => {
await ReactNoop.act(async () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I rewrote this test to decouple it from the weird current behavior of discrete updates (async for a bit, unless another event comes in). Passes in both implementations.

// Add a few items at the end.
updateLowPri(true);

// Flush partially through.
expect(Scheduler).toFlushAndYieldThrough(['B', 'C']);

// Schedule another update at higher priority.
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => updateHighPri(true),
);
ReactNoop.flushSync(() => updateHighPri(true));

// That will intercept the previous render.
});
expect(Scheduler).toHaveYielded([
'Suspend! [A]',
'Loading A',
// Re-render at forced.
'Suspend! [A]',
'Loading A',
]);
expect(ReactNoop).toMatchRenderedOutput(<span>Loading A</span>);

jest.runAllTimers();

expect(Scheduler).toHaveYielded([
// First attempt at high pri.
'Suspend! [A]',
'Loading A',
// Re-render at forced.
'Suspend! [A]',
'Loading A',
// We auto-commit this on DEV.
// Try again on low-pri.
'Suspend! [A]',
'Loading A',
// Re-render at forced.
'Suspend! [A]',
'Loading A',
]);

expect(ReactNoop).toMatchRenderedOutput(<span>Loading A</span>);
expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading A']);
expect(ReactNoop).toMatchRenderedOutput(<span>Loading A</span>);
});

await AsyncA.resolve();

Expand Down
3 changes: 3 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,3 +143,6 @@ export const enableUseRefAccessWarning = false;
export const enableRecursiveCommitTraversal = false;

export const disableSchedulerTimeoutInWorkLoop = false;

// Experiment to simplify/improve how transitions are scheduled
export const enableTransitionEntanglement = false;
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export const enableUseRefAccessWarning = false;

export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export const enableUseRefAccessWarning = false;

export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export const enableUseRefAccessWarning = false;

export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export const enableUseRefAccessWarning = false;

export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export const enableUseRefAccessWarning = false;

export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export const enableUseRefAccessWarning = false;

export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export const enableUseRefAccessWarning = false;

export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,4 @@ export const enableUseRefAccessWarning = __VARIANT__;

export const enableProfilerNestedUpdateScheduledHook = __VARIANT__;
export const disableSchedulerTimeoutInWorkLoop = __VARIANT__;
export const enableTransitionEntanglement = __VARIANT__;
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export const {
enableUseRefAccessWarning,
disableNativeComponentFrames,
disableSchedulerTimeoutInWorkLoop,
enableTransitionEntanglement,
} = dynamicFeatureFlags;

// On WWW, __EXPERIMENTAL__ is used for a new modern build.
Expand Down