From 100239e1fe787d9ec2a07b1d9a09ea2de4497516 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 22 Mar 2021 22:31:52 -0500 Subject: [PATCH] Combine into single enum flag I combined `enableStrongMemoryCleanup` and `enableDetachOldChildList` into a single enum flag. The flag has three possible values. Each level is a superset of the previous one and performs more aggressive clean up. We will use this to compare the memory impact of each level. --- .../src/ReactFiberCommitWork.new.js | 127 ++++++++++++------ .../src/ReactFiberCommitWork.old.js | 127 ++++++++++++------ packages/shared/ReactFeatureFlags.js | 15 ++- .../forks/ReactFeatureFlags.native-fb.js | 3 +- .../forks/ReactFeatureFlags.native-oss.js | 3 +- .../forks/ReactFeatureFlags.test-renderer.js | 3 +- .../ReactFeatureFlags.test-renderer.native.js | 3 +- .../ReactFeatureFlags.test-renderer.www.js | 3 +- .../shared/forks/ReactFeatureFlags.testing.js | 3 +- .../forks/ReactFeatureFlags.testing.www.js | 3 +- .../forks/ReactFeatureFlags.www-dynamic.js | 3 +- .../shared/forks/ReactFeatureFlags.www.js | 3 +- 12 files changed, 193 insertions(+), 103 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 36472020c602e..41900aa28aca3 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -35,8 +35,7 @@ import { enableSuspenseCallback, enableScopeAPI, enableStrictEffects, - enableStrongMemoryCleanup, - enableDetachOldChildList, + deletedTreeCleanUpLevel, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -1212,40 +1211,93 @@ function detachFiberMutation(fiber: Fiber) { // Don't reset the alternate yet, either. We need that so we can detach the // alternate's fields in the passive phase. Clearing the return pointer is // sufficient for findDOMNode semantics. + const alternate = fiber.alternate; + if (alternate !== null) { + alternate.return = null; + } fiber.return = null; } -export function detachFiberAfterEffects( - fiber: Fiber, - recurseIntoSibbling: ?boolean, -): void { - // Null out fields to improve GC for references that may be lingering (e.g. DevTools). - // Note that we already cleared the return pointer in detachFiberMutation(). - if (enableStrongMemoryCleanup) { - if (fiber.child) { - detachFiberAfterEffects(fiber.child, true); - } - if (fiber.sibling && recurseIntoSibbling === true) { - detachFiberAfterEffects(fiber.sibling, true); - } - if (fiber.stateNode) { - unmountNode(fiber.stateNode); - } - fiber.return = null; - } - fiber.alternate = null; - 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; +function detachFiberAfterEffects(fiber: Fiber) { + const alternate = fiber.alternate; + if (alternate !== null) { + fiber.alternate = null; + detachFiberAfterEffects(alternate); + } + + // Note: Defensively using negation instead of < in case + // `deletedTreeCleanUpLevel` is undefined. + if (!(deletedTreeCleanUpLevel >= 2)) { + // This is the default branch (level 0). We do not recursively clear all the + // fiber fields. Only the root of the deleted subtree. + 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; + if (__DEV__) { + fiber._debugOwner = null; + } + } else { + // 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. + let child = fiber.child; + while (child !== null) { + detachFiberAfterEffects(child); + child = child.sibling; + } + // 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; + + // 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. + + 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. + + // For host components, disconnect host instance -> fiber pointer. + if (fiber.tag === HostComponent) { + const hostInstance: Instance = fiber.stateNode; + if (hostInstance !== null) { + unmountNode(hostInstance); + } + } + + 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; + } } } @@ -1637,11 +1689,8 @@ function commitDeletion( renderPriorityLevel, ); } - const alternate = current.alternate; + detachFiberMutation(current); - if (alternate !== null) { - detachFiberMutation(alternate); - } } function commitWork(current: Fiber | null, finishedWork: Fiber): void { @@ -2318,14 +2367,10 @@ function commitPassiveUnmountEffects_begin() { ); // Now that passive effects have been processed, it's safe to detach lingering pointers. - const alternate = fiberToDelete.alternate; detachFiberAfterEffects(fiberToDelete); - if (alternate !== null) { - detachFiberAfterEffects(alternate); - } } - if (enableDetachOldChildList) { + 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 diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index b21ded0035759..3acdd547adf02 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -35,8 +35,7 @@ import { enableSuspenseCallback, enableScopeAPI, enableStrictEffects, - enableStrongMemoryCleanup, - enableDetachOldChildList, + deletedTreeCleanUpLevel, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -1212,40 +1211,93 @@ function detachFiberMutation(fiber: Fiber) { // Don't reset the alternate yet, either. We need that so we can detach the // alternate's fields in the passive phase. Clearing the return pointer is // sufficient for findDOMNode semantics. + const alternate = fiber.alternate; + if (alternate !== null) { + alternate.return = null; + } fiber.return = null; } -export function detachFiberAfterEffects( - fiber: Fiber, - recurseIntoSibbling: ?boolean, -): void { - // Null out fields to improve GC for references that may be lingering (e.g. DevTools). - // Note that we already cleared the return pointer in detachFiberMutation(). - if (enableStrongMemoryCleanup) { - if (fiber.child) { - detachFiberAfterEffects(fiber.child, true); - } - if (fiber.sibling && recurseIntoSibbling === true) { - detachFiberAfterEffects(fiber.sibling, true); - } - if (fiber.stateNode) { - unmountNode(fiber.stateNode); - } - fiber.return = null; - } - fiber.alternate = null; - 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; +function detachFiberAfterEffects(fiber: Fiber) { + const alternate = fiber.alternate; + if (alternate !== null) { + fiber.alternate = null; + detachFiberAfterEffects(alternate); + } + + // Note: Defensively using negation instead of < in case + // `deletedTreeCleanUpLevel` is undefined. + if (!(deletedTreeCleanUpLevel >= 2)) { + // This is the default branch (level 0). We do not recursively clear all the + // fiber fields. Only the root of the deleted subtree. + 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; + if (__DEV__) { + fiber._debugOwner = null; + } + } else { + // 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. + let child = fiber.child; + while (child !== null) { + detachFiberAfterEffects(child); + child = child.sibling; + } + // 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; + + // 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. + + 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. + + // For host components, disconnect host instance -> fiber pointer. + if (fiber.tag === HostComponent) { + const hostInstance: Instance = fiber.stateNode; + if (hostInstance !== null) { + unmountNode(hostInstance); + } + } + + 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; + } } } @@ -1637,11 +1689,8 @@ function commitDeletion( renderPriorityLevel, ); } - const alternate = current.alternate; + detachFiberMutation(current); - if (alternate !== null) { - detachFiberMutation(alternate); - } } function commitWork(current: Fiber | null, finishedWork: Fiber): void { @@ -2318,14 +2367,10 @@ function commitPassiveUnmountEffects_begin() { ); // Now that passive effects have been processed, it's safe to detach lingering pointers. - const alternate = fiberToDelete.alternate; detachFiberAfterEffects(fiberToDelete); - if (alternate !== null) { - detachFiberAfterEffects(alternate); - } } - if (enableDetachOldChildList) { + 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 diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 591a24c3cd317..a7c4ffe5328f0 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -112,9 +112,18 @@ export const disableNativeComponentFrames = false; // If there are no still-mounted boundaries, the errors should be rethrown. export const skipUnmountedBoundaries = false; -// When a node is unmounted, recurse into the Fiber subtree and clean out references. -export const enableStrongMemoryCleanup = false; -export const enableDetachOldChildList = false; +// 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 = 1; // -------------------------- // Future APIs to be deprecated diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 913063d587be3..8ec54b57f88f2 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -46,8 +46,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const enableStrongMemoryCleanup = false; -export const enableDetachOldChildList = false; +export const deletedTreeCleanUpLevel = 1; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 8714352a17686..6c0740a2a78ac 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -45,8 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const enableStrongMemoryCleanup = false; -export const enableDetachOldChildList = false; +export const deletedTreeCleanUpLevel = 1; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 0ea1b5fda395a..60284af873279 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -45,8 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const enableStrongMemoryCleanup = false; -export const enableDetachOldChildList = false; +export const deletedTreeCleanUpLevel = 1; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index db62ddc1dc10e..2bd3f6a78e360 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -45,8 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const enableStrongMemoryCleanup = false; -export const enableDetachOldChildList = false; +export const deletedTreeCleanUpLevel = 1; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 46fd65d187658..65bec28f22b5f 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -45,8 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const enableStrongMemoryCleanup = false; -export const enableDetachOldChildList = false; +export const deletedTreeCleanUpLevel = 1; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 144ce7f4dca87..70b89ebe2bc85 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -45,8 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; -export const enableStrongMemoryCleanup = false; -export const enableDetachOldChildList = false; +export const deletedTreeCleanUpLevel = 1; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index 188c6bdcffdfe..145590d2c2c72 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -45,8 +45,7 @@ export const enableLegacyFBSupport = !__EXPERIMENTAL__; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = true; -export const enableStrongMemoryCleanup = false; -export const enableDetachOldChildList = false; +export const deletedTreeCleanUpLevel = 1; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index bd666333c5f72..efd9e7512dc59 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -52,8 +52,7 @@ export const disableNativeComponentFrames = false; export const createRootStrictEffectsByDefault = false; export const enableStrictEffects = false; export const enableUseRefAccessWarning = __VARIANT__; -export const enableStrongMemoryCleanup = __VARIANT__; -export const enableDetachOldChildList = __VARIANT__; +export const deletedTreeCleanUpLevel = __VARIANT__ ? 3 : 1; export const enableProfilerNestedUpdateScheduledHook = __VARIANT__; export const disableSchedulerTimeoutInWorkLoop = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 0f9e27011af06..0c494fb3200e2 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -33,8 +33,7 @@ export const { disableSchedulerTimeoutInWorkLoop, enableSyncMicroTasks, enableLazyContextPropagation, - enableStrongMemoryCleanup, - enableDetachOldChildList, + deletedTreeCleanUpLevel, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build.