From d5f80f1c4513de97ae88ebb274a078688b1d51ee Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 26 Jan 2021 14:23:34 -0600 Subject: [PATCH] Experiment: Unsuspend all lanes on update (#20660) Adds a feature flag to tweak the internal heuristic used to "unsuspend" lanes when a new update comes in. A lane is "suspended" if we couldn't finish rendering it because it was missing data, and we chose not to commit the fallback. (In this context, "suspended" does not include updates that finished with a fallback.) When we receive new data in the form of an update, we need to retry rendering the suspended lanes, since the new data may have unblocked the previously suspended work. For example, the new update could navigate back to an already loaded route. It's impractical to retry every combination of suspended lanes, so we need some heuristic that decides which lanes to retry and in which order. The existing heuristic roughly approximates the old Expiration Times model. It unsuspends all lower priority lanes, but leaves higher priority lanes suspended. Then when we start rendering, we choose the lanes that have the highest LanePriority and render those -- and then we add to that all the lanes that are highher priority. If this sounds terribly confusing, it's because it barely makes sense. (It made more sense in the Expiration Times world, I promise, but it was still confusing.) I don't think it's worth me trying to explain the old behavior too much because the point here is that we can replace it with something simpler. The new heurstic is to unsuspend all suspended lanes whenever there's an update. This is effectively what we already do except in a few very specific edge cases, ever since we removed the delayed suspense feature from everything that's not a refresh transition. We can optimize this in the future to only unsuspend lanes that are either 1) in the `lanes` or `subtreeLanes` of the node that was updated, or 2) in the `lanes` of the return path of the node that was updated. This would exclude lanes that are only located in unrelated sibling trees. But, this optimization wouldn't be useful currently because we assign the same transition lane to all transitions. It will become relevant again once we start assigning arbitrary lanes to transitions -- but that in turn requires us to implement entanglement of overlapping transitions, one of our planned projects. So to sum up: the goal here is to remove the weird edge cases and switch to a simpler model, on top of which we can make more substantial improvements. I put it behind a flag so I can run an A/B test and confirm it doesn't cause a regression. --- .../src/ReactFiberLane.new.js | 40 ++++++++++++++++--- .../src/ReactFiberLane.old.js | 40 ++++++++++++++++--- .../src/__tests__/ReactExpiration-test.js | 20 ++++++---- .../src/__tests__/ReactSuspenseList-test.js | 37 ++++++----------- packages/shared/ReactFeatureFlags.js | 3 ++ .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.native.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.testing.js | 1 + .../forks/ReactFeatureFlags.testing.www.js | 1 + .../forks/ReactFeatureFlags.www-dynamic.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 14 files changed, 106 insertions(+), 43 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js index 351035d0c341a..4570c10a62b42 100644 --- a/packages/react-reconciler/src/ReactFiberLane.new.js +++ b/packages/react-reconciler/src/ReactFiberLane.new.js @@ -36,7 +36,10 @@ export type Lane = number; export type LaneMap = Array; import invariant from 'shared/invariant'; -import {enableCache} from 'shared/ReactFeatureFlags'; +import { + enableCache, + enableTransitionEntanglement, +} from 'shared/ReactFeatureFlags'; import { ImmediatePriority as ImmediateSchedulerPriority, @@ -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 @@ -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); diff --git a/packages/react-reconciler/src/ReactFiberLane.old.js b/packages/react-reconciler/src/ReactFiberLane.old.js index 7a00fe095e6c6..7be355d2c2458 100644 --- a/packages/react-reconciler/src/ReactFiberLane.old.js +++ b/packages/react-reconciler/src/ReactFiberLane.old.js @@ -36,7 +36,10 @@ export type Lane = number; export type LaneMap = Array; import invariant from 'shared/invariant'; -import {enableCache} from 'shared/ReactFeatureFlags'; +import { + enableCache, + enableTransitionEntanglement, +} from 'shared/ReactFeatureFlags'; import { ImmediatePriority as ImmediateSchedulerPriority, @@ -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 @@ -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); diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js index 1b4433670974f..fd224b2a1f813 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js @@ -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. - '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', ]); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js index c45a10c99d6d8..de74c18e5ca6a 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js @@ -2443,7 +2443,7 @@ describe('ReactSuspenseList', () => { expect(ReactNoop).toMatchRenderedOutput(null); - ReactNoop.act(() => { + await ReactNoop.act(async () => { // Add a few items at the end. updateLowPri(true); @@ -2451,33 +2451,22 @@ describe('ReactSuspenseList', () => { 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(Loading A); - 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(Loading A); + expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading A']); + expect(ReactNoop).toMatchRenderedOutput(Loading A); + }); await AsyncA.resolve(); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 8611a9da06bc6..e09482a525ac3 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -149,3 +149,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; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 0fd02c2fd3f18..494cae50c8b4d 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -58,6 +58,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 diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index c76048a8b2b44..6c4616798eb37 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -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 diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 447d77effbc4e..3971598060d52 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -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 diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index 6a6d8f39c59b4..0d31d83b657f0 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -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 diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 56e7dde89dfbd..f0479741a570e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -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 diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 537c3fbf1c090..3c2da8f5fc9d9 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -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 diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index 24dd557047005..26eda01b2a244 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -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 diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index cf858b767cc3b..e6cbe5c387f5d 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -55,3 +55,4 @@ export const enableUseRefAccessWarning = __VARIANT__; export const enableProfilerNestedUpdateScheduledHook = __VARIANT__; export const disableSchedulerTimeoutInWorkLoop = __VARIANT__; +export const enableTransitionEntanglement = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 85ce6eca57284..f39c8bee578e2 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -31,6 +31,7 @@ export const { enableUseRefAccessWarning, disableNativeComponentFrames, disableSchedulerTimeoutInWorkLoop, + enableTransitionEntanglement, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build.