From ee85098019bf9703b32f608f8bbd5f8fb1a7d60b Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Tue, 17 Jan 2023 11:03:29 -0500 Subject: [PATCH] [cleanup] remove deletedTreeCleanUpLevel feature flag (#25529) I noticed this was an experiment concluded 16 months ago (#21679) that this extra work is beneficial to break up cycles leaking memory in product code. --- .../src/ReactFiberCommitWork.js | 175 +++++++----------- packages/shared/ReactFeatureFlags.js | 13 -- .../forks/ReactFeatureFlags.native-fb.js | 1 - .../forks/ReactFeatureFlags.native-oss.js | 1 - .../forks/ReactFeatureFlags.test-renderer.js | 1 - .../ReactFeatureFlags.test-renderer.native.js | 1 - .../ReactFeatureFlags.test-renderer.www.js | 1 - .../shared/forks/ReactFeatureFlags.testing.js | 1 - .../forks/ReactFeatureFlags.testing.www.js | 1 - .../forks/ReactFeatureFlags.www-dynamic.js | 1 - .../shared/forks/ReactFeatureFlags.www.js | 2 - 11 files changed, 65 insertions(+), 133 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 5002942ab3521..68aaaad91ae6e 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -46,7 +46,6 @@ import { enableSchedulingProfiler, enableSuspenseCallback, enableScopeAPI, - deletedTreeCleanUpLevel, enableUpdaterTracking, enableCache, enableTransitionTracing, @@ -1658,74 +1657,43 @@ function detachFiberAfterEffects(fiber: Fiber) { detachFiberAfterEffects(alternate); } - // Note: Defensively using negation instead of < in case - // `deletedTreeCleanUpLevel` is undefined. - if (!(deletedTreeCleanUpLevel >= 2)) { - // This is the default branch (level 0). - fiber.child = null; - fiber.deletions = null; - fiber.dependencies = null; - fiber.memoizedProps = null; - fiber.memoizedState = null; - fiber.pendingProps = null; - fiber.sibling = null; - fiber.stateNode = null; - fiber.updateQueue = null; - - if (__DEV__) { - fiber._debugOwner = null; + // Clear cyclical Fiber fields. This level alone is designed to roughly + // approximate the planned Fiber refactor. In that world, `setState` will be + // bound to a special "instance" object instead of a Fiber. The Instance + // object will not have any of these fields. It will only be connected to + // the fiber tree via a single link at the root. So if this level alone is + // sufficient to fix memory issues, that bodes well for our plans. + fiber.child = null; + fiber.deletions = null; + fiber.sibling = null; + + // The `stateNode` is cyclical because on host nodes it points to the host + // tree, which has its own pointers to children, parents, and siblings. + // The other host nodes also point back to fibers, so we should detach that + // one, too. + if (fiber.tag === HostComponent) { + const hostInstance: Instance = fiber.stateNode; + if (hostInstance !== null) { + detachDeletedInstance(hostInstance); } - } else { - // Clear cyclical Fiber fields. This level alone is designed to roughly - // approximate the planned Fiber refactor. In that world, `setState` will be - // bound to a special "instance" object instead of a Fiber. The Instance - // object will not have any of these fields. It will only be connected to - // the fiber tree via a single link at the root. So if this level alone is - // sufficient to fix memory issues, that bodes well for our plans. - fiber.child = null; - fiber.deletions = null; - fiber.sibling = null; - - // The `stateNode` is cyclical because on host nodes it points to the host - // tree, which has its own pointers to children, parents, and siblings. - // The other host nodes also point back to fibers, so we should detach that - // one, too. - if (fiber.tag === HostComponent) { - const hostInstance: Instance = fiber.stateNode; - if (hostInstance !== null) { - detachDeletedInstance(hostInstance); - } - } - fiber.stateNode = null; - - // I'm intentionally not clearing the `return` field in this level. We - // already disconnect the `return` pointer at the root of the deleted - // subtree (in `detachFiberMutation`). Besides, `return` by itself is not - // cyclical — it's only cyclical when combined with `child`, `sibling`, and - // `alternate`. But we'll clear it in the next level anyway, just in case. + } + fiber.stateNode = null; - if (__DEV__) { - fiber._debugOwner = null; - } - - if (deletedTreeCleanUpLevel >= 3) { - // Theoretically, nothing in here should be necessary, because we already - // disconnected the fiber from the tree. So even if something leaks this - // particular fiber, it won't leak anything else - // - // The purpose of this branch is to be super aggressive so we can measure - // if there's any difference in memory impact. If there is, that could - // indicate a React leak we don't know about. - fiber.return = null; - fiber.dependencies = null; - fiber.memoizedProps = null; - fiber.memoizedState = null; - fiber.pendingProps = null; - fiber.stateNode = null; - // TODO: Move to `commitPassiveUnmountInsideDeletedTreeOnFiber` instead. - fiber.updateQueue = null; - } + if (__DEV__) { + fiber._debugOwner = null; } + + // Theoretically, nothing in here should be necessary, because we already + // disconnected the fiber from the tree. So even if something leaks this + // particular fiber, it won't leak anything else. + fiber.return = null; + fiber.dependencies = null; + fiber.memoizedProps = null; + fiber.memoizedState = null; + fiber.pendingProps = null; + fiber.stateNode = null; + // TODO: Move to `commitPassiveUnmountInsideDeletedTreeOnFiber` instead. + fiber.updateQueue = null; } function emptyPortalContainer(current: Fiber) { @@ -3986,31 +3954,29 @@ export function commitPassiveUnmountEffects(finishedWork: Fiber): void { } function detachAlternateSiblings(parentFiber: Fiber) { - if (deletedTreeCleanUpLevel >= 1) { - // A fiber was deleted from this parent fiber, but it's still part of the - // previous (alternate) parent fiber's list of children. Because children - // are a linked list, an earlier sibling that's still alive will be - // connected to the deleted fiber via its `alternate`: - // - // live fiber --alternate--> previous live fiber --sibling--> deleted - // fiber - // - // We can't disconnect `alternate` on nodes that haven't been deleted yet, - // but we can disconnect the `sibling` and `child` pointers. - - const previousFiber = parentFiber.alternate; - if (previousFiber !== null) { - let detachedChild = previousFiber.child; - if (detachedChild !== null) { - previousFiber.child = null; - do { - // $FlowFixMe[incompatible-use] found when upgrading Flow - const detachedSibling = detachedChild.sibling; - // $FlowFixMe[incompatible-use] found when upgrading Flow - detachedChild.sibling = null; - detachedChild = detachedSibling; - } while (detachedChild !== null); - } + // A fiber was deleted from this parent fiber, but it's still part of the + // previous (alternate) parent fiber's list of children. Because children + // are a linked list, an earlier sibling that's still alive will be + // connected to the deleted fiber via its `alternate`: + // + // live fiber --alternate--> previous live fiber --sibling--> deleted + // fiber + // + // We can't disconnect `alternate` on nodes that haven't been deleted yet, + // but we can disconnect the `sibling` and `child` pointers. + + const previousFiber = parentFiber.alternate; + if (previousFiber !== null) { + let detachedChild = previousFiber.child; + if (detachedChild !== null) { + previousFiber.child = null; + do { + // $FlowFixMe[incompatible-use] found when upgrading Flow + const detachedSibling = detachedChild.sibling; + // $FlowFixMe[incompatible-use] found when upgrading Flow + detachedChild.sibling = null; + detachedChild = detachedSibling; + } while (detachedChild !== null); } } } @@ -4196,8 +4162,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( resetCurrentDebugFiberInDEV(); const child = fiber.child; - // TODO: Only traverse subtree if it has a PassiveStatic flag. (But, if we - // do this, still need to handle `deletedTreeCleanUpLevel` correctly.) + // TODO: Only traverse subtree if it has a PassiveStatic flag. if (child !== null) { child.return = fiber; nextEffect = child; @@ -4217,23 +4182,13 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete( const sibling = fiber.sibling; const returnFiber = fiber.return; - if (deletedTreeCleanUpLevel >= 2) { - // Recursively traverse the entire deleted tree and clean up fiber fields. - // This is more aggressive than ideal, and the long term goal is to only - // have to detach the deleted tree at the root. - detachFiberAfterEffects(fiber); - if (fiber === deletedSubtreeRoot) { - nextEffect = null; - return; - } - } else { - // This is the default branch (level 0). We do not recursively clear all - // the fiber fields. Only the root of the deleted subtree. - if (fiber === deletedSubtreeRoot) { - detachFiberAfterEffects(fiber); - nextEffect = null; - return; - } + // Recursively traverse the entire deleted tree and clean up fiber fields. + // This is more aggressive than ideal, and the long term goal is to only + // have to detach the deleted tree at the root. + detachFiberAfterEffects(fiber); + if (fiber === deletedSubtreeRoot) { + nextEffect = null; + return; } if (sibling !== null) { diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 18f38d3aa81e7..63d1aae161181 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -101,19 +101,6 @@ export const enableHostSingletons = true; export const enableFloat = true; -// When a node is unmounted, recurse into the Fiber subtree and clean out -// references. Each level cleans up more fiber fields than the previous level. -// As far as we know, React itself doesn't leak, but because the Fiber contains -// cycles, even a single leak in product code can cause us to retain large -// amounts of memory. -// -// The long term plan is to remove the cycles, but in the meantime, we clear -// additional fields to mitigate. -// -// It's an enum so that we can experiment with different levels of -// aggressiveness. -export const deletedTreeCleanUpLevel = 3; - export const enableUseHook = true; // Enables unstable_useMemoCache hook, intended as a compilation target for diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 056672c914057..95a330b576c12 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -59,7 +59,6 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const deletedTreeCleanUpLevel = 3; export const enableGetInspectorDataForInstanceInProduction = true; export const deferRenderPhaseUpdateToNextBatch = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 97723bdc9d765..d8a1d8c5d7427 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const deletedTreeCleanUpLevel = 3; export const enableGetInspectorDataForInstanceInProduction = false; export const deferRenderPhaseUpdateToNextBatch = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 8941aa4b39d60..92a4ecaf5dfad 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const deletedTreeCleanUpLevel = 3; export const enableGetInspectorDataForInstanceInProduction = false; export const deferRenderPhaseUpdateToNextBatch = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index faa6aa3ebf760..dd2c58d33f4ec 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -41,7 +41,6 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const deletedTreeCleanUpLevel = 3; export const enableGetInspectorDataForInstanceInProduction = false; export const deferRenderPhaseUpdateToNextBatch = false; export const enableSuspenseAvoidThisFallback = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 9d97f6cba6a06..78652d8835bb2 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const deletedTreeCleanUpLevel = 3; export const enableGetInspectorDataForInstanceInProduction = false; export const deferRenderPhaseUpdateToNextBatch = false; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index bf00a167693ce..72a1a74523dc0 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const deletedTreeCleanUpLevel = 3; export const enableGetInspectorDataForInstanceInProduction = false; export const deferRenderPhaseUpdateToNextBatch = false; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index 727d2b0770825..d5bc91306df4a 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -49,7 +49,6 @@ export const enableLegacyFBSupport = !__EXPERIMENTAL__; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = true; -export const deletedTreeCleanUpLevel = 3; export const enableGetInspectorDataForInstanceInProduction = false; export const deferRenderPhaseUpdateToNextBatch = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 8cdbc224a8744..0297c7dd44fcd 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -19,7 +19,6 @@ export const enableFilterEmptyStringAttributesDOM = __VARIANT__; export const enableLegacyFBSupport = __VARIANT__; export const skipUnmountedBoundaries = __VARIANT__; export const enableUseRefAccessWarning = __VARIANT__; -export const deletedTreeCleanUpLevel: number = __VARIANT__ ? 3 : 1; export const enableProfilerNestedUpdateScheduledHook = __VARIANT__; export const disableSchedulerTimeoutInWorkLoop = __VARIANT__; export const enableLazyContextPropagation = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 04261829f25e8..76ea71d342a32 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -95,8 +95,6 @@ export const disableTextareaChildren = __EXPERIMENTAL__; export const allowConcurrentByDefault = true; -export const deletedTreeCleanUpLevel = 3; - export const consoleManagedByDevToolsDuringStrictMode = true; export const enableServerContext = true;