From 8fea6d3c72e71b1e8c14add8876db485db585e0f Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 31 May 2022 13:31:45 -0400 Subject: [PATCH] Always push updates to interleaved queue first Interleaves updates (updates that are scheduled while another render is already is progress) go into a special queue that isn't applied until the end of the current render. They are transferred to the "real" queue at the beginning of the next render. Currently we check during `setState` whether an update should go directly onto the real queue or onto the special interleaved queue. The logic is subtle and it can lead to bugs if you mess it up, as in #24400. Instead, this changes it to always go onto the interleaved queue. The behavior is the same but the logic is simpler. As a further step, we can also wait to update the `childLanes` until the end of the current render. I'll do this in the next step. --- .../src/ReactFiberHooks.new.js | 33 ++++++------------- .../src/ReactFiberHooks.old.js | 33 ++++++------------- .../src/ReactFiberInterleavedUpdates.new.js | 4 --- .../src/ReactFiberInterleavedUpdates.old.js | 4 --- .../src/ReactFiberWorkLoop.new.js | 30 ++++------------- .../src/ReactFiberWorkLoop.old.js | 30 ++++------------- .../src/ReactUpdateQueue.new.js | 26 ++++++++------- .../src/ReactUpdateQueue.old.js | 26 ++++++++------- 8 files changed, 60 insertions(+), 126 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 1742d00ae1580..547ca69a281b8 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -87,7 +87,6 @@ import { requestUpdateLane, requestEventTime, markSkippedUpdateLanes, - isInterleavedUpdate, } from './ReactFiberWorkLoop.new'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; @@ -2337,30 +2336,18 @@ function enqueueUpdate( update: Update, lane: Lane, ) { - 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 transferred to the pending queue. - pushInterleavedQueue(queue); - } else { - update.next = interleaved.next; - interleaved.next = update; - } - queue.interleaved = update; + 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 transferred to the pending queue. + pushInterleavedQueue(queue); } 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; + update.next = interleaved.next; + interleaved.next = update; } + queue.interleaved = update; } function entangleTransitionUpdate( diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 20717bde1cd74..20318d986e6e1 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -87,7 +87,6 @@ import { requestUpdateLane, requestEventTime, markSkippedUpdateLanes, - isInterleavedUpdate, } from './ReactFiberWorkLoop.old'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; @@ -2337,30 +2336,18 @@ function enqueueUpdate( update: Update, lane: Lane, ) { - 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 transferred to the pending queue. - pushInterleavedQueue(queue); - } else { - update.next = interleaved.next; - interleaved.next = update; - } - queue.interleaved = update; + 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 transferred to the pending queue. + pushInterleavedQueue(queue); } 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; + update.next = interleaved.next; + interleaved.next = update; } + queue.interleaved = update; } function entangleTransitionUpdate( diff --git a/packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js b/packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js index 010730b1e78e0..65460c7658abc 100644 --- a/packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js +++ b/packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js @@ -28,10 +28,6 @@ export function pushInterleavedQueue( } } -export function hasInterleavedUpdates() { - return interleavedQueues !== null; -} - 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 diff --git a/packages/react-reconciler/src/ReactFiberInterleavedUpdates.old.js b/packages/react-reconciler/src/ReactFiberInterleavedUpdates.old.js index 0d3319801daa6..5fd769684ebd6 100644 --- a/packages/react-reconciler/src/ReactFiberInterleavedUpdates.old.js +++ b/packages/react-reconciler/src/ReactFiberInterleavedUpdates.old.js @@ -28,10 +28,6 @@ export function pushInterleavedQueue( } } -export function hasInterleavedUpdates() { - return interleavedQueues !== null; -} - 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 diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 6e438aaa7bb60..9dfa5b9665d9f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -195,10 +195,7 @@ import { pop as popFromStack, createCursor, } from './ReactFiberStack.new'; -import { - enqueueInterleavedUpdates, - hasInterleavedUpdates, -} from './ReactFiberInterleavedUpdates.new'; +import {enqueueInterleavedUpdates} from './ReactFiberInterleavedUpdates.new'; import { markNestedUpdateScheduled, @@ -606,8 +603,6 @@ export function scheduleUpdateOnFiber( } if (root === workInProgressRoot) { - // TODO: Consolidate with `isInterleavedUpdate` check - // 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 // `deferRenderPhaseUpdateToNextBatch` flag is off and this is a render @@ -721,25 +716,12 @@ function markUpdateLaneFromFiberToRoot( } } -export function isInterleavedUpdate(fiber: Fiber, lane: Lane) { +export function isUnsafeClassRenderPhaseUpdate(fiber: Fiber) { + // Check if this is a render phase update. Only called by class components, + // which special (deprecated) behavior for UNSAFE_componentWillReceive props. 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 || - // If the interleaved updates queue hasn't been cleared yet, then - // we should treat this as an interleaved update, too. This is also a - // defensive coding measure in case a new update comes in between when - // rendering has finished and when the interleaved updates are transferred - // to the main queue. - hasInterleavedUpdates()) && - (fiber.mode & ConcurrentMode) !== 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) + !deferRenderPhaseUpdateToNextBatch && + (executionContext & RenderContext) !== NoContext ); } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 3e02a4fe37697..8f51c66dde0d4 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -195,10 +195,7 @@ import { pop as popFromStack, createCursor, } from './ReactFiberStack.old'; -import { - enqueueInterleavedUpdates, - hasInterleavedUpdates, -} from './ReactFiberInterleavedUpdates.old'; +import {enqueueInterleavedUpdates} from './ReactFiberInterleavedUpdates.old'; import { markNestedUpdateScheduled, @@ -606,8 +603,6 @@ export function scheduleUpdateOnFiber( } if (root === workInProgressRoot) { - // TODO: Consolidate with `isInterleavedUpdate` check - // 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 // `deferRenderPhaseUpdateToNextBatch` flag is off and this is a render @@ -721,25 +716,12 @@ function markUpdateLaneFromFiberToRoot( } } -export function isInterleavedUpdate(fiber: Fiber, lane: Lane) { +export function isUnsafeClassRenderPhaseUpdate(fiber: Fiber) { + // Check if this is a render phase update. Only called by class components, + // which special (deprecated) behavior for UNSAFE_componentWillReceive props. 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 || - // If the interleaved updates queue hasn't been cleared yet, then - // we should treat this as an interleaved update, too. This is also a - // defensive coding measure in case a new update comes in between when - // rendering has finished and when the interleaved updates are transferred - // to the main queue. - hasInterleavedUpdates()) && - (fiber.mode & ConcurrentMode) !== 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) + !deferRenderPhaseUpdateToNextBatch && + (executionContext & RenderContext) !== NoContext ); } diff --git a/packages/react-reconciler/src/ReactUpdateQueue.new.js b/packages/react-reconciler/src/ReactUpdateQueue.new.js index 1f665a88dfa49..442747c5ed0f3 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.new.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.new.js @@ -107,7 +107,7 @@ import {debugRenderPhaseSideEffectsForStrictMode} from 'shared/ReactFeatureFlags import {StrictLegacyMode} from './ReactTypeOfMode'; import { markSkippedUpdateLanes, - isInterleavedUpdate, + isUnsafeClassRenderPhaseUpdate, } from './ReactFiberWorkLoop.new'; import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.new'; import {setIsStrictModeForDevtools} from './ReactFiberDevToolsHook.new'; @@ -223,7 +223,19 @@ export function enqueueUpdate( const sharedQueue: SharedQueue = (updateQueue: any).shared; - if (isInterleavedUpdate(fiber, lane)) { + if (isUnsafeClassRenderPhaseUpdate(fiber)) { + // This is an unsafe render phase update. Add directly to the update + // queue so we can process it immediately during the current render. + 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; + } else { const interleaved = sharedQueue.interleaved; if (interleaved === null) { // This is the first update. Create a circular list. @@ -236,16 +248,6 @@ export function enqueueUpdate( interleaved.next = update; } sharedQueue.interleaved = update; - } else { - 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; } if (__DEV__) { diff --git a/packages/react-reconciler/src/ReactUpdateQueue.old.js b/packages/react-reconciler/src/ReactUpdateQueue.old.js index e59ea26f2161a..a1f0f78a4b873 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.old.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.old.js @@ -107,7 +107,7 @@ import {debugRenderPhaseSideEffectsForStrictMode} from 'shared/ReactFeatureFlags import {StrictLegacyMode} from './ReactTypeOfMode'; import { markSkippedUpdateLanes, - isInterleavedUpdate, + isUnsafeClassRenderPhaseUpdate, } from './ReactFiberWorkLoop.old'; import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.old'; import {setIsStrictModeForDevtools} from './ReactFiberDevToolsHook.old'; @@ -223,7 +223,19 @@ export function enqueueUpdate( const sharedQueue: SharedQueue = (updateQueue: any).shared; - if (isInterleavedUpdate(fiber, lane)) { + if (isUnsafeClassRenderPhaseUpdate(fiber)) { + // This is an unsafe render phase update. Add directly to the update + // queue so we can process it immediately during the current render. + 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; + } else { const interleaved = sharedQueue.interleaved; if (interleaved === null) { // This is the first update. Create a circular list. @@ -236,16 +248,6 @@ export function enqueueUpdate( interleaved.next = update; } sharedQueue.interleaved = update; - } else { - 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; } if (__DEV__) {