From 104d586c9093c3a78d6581d0413eb7921595ce9c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 27 Jan 2021 00:55:02 -0600 Subject: [PATCH] Assign different lanes to consecutive transitions Currently we always assign the same lane to all transitions. This means if there are two pending transitions at the same time, neither transition can finish until both can finish, even if they affect completely separate parts of the UI. The new approach is to assign a different lane to each consecutive transition, by shifting the bit to the right each time. When we reach the end of the bit range, we cycle back to the first bit. In practice, this should mean that all transitions get their own dedicated lane, unless we have more pending transitions than lanes, which should be rare. We retain our existing behavior of assigning the same lane to all transitions within the same event. This is achieved by caching the first lane assigned to a transition, then re-using that one until the next React task, by which point the event must have finished. This preserves the guarantee that all transition updates that result from a single event will be consistent. --- .../src/ReactFiberLane.new.js | 112 ++--- .../src/ReactFiberWorkLoop.new.js | 33 +- .../src/__tests__/ReactExpiration-test.js | 2 +- .../src/__tests__/ReactTransition-test.js | 394 +++++++++++++++++- 4 files changed, 426 insertions(+), 115 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js index 88c4d1946cb6d..6444ca8df784b 100644 --- a/packages/react-reconciler/src/ReactFiberLane.new.js +++ b/packages/react-reconciler/src/ReactFiberLane.new.js @@ -95,6 +95,7 @@ export const DefaultLanes: Lanes = /* */ 0b0000000000000000000 const TransitionHydrationLane: Lane = /* */ 0b0000000000000000001000000000000; const TransitionLanes: Lanes = /* */ 0b0000000001111111110000000000000; +const SomeTransitionLane: Lane = /* */ 0b0000000000000000010000000000000; const RetryLanes: Lanes = /* */ 0b0000011110000000000000000000000; @@ -113,6 +114,9 @@ export const NoTimestamp = -1; let currentUpdateLanePriority: LanePriority = NoLanePriority; +let nextTransitionLane: Lane = SomeTransitionLane; +let nextRetryLane: Lane = SomeRetryLane; + export function getCurrentUpdateLanePriority(): LanePriority { return currentUpdateLanePriority; } @@ -313,9 +317,11 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes { // We don't need to do anything extra here, because we apply per-lane // transition entanglement in the entanglement loop below. } else { - // If there are higher priority lanes, we'll include them even if they - // are suspended. - nextLanes = pendingLanes & getEqualOrHigherPriorityLanes(nextLanes); + // When per-lane entanglement is not enabled, always entangle all pending + // transitions together. + if (nextLanes & TransitionLanes) { + nextLanes |= pendingLanes & TransitionLanes; + } } // If we're already in the middle of a render, switching lanes will interrupt @@ -350,6 +356,11 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes { // entanglement is usually "best effort": we'll try our best to render the // lanes in the same batch, but it's not worth throwing out partially // completed work in order to do it. + // TODO: Reconsider this. The counter-argument is that the partial work + // represents an intermediate state, which we don't want to show to the user. + // And by spending extra time finishing it, we're increasing the amount of + // time it takes to show the final state, which is what they are actually + // waiting for. // // For those exceptions where entanglement is semantically important, like // useMutableSource, we should ensure that there is no partial work at the @@ -559,34 +570,23 @@ export function findUpdateLane( ); } -// 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 findTransitionLane(wipLanes: Lanes, pendingLanes: Lanes): Lane { - // First look for lanes that are completely unclaimed, i.e. have no - // pending work. - let lane = pickArbitraryLane(TransitionLanes & ~pendingLanes); - if (lane === NoLane) { - // If all lanes have pending work, look for a lane that isn't currently - // being worked on. - lane = pickArbitraryLane(TransitionLanes & ~wipLanes); - if (lane === NoLane) { - // If everything is being worked on, pick any lane. This has the - // effect of interrupting the current work-in-progress. - lane = pickArbitraryLane(TransitionLanes); - } +export function claimNextTransitionLane(): Lane { + // Cycle through the lanes, assigning each new transition to the next lane. + // In most cases, this means every transition gets its own lane, until we + // run out of lanes and cycle back to the beginning. + const lane = nextTransitionLane; + nextTransitionLane <<= 1; + if ((nextTransitionLane & TransitionLanes) === 0) { + nextTransitionLane = SomeTransitionLane; } return lane; } -// 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 = pickArbitraryLane(RetryLanes & ~wipLanes); - if (lane === NoLane) { - lane = pickArbitraryLane(RetryLanes); +export function claimNextRetryLane(): Lane { + const lane = nextRetryLane; + nextRetryLane <<= 1; + if ((nextRetryLane & RetryLanes) === 0) { + nextRetryLane = SomeRetryLane; } return lane; } @@ -595,16 +595,6 @@ function getHighestPriorityLane(lanes: Lanes) { return lanes & -lanes; } -function getLowestPriorityLane(lanes: Lanes): Lane { - // This finds the most significant non-zero bit. - const index = 31 - clz32(lanes); - return index < 0 ? NoLanes : 1 << index; -} - -function getEqualOrHigherPriorityLanes(lanes: Lanes | Lane): Lanes { - return (getLowestPriorityLane(lanes) << 1) - 1; -} - export function pickArbitraryLane(lanes: Lanes): Lane { // This wrapper function gets inlined. Only exists so to communicate that it // doesn't matter which bit is selected; you can pick any bit without @@ -676,39 +666,21 @@ export function markRootUpdated( ) { root.pendingLanes |= updateLane; - // TODO: Theoretically, any update to any lane can unblock any other lane. But - // it's not practical to try every single possible combination. We need a - // heuristic to decide which lanes to attempt to render, and in which batches. - // For now, we use the same heuristic as in the old ExpirationTimes model: - // retry any lane at equal or lower priority, but don't try updates at higher - // priority without also including the lower priority updates. This works well - // when considering updates across different priority levels, but isn't - // sufficient for updates within the same priority, since we want to treat - // those updates as parallel. - - // Unsuspend any update at equal or lower priority. - const higherPriorityLanes = updateLane - 1; // Turns 0b1000 into 0b0111 - - 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; + // 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; } const eventTimes = root.eventTimes; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 59fce3d254f0b..9908c2a8a607d 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -144,8 +144,8 @@ import { SyncBatchedLane, NoTimestamp, findUpdateLane, - findTransitionLane, - findRetryLane, + claimNextTransitionLane, + claimNextRetryLane, includesSomeLane, isSubsetOfLanes, mergeLanes, @@ -300,8 +300,6 @@ let workInProgressRootUpdatedLanes: Lanes = NoLanes; // Lanes that were pinged (in an interleaved event) during this render. let workInProgressRootPingedLanes: Lanes = NoLanes; -let mostRecentlyUpdatedRoot: FiberRoot | null = null; - // The most recent time we committed a fallback. This lets us ensure a train // model where we don't commit new loading states in too quick succession. let globalMostRecentFallbackTime: number = 0; @@ -358,7 +356,7 @@ let spawnedWorkDuringRender: null | Array = null; // between the first and second call. let currentEventTime: number = NoTimestamp; let currentEventWipLanes: Lanes = NoLanes; -let currentEventPendingLanes: Lanes = NoLanes; +let currentEventTransitionLane: Lanes = NoLanes; // Dev only flag that tracks if passive effects are currently being flushed. // We warn about state updates for unmounted components differently in this case. @@ -426,20 +424,17 @@ export function requestUpdateLane(fiber: Fiber): Lane { // event. Then reset the cached values once we can be sure the event is over. // Our heuristic for that is whenever we enter a concurrent work loop. // - // We'll do the same for `currentEventPendingLanes` below. + // We'll do the same for `currentEventTransitionLane` below. if (currentEventWipLanes === NoLanes) { currentEventWipLanes = workInProgressRootIncludedLanes; } const isTransition = requestCurrentTransition() !== NoTransition; if (isTransition) { - if (currentEventPendingLanes !== NoLanes) { - currentEventPendingLanes = - mostRecentlyUpdatedRoot !== null - ? mostRecentlyUpdatedRoot.pendingLanes - : NoLanes; + if (currentEventTransitionLane === NoLane) { + currentEventTransitionLane = claimNextTransitionLane(); } - return findTransitionLane(currentEventWipLanes, currentEventPendingLanes); + return currentEventTransitionLane; } // TODO: Remove this dependency on the Scheduler priority. @@ -492,7 +487,8 @@ function requestRetryLane(fiber: Fiber) { if (currentEventWipLanes === NoLanes) { currentEventWipLanes = workInProgressRootIncludedLanes; } - return findRetryLane(currentEventWipLanes); + + return claimNextRetryLane(); } export function scheduleUpdateOnFiber( @@ -616,13 +612,6 @@ export function scheduleUpdateOnFiber( schedulePendingInteractions(root, lane); } - // We use this when assigning a lane for a transition inside - // `requestUpdateLane`. We assume it's the same as the root being updated, - // since in the common case of a single root app it probably is. If it's not - // the same root, then it's not a huge deal, we just might batch more stuff - // together more than necessary. - mostRecentlyUpdatedRoot = root; - return root; } @@ -770,7 +759,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) { // event time. The next update will compute a new event time. currentEventTime = NoTimestamp; currentEventWipLanes = NoLanes; - currentEventPendingLanes = NoLanes; + currentEventTransitionLane = NoLanes; invariant( (executionContext & (RenderContext | CommitContext)) === NoContext, @@ -2437,6 +2426,8 @@ 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) { + // TODO: Assign this to `suspenseState.retryLane`? to avoid + // unnecessary entanglement? retryLane = requestRetryLane(boundaryFiber); } // TODO: Special case idle priority? diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js index fd224b2a1f813..c28a3ba155824 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js @@ -733,7 +733,7 @@ describe('ReactExpiration', () => { // Both normal pri updates should have expired. expect(Scheduler).toFlushExpired([ 'Sibling', - gate(flags => flags.enableTransitionEntanglement) + gate(flags => flags.enableTransitionEntanglement || flags.new) ? // 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 diff --git a/packages/react-reconciler/src/__tests__/ReactTransition-test.js b/packages/react-reconciler/src/__tests__/ReactTransition-test.js index 1b25cf7dd1879..390dbccf362fa 100644 --- a/packages/react-reconciler/src/__tests__/ReactTransition-test.js +++ b/packages/react-reconciler/src/__tests__/ReactTransition-test.js @@ -16,7 +16,12 @@ let Scheduler; let Suspense; let useState; let useTransition; +let startTransition; let act; +let getCacheForType; + +let caches; +let seededCache; describe('ReactTransition', () => { beforeEach(() => { @@ -27,44 +32,142 @@ describe('ReactTransition', () => { useState = React.useState; useTransition = React.unstable_useTransition; Suspense = React.Suspense; + startTransition = React.unstable_startTransition; + getCacheForType = React.unstable_getCacheForType; act = ReactNoop.act; + + caches = []; + seededCache = null; }); - function Text(props) { - Scheduler.unstable_yieldValue(props.text); - return props.text; + function createTextCache() { + if (seededCache !== null) { + // Trick to seed a cache before it exists. + // TODO: Need a built-in API to seed data before the initial render (i.e. + // not a refresh because nothing has mounted yet). + const cache = seededCache; + seededCache = null; + return cache; + } + + const data = new Map(); + const version = caches.length + 1; + const cache = { + version, + data, + resolve(text) { + const record = data.get(text); + if (record === undefined) { + const newRecord = { + status: 'resolved', + value: text, + }; + data.set(text, newRecord); + } else if (record.status === 'pending') { + const thenable = record.value; + record.status = 'resolved'; + record.value = text; + thenable.pings.forEach(t => t()); + } + }, + reject(text, error) { + const record = data.get(text); + if (record === undefined) { + const newRecord = { + status: 'rejected', + value: error, + }; + data.set(text, newRecord); + } else if (record.status === 'pending') { + const thenable = record.value; + record.status = 'rejected'; + record.value = error; + thenable.pings.forEach(t => t()); + } + }, + }; + caches.push(cache); + return cache; } - function createAsyncText(text) { - let resolved = false; - const Component = function() { - if (!resolved) { - Scheduler.unstable_yieldValue('Suspend! [' + text + ']'); - throw promise; + function readText(text) { + const textCache = getCacheForType(createTextCache); + const record = textCache.data.get(text); + if (record !== undefined) { + switch (record.status) { + case 'pending': + Scheduler.unstable_yieldValue(`Suspend! [${text}]`); + throw record.value; + case 'rejected': + Scheduler.unstable_yieldValue(`Error! [${text}]`); + throw record.value; + case 'resolved': + return textCache.version; } - return ; - }; - const promise = new Promise(resolve => { - Component.resolve = function() { - resolved = true; - return resolve(); + } else { + Scheduler.unstable_yieldValue(`Suspend! [${text}]`); + + const thenable = { + pings: [], + then(resolve) { + if (newRecord.status === 'pending') { + thenable.pings.push(resolve); + } else { + Promise.resolve().then(() => resolve(newRecord.value)); + } + }, }; - }); - return Component; + + const newRecord = { + status: 'pending', + value: thenable, + }; + textCache.data.set(text, newRecord); + + throw thenable; + } + } + + function Text({text}) { + Scheduler.unstable_yieldValue(text); + return text; + } + + function AsyncText({text}) { + readText(text); + Scheduler.unstable_yieldValue(text); + return text; + } + + function seedNextTextCache(text) { + if (seededCache === null) { + seededCache = createTextCache(); + } + seededCache.resolve(text); + } + + function resolveText(text) { + if (caches.length === 0) { + throw Error('Cache does not exist.'); + } else { + // Resolve the most recently created cache. An older cache can by + // resolved with `caches[index].resolve(text)`. + caches[caches.length - 1].resolve(text); + } } // @gate experimental - it('isPending works even if called from outside an input event', async () => { - const Async = createAsyncText('Async'); + // @gate enableCache + test('isPending works even if called from outside an input event', async () => { let start; function App() { const [show, setShow] = useState(false); - const [startTransition, isPending] = useTransition(); - start = () => startTransition(() => setShow(true)); + const [_start, isPending] = useTransition(); + start = () => _start(() => setShow(true)); return ( }> {isPending ? : null} - {show ? : } + {show ? : } ); } @@ -89,9 +192,254 @@ describe('ReactTransition', () => { expect(root).toMatchRenderedOutput('Pending...(empty)'); - await Async.resolve(); + await resolveText('Async'); }); expect(Scheduler).toHaveYielded(['Async']); expect(root).toMatchRenderedOutput('Async'); }); + + // @gate experimental + // @gate enableCache + test( + 'when multiple transitions update the same queue, only the most recent ' + + 'one is allowed to finish (no intermediate states)', + async () => { + let update; + function App() { + const [startContentChange, isContentPending] = useTransition(); + const [label, setLabel] = useState('A'); + const [contents, setContents] = useState('A'); + update = value => { + ReactNoop.discreteUpdates(() => { + setLabel(value); + startContentChange(() => { + setContents(value); + }); + }); + }; + return ( + <> + +
+ }> + + +
+ + ); + } + + // Initial render + const root = ReactNoop.createRoot(); + await act(async () => { + seedNextTextCache('A content'); + root.render(); + }); + expect(Scheduler).toHaveYielded(['A label', 'A content']); + expect(root).toMatchRenderedOutput( + <> + A label
A content
+ , + ); + + // Switch to B + await act(async () => { + update('B'); + }); + expect(Scheduler).toHaveYielded([ + // Commit pending state + 'B label (loading...)', + 'A content', + + // Attempt to render B, but it suspends + 'B label', + 'Suspend! [B content]', + 'Loading...', + ]); + // This is a refresh transition so it shouldn't show a fallback + expect(root).toMatchRenderedOutput( + <> + B label (loading...)
A content
+ , + ); + + // Before B finishes loading, switch to C + await act(async () => { + update('C'); + }); + expect(Scheduler).toHaveYielded([ + // Commit pending state + 'C label (loading...)', + 'A content', + + // Attempt to render C, but it suspends + 'C label', + 'Suspend! [C content]', + 'Loading...', + ]); + expect(root).toMatchRenderedOutput( + <> + C label (loading...)
A content
+ , + ); + + // Finish loading B. But we're not allowed to render B because it's + // entangled with C. So we're still pending. + await act(async () => { + resolveText('B content'); + }); + expect(Scheduler).toHaveYielded([ + // Attempt to render C, but it suspends + 'C label', + 'Suspend! [C content]', + 'Loading...', + ]); + expect(root).toMatchRenderedOutput( + <> + C label (loading...)
A content
+ , + ); + + // Now finish loading C. This is the terminal update, so it can finish. + await act(async () => { + resolveText('C content'); + }); + expect(Scheduler).toHaveYielded(['C label', 'C content']); + expect(root).toMatchRenderedOutput( + <> + C label
C content
+ , + ); + }, + ); + + // Same as previous test, but for class update queue. + // @gate experimental + // @gate enableCache + test( + 'when multiple transitions update the same queue, only the most recent ' + + 'one is allowed to finish (no intermediate states) (classes)', + async () => { + let update; + class App extends React.Component { + state = { + label: 'A', + contents: 'A', + }; + render() { + update = value => { + ReactNoop.discreteUpdates(() => { + this.setState({label: value}); + startTransition(() => { + this.setState({contents: value}); + }); + }); + }; + const label = this.state.label; + const contents = this.state.contents; + const isContentPending = label !== contents; + return ( + <> + +
+ }> + + +
+ + ); + } + } + + // Initial render + const root = ReactNoop.createRoot(); + await act(async () => { + seedNextTextCache('A content'); + root.render(); + }); + expect(Scheduler).toHaveYielded(['A label', 'A content']); + expect(root).toMatchRenderedOutput( + <> + A label
A content
+ , + ); + + // Switch to B + await act(async () => { + update('B'); + }); + expect(Scheduler).toHaveYielded([ + // Commit pending state + 'B label (loading...)', + 'A content', + + // Attempt to render B, but it suspends + 'B label', + 'Suspend! [B content]', + 'Loading...', + ]); + // This is a refresh transition so it shouldn't show a fallback + expect(root).toMatchRenderedOutput( + <> + B label (loading...)
A content
+ , + ); + + // Before B finishes loading, switch to C + await act(async () => { + update('C'); + }); + expect(Scheduler).toHaveYielded([ + // Commit pending state + 'C label (loading...)', + 'A content', + + // Attempt to render C, but it suspends + 'C label', + 'Suspend! [C content]', + 'Loading...', + ]); + expect(root).toMatchRenderedOutput( + <> + C label (loading...)
A content
+ , + ); + + // Finish loading B. But we're not allowed to render B because it's + // entangled with C. So we're still pending. + await act(async () => { + resolveText('B content'); + }); + expect(Scheduler).toHaveYielded([ + // Attempt to render C, but it suspends + 'C label', + 'Suspend! [C content]', + 'Loading...', + ]); + expect(root).toMatchRenderedOutput( + <> + C label (loading...)
A content
+ , + ); + + // Now finish loading C. This is the terminal update, so it can finish. + await act(async () => { + resolveText('C content'); + }); + expect(Scheduler).toHaveYielded(['C label', 'C content']); + expect(root).toMatchRenderedOutput( + <> + C label
C content
+ , + ); + }, + ); });