@samitouri / QOS-React-2 / commits / ee85098019

[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.

Jan Kassens committed Jan 17, 2023 at 11:03 UTC ee85098019bf9703b32f608f8bbd5f8fb1a7d60b

11 files changed +65 -133

packages/react-reconciler/src/ReactFiberCommitWork.js

+65 -110

@@ -46,7 +46,6 @@ import {

46 enableSchedulingProfiler,

47 enableSuspenseCallback,

48 enableScopeAPI,

49 - deletedTreeCleanUpLevel,

49 enableUpdaterTracking,

50 enableCache,

51 enableTransitionTracing,

@@ -1658,74 +1657,43 @@ function detachFiberAfterEffects(fiber: Fiber) {

1657 detachFiberAfterEffects(alternate);

1658 }

1659

1661 - // Note: Defensively using negation instead of < in case

1662 - // `deletedTreeCleanUpLevel` is undefined.

1663 - if (!(deletedTreeCleanUpLevel >= 2)) {

1664 - // This is the default branch (level 0).

1665 - fiber.child = null;

1666 - fiber.deletions = null;

1667 - fiber.dependencies = null;

1668 - fiber.memoizedProps = null;

1669 - fiber.memoizedState = null;

1670 - fiber.pendingProps = null;

1671 - fiber.sibling = null;

1672 - fiber.stateNode = null;

1673 - fiber.updateQueue = null;

1674 -

1675 - if (__DEV__) {

1676 - fiber._debugOwner = null;

1660 + // Clear cyclical Fiber fields. This level alone is designed to roughly

1661 + // approximate the planned Fiber refactor. In that world, `setState` will be

1662 + // bound to a special "instance" object instead of a Fiber. The Instance

1663 + // object will not have any of these fields. It will only be connected to

1664 + // the fiber tree via a single link at the root. So if this level alone is

1665 + // sufficient to fix memory issues, that bodes well for our plans.

1666 + fiber.child = null;

1667 + fiber.deletions = null;

1668 + fiber.sibling = null;

1669 +

1670 + // The `stateNode` is cyclical because on host nodes it points to the host

1671 + // tree, which has its own pointers to children, parents, and siblings.

1672 + // The other host nodes also point back to fibers, so we should detach that

1673 + // one, too.

1674 + if (fiber.tag === HostComponent) {

1675 + const hostInstance: Instance = fiber.stateNode;

1676 + if (hostInstance !== null) {

1677 + detachDeletedInstance(hostInstance);

1678 }

1678 - } else {

1679 - // Clear cyclical Fiber fields. This level alone is designed to roughly

1680 - // approximate the planned Fiber refactor. In that world, `setState` will be

1681 - // bound to a special "instance" object instead of a Fiber. The Instance

1682 - // object will not have any of these fields. It will only be connected to

1683 - // the fiber tree via a single link at the root. So if this level alone is

1684 - // sufficient to fix memory issues, that bodes well for our plans.

1685 - fiber.child = null;

1686 - fiber.deletions = null;

1687 - fiber.sibling = null;

1688 -

1689 - // The `stateNode` is cyclical because on host nodes it points to the host

1690 - // tree, which has its own pointers to children, parents, and siblings.

1691 - // The other host nodes also point back to fibers, so we should detach that

1692 - // one, too.

1693 - if (fiber.tag === HostComponent) {

1694 - const hostInstance: Instance = fiber.stateNode;

1695 - if (hostInstance !== null) {

1696 - detachDeletedInstance(hostInstance);

1697 - }

1698 - }

1699 - fiber.stateNode = null;

1700 -

1701 - // I'm intentionally not clearing the `return` field in this level. We

1702 - // already disconnect the `return` pointer at the root of the deleted

1703 - // subtree (in `detachFiberMutation`). Besides, `return` by itself is not

1704 - // cyclical — it's only cyclical when combined with `child`, `sibling`, and

1705 - // `alternate`. But we'll clear it in the next level anyway, just in case.

1679 + }

1680 + fiber.stateNode = null;

1681

1707 - if (__DEV__) {

1708 - fiber._debugOwner = null;

1709 - }

1710 -

1711 - if (deletedTreeCleanUpLevel >= 3) {

1712 - // Theoretically, nothing in here should be necessary, because we already

1713 - // disconnected the fiber from the tree. So even if something leaks this

1714 - // particular fiber, it won't leak anything else

1715 - //

1716 - // The purpose of this branch is to be super aggressive so we can measure

1717 - // if there's any difference in memory impact. If there is, that could

1718 - // indicate a React leak we don't know about.

1719 - fiber.return = null;

1720 - fiber.dependencies = null;

1721 - fiber.memoizedProps = null;

1722 - fiber.memoizedState = null;

1723 - fiber.pendingProps = null;

1724 - fiber.stateNode = null;

1725 - // TODO: Move to `commitPassiveUnmountInsideDeletedTreeOnFiber` instead.

1726 - fiber.updateQueue = null;

1727 - }

1682 + if (__DEV__) {

1683 + fiber._debugOwner = null;

1684 }

1685 +

1686 + // Theoretically, nothing in here should be necessary, because we already

1687 + // disconnected the fiber from the tree. So even if something leaks this

1688 + // particular fiber, it won't leak anything else.

1689 + fiber.return = null;

1690 + fiber.dependencies = null;

1691 + fiber.memoizedProps = null;

1692 + fiber.memoizedState = null;

1693 + fiber.pendingProps = null;

1694 + fiber.stateNode = null;

1695 + // TODO: Move to `commitPassiveUnmountInsideDeletedTreeOnFiber` instead.

1696 + fiber.updateQueue = null;

1697 }

1698

1699 function emptyPortalContainer(current: Fiber) {

@@ -3986,31 +3954,29 @@ export function commitPassiveUnmountEffects(finishedWork: Fiber): void {

3954 }

3955

3956 function detachAlternateSiblings(parentFiber: Fiber) {

3989 - if (deletedTreeCleanUpLevel >= 1) {

3990 - // A fiber was deleted from this parent fiber, but it's still part of the

3991 - // previous (alternate) parent fiber's list of children. Because children

3992 - // are a linked list, an earlier sibling that's still alive will be

3993 - // connected to the deleted fiber via its `alternate`:

3994 - //

3995 - // live fiber --alternate--> previous live fiber --sibling--> deleted

3996 - // fiber

3997 - //

3998 - // We can't disconnect `alternate` on nodes that haven't been deleted yet,

3999 - // but we can disconnect the `sibling` and `child` pointers.

4000 -

4001 - const previousFiber = parentFiber.alternate;

4002 - if (previousFiber !== null) {

4003 - let detachedChild = previousFiber.child;

4004 - if (detachedChild !== null) {

4005 - previousFiber.child = null;

4006 - do {

4007 - // $FlowFixMe[incompatible-use] found when upgrading Flow

4008 - const detachedSibling = detachedChild.sibling;

4009 - // $FlowFixMe[incompatible-use] found when upgrading Flow

4010 - detachedChild.sibling = null;

4011 - detachedChild = detachedSibling;

4012 - } while (detachedChild !== null);

4013 - }

3957 + // A fiber was deleted from this parent fiber, but it's still part of the

3958 + // previous (alternate) parent fiber's list of children. Because children

3959 + // are a linked list, an earlier sibling that's still alive will be

3960 + // connected to the deleted fiber via its `alternate`:

3961 + //

3962 + // live fiber --alternate--> previous live fiber --sibling--> deleted

3963 + // fiber

3964 + //

3965 + // We can't disconnect `alternate` on nodes that haven't been deleted yet,

3966 + // but we can disconnect the `sibling` and `child` pointers.

3967 +

3968 + const previousFiber = parentFiber.alternate;

3969 + if (previousFiber !== null) {

3970 + let detachedChild = previousFiber.child;

3971 + if (detachedChild !== null) {

3972 + previousFiber.child = null;

3973 + do {

3974 + // $FlowFixMe[incompatible-use] found when upgrading Flow

3975 + const detachedSibling = detachedChild.sibling;

3976 + // $FlowFixMe[incompatible-use] found when upgrading Flow

3977 + detachedChild.sibling = null;

3978 + detachedChild = detachedSibling;

3979 + } while (detachedChild !== null);

3980 }

3981 }

3982 }

@@ -4196,8 +4162,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin(

4162 resetCurrentDebugFiberInDEV();

4163

4164 const child = fiber.child;

4199 - // TODO: Only traverse subtree if it has a PassiveStatic flag. (But, if we

4200 - // do this, still need to handle `deletedTreeCleanUpLevel` correctly.)

4165 + // TODO: Only traverse subtree if it has a PassiveStatic flag.

4166 if (child !== null) {

4167 child.return = fiber;

4168 nextEffect = child;

@@ -4217,23 +4182,13 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete(

4182 const sibling = fiber.sibling;

4183 const returnFiber = fiber.return;

4184

4220 - if (deletedTreeCleanUpLevel >= 2) {

4221 - // Recursively traverse the entire deleted tree and clean up fiber fields.

4222 - // This is more aggressive than ideal, and the long term goal is to only

4223 - // have to detach the deleted tree at the root.

4224 - detachFiberAfterEffects(fiber);

4225 - if (fiber === deletedSubtreeRoot) {

4226 - nextEffect = null;

4227 - return;

4228 - }

4229 - } else {

4230 - // This is the default branch (level 0). We do not recursively clear all

4231 - // the fiber fields. Only the root of the deleted subtree.

4232 - if (fiber === deletedSubtreeRoot) {

4233 - detachFiberAfterEffects(fiber);

4234 - nextEffect = null;

4235 - return;

4236 - }

4185 + // Recursively traverse the entire deleted tree and clean up fiber fields.

4186 + // This is more aggressive than ideal, and the long term goal is to only

4187 + // have to detach the deleted tree at the root.

4188 + detachFiberAfterEffects(fiber);

4189 + if (fiber === deletedSubtreeRoot) {

4190 + nextEffect = null;

4191 + return;

4192 }

4193

4194 if (sibling !== null) {

packages/shared/ReactFeatureFlags.js

-13

@@ -101,19 +101,6 @@ export const enableHostSingletons = true;

101

102 export const enableFloat = true;

103

104 -// When a node is unmounted, recurse into the Fiber subtree and clean out

105 -// references. Each level cleans up more fiber fields than the previous level.

106 -// As far as we know, React itself doesn't leak, but because the Fiber contains

107 -// cycles, even a single leak in product code can cause us to retain large

108 -// amounts of memory.

109 -//

110 -// The long term plan is to remove the cycles, but in the meantime, we clear

111 -// additional fields to mitigate.

112 -//

113 -// It's an enum so that we can experiment with different levels of

114 -// aggressiveness.

115 -export const deletedTreeCleanUpLevel = 3;

116 -

104 export const enableUseHook = true;

105

106 // Enables unstable_useMemoCache hook, intended as a compilation target for

packages/shared/forks/ReactFeatureFlags.native-fb.js

-1

@@ -59,7 +59,6 @@ export const enableLegacyFBSupport = false;

59 export const enableFilterEmptyStringAttributesDOM = false;

60 export const disableNativeComponentFrames = false;

61 export const skipUnmountedBoundaries = false;

62 -export const deletedTreeCleanUpLevel = 3;

62 export const enableGetInspectorDataForInstanceInProduction = true;

63 export const deferRenderPhaseUpdateToNextBatch = false;

packages/shared/forks/ReactFeatureFlags.native-oss.js

-1

@@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false;

49 export const enableFilterEmptyStringAttributesDOM = false;

50 export const disableNativeComponentFrames = false;

51 export const skipUnmountedBoundaries = false;

52 -export const deletedTreeCleanUpLevel = 3;

52 export const enableGetInspectorDataForInstanceInProduction = false;

53 export const deferRenderPhaseUpdateToNextBatch = false;

packages/shared/forks/ReactFeatureFlags.test-renderer.js

-1

@@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false;

49 export const enableFilterEmptyStringAttributesDOM = false;

50 export const disableNativeComponentFrames = false;

51 export const skipUnmountedBoundaries = false;

52 -export const deletedTreeCleanUpLevel = 3;

52 export const enableGetInspectorDataForInstanceInProduction = false;

53 export const deferRenderPhaseUpdateToNextBatch = false;

packages/shared/forks/ReactFeatureFlags.test-renderer.native.js

-1

@@ -41,7 +41,6 @@ export const enableLegacyFBSupport = false;

41 export const enableFilterEmptyStringAttributesDOM = false;

42 export const disableNativeComponentFrames = false;

43 export const skipUnmountedBoundaries = false;

44 -export const deletedTreeCleanUpLevel = 3;

44 export const enableGetInspectorDataForInstanceInProduction = false;

45 export const deferRenderPhaseUpdateToNextBatch = false;

46 export const enableSuspenseAvoidThisFallback = false;

packages/shared/forks/ReactFeatureFlags.test-renderer.www.js

-1

@@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false;

49 export const enableFilterEmptyStringAttributesDOM = false;

50 export const disableNativeComponentFrames = false;

51 export const skipUnmountedBoundaries = false;

52 -export const deletedTreeCleanUpLevel = 3;

52 export const enableGetInspectorDataForInstanceInProduction = false;

53 export const deferRenderPhaseUpdateToNextBatch = false;

packages/shared/forks/ReactFeatureFlags.testing.js

-1

@@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false;

49 export const enableFilterEmptyStringAttributesDOM = false;

50 export const disableNativeComponentFrames = false;

51 export const skipUnmountedBoundaries = false;

52 -export const deletedTreeCleanUpLevel = 3;

52 export const enableGetInspectorDataForInstanceInProduction = false;

53 export const deferRenderPhaseUpdateToNextBatch = false;

packages/shared/forks/ReactFeatureFlags.testing.www.js

-1

@@ -49,7 +49,6 @@ export const enableLegacyFBSupport = !__EXPERIMENTAL__;

49 export const enableFilterEmptyStringAttributesDOM = false;

50 export const disableNativeComponentFrames = false;

51 export const skipUnmountedBoundaries = true;

52 -export const deletedTreeCleanUpLevel = 3;

52 export const enableGetInspectorDataForInstanceInProduction = false;

53 export const deferRenderPhaseUpdateToNextBatch = false;

packages/shared/forks/ReactFeatureFlags.www-dynamic.js

-1

@@ -19,7 +19,6 @@ export const enableFilterEmptyStringAttributesDOM = __VARIANT__;