From f15f8f64bbc3e02911d1a112fa9bb8f7066a56ee Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 22 Jan 2021 16:01:54 -0600 Subject: [PATCH] Store interleaved updates on separate queue until end of render (#20615) ## Motivation An *interleaved* update is one that is scheduled while a render is already in progress, typically from a concurrent user input event. We have to take care not to process these updates during the current render, because a multiple interleaved updates may have been scheduled across many components; to avoid tearing, we cannot render some of those updates without rendering all of them. ## Old approach What we currently do when we detect an interleaved update is assign a lane that is not part of the current render. This has some unfortunate drawbacks. For example, we will eventually run out of lanes at a given priority level. When this happens, our last resort is to interrupt the current render and start over from scratch. If this happens enough, it can lead to starvation. More concerning, there are a suprising number of places that must separately account for this case, often in subtle ways. The maintenance complexity has led to a number of tearing bugs. ## New approach I added a new field to the update queue, `interleaved`. It's a linked list, just like the `pending` field. When an interleaved update is scheduled, we add it to the `interleaved` list instead of `pending`. Then we push the entire queue object onto a global array. When the current render exits, we iterate through the array of interleaved queues and transfer the `interleaved` list to the `pending` list. So, until the current render has exited (whether due to a commit or an interruption), it's impossible to process an interleaved update, because they have not yet been enqueued. In this new approach, we don't need to resort to clever lanes tricks to avoid inconsistencies. This should allow us to simplify a lot of the logic that's currently in ReactFiberWorkLoop and ReactFiberLane, especially `findUpdateLane` and `getNextLanes`. All the logic for interleaved updates is isolated to one place. --- .../src/ReactFiberClassComponent.new.js | 6 +- .../src/ReactFiberHooks.new.js | 72 ++++++++-- .../src/ReactFiberInterleavedUpdates.new.js | 55 ++++++++ .../src/ReactFiberNewContext.new.js | 27 +++- .../src/ReactFiberReconciler.new.js | 2 +- .../src/ReactFiberThrow.new.js | 2 +- .../src/ReactFiberWorkLoop.new.js | 24 +++- .../src/ReactUpdateQueue.new.js | 58 ++++++-- .../__tests__/ReactInterleavedUpdates-test.js | 129 ++++++++++++++++++ 9 files changed, 339 insertions(+), 36 deletions(-) create mode 100644 packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js create mode 100644 packages/react-reconciler/src/__tests__/ReactInterleavedUpdates-test.js diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.new.js b/packages/react-reconciler/src/ReactFiberClassComponent.new.js index 83004c3f2063a..9ed6dc23c9f16 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.new.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.new.js @@ -213,7 +213,7 @@ const classComponentUpdater = { update.callback = callback; } - enqueueUpdate(fiber, update); + enqueueUpdate(fiber, update, lane); scheduleUpdateOnFiber(fiber, lane, eventTime); if (__DEV__) { @@ -245,7 +245,7 @@ const classComponentUpdater = { update.callback = callback; } - enqueueUpdate(fiber, update); + enqueueUpdate(fiber, update, lane); scheduleUpdateOnFiber(fiber, lane, eventTime); if (__DEV__) { @@ -276,7 +276,7 @@ const classComponentUpdater = { update.callback = callback; } - enqueueUpdate(fiber, update); + enqueueUpdate(fiber, update, lane); scheduleUpdateOnFiber(fiber, lane, eventTime); if (__DEV__) { diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index ff4b18672e0c3..374d383030cdb 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -75,6 +75,7 @@ import { warnIfNotCurrentlyActingUpdatesInDev, warnIfNotScopedWithMatchingAct, markSkippedUpdateLanes, + isInterleavedUpdate, } from './ReactFiberWorkLoop.new'; import invariant from 'shared/invariant'; @@ -104,6 +105,7 @@ import {logStateUpdateScheduled} from './DebugTracing'; import {markStateUpdateScheduled} from './SchedulingProfiler'; import {CacheContext} from './ReactFiberCacheComponent.new'; import {createUpdate, enqueueUpdate} from './ReactUpdateQueue.new'; +import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.new'; const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals; @@ -116,8 +118,9 @@ type Update = {| priority?: ReactPriorityLevel, |}; -type UpdateQueue = {| +export type UpdateQueue = {| pending: Update | null, + interleaved: Update | null, dispatch: (A => mixed) | null, lastRenderedReducer: ((S, A) => S) | null, lastRenderedState: S | null, @@ -650,6 +653,7 @@ function mountReducer( hook.memoizedState = hook.baseState = initialState; const queue = (hook.queue = { pending: null, + interleaved: null, dispatch: null, lastRenderedReducer: reducer, lastRenderedState: (initialState: any), @@ -792,6 +796,23 @@ function updateReducer( queue.lastRenderedState = newState; } + // Interleaved updates are stored on a separate queue. We aren't going to + // process them during this render, but we do need to track which lanes + // are remaining. + const lastInterleaved = queue.interleaved; + if (lastInterleaved !== null) { + let interleaved = lastInterleaved; + do { + const interleavedLane = interleaved.lane; + currentlyRenderingFiber.lanes = mergeLanes( + currentlyRenderingFiber.lanes, + interleavedLane, + ); + markSkippedUpdateLanes(interleavedLane); + interleaved = ((interleaved: any).next: Update); + } while (interleaved !== lastInterleaved); + } + const dispatch: Dispatch = (queue.dispatch: any); return [hook.memoizedState, dispatch]; } @@ -1080,6 +1101,7 @@ function useMutableSource( // including any interleaving updates that occur. const newQueue = { pending: null, + interleaved: null, dispatch: null, lastRenderedReducer: basicStateReducer, lastRenderedState: snapshot, @@ -1135,6 +1157,7 @@ function mountState( hook.memoizedState = hook.baseState = initialState; const queue = (hook.queue = { pending: null, + interleaved: null, dispatch: null, lastRenderedReducer: basicStateReducer, lastRenderedState: (initialState: any), @@ -1812,7 +1835,7 @@ function refreshCache(fiber: Fiber, seedKey: ?() => T, seedValue: T) { cache: seededCache, }; refreshUpdate.payload = payload; - enqueueUpdate(provider, refreshUpdate); + enqueueUpdate(provider, refreshUpdate, lane); return; } } @@ -1847,17 +1870,6 @@ function dispatchAction( next: (null: any), }; - // Append the update to the end of the list. - const pending = queue.pending; - if (pending === null) { - // This is the first update. Create a circular list. - update.next = update; - } else { - update.next = pending.next; - pending.next = update; - } - queue.pending = update; - const alternate = fiber.alternate; if ( fiber === currentlyRenderingFiber || @@ -1867,7 +1879,41 @@ function dispatchAction( // queue -> linked list of updates. After this render pass, we'll restart // and apply the stashed updates on top of the work-in-progress hook. didScheduleRenderPhaseUpdateDuringThisPass = didScheduleRenderPhaseUpdate = true; + const pending = queue.pending; + if (pending === null) { + // This is the first update. Create a circular list. + update.next = update; + } else { + update.next = pending.next; + pending.next = update; + } + queue.pending = update; } else { + if (isInterleavedUpdate(fiber, lane)) { + const interleaved = queue.interleaved; + if (interleaved === null) { + // This is the first update. Create a circular list. + update.next = update; + // At the end of the current render, this queue's interleaved updates will + // be transfered to the pending queue. + pushInterleavedQueue(queue); + } else { + update.next = interleaved.next; + interleaved.next = update; + } + queue.interleaved = update; + } else { + const pending = queue.pending; + if (pending === null) { + // This is the first update. Create a circular list. + update.next = update; + } else { + update.next = pending.next; + pending.next = update; + } + queue.pending = update; + } + if ( fiber.lanes === NoLanes && (alternate === null || alternate.lanes === NoLanes) diff --git a/packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js b/packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js new file mode 100644 index 0000000000000..2b8cf9529ddd5 --- /dev/null +++ b/packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js @@ -0,0 +1,55 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import type {UpdateQueue as HookQueue} from './ReactFiberHooks.new'; +import type {SharedQueue as ClassQueue} from './ReactUpdateQueue.new'; + +// An array of all update queues that received updates during the current +// render. When this render exits, either because it finishes or because it is +// interrupted, the interleaved updates will be transfered onto the main part +// of the queue. +let interleavedQueues: Array< + HookQueue | ClassQueue, +> | null = null; + +export function pushInterleavedQueue( + queue: HookQueue | ClassQueue, +) { + if (interleavedQueues === null) { + interleavedQueues = [queue]; + } else { + interleavedQueues.push(queue); + } +} + +export function enqueueInterleavedUpdates() { + // Transfer the interleaved updates onto the main queue. Each queue has a + // `pending` field and an `interleaved` field. When they are not null, they + // point to the last node in a circular linked list. We need to append the + // interleaved list to the end of the pending list by joining them into a + // single, circular list. + if (interleavedQueues !== null) { + for (let i = 0; i < interleavedQueues.length; i++) { + const queue = interleavedQueues[i]; + const lastInterleavedUpdate = queue.interleaved; + if (lastInterleavedUpdate !== null) { + queue.interleaved = null; + const firstInterleavedUpdate = lastInterleavedUpdate.next; + const lastPendingUpdate = queue.pending; + if (lastPendingUpdate !== null) { + const firstPendingUpdate = lastPendingUpdate.next; + lastPendingUpdate.next = (firstInterleavedUpdate: any); + lastInterleavedUpdate.next = (firstPendingUpdate: any); + } + queue.pending = (lastInterleavedUpdate: any); + } + } + interleavedQueues = null; + } +} diff --git a/packages/react-reconciler/src/ReactFiberNewContext.new.js b/packages/react-reconciler/src/ReactFiberNewContext.new.js index 2a084390dcc75..ad524da736d0c 100644 --- a/packages/react-reconciler/src/ReactFiberNewContext.new.js +++ b/packages/react-reconciler/src/ReactFiberNewContext.new.js @@ -11,6 +11,7 @@ import type {ReactContext} from 'shared/ReactTypes'; import type {Fiber, ContextDependency} from './ReactInternalTypes'; import type {StackCursor} from './ReactFiberStack.new'; import type {Lanes} from './ReactFiberLane.new'; +import type {SharedQueue} from './ReactUpdateQueue.new'; import {isPrimaryRenderer} from './ReactFiberHostConfig'; import {createCursor, push, pop} from './ReactFiberStack.new'; @@ -31,7 +32,7 @@ import { import invariant from 'shared/invariant'; import is from 'shared/objectIs'; -import {createUpdate, enqueueUpdate, ForceUpdate} from './ReactUpdateQueue.new'; +import {createUpdate, ForceUpdate} from './ReactUpdateQueue.new'; import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork.new'; import {enableSuspenseServerRenderer} from 'shared/ReactFeatureFlags'; @@ -211,16 +212,30 @@ export function propagateContextChange( if (fiber.tag === ClassComponent) { // Schedule a force update on the work-in-progress. - const update = createUpdate( - NoTimestamp, - pickArbitraryLane(renderLanes), - ); + const lane = pickArbitraryLane(renderLanes); + const update = createUpdate(NoTimestamp, lane); update.tag = ForceUpdate; // TODO: Because we don't have a work-in-progress, this will add the // update to the current fiber, too, which means it will persist even if // this render is thrown away. Since it's a race condition, not sure it's // worth fixing. - enqueueUpdate(fiber, update); + + // Inlined `enqueueUpdate` to remove interleaved update check + const updateQueue = fiber.updateQueue; + if (updateQueue === null) { + // Only occurs if the fiber has been unmounted. + } else { + const sharedQueue: SharedQueue = (updateQueue: any).shared; + const pending = sharedQueue.pending; + if (pending === null) { + // This is the first update. Create a circular list. + update.next = update; + } else { + update.next = pending.next; + pending.next = update; + } + sharedQueue.pending = update; + } } fiber.lanes = mergeLanes(fiber.lanes, renderLanes); const alternate = fiber.alternate; diff --git a/packages/react-reconciler/src/ReactFiberReconciler.new.js b/packages/react-reconciler/src/ReactFiberReconciler.new.js index 980b623343dc4..08ec3c8eac87e 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.new.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.new.js @@ -314,7 +314,7 @@ export function updateContainer( update.callback = callback; } - enqueueUpdate(current, update); + enqueueUpdate(current, update, lane); scheduleUpdateOnFiber(current, lane, eventTime); return lane; diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js index 27baacda7daed..af199f0aa8e81 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -293,7 +293,7 @@ function throwException( // prevent a bail out. const update = createUpdate(NoTimestamp, SyncLane); update.tag = ForceUpdate; - enqueueUpdate(sourceFiber, update); + enqueueUpdate(sourceFiber, update, SyncLane); } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index dff64f9cae95d..d83cf9db9389d 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -204,6 +204,7 @@ import { pop as popFromStack, createCursor, } from './ReactFiberStack.new'; +import {enqueueInterleavedUpdates} from './ReactFiberInterleavedUpdates.new'; import { markNestedUpdateScheduled, @@ -534,6 +535,7 @@ export function scheduleUpdateOnFiber( } } + // TODO: Consolidate with `isInterleavedUpdate` check if (root === workInProgressRoot) { // Received an update to a tree that's in the middle of rendering. Mark // that there was an interleaved update work on this root. Unless the @@ -671,6 +673,22 @@ function markUpdateLaneFromFiberToRoot( } } +export function isInterleavedUpdate(fiber: Fiber, lane: Lane) { + return ( + // TODO: Optimize slightly by comparing to root that fiber belongs to. + // Requires some refactoring. Not a big deal though since it's rare for + // concurrent apps to have more than a single root. + workInProgressRoot !== null && + (fiber.mode & BlockingMode) !== NoMode && + // If this is a render phase update (i.e. UNSAFE_componentWillReceiveProps), + // then don't treat this as an interleaved update. This pattern is + // accompanied by a warning but we haven't fully deprecated it yet. We can + // remove once the deferRenderPhaseUpdateToNextBatch flag is enabled. + (deferRenderPhaseUpdateToNextBatch || + (executionContext & RenderContext) === NoContext) + ); +} + // Use this function to schedule a task for a root. There's only one task per // root; if a task was already scheduled, we'll check to make sure the priority // of the existing task is the same as the priority of the next level that the @@ -1352,6 +1370,8 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRootUpdatedLanes = NoLanes; workInProgressRootPingedLanes = NoLanes; + enqueueInterleavedUpdates(); + if (enableSchedulerTracing) { spawnedWorkDuringRender = null; } @@ -2282,7 +2302,7 @@ function captureCommitPhaseErrorOnRoot( ) { const errorInfo = createCapturedValue(error, sourceFiber); const update = createRootErrorUpdate(rootFiber, errorInfo, (SyncLane: Lane)); - enqueueUpdate(rootFiber, update); + enqueueUpdate(rootFiber, update, (SyncLane: Lane)); const eventTime = requestEventTime(); const root = markUpdateLaneFromFiberToRoot(rootFiber, (SyncLane: Lane)); if (root !== null) { @@ -2319,7 +2339,7 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) { errorInfo, (SyncLane: Lane), ); - enqueueUpdate(fiber, update); + enqueueUpdate(fiber, update, (SyncLane: Lane)); const eventTime = requestEventTime(); const root = markUpdateLaneFromFiberToRoot(fiber, (SyncLane: Lane)); if (root !== null) { diff --git a/packages/react-reconciler/src/ReactUpdateQueue.new.js b/packages/react-reconciler/src/ReactUpdateQueue.new.js index 132f83ab22c3a..84575950c3465 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.new.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.new.js @@ -102,7 +102,11 @@ import {Callback, ShouldCapture, DidCapture} from './ReactFiberFlags'; import {debugRenderPhaseSideEffectsForStrictMode} from 'shared/ReactFeatureFlags'; import {StrictMode} from './ReactTypeOfMode'; -import {markSkippedUpdateLanes} from './ReactFiberWorkLoop.new'; +import { + markSkippedUpdateLanes, + isInterleavedUpdate, +} from './ReactFiberWorkLoop.new'; +import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.new'; import invariant from 'shared/invariant'; @@ -121,8 +125,9 @@ export type Update = {| next: Update | null, |}; -type SharedQueue = {| +export type SharedQueue = {| pending: Update | null, + interleaved: Update | null, |}; export type UpdateQueue = {| @@ -161,6 +166,7 @@ export function initializeUpdateQueue(fiber: Fiber): void { lastBaseUpdate: null, shared: { pending: null, + interleaved: null, }, effects: null, }; @@ -200,7 +206,11 @@ export function createUpdate(eventTime: number, lane: Lane): Update<*> { return update; } -export function enqueueUpdate(fiber: Fiber, update: Update) { +export function enqueueUpdate( + fiber: Fiber, + update: Update, + lane: Lane, +) { const updateQueue = fiber.updateQueue; if (updateQueue === null) { // Only occurs if the fiber has been unmounted. @@ -208,15 +218,31 @@ export function enqueueUpdate(fiber: Fiber, update: Update) { } const sharedQueue: SharedQueue = (updateQueue: any).shared; - const pending = sharedQueue.pending; - if (pending === null) { - // This is the first update. Create a circular list. - update.next = update; + + if (isInterleavedUpdate(fiber, lane)) { + const interleaved = sharedQueue.interleaved; + if (interleaved === null) { + // This is the first update. Create a circular list. + update.next = update; + // At the end of the current render, this queue's interleaved updates will + // be transfered to the pending queue. + pushInterleavedQueue(sharedQueue); + } else { + update.next = interleaved.next; + interleaved.next = update; + } + sharedQueue.interleaved = update; } else { - update.next = pending.next; - pending.next = update; + const pending = sharedQueue.pending; + if (pending === null) { + // This is the first update. Create a circular list. + update.next = update; + } else { + update.next = pending.next; + pending.next = update; + } + sharedQueue.pending = update; } - sharedQueue.pending = update; if (__DEV__) { if ( @@ -559,6 +585,18 @@ export function processUpdateQueue( queue.firstBaseUpdate = newFirstBaseUpdate; queue.lastBaseUpdate = newLastBaseUpdate; + // Interleaved updates are stored on a separate queue. We aren't going to + // process them during this render, but we do need to track which lanes + // are remaining. + const lastInterleaved = queue.shared.interleaved; + if (lastInterleaved !== null) { + let interleaved = lastInterleaved; + do { + newLanes = mergeLanes(newLanes, interleaved.lane); + interleaved = ((interleaved: any).next: Update); + } while (interleaved !== lastInterleaved); + } + // Set the remaining expiration time to be whatever is remaining in the queue. // This should be fine because the only two other things that contribute to // expiration time are props and context. We're already in the middle of the diff --git a/packages/react-reconciler/src/__tests__/ReactInterleavedUpdates-test.js b/packages/react-reconciler/src/__tests__/ReactInterleavedUpdates-test.js new file mode 100644 index 0000000000000..d21d39aacf431 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactInterleavedUpdates-test.js @@ -0,0 +1,129 @@ +let React; +let ReactNoop; +let Scheduler; +let startTransition; +let useState; +let useEffect; + +describe('ReactInterleavedUpdates', () => { + beforeEach(() => { + jest.resetModules(); + + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + startTransition = React.unstable_startTransition; + useState = React.useState; + useEffect = React.useEffect; + }); + + function Text({text}) { + Scheduler.unstable_yieldValue(text); + return text; + } + + test('update during an interleaved event is not processed during the current render', async () => { + const updaters = []; + + function Child() { + const [state, setState] = useState(0); + useEffect(() => { + updaters.push(setState); + }, []); + return ; + } + + function updateChildren(value) { + for (let i = 0; i < updaters.length; i++) { + const setState = updaters[i]; + setState(value); + } + } + + const root = ReactNoop.createRoot(); + + await ReactNoop.act(async () => { + root.render( + <> + + + + , + ); + }); + expect(Scheduler).toHaveYielded([0, 0, 0]); + expect(root).toMatchRenderedOutput('000'); + + await ReactNoop.act(async () => { + updateChildren(1); + // Partially render the children. Only the first one. + expect(Scheduler).toFlushAndYieldThrough([1]); + + // In an interleaved event, schedule an update on each of the children. + // Including the two that haven't rendered yet. + updateChildren(2); + + // We should continue rendering without including the interleaved updates. + expect(Scheduler).toFlushUntilNextPaint([1, 1]); + expect(root).toMatchRenderedOutput('111'); + }); + // The interleaved updates flush in a separate render. + expect(Scheduler).toHaveYielded([2, 2, 2]); + expect(root).toMatchRenderedOutput('222'); + }); + + // @gate experimental + test('low priority update during an interleaved event is not processed during the current render', async () => { + // Same as previous test, but the interleaved update is lower priority than + // the in-progress render. + const updaters = []; + + function Child() { + const [state, setState] = useState(0); + useEffect(() => { + updaters.push(setState); + }, []); + return ; + } + + function updateChildren(value) { + for (let i = 0; i < updaters.length; i++) { + const setState = updaters[i]; + setState(value); + } + } + + const root = ReactNoop.createRoot(); + + await ReactNoop.act(async () => { + root.render( + <> + + + + , + ); + }); + expect(Scheduler).toHaveYielded([0, 0, 0]); + expect(root).toMatchRenderedOutput('000'); + + await ReactNoop.act(async () => { + updateChildren(1); + // Partially render the children. Only the first one. + expect(Scheduler).toFlushAndYieldThrough([1]); + + // In an interleaved event, schedule an update on each of the children. + // Including the two that haven't rendered yet. + startTransition(() => { + updateChildren(2); + }); + + // We should continue rendering without including the interleaved updates. + expect(Scheduler).toFlushUntilNextPaint([1, 1]); + expect(root).toMatchRenderedOutput('111'); + }); + // The interleaved updates flush in a separate render. + expect(Scheduler).toHaveYielded([2, 2, 2]); + expect(root).toMatchRenderedOutput('222'); + }); +});