From 34ee3919c39bc9b149462322713a9811db4b8498 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Tue, 17 Dec 2024 11:56:00 -0500 Subject: [PATCH 1/4] Clean up enableLazyContextPropagation (#31810) This flag has shipped everywhere, let's clean it up. --- ...eactLegacyContextDisabled-test.internal.js | 10 +- .../src/ReactFiberBeginWork.js | 71 +----- .../src/ReactFiberClassComponent.js | 5 +- .../react-reconciler/src/ReactFiberHooks.js | 33 ++- .../src/ReactFiberNewContext.js | 214 ++---------------- .../react-reconciler/src/ReactFiberThrow.js | 29 ++- .../__tests__/ReactContextPropagation-test.js | 5 - .../__tests__/ReactContextValidator-test.js | 8 +- packages/shared/ReactFeatureFlags.js | 2 - .../forks/ReactFeatureFlags.native-fb.js | 1 - .../forks/ReactFeatureFlags.native-oss.js | 1 - .../forks/ReactFeatureFlags.test-renderer.js | 1 - ...actFeatureFlags.test-renderer.native-fb.js | 2 - .../ReactFeatureFlags.test-renderer.www.js | 1 - .../shared/forks/ReactFeatureFlags.www.js | 1 - 15 files changed, 53 insertions(+), 331 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactLegacyContextDisabled-test.internal.js b/packages/react-dom/src/__tests__/ReactLegacyContextDisabled-test.internal.js index 7c68b275c232b..257c061fb46a0 100644 --- a/packages/react-dom/src/__tests__/ReactLegacyContextDisabled-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactLegacyContextDisabled-test.internal.js @@ -233,15 +233,7 @@ describe('ReactLegacyContextDisabled', () => { ); }); expect(container.textContent).toBe('bbb'); - if (gate(flags => flags.enableLazyContextPropagation)) { - // In the lazy propagation implementation, we don't check if context - // changed until after shouldComponentUpdate is run. - expect(lifecycleContextLog).toEqual(['b', 'b', 'b']); - } else { - // In the eager implementation, a dirty flag was set when the parent - // changed, so we skipped sCU. - expect(lifecycleContextLog).toEqual(['b', 'b']); - } + expect(lifecycleContextLog).toEqual(['b', 'b', 'b']); root.unmount(); }); }); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 2d3e3c04c48ea..632b2df70c097 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -37,8 +37,6 @@ import type { import type {UpdateQueue} from './ReactFiberClassUpdateQueue'; import type {RootState} from './ReactFiberRoot'; import type {TracingMarkerInstance} from './ReactFiberTracingMarkerComponent'; -import type {TransitionStatus} from './ReactFiberConfig'; -import type {Hook} from './ReactFiberHooks'; import { markComponentRenderStarted, @@ -100,7 +98,6 @@ import { enableProfilerCommitHooks, enableProfilerTimer, enableScopeAPI, - enableLazyContextPropagation, enableSchedulingProfiler, enableTransitionTracing, enableLegacyHidden, @@ -272,7 +269,6 @@ import { createClassErrorUpdate, initializeClassErrorUpdate, } from './ReactFiberThrow'; -import is from 'shared/objectIs'; import { getForksAtLevel, isForkedChild, @@ -843,7 +839,7 @@ function deferHiddenOffscreenComponent( pushOffscreenSuspenseHandler(workInProgress); - if (enableLazyContextPropagation && current !== null) { + if (current !== null) { // Since this tree will resume rendering in a separate render, we need // to propagate parent contexts now so we don't lose track of which // ones changed. @@ -1636,26 +1632,6 @@ function updateHostComponent( } else { HostTransitionContext._currentValue2 = newState; } - if (enableLazyContextPropagation) { - // In the lazy propagation implementation, we don't scan for matching - // consumers until something bails out. - } else { - if (didReceiveUpdate) { - if (current !== null) { - const oldStateHook: Hook = current.memoizedState; - const oldState: TransitionStatus = oldStateHook.memoizedState; - // This uses regular equality instead of Object.is because we assume - // that host transition state doesn't include NaN as a valid type. - if (oldState !== newState) { - propagateContextChange( - workInProgress, - HostTransitionContext, - renderLanes, - ); - } - } - } - } } markRef(current, workInProgress); @@ -2706,10 +2682,7 @@ function updateDehydratedSuspenseComponent( } if ( - enableLazyContextPropagation && // TODO: Factoring is a little weird, since we check this right below, too. - // But don't want to re-arrange the if-else chain until/unless this - // feature lands. !didReceiveUpdate ) { // We need to check if any children have context before we decide to bail @@ -3300,8 +3273,6 @@ function updateContextProvider( context = workInProgress.type._context; } const newProps = workInProgress.pendingProps; - const oldProps = workInProgress.memoizedProps; - const newValue = newProps.value; if (__DEV__) { @@ -3317,34 +3288,6 @@ function updateContextProvider( pushProvider(workInProgress, context, newValue); - if (enableLazyContextPropagation) { - // In the lazy propagation implementation, we don't scan for matching - // consumers until something bails out, because until something bails out - // we're going to visit those nodes, anyway. The trade-off is that it shifts - // responsibility to the consumer to track whether something has changed. - } else { - if (oldProps !== null) { - const oldValue = oldProps.value; - if (is(oldValue, newValue)) { - // No change. Bailout early if children are the same. - if ( - oldProps.children === newProps.children && - !hasLegacyContextChanged() - ) { - return bailoutOnAlreadyFinishedWork( - current, - workInProgress, - renderLanes, - ); - } - } else { - // The context value changed. Search for matching consumers and schedule - // them to update. - propagateContextChange(workInProgress, context, renderLanes); - } - } - } - const newChildren = newProps.children; reconcileChildren(current, workInProgress, newChildren, renderLanes); return workInProgress.child; @@ -3463,7 +3406,7 @@ function bailoutOnAlreadyFinishedWork( // TODO: Once we add back resuming, we should check if the children are // a work-in-progress set. If so, we need to transfer their effects. - if (enableLazyContextPropagation && current !== null) { + if (current !== null) { // Before bailing out, check if there are any context changes in // the children. lazilyPropagateParentContextChanges(current, workInProgress, renderLanes); @@ -3564,11 +3507,9 @@ function checkScheduledUpdateOrContext( } // No pending update, but because context is propagated lazily, we need // to check for a context change before we bail out. - if (enableLazyContextPropagation) { - const dependencies = current.dependencies; - if (dependencies !== null && checkIfContextChanged(dependencies)) { - return true; - } + const dependencies = current.dependencies; + if (dependencies !== null && checkIfContextChanged(dependencies)) { + return true; } return false; } @@ -3706,7 +3647,7 @@ function attemptEarlyBailoutIfNoScheduledUpdate( workInProgress.childLanes, ); - if (enableLazyContextPropagation && !hasChildWork) { + if (!hasChildWork) { // Context changes may not have been propagated yet. We need to do // that now, before we can decide whether to bail out. // TODO: We use `childLanes` as a heuristic for whether there is diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index f5d5c96a2a93c..7ebf0fb9f4b92 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -21,7 +21,6 @@ import { debugRenderPhaseSideEffectsForStrictMode, disableLegacyContext, enableSchedulingProfiler, - enableLazyContextPropagation, disableDefaultPropsExceptForClasses, } from 'shared/ReactFeatureFlags'; import ReactStrictModeWarnings from './ReactStrictModeWarnings'; @@ -1092,7 +1091,6 @@ function updateClassInstance( !hasContextChanged() && !checkHasForceUpdateAfterProcessing() && !( - enableLazyContextPropagation && current !== null && current.dependencies !== null && checkIfContextChanged(current.dependencies) @@ -1144,8 +1142,7 @@ function updateClassInstance( // both before and after `shouldComponentUpdate` has been called. Not ideal, // but I'm loath to refactor this function. This only happens for memoized // components so it's not that common. - (enableLazyContextPropagation && - current !== null && + (current !== null && current.dependencies !== null && checkIfContextChanged(current.dependencies)); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 771b60cbb0894..12b110b16d3c9 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -36,7 +36,6 @@ import { import ReactSharedInternals from 'shared/ReactSharedInternals'; import { enableSchedulingProfiler, - enableLazyContextPropagation, enableTransitionTracing, enableUseEffectEventHook, enableUseResourceEffectHook, @@ -743,23 +742,21 @@ function finishRenderingHooks( ); } - if (enableLazyContextPropagation) { - if (current !== null) { - if (!checkIfWorkInProgressReceivedUpdate()) { - // If there were no changes to props or state, we need to check if there - // was a context change. We didn't already do this because there's no - // 1:1 correspondence between dependencies and hooks. Although, because - // there almost always is in the common case (`readContext` is an - // internal API), we could compare in there. OTOH, we only hit this case - // if everything else bails out, so on the whole it might be better to - // keep the comparison out of the common path. - const currentDependencies = current.dependencies; - if ( - currentDependencies !== null && - checkIfContextChanged(currentDependencies) - ) { - markWorkInProgressReceivedUpdate(); - } + if (current !== null) { + if (!checkIfWorkInProgressReceivedUpdate()) { + // If there were no changes to props or state, we need to check if there + // was a context change. We didn't already do this because there's no + // 1:1 correspondence between dependencies and hooks. Although, because + // there almost always is in the common case (`readContext` is an + // internal API), we could compare in there. OTOH, we only hit this case + // if everything else bails out, so on the whole it might be better to + // keep the comparison out of the common path. + const currentDependencies = current.dependencies; + if ( + currentDependencies !== null && + checkIfContextChanged(currentDependencies) + ) { + markWorkInProgressReceivedUpdate(); } } } diff --git a/packages/react-reconciler/src/ReactFiberNewContext.js b/packages/react-reconciler/src/ReactFiberNewContext.js index 6a779cfd4ab4c..b10dc5ce547ae 100644 --- a/packages/react-reconciler/src/ReactFiberNewContext.js +++ b/packages/react-reconciler/src/ReactFiberNewContext.js @@ -15,24 +15,13 @@ import type { } from './ReactInternalTypes'; import type {StackCursor} from './ReactFiberStack'; import type {Lanes} from './ReactFiberLane'; -import type {SharedQueue} from './ReactFiberClassUpdateQueue'; import type {TransitionStatus} from './ReactFiberConfig'; import type {Hook} from './ReactFiberHooks'; import {isPrimaryRenderer, HostTransitionContext} from './ReactFiberConfig'; import {createCursor, push, pop} from './ReactFiberStack'; -import { - ContextProvider, - ClassComponent, - DehydratedFragment, -} from './ReactWorkTags'; -import { - NoLanes, - isSubsetOfLanes, - includesSomeLane, - mergeLanes, - pickArbitraryLane, -} from './ReactFiberLane'; +import {ContextProvider, DehydratedFragment} from './ReactWorkTags'; +import {NoLanes, isSubsetOfLanes, mergeLanes} from './ReactFiberLane'; import { NoFlags, DidPropagateContext, @@ -40,12 +29,7 @@ import { } from './ReactFiberFlags'; import is from 'shared/objectIs'; -import {createUpdate, ForceUpdate} from './ReactFiberClassUpdateQueue'; -import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork'; -import { - enableLazyContextPropagation, - enableRenderableContext, -} from 'shared/ReactFeatureFlags'; +import {enableRenderableContext} from 'shared/ReactFeatureFlags'; import {getHostTransitionProvider} from './ReactFiberHostContext'; const valueCursor: StackCursor = createCursor(null); @@ -210,157 +194,16 @@ export function propagateContextChange( context: ReactContext, renderLanes: Lanes, ): void { - if (enableLazyContextPropagation) { - // TODO: This path is only used by Cache components. Update - // lazilyPropagateParentContextChanges to look for Cache components so they - // can take advantage of lazy propagation. - const forcePropagateEntireTree = true; - propagateContextChanges( - workInProgress, - [context], - renderLanes, - forcePropagateEntireTree, - ); - } else { - propagateContextChange_eager(workInProgress, context, renderLanes); - } -} - -function propagateContextChange_eager( - workInProgress: Fiber, - context: ReactContext, - renderLanes: Lanes, -): void { - // Only used by eager implementation - if (enableLazyContextPropagation) { - return; - } - let fiber = workInProgress.child; - if (fiber !== null) { - // Set the return pointer of the child to the work-in-progress fiber. - fiber.return = workInProgress; - } - while (fiber !== null) { - let nextFiber; - - // Visit this fiber. - const list = fiber.dependencies; - if (list !== null) { - nextFiber = fiber.child; - - let dependency = list.firstContext; - while (dependency !== null) { - // Check if the context matches. - if (dependency.context === context) { - // Match! Schedule an update on this fiber. - if (fiber.tag === ClassComponent) { - // Schedule a force update on the work-in-progress. - const lane = pickArbitraryLane(renderLanes); - const update = createUpdate(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. - - // 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; - if (alternate !== null) { - alternate.lanes = mergeLanes(alternate.lanes, renderLanes); - } - scheduleContextWorkOnParentPath( - fiber.return, - renderLanes, - workInProgress, - ); - - // Mark the updated lanes on the list, too. - list.lanes = mergeLanes(list.lanes, renderLanes); - - // Since we already found a match, we can stop traversing the - // dependency list. - break; - } - dependency = dependency.next; - } - } else if (fiber.tag === ContextProvider) { - // Don't scan deeper if this is a matching provider - nextFiber = fiber.type === workInProgress.type ? null : fiber.child; - } else if (fiber.tag === DehydratedFragment) { - // If a dehydrated suspense boundary is in this subtree, we don't know - // if it will have any context consumers in it. The best we can do is - // mark it as having updates. - const parentSuspense = fiber.return; - - if (parentSuspense === null) { - throw new Error( - 'We just came from a parent so we must have had a parent. This is a bug in React.', - ); - } - - parentSuspense.lanes = mergeLanes(parentSuspense.lanes, renderLanes); - const alternate = parentSuspense.alternate; - if (alternate !== null) { - alternate.lanes = mergeLanes(alternate.lanes, renderLanes); - } - // This is intentionally passing this fiber as the parent - // because we want to schedule this fiber as having work - // on its children. We'll use the childLanes on - // this fiber to indicate that a context has changed. - scheduleContextWorkOnParentPath( - parentSuspense, - renderLanes, - workInProgress, - ); - nextFiber = fiber.sibling; - } else { - // Traverse down. - nextFiber = fiber.child; - } - - if (nextFiber !== null) { - // Set the return pointer of the child to the work-in-progress fiber. - nextFiber.return = fiber; - } else { - // No child. Traverse to next sibling. - nextFiber = fiber; - while (nextFiber !== null) { - if (nextFiber === workInProgress) { - // We're back to the root of this subtree. Exit. - nextFiber = null; - break; - } - const sibling = nextFiber.sibling; - if (sibling !== null) { - // Set the return pointer of the sibling to the work-in-progress fiber. - sibling.return = nextFiber.return; - nextFiber = sibling; - break; - } - // No more siblings. Traverse up. - nextFiber = nextFiber.return; - } - } - fiber = nextFiber; - } + // TODO: This path is only used by Cache components. Update + // lazilyPropagateParentContextChanges to look for Cache components so they + // can take advantage of lazy propagation. + const forcePropagateEntireTree = true; + propagateContextChanges( + workInProgress, + [context], + renderLanes, + forcePropagateEntireTree, + ); } function propagateContextChanges( @@ -369,10 +212,6 @@ function propagateContextChanges( renderLanes: Lanes, forcePropagateEntireTree: boolean, ): void { - // Only used by lazy implementation - if (!enableLazyContextPropagation) { - return; - } let fiber = workInProgress.child; if (fiber !== null) { // Set the return pointer of the child to the work-in-progress fiber. @@ -527,10 +366,6 @@ function propagateParentContextChanges( renderLanes: Lanes, forcePropagateEntireTree: boolean, ) { - if (!enableLazyContextPropagation) { - return; - } - // Collect all the parent providers that changed. Since this is usually small // number, we use an Array instead of Set. let contexts = null; @@ -637,9 +472,6 @@ function propagateParentContextChanges( export function checkIfContextChanged( currentDependencies: Dependencies, ): boolean { - if (!enableLazyContextPropagation) { - return false; - } // Iterate over the current dependencies to see if something changed. This // only gets called if props and state has already bailed out, so it's a // relatively uncommon path, except at the root of a changed subtree. @@ -669,20 +501,8 @@ export function prepareToReadContext( const dependencies = workInProgress.dependencies; if (dependencies !== null) { - if (enableLazyContextPropagation) { - // Reset the work-in-progress list - dependencies.firstContext = null; - } else { - const firstContext = dependencies.firstContext; - if (firstContext !== null) { - if (includesSomeLane(dependencies.lanes, renderLanes)) { - // Context list has a pending update. Mark that this fiber performed work. - markWorkInProgressReceivedUpdate(); - } - // Reset the work-in-progress list - dependencies.firstContext = null; - } - } + // Reset the work-in-progress list + dependencies.firstContext = null; } } @@ -749,9 +569,7 @@ function readContextForConsumer( lanes: NoLanes, firstContext: contextItem, }; - if (enableLazyContextPropagation) { - consumer.flags |= NeedsPropagation; - } + consumer.flags |= NeedsPropagation; } else { // Append a new context item. lastContextDependency = lastContextDependency.next = contextItem; diff --git a/packages/react-reconciler/src/ReactFiberThrow.js b/packages/react-reconciler/src/ReactFiberThrow.js index a4bdb84e6c4e7..a777331699082 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.js +++ b/packages/react-reconciler/src/ReactFiberThrow.js @@ -39,7 +39,6 @@ import { } from './ReactFiberFlags'; import {NoMode, ConcurrentMode} from './ReactTypeOfMode'; import { - enableLazyContextPropagation, enableUpdaterTracking, enablePostpone, disableLegacyMode, @@ -199,21 +198,19 @@ function initializeClassErrorUpdate( } function resetSuspendedComponent(sourceFiber: Fiber, rootRenderLanes: Lanes) { - if (enableLazyContextPropagation) { - const currentSourceFiber = sourceFiber.alternate; - if (currentSourceFiber !== null) { - // Since we never visited the children of the suspended component, we - // need to propagate the context change now, to ensure that we visit - // them during the retry. - // - // We don't have to do this for errors because we retry errors without - // committing in between. So this is specific to Suspense. - propagateParentContextChangesToDeferredTree( - currentSourceFiber, - sourceFiber, - rootRenderLanes, - ); - } + const currentSourceFiber = sourceFiber.alternate; + if (currentSourceFiber !== null) { + // Since we never visited the children of the suspended component, we + // need to propagate the context change now, to ensure that we visit + // them during the retry. + // + // We don't have to do this for errors because we retry errors without + // committing in between. So this is specific to Suspense. + propagateParentContextChangesToDeferredTree( + currentSourceFiber, + sourceFiber, + rootRenderLanes, + ); } // Reset the memoizedState to what it was before we attempted to render it. diff --git a/packages/react-reconciler/src/__tests__/ReactContextPropagation-test.js b/packages/react-reconciler/src/__tests__/ReactContextPropagation-test.js index 340e973b2f0b3..019119edf7ce8 100644 --- a/packages/react-reconciler/src/__tests__/ReactContextPropagation-test.js +++ b/packages/react-reconciler/src/__tests__/ReactContextPropagation-test.js @@ -35,11 +35,6 @@ describe('ReactLazyContextPropagation', () => { seededCache = null; }); - // NOTE: These tests are not specific to the lazy propagation (as opposed to - // eager propagation). The behavior should be the same in both - // implementations. These are tests that are more relevant to the lazy - // propagation implementation, though. - function createTextCache() { if (seededCache !== null) { // Trick to seed a cache before it exists. diff --git a/packages/react/src/__tests__/ReactContextValidator-test.js b/packages/react/src/__tests__/ReactContextValidator-test.js index 3f24e5bed8d18..359857af15c61 100644 --- a/packages/react/src/__tests__/ReactContextValidator-test.js +++ b/packages/react/src/__tests__/ReactContextValidator-test.js @@ -350,13 +350,7 @@ describe('ReactContextValidator', () => { expect(componentWillUpdateNextContext).toBe(secondContext); expect(renderContext).toBe(secondContext); expect(componentDidUpdateContext).toBe(secondContext); - - if (gate(flags => flags.enableLazyContextPropagation)) { - expect(shouldComponentUpdateWasCalled).toBe(true); - } else { - // sCU is not called in this case because React force updates when a provider re-renders - expect(shouldComponentUpdateWasCalled).toBe(false); - } + expect(shouldComponentUpdateWasCalled).toBe(true); }); it('should re-render PureComponents when context Provider updates', async () => { diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 8caf79fbf285d..fff1dcf4d9ddb 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -99,8 +99,6 @@ export const enableObjectFiber = false; export const enableTransitionTracing = false; -export const enableLazyContextPropagation = true; - // FB-only usage. The new API has different semantics. export const enableLegacyHidden = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 6c14a9ece37a0..2e37d9aeee5f1 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -53,7 +53,6 @@ export const enableFizzExternalRuntime = true; export const enableGetInspectorDataForInstanceInProduction = true; export const enableHalt = false; export const enableInfiniteRenderLoopDetection = false; -export const enableLazyContextPropagation = true; export const enableLegacyCache = false; export const enableLegacyFBSupport = false; export const enableLegacyHidden = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 6dce24e57fcd7..6c86d9f60d405 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -42,7 +42,6 @@ export const enableGetInspectorDataForInstanceInProduction = false; export const enableHalt = false; export const enableHiddenSubtreeInsertionEffectCleanup = false; export const enableInfiniteRenderLoopDetection = false; -export const enableLazyContextPropagation = true; export const enableLegacyCache = false; export const enableLegacyFBSupport = false; export const enableLegacyHidden = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 53057d78d7390..6ba73ae150413 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -48,7 +48,6 @@ export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; export const disableSchedulerTimeoutInWorkLoop = false; -export const enableLazyContextPropagation = true; export const enableLegacyHidden = false; export const enableTransitionTracing = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js index 66e4329cb829d..14e351fb2f078 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -32,8 +32,6 @@ export const enableFizzExternalRuntime = true; export const enableGetInspectorDataForInstanceInProduction = false; export const enableHalt = false; export const enableInfiniteRenderLoopDetection = false; -export const enableLazyContextPropagation = true; -export const enableContextProfiling = false; export const enableHiddenSubtreeInsertionEffectCleanup = true; export const enableLegacyCache = false; export const enableLegacyFBSupport = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index b71ff7a266b4a..91eab54c310a4 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -50,7 +50,6 @@ export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; export const disableSchedulerTimeoutInWorkLoop = false; -export const enableLazyContextPropagation = true; export const enableLegacyHidden = false; export const enableTransitionTracing = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 8054a14bbc8a6..a0efa66cb348b 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -56,7 +56,6 @@ export const enableUseEffectEventHook = true; export const enableMoveBefore = false; export const disableInputAttributeSyncing = false; export const enableLegacyFBSupport = true; -export const enableLazyContextPropagation = true; export const enableHydrationLaneScheduling = true; From f5077bcc925aa6d0ba2ca4041c875d35e24f6266 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 17 Dec 2024 16:49:01 -0500 Subject: [PATCH 2/4] [Scheduler] Always yield to native macro tasks when a virtual task completes (#31787) As an alternative to #31784. We should really just always yield each virtual task to a native task. So that it's 1:1 with native tasks. This affects when microtasks within each task happens. This brings us closer to native `postTask` semantics which makes it more seamless to just use that when available. This still doesn't yield when a task expires to protect against starvation. --- .../scheduler/src/SchedulerFeatureFlags.js | 2 ++ .../scheduler/src/__tests__/Scheduler-test.js | 19 +++++++++++---- .../__tests__/SchedulerSetImmediate-test.js | 24 ++++++++++++++++--- packages/scheduler/src/forks/Scheduler.js | 17 +++++++++---- .../src/forks/SchedulerFeatureFlags.www.js | 2 ++ 5 files changed, 53 insertions(+), 11 deletions(-) diff --git a/packages/scheduler/src/SchedulerFeatureFlags.js b/packages/scheduler/src/SchedulerFeatureFlags.js index 08a0d979b621b..a5166f9813193 100644 --- a/packages/scheduler/src/SchedulerFeatureFlags.js +++ b/packages/scheduler/src/SchedulerFeatureFlags.js @@ -15,3 +15,5 @@ export const userBlockingPriorityTimeout = 250; export const normalPriorityTimeout = 5000; export const lowPriorityTimeout = 10000; export const enableRequestPaint = true; + +export const enableAlwaysYieldScheduler = __EXPERIMENTAL__; diff --git a/packages/scheduler/src/__tests__/Scheduler-test.js b/packages/scheduler/src/__tests__/Scheduler-test.js index fee1216d15730..afc9981762989 100644 --- a/packages/scheduler/src/__tests__/Scheduler-test.js +++ b/packages/scheduler/src/__tests__/Scheduler-test.js @@ -201,9 +201,10 @@ describe('SchedulerBrowser', () => { runtime.assertLog([ 'Message Event', 'Task', - SchedulerFeatureFlags.enableRequestPaint - ? 'Yield at 0ms' - : `Yield at ${SchedulerFeatureFlags.frameYieldMs}ms`, + gate(flags => flags.enableAlwaysYieldScheduler) || + !SchedulerFeatureFlags.enableRequestPaint + ? gate(flags => (flags.www ? 'Yield at 10ms' : 'Yield at 5ms')) + : 'Yield at 0ms', 'Post Message', ]); @@ -220,7 +221,13 @@ describe('SchedulerBrowser', () => { }); runtime.assertLog(['Post Message']); runtime.fireMessageEvent(); - runtime.assertLog(['Message Event', 'A', 'B']); + if (gate(flags => flags.enableAlwaysYieldScheduler)) { + runtime.assertLog(['Message Event', 'A', 'Post Message']); + runtime.fireMessageEvent(); + runtime.assertLog(['Message Event', 'B']); + } else { + runtime.assertLog(['Message Event', 'A', 'B']); + } }); it('multiple tasks with a yield in between', () => { @@ -267,6 +274,10 @@ describe('SchedulerBrowser', () => { runtime.assertLog(['Message Event', 'Oops!', 'Post Message']); runtime.fireMessageEvent(); + if (gate(flags => flags.enableAlwaysYieldScheduler)) { + runtime.assertLog(['Message Event', 'Post Message']); + runtime.fireMessageEvent(); + } runtime.assertLog(['Message Event', 'Yay']); }); diff --git a/packages/scheduler/src/__tests__/SchedulerSetImmediate-test.js b/packages/scheduler/src/__tests__/SchedulerSetImmediate-test.js index 7fd4a18db1930..74bbb8d55fbb7 100644 --- a/packages/scheduler/src/__tests__/SchedulerSetImmediate-test.js +++ b/packages/scheduler/src/__tests__/SchedulerSetImmediate-test.js @@ -188,7 +188,13 @@ describe('SchedulerDOMSetImmediate', () => { }); runtime.assertLog(['Set Immediate']); runtime.fireSetImmediate(); - runtime.assertLog(['setImmediate Callback', 'A', 'B']); + if (gate(flags => flags.enableAlwaysYieldScheduler)) { + runtime.assertLog(['setImmediate Callback', 'A', 'Set Immediate']); + runtime.fireSetImmediate(); + runtime.assertLog(['setImmediate Callback', 'B']); + } else { + runtime.assertLog(['setImmediate Callback', 'A', 'B']); + } }); it('multiple tasks at different priority', () => { @@ -200,7 +206,13 @@ describe('SchedulerDOMSetImmediate', () => { }); runtime.assertLog(['Set Immediate']); runtime.fireSetImmediate(); - runtime.assertLog(['setImmediate Callback', 'B', 'A']); + if (gate(flags => flags.enableAlwaysYieldScheduler)) { + runtime.assertLog(['setImmediate Callback', 'B', 'Set Immediate']); + runtime.fireSetImmediate(); + runtime.assertLog(['setImmediate Callback', 'A']); + } else { + runtime.assertLog(['setImmediate Callback', 'B', 'A']); + } }); it('multiple tasks with a yield in between', () => { @@ -246,7 +258,13 @@ describe('SchedulerDOMSetImmediate', () => { runtime.assertLog(['setImmediate Callback', 'Oops!', 'Set Immediate']); runtime.fireSetImmediate(); - runtime.assertLog(['setImmediate Callback', 'Yay']); + if (gate(flags => flags.enableAlwaysYieldScheduler)) { + runtime.assertLog(['setImmediate Callback', 'Set Immediate']); + runtime.fireSetImmediate(); + runtime.assertLog(['setImmediate Callback', 'Yay']); + } else { + runtime.assertLog(['setImmediate Callback', 'Yay']); + } }); it('schedule new task after queue has emptied', () => { diff --git a/packages/scheduler/src/forks/Scheduler.js b/packages/scheduler/src/forks/Scheduler.js index 68149a9fec14e..3fe4d1720fc38 100644 --- a/packages/scheduler/src/forks/Scheduler.js +++ b/packages/scheduler/src/forks/Scheduler.js @@ -19,6 +19,7 @@ import { lowPriorityTimeout, normalPriorityTimeout, enableRequestPaint, + enableAlwaysYieldScheduler, } from '../SchedulerFeatureFlags'; import {push, pop, peek} from '../SchedulerMinHeap'; @@ -196,9 +197,11 @@ function workLoop(initialTime: number) { currentTask !== null && !(enableSchedulerDebugging && isSchedulerPaused) ) { - if (currentTask.expirationTime > currentTime && shouldYieldToHost()) { - // This currentTask hasn't expired, and we've reached the deadline. - break; + if (!enableAlwaysYieldScheduler) { + if (currentTask.expirationTime > currentTime && shouldYieldToHost()) { + // This currentTask hasn't expired, and we've reached the deadline. + break; + } } // $FlowFixMe[incompatible-use] found when upgrading Flow const callback = currentTask.callback; @@ -242,6 +245,12 @@ function workLoop(initialTime: number) { pop(taskQueue); } currentTask = peek(taskQueue); + if (enableAlwaysYieldScheduler) { + if (currentTask === null || currentTask.expirationTime > currentTime) { + // This currentTask hasn't expired we yield to the browser task. + break; + } + } } // Return whether there's additional work if (currentTask !== null) { @@ -459,7 +468,7 @@ let frameInterval = frameYieldMs; let startTime = -1; function shouldYieldToHost(): boolean { - if (enableRequestPaint && needsPaint) { + if (!enableAlwaysYieldScheduler && enableRequestPaint && needsPaint) { // Yield now. return true; } diff --git a/packages/scheduler/src/forks/SchedulerFeatureFlags.www.js b/packages/scheduler/src/forks/SchedulerFeatureFlags.www.js index c65e2aea241f9..1d6b955eb8888 100644 --- a/packages/scheduler/src/forks/SchedulerFeatureFlags.www.js +++ b/packages/scheduler/src/forks/SchedulerFeatureFlags.www.js @@ -19,3 +19,5 @@ export const frameYieldMs = 10; export const userBlockingPriorityTimeout = 250; export const normalPriorityTimeout = 5000; export const lowPriorityTimeout = 10000; + +export const enableAlwaysYieldScheduler = false; From facec3ee71fff8b23f1e91005fce730cc96e4021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 17 Dec 2024 17:01:31 -0500 Subject: [PATCH 3/4] [Fiber] Schedule passive effects using the regular ensureRootIsScheduled flow (#31785) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This treats workInProgressRoot work and rootWithPendingPassiveEffects the same way. Basically as long as there's some work on the root, yield the current task. Including passive effects. This means that passive effects are now a continuation instead of a separate callback. This can mean they're earlier or later than before. Later for Idle in case there's other non-React work. Earlier for same Default if there's other Default priority work. This makes sense since increasing priority of the passive effects beyond Idle doesn't really make sense for an Idle render. However, for any given render at same priority it's more important to complete this work than start something new. Since we special case continuations to always yield to the browser, this has the same effect as #31784 without implementing `requestPaint`. At least assuming nothing else calls `requestPaint`. Screenshot 2024-12-14 at 5 37 37 PM --- .../src/__tests__/ReactDOMSelect-test.js | 4 +- ...DOMServerPartialHydration-test.internal.js | 4 + .../src/ReactFiberRootScheduler.js | 20 ++++- .../src/ReactFiberWorkLoop.js | 76 +++++++++++++------ .../src/__tests__/ReactDeferredValue-test.js | 4 + .../src/__tests__/ReactExpiration-test.js | 12 ++- .../ReactHooksWithNoopRenderer-test.js | 24 ++++-- .../ReactSiblingPrerendering-test.js | 16 ++++ .../ReactSuspenseWithNoopRenderer-test.js | 4 + .../src/__tests__/ReactUpdatePriority-test.js | 28 +++++-- packages/shared/ReactFeatureFlags.js | 3 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 2 + .../forks/ReactFeatureFlags.test-renderer.js | 2 + ...actFeatureFlags.test-renderer.native-fb.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 2 + .../shared/forks/ReactFeatureFlags.www.js | 2 + .../src/__tests__/useSubscription-test.js | 4 +- 18 files changed, 161 insertions(+), 48 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMSelect-test.js b/packages/react-dom/src/__tests__/ReactDOMSelect-test.js index 72ebfd1bbe67b..e450cf9a8166e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSelect-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMSelect-test.js @@ -911,9 +911,7 @@ describe('ReactDOMSelect', () => { const container = document.createElement('div'); const root = ReactDOMClient.createRoot(container); async function changeView() { - await act(() => { - root.unmount(); - }); + root.unmount(); } const stub = ( diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index e9354b0bf259d..9615c34256c3c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -3743,6 +3743,10 @@ describe('ReactDOMServerPartialHydration', () => { await waitForPaint(['App']); expect(visibleRef.current).toBe(visibleSpan); + if (gate(flags => flags.enableYieldingBeforePassive)) { + // Passive effects. + await waitForPaint([]); + } // Subsequently, the hidden child is prerendered on the client await waitForPaint(['HiddenChild']); expect(container).toMatchInlineSnapshot(` diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js index 8756a9c89009c..dcaadc5a6ef18 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -20,6 +20,7 @@ import { enableProfilerNestedUpdatePhase, enableComponentPerformanceTrack, enableSiblingPrerendering, + enableYieldingBeforePassive, } from 'shared/ReactFeatureFlags'; import { NoLane, @@ -41,6 +42,8 @@ import { getExecutionContext, getWorkInProgressRoot, getWorkInProgressRootRenderLanes, + getRootWithPendingPassiveEffects, + getPendingPassiveEffectsLanes, isWorkLoopSuspendedOnData, performWorkOnRoot, } from './ReactFiberWorkLoop'; @@ -324,12 +327,21 @@ function scheduleTaskForRootDuringMicrotask( markStarvedLanesAsExpired(root, currentTime); // Determine the next lanes to work on, and their priority. + const rootWithPendingPassiveEffects = getRootWithPendingPassiveEffects(); + const pendingPassiveEffectsLanes = getPendingPassiveEffectsLanes(); const workInProgressRoot = getWorkInProgressRoot(); const workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes(); - const nextLanes = getNextLanes( - root, - root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes, - ); + const nextLanes = + enableYieldingBeforePassive && root === rootWithPendingPassiveEffects + ? // This will schedule the callback at the priority of the lane but we used to + // always schedule it at NormalPriority. Discrete will flush it sync anyway. + // So the only difference is Idle and it doesn't seem necessarily right for that + // to get upgraded beyond something important just because we're past commit. + pendingPassiveEffectsLanes + : getNextLanes( + root, + root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes, + ); const existingCallbackNode = root.callbackNode; if ( diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 9a002ce4b6fd9..d524dd8599621 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -40,6 +40,7 @@ import { disableDefaultPropsExceptForClasses, enableSiblingPrerendering, enableComponentPerformanceTrack, + enableYieldingBeforePassive, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import is from 'shared/objectIs'; @@ -610,7 +611,6 @@ export function getRenderTargetTime(): number { let legacyErrorBoundariesThatAlreadyFailed: Set | null = null; -let rootDoesHavePassiveEffects: boolean = false; let rootWithPendingPassiveEffects: FiberRoot | null = null; let pendingPassiveEffectsLanes: Lanes = NoLanes; let pendingPassiveEffectsRemainingLanes: Lanes = NoLanes; @@ -638,6 +638,14 @@ export function getWorkInProgressRootRenderLanes(): Lanes { return workInProgressRootRenderLanes; } +export function getRootWithPendingPassiveEffects(): FiberRoot | null { + return rootWithPendingPassiveEffects; +} + +export function getPendingPassiveEffectsLanes(): Lanes { + return pendingPassiveEffectsLanes; +} + export function isWorkLoopSuspendedOnData(): boolean { return ( workInProgressSuspendedReason === SuspendedOnData || @@ -3210,12 +3218,6 @@ function commitRootImpl( ); } - // commitRoot never returns a continuation; it always finishes synchronously. - // So we can clear these now to allow a new callback to be scheduled. - root.callbackNode = null; - root.callbackPriority = NoLane; - root.cancelPendingCommit = null; - // Check which lanes no longer have any work scheduled on them, and mark // those as finished. let remainingLanes = mergeLanes(finishedWork.lanes, finishedWork.childLanes); @@ -3253,6 +3255,7 @@ function commitRootImpl( // might get scheduled in the commit phase. (See #16714.) // TODO: Delete all other places that schedule the passive effect callback // They're redundant. + let rootDoesHavePassiveEffects: boolean = false; if ( // If this subtree rendered with profiling this commit, we need to visit it to log it. (enableProfilerTimer && @@ -3261,17 +3264,25 @@ function commitRootImpl( (finishedWork.subtreeFlags & PassiveMask) !== NoFlags || (finishedWork.flags & PassiveMask) !== NoFlags ) { - if (!rootDoesHavePassiveEffects) { - rootDoesHavePassiveEffects = true; - pendingPassiveEffectsRemainingLanes = remainingLanes; - pendingPassiveEffectsRenderEndTime = completedRenderEndTime; - // workInProgressTransitions might be overwritten, so we want - // to store it in pendingPassiveTransitions until they get processed - // We need to pass this through as an argument to commitRoot - // because workInProgressTransitions might have changed between - // the previous render and commit if we throttle the commit - // with setTimeout - pendingPassiveTransitions = transitions; + rootDoesHavePassiveEffects = true; + pendingPassiveEffectsRemainingLanes = remainingLanes; + pendingPassiveEffectsRenderEndTime = completedRenderEndTime; + // workInProgressTransitions might be overwritten, so we want + // to store it in pendingPassiveTransitions until they get processed + // We need to pass this through as an argument to commitRoot + // because workInProgressTransitions might have changed between + // the previous render and commit if we throttle the commit + // with setTimeout + pendingPassiveTransitions = transitions; + if (enableYieldingBeforePassive) { + // We don't schedule a separate task for flushing passive effects. + // Instead, we just rely on ensureRootIsScheduled below to schedule + // a callback for us to flush the passive effects. + } else { + // So we can clear these now to allow a new callback to be scheduled. + root.callbackNode = null; + root.callbackPriority = NoLane; + root.cancelPendingCommit = null; scheduleCallback(NormalSchedulerPriority, () => { if (enableProfilerTimer && enableComponentPerformanceTrack) { // Track the currently executing event if there is one so we can ignore this @@ -3285,6 +3296,12 @@ function commitRootImpl( return null; }); } + } else { + // If we don't have passive effects, we're not going to need to perform more work + // so we can clear the callback now. + root.callbackNode = null; + root.callbackPriority = NoLane; + root.cancelPendingCommit = null; } if (enableProfilerTimer) { @@ -3441,10 +3458,6 @@ function commitRootImpl( onCommitRootTestSelector(); } - // Always call this before exiting `commitRoot`, to ensure that any - // additional work on this root is scheduled. - ensureRootIsScheduled(root); - if (recoverableErrors !== null) { // There were errors during this render, but recovered from them without // needing to surface it to the UI. We log them here. @@ -3480,6 +3493,10 @@ function commitRootImpl( flushPassiveEffects(); } + // Always call this before exiting `commitRoot`, to ensure that any + // additional work on this root is scheduled. + ensureRootIsScheduled(root); + // Read this again, since a passive effect might have updated it remainingLanes = root.pendingLanes; @@ -3648,6 +3665,13 @@ function flushPassiveEffectsImpl(wasDelayedCommit: void | boolean) { // because it's only used for profiling), but it's a refactor hazard. pendingPassiveEffectsLanes = NoLanes; + if (enableYieldingBeforePassive) { + // We've finished our work for this render pass. + root.callbackNode = null; + root.callbackPriority = NoLane; + root.cancelPendingCommit = null; + } + if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { throw new Error('Cannot flush passive effects while already rendering.'); } @@ -3745,6 +3769,14 @@ function flushPassiveEffectsImpl(wasDelayedCommit: void | boolean) { didScheduleUpdateDuringPassiveEffects = false; } + if (enableYieldingBeforePassive) { + // Next, we reschedule any remaining work in a new task since it's a new + // sequence of work. We wait until the end to do this in case the passive + // effect schedules higher priority work than we had remaining. That way + // we don't schedule an early callback that gets cancelled anyway. + ensureRootIsScheduled(root); + } + // TODO: Move to commitPassiveMountEffects onPostCommitRootDevTools(root); if (enableProfilerTimer && enableProfilerCommitHooks) { diff --git a/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js b/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js index 8ca142cb50517..bbd01cd551e86 100644 --- a/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js +++ b/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js @@ -753,6 +753,10 @@ describe('ReactDeferredValue', () => { revealContent(); // Because the preview state was already prerendered, we can reveal it // without any addditional work. + if (gate(flags => flags.enableYieldingBeforePassive)) { + // Passive effects. + await waitForPaint([]); + } await waitForPaint([]); expect(root).toMatchRenderedOutput(
Preview [B]
); }); diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js index aef06288667db..5b09d7d0b8821 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js @@ -755,10 +755,16 @@ describe('ReactExpiration', () => { // The update finishes without yielding. But it does not flush the effect. await waitFor(['B1'], { - additionalLogsAfterAttemptingToYield: ['C1'], + additionalLogsAfterAttemptingToYield: gate( + flags => flags.enableYieldingBeforePassive, + ) + ? ['C1', 'Effect: 1'] + : ['C1'], }); }); - // The effect flushes after paint. - assertLog(['Effect: 1']); + if (!gate(flags => flags.enableYieldingBeforePassive)) { + // The effect flushes after paint. + assertLog(['Effect: 1']); + } }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index a43b6c124df7d..dcb362669928e 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2695,13 +2695,23 @@ describe('ReactHooksWithNoopRenderer', () => { React.startTransition(() => { ReactNoop.render(); }); - await waitForPaint([ - 'Create passive [current: 0]', - 'Destroy insertion [current: 0]', - 'Create insertion [current: 0]', - 'Destroy layout [current: 1]', - 'Create layout [current: 1]', - ]); + if (gate(flags => flags.enableYieldingBeforePassive)) { + await waitForPaint(['Create passive [current: 0]']); + await waitForPaint([ + 'Destroy insertion [current: 0]', + 'Create insertion [current: 0]', + 'Destroy layout [current: 1]', + 'Create layout [current: 1]', + ]); + } else { + await waitForPaint([ + 'Create passive [current: 0]', + 'Destroy insertion [current: 0]', + 'Create insertion [current: 0]', + 'Destroy layout [current: 1]', + 'Create layout [current: 1]', + ]); + } expect(committedText).toEqual('1'); }); assertLog([ diff --git a/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js b/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js index 235e8df2201de..ceb32160976e0 100644 --- a/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js @@ -200,6 +200,10 @@ describe('ReactSiblingPrerendering', () => { await waitForPaint(['A']); expect(root).toMatchRenderedOutput('A'); + if (gate(flags => flags.enableYieldingBeforePassive)) { + // Passive effects. + await waitForPaint([]); + } // The second render is a prerender of the hidden content. await waitForPaint([ 'Suspend! [B]', @@ -237,6 +241,10 @@ describe('ReactSiblingPrerendering', () => { // Immediately after the fallback commits, retry the boundary again. This // time we include B, since we're not blocking the fallback from showing. if (gate('enableSiblingPrerendering')) { + if (gate(flags => flags.enableYieldingBeforePassive)) { + // Passive effects. + await waitForPaint([]); + } await waitForPaint(['Suspend! [A]', 'Suspend! [B]']); } }); @@ -452,6 +460,10 @@ describe('ReactSiblingPrerendering', () => { , ); + if (gate(flags => flags.enableYieldingBeforePassive)) { + // Passive effects. + await waitForPaint([]); + } // Immediately after the fallback commits, retry the boundary again. // Because the promise for A resolved, this is a normal render, _not_ // a prerender. So when we proceed to B, and B suspends, we unwind again @@ -471,6 +483,10 @@ describe('ReactSiblingPrerendering', () => { , ); + if (gate(flags => flags.enableYieldingBeforePassive)) { + // Passive effects. + await waitForPaint([]); + } // Now we can proceed to prerendering C. if (gate('enableSiblingPrerendering')) { await waitForPaint(['Suspend! [B]', 'Suspend! [C]']); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index 39cfebec3487e..0dcf0eb4705ee 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -1901,6 +1901,10 @@ describe('ReactSuspenseWithNoopRenderer', () => { // be throttled because the fallback would have appeared too recently. Scheduler.unstable_advanceTime(10000); jest.advanceTimersByTime(10000); + if (gate(flags => flags.enableYieldingBeforePassive)) { + // Passive effects. + await waitForPaint([]); + } await waitForPaint(['A']); expect(ReactNoop).toMatchRenderedOutput( <> diff --git a/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js b/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js index aa146558917c1..b2eebfa51611e 100644 --- a/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js +++ b/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js @@ -81,14 +81,26 @@ describe('ReactUpdatePriority', () => { // Schedule another update at default priority setDefaultState(2); - // The default update flushes first, because - await waitForPaint([ - // Idle update is scheduled - 'Idle update', - - // The default update flushes first, without including the idle update - 'Idle: 1, Default: 2', - ]); + if (gate(flags => flags.enableYieldingBeforePassive)) { + // The default update flushes first, because + await waitForPaint([ + // Idle update is scheduled + 'Idle update', + ]); + await waitForPaint([ + // The default update flushes first, without including the idle update + 'Idle: 1, Default: 2', + ]); + } else { + // The default update flushes first, because + await waitForPaint([ + // Idle update is scheduled + 'Idle update', + + // The default update flushes first, without including the idle update + 'Idle: 1, Default: 2', + ]); + } }); // Now the idle update has flushed assertLog(['Idle: 2, Default: 2']); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index fff1dcf4d9ddb..8540d6e14721a 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -77,6 +77,9 @@ export const enableLegacyFBSupport = false; // likely to include in an upcoming release. // ----------------------------------------------------------------------------- +// Yield to the browser event loop and not just the scheduler event loop before passive effects. +export const enableYieldingBeforePassive = __EXPERIMENTAL__; + export const enableLegacyCache = __EXPERIMENTAL__; export const enableAsyncIterableChildren = __EXPERIMENTAL__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 2e37d9aeee5f1..45807e9d876af 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -82,6 +82,7 @@ export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; export const useModernStrictMode = true; export const enableHydrationLaneScheduling = true; +export const enableYieldingBeforePassive = false; // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 6c86d9f60d405..c29da18a42055 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -76,6 +76,8 @@ export const enableUseResourceEffectHook = false; export const enableHydrationLaneScheduling = true; +export const enableYieldingBeforePassive = false; + // Profiling Only export const enableProfilerTimer = __PROFILE__; export const enableProfilerCommitHooks = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 6ba73ae150413..df58c53eda9a4 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -72,6 +72,8 @@ export const enableSiblingPrerendering = true; export const enableUseResourceEffectHook = false; +export const enableYieldingBeforePassive = true; + // TODO: This must be in sync with the main ReactFeatureFlags file because // the Test Renderer's value must be the same as the one used by the // react package. diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js index 14e351fb2f078..a4dc5cb76d109 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -70,6 +70,7 @@ export const enableFabricCompleteRootInCommitPhase = false; export const enableSiblingPrerendering = true; export const enableUseResourceEffectHook = true; export const enableHydrationLaneScheduling = true; +export const enableYieldingBeforePassive = true; // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 91eab54c310a4..e7c92602a0abf 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -84,5 +84,7 @@ export const enableUseResourceEffectHook = false; export const enableHydrationLaneScheduling = true; +export const enableYieldingBeforePassive = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index a0efa66cb348b..da62754a13359 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -57,6 +57,8 @@ export const enableMoveBefore = false; export const disableInputAttributeSyncing = false; export const enableLegacyFBSupport = true; +export const enableYieldingBeforePassive = false; + export const enableHydrationLaneScheduling = true; export const enableComponentPerformanceTrack = false; diff --git a/packages/use-subscription/src/__tests__/useSubscription-test.js b/packages/use-subscription/src/__tests__/useSubscription-test.js index 4ecc405789f91..6642ab35afb1f 100644 --- a/packages/use-subscription/src/__tests__/useSubscription-test.js +++ b/packages/use-subscription/src/__tests__/useSubscription-test.js @@ -19,6 +19,7 @@ let ReplaySubject; let assertLog; let waitForAll; let waitFor; +let waitForPaint; describe('useSubscription', () => { beforeEach(() => { @@ -37,6 +38,7 @@ describe('useSubscription', () => { const InternalTestUtils = require('internal-test-utils'); waitForAll = InternalTestUtils.waitForAll; + waitForPaint = InternalTestUtils.waitForPaint; assertLog = InternalTestUtils.assertLog; waitFor = InternalTestUtils.waitFor; }); @@ -595,7 +597,7 @@ describe('useSubscription', () => { React.startTransition(() => { mutate('C'); }); - await waitFor(['render:first:C', 'render:second:C']); + await waitForPaint(['render:first:C', 'render:second:C']); React.startTransition(() => { mutate('D'); }); From 6a4b46cd70d2672bc4be59dcb5b8dede22ed0cef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 17 Dec 2024 19:46:03 -0500 Subject: [PATCH 4/4] [Fiber] Log Effect and Render Times in Offscreen Commit Phase (#31788) In https://github.com/facebook/react/pull/30967 and https://github.com/facebook/react/pull/30983 I added logging of the just rendered components and the effects. However this didn't consider the special Offscreen passes. So this adds the same thing to those passes. Log component effect timings for disconnected/reconnected offscreen subtrees. This includes initial mount of a Suspense boundary. Log component render timings for reconnected and already offscreen offscreen subtrees. --- .../src/ReactFiberCommitWork.js | 197 ++++++++++++++++-- .../src/ReactFiberWorkLoop.js | 2 +- 2 files changed, 183 insertions(+), 16 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index df16eb96ef216..83922deed4b26 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -2209,6 +2209,8 @@ function recursivelyTraverseLayoutEffects( } export function disappearLayoutEffects(finishedWork: Fiber) { + const prevEffectStart = pushComponentEffectStart(); + switch (finishedWork.tag) { case FunctionComponent: case ForwardRef: @@ -2266,6 +2268,25 @@ export function disappearLayoutEffects(finishedWork: Fiber) { break; } } + + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + enableComponentPerformanceTrack && + (finishedWork.mode & ProfileMode) !== NoMode && + componentEffectStartTime >= 0 && + componentEffectEndTime >= 0 && + componentEffectDuration > 0.05 + ) { + logComponentEffect( + finishedWork, + componentEffectStartTime, + componentEffectEndTime, + componentEffectDuration, + ); + } + + popComponentEffectStart(prevEffectStart); } function recursivelyTraverseDisappearLayoutEffects(parentFiber: Fiber) { @@ -2286,6 +2307,8 @@ export function reappearLayoutEffects( // node was reused. includeWorkInProgressEffects: boolean, ) { + const prevEffectStart = pushComponentEffectStart(); + // Turn on layout effects in a tree that previously disappeared. const flags = finishedWork.flags; switch (finishedWork.tag) { @@ -2421,6 +2444,25 @@ export function reappearLayoutEffects( break; } } + + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + enableComponentPerformanceTrack && + (finishedWork.mode & ProfileMode) !== NoMode && + componentEffectStartTime >= 0 && + componentEffectEndTime >= 0 && + componentEffectDuration > 0.05 + ) { + logComponentEffect( + finishedWork, + componentEffectStartTime, + componentEffectEndTime, + componentEffectDuration, + ); + } + + popComponentEffectStart(prevEffectStart); } function recursivelyTraverseReappearLayoutEffects( @@ -2846,6 +2888,7 @@ function commitPassiveMountOnFiber( finishedWork, committedLanes, committedTransitions, + endTime, ); } else { // Legacy Mode: Fire the effects even if the tree is hidden. @@ -2884,6 +2927,7 @@ function commitPassiveMountOnFiber( committedLanes, committedTransitions, includeWorkInProgressEffects, + endTime, ); } } @@ -2963,6 +3007,7 @@ function recursivelyTraverseReconnectPassiveEffects( committedLanes: Lanes, committedTransitions: Array | null, includeWorkInProgressEffects: boolean, + endTime: number, ) { // This function visits both newly finished work and nodes that were re-used // from a previously committed tree. We cannot check non-static flags if the @@ -2974,14 +3019,30 @@ function recursivelyTraverseReconnectPassiveEffects( // TODO (Offscreen) Check: flags & (RefStatic | LayoutStatic) let child = parentFiber.child; while (child !== null) { - reconnectPassiveEffects( - finishedRoot, - child, - committedLanes, - committedTransitions, - childShouldIncludeWorkInProgressEffects, - ); - child = child.sibling; + if (enableProfilerTimer && enableComponentPerformanceTrack) { + const nextSibling = child.sibling; + reconnectPassiveEffects( + finishedRoot, + child, + committedLanes, + committedTransitions, + childShouldIncludeWorkInProgressEffects, + nextSibling !== null + ? ((nextSibling.actualStartTime: any): number) + : endTime, + ); + child = nextSibling; + } else { + reconnectPassiveEffects( + finishedRoot, + child, + committedLanes, + committedTransitions, + childShouldIncludeWorkInProgressEffects, + endTime, + ); + child = child.sibling; + } } } @@ -2994,7 +3055,28 @@ export function reconnectPassiveEffects( // from a previously committed tree. We cannot check non-static flags if the // node was reused. includeWorkInProgressEffects: boolean, + endTime: number, // Profiling-only. The start time of the next Fiber or root completion. ) { + const prevEffectStart = pushComponentEffectStart(); + + // If this component rendered in Profiling mode (DEV or in Profiler component) then log its + // render time. We do this after the fact in the passive effect to avoid the overhead of this + // getting in the way of the render characteristics and avoid the overhead of unwinding + // uncommitted renders. + if ( + enableProfilerTimer && + enableComponentPerformanceTrack && + (finishedWork.mode & ProfileMode) !== NoMode && + ((finishedWork.actualStartTime: any): number) > 0 && + (finishedWork.flags & PerformedWork) !== NoFlags + ) { + logComponentRender( + finishedWork, + ((finishedWork.actualStartTime: any): number), + endTime, + ); + } + const flags = finishedWork.flags; switch (finishedWork.tag) { case FunctionComponent: @@ -3006,6 +3088,7 @@ export function reconnectPassiveEffects( committedLanes, committedTransitions, includeWorkInProgressEffects, + endTime, ); // TODO: Check for PassiveStatic flag commitHookPassiveMountEffects(finishedWork, HookPassive); @@ -3025,6 +3108,7 @@ export function reconnectPassiveEffects( committedLanes, committedTransitions, includeWorkInProgressEffects, + endTime, ); if (includeWorkInProgressEffects && flags & Passive) { @@ -3051,6 +3135,7 @@ export function reconnectPassiveEffects( committedLanes, committedTransitions, includeWorkInProgressEffects, + endTime, ); } else { if (disableLegacyMode || finishedWork.mode & ConcurrentMode) { @@ -3064,6 +3149,7 @@ export function reconnectPassiveEffects( finishedWork, committedLanes, committedTransitions, + endTime, ); } else { // Legacy Mode: Fire the effects even if the tree is hidden. @@ -3074,6 +3160,7 @@ export function reconnectPassiveEffects( committedLanes, committedTransitions, includeWorkInProgressEffects, + endTime, ); } } @@ -3093,6 +3180,7 @@ export function reconnectPassiveEffects( committedLanes, committedTransitions, includeWorkInProgressEffects, + endTime, ); } @@ -3110,6 +3198,7 @@ export function reconnectPassiveEffects( committedLanes, committedTransitions, includeWorkInProgressEffects, + endTime, ); if (includeWorkInProgressEffects && flags & Passive) { // TODO: Pass `current` as argument to this function @@ -3126,6 +3215,7 @@ export function reconnectPassiveEffects( committedLanes, committedTransitions, includeWorkInProgressEffects, + endTime, ); if (includeWorkInProgressEffects && flags & Passive) { commitTracingMarkerPassiveMountEffect(finishedWork); @@ -3141,10 +3231,30 @@ export function reconnectPassiveEffects( committedLanes, committedTransitions, includeWorkInProgressEffects, + endTime, ); break; } } + + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + enableComponentPerformanceTrack && + (finishedWork.mode & ProfileMode) !== NoMode && + componentEffectStartTime >= 0 && + componentEffectEndTime >= 0 && + componentEffectDuration > 0.05 + ) { + logComponentEffect( + finishedWork, + componentEffectStartTime, + componentEffectEndTime, + componentEffectDuration, + ); + } + + popComponentEffectStart(prevEffectStart); } function recursivelyTraverseAtomicPassiveEffects( @@ -3152,6 +3262,7 @@ function recursivelyTraverseAtomicPassiveEffects( parentFiber: Fiber, committedLanes: Lanes, committedTransitions: Array | null, + endTime: number, // Profiling-only. The start time of the next Fiber or root completion. ) { // "Atomic" effects are ones that need to fire on every commit, even during // pre-rendering. We call this function when traversing a hidden tree whose @@ -3160,13 +3271,28 @@ function recursivelyTraverseAtomicPassiveEffects( if (parentFiber.subtreeFlags & PassiveMask) { let child = parentFiber.child; while (child !== null) { - commitAtomicPassiveEffects( - finishedRoot, - child, - committedLanes, - committedTransitions, - ); - child = child.sibling; + if (enableProfilerTimer && enableComponentPerformanceTrack) { + const nextSibling = child.sibling; + commitAtomicPassiveEffects( + finishedRoot, + child, + committedLanes, + committedTransitions, + nextSibling !== null + ? ((nextSibling.actualStartTime: any): number) + : endTime, + ); + child = nextSibling; + } else { + commitAtomicPassiveEffects( + finishedRoot, + child, + committedLanes, + committedTransitions, + endTime, + ); + child = child.sibling; + } } } } @@ -3176,7 +3302,24 @@ function commitAtomicPassiveEffects( finishedWork: Fiber, committedLanes: Lanes, committedTransitions: Array | null, + endTime: number, // Profiling-only. The start time of the next Fiber or root completion. ) { + // If this component rendered in Profiling mode (DEV or in Profiler component) then log its + // render time. A render can happen even if the subtree is offscreen. + if ( + enableProfilerTimer && + enableComponentPerformanceTrack && + (finishedWork.mode & ProfileMode) !== NoMode && + ((finishedWork.actualStartTime: any): number) > 0 && + (finishedWork.flags & PerformedWork) !== NoFlags + ) { + logComponentRender( + finishedWork, + ((finishedWork.actualStartTime: any): number), + endTime, + ); + } + // "Atomic" effects are ones that need to fire on every commit, even during // pre-rendering. We call this function when traversing a hidden tree whose // regular effects are currently disconnected. @@ -3188,6 +3331,7 @@ function commitAtomicPassiveEffects( finishedWork, committedLanes, committedTransitions, + endTime, ); if (flags & Passive) { // TODO: Pass `current` as argument to this function @@ -3203,6 +3347,7 @@ function commitAtomicPassiveEffects( finishedWork, committedLanes, committedTransitions, + endTime, ); if (flags & Passive) { // TODO: Pass `current` as argument to this function @@ -3217,6 +3362,7 @@ function commitAtomicPassiveEffects( finishedWork, committedLanes, committedTransitions, + endTime, ); break; } @@ -3591,6 +3737,8 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber( current: Fiber, nearestMountedAncestor: Fiber | null, ): void { + const prevEffectStart = pushComponentEffectStart(); + switch (current.tag) { case FunctionComponent: case ForwardRef: @@ -3702,6 +3850,25 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber( break; } } + + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + enableComponentPerformanceTrack && + (current.mode & ProfileMode) !== NoMode && + componentEffectStartTime >= 0 && + componentEffectEndTime >= 0 && + componentEffectDuration > 0.05 + ) { + logComponentEffect( + current, + componentEffectStartTime, + componentEffectEndTime, + componentEffectDuration, + ); + } + + popComponentEffectStart(prevEffectStart); } export function invokeLayoutEffectMountInDEV(fiber: Fiber): void { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index d524dd8599621..a71eb543cfe5d 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -4139,7 +4139,7 @@ function doubleInvokeEffectsOnFiber( } reappearLayoutEffects(root, fiber.alternate, fiber, false); if (shouldDoubleInvokePassiveEffects) { - reconnectPassiveEffects(root, fiber, NoLanes, null, false); + reconnectPassiveEffects(root, fiber, NoLanes, null, false, 0); } } finally { setIsStrictModeForDevtools(false);