From 82c79166cfc8657df3740818a90f7b4d9b59cf5b Mon Sep 17 00:00:00 2001 From: Benoit Girard Date: Mon, 22 Mar 2021 20:21:24 -0500 Subject: [PATCH 1/5] Add feature flag: enableStrongMemoryCleanup Add a feature flag that will test doing a recursive clean of an unmount node. This will disconnect the fiber graph making leaks less severe. --- packages/react-art/src/ReactARTHostConfig.js | 4 ++++ .../src/client/ReactDOMComponentTree.js | 11 +++++++++++ .../src/client/ReactDOMHostConfig.js | 5 +++++ .../src/ReactFabricHostConfig.js | 4 ++++ .../src/ReactNativeHostConfig.js | 4 ++++ .../src/createReactNoop.js | 2 ++ .../src/ReactFiberCommitWork.new.js | 19 ++++++++++++++++++- .../src/ReactFiberCommitWork.old.js | 19 ++++++++++++++++++- .../src/forks/ReactFiberHostConfig.custom.js | 1 + .../src/ReactTestHostConfig.js | 4 ++++ packages/shared/ReactFeatureFlags.js | 3 +++ .../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 | 1 + 20 files changed, 83 insertions(+), 2 deletions(-) diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index cadf311d54ffd..6a02366979a07 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -471,3 +471,7 @@ export function afterActiveInstanceBlur() { export function preparePortalMount(portalInstance: any): void { // noop } + +export function unmountNode(node: any): void { + // noop +} diff --git a/packages/react-dom/src/client/ReactDOMComponentTree.js b/packages/react-dom/src/client/ReactDOMComponentTree.js index fbf0a81e29064..1c09f24931aee 100644 --- a/packages/react-dom/src/client/ReactDOMComponentTree.js +++ b/packages/react-dom/src/client/ReactDOMComponentTree.js @@ -43,6 +43,17 @@ const internalEventHandlersKey = '__reactEvents$' + randomKey; const internalEventHandlerListenersKey = '__reactListeners$' + randomKey; const internalEventHandlesSetKey = '__reactHandles$' + randomKey; +export function unmountNode( + node: Instance | TextInstance | SuspenseInstance | ReactScopeInstance, +): void { + delete (node: any)[internalInstanceKey]; + delete (node: any)[internalPropsKey]; + delete (node: any)[internalContainerInstanceKey]; + delete (node: any)[internalEventHandlersKey]; + delete (node: any)[internalEventHandlerListenersKey]; + delete (node: any)[internalEventHandlesSetKey]; +} + export function precacheFiberNode( hostInst: Fiber, node: Instance | TextInstance | SuspenseInstance | ReactScopeInstance, diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index dfd29b375a41b..301bd2e72ef8a 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -24,6 +24,7 @@ import { getFiberFromScopeInstance, getInstanceFromNode as getInstanceFromNodeDOMTree, isContainerMarkedAsRoot, + unmountNode as unmountNodeFromDOMTree, } from './ReactDOMComponentTree'; import {hasRole} from './DOMAccessibilityRoles'; import { @@ -1209,3 +1210,7 @@ export function setupIntersectionObserver( }, }; } + +export function unmountNode(node: any): void { + unmountNodeFromDOMTree(node); +} diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index 85eb2a741ffac..121480051a4a5 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -481,3 +481,7 @@ export function afterActiveInstanceBlur() { export function preparePortalMount(portalInstance: Instance): void { // noop } + +export function unmountNode(node: any): void { + // noop +} diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index d626a175d9340..f1ec3143020ed 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -538,3 +538,7 @@ export function afterActiveInstanceBlur() { export function preparePortalMount(portalInstance: Instance): void { // noop } + +export function unmountNode(node: any): void { + // noop +} diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 80c888a88e53d..80b77ff9fe256 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -420,6 +420,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { getInstanceFromScope() { throw new Error('Not yet implemented.'); }, + + unmountNode() {}, }; const hostConfig = useMutation diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 7985366d3f67c..1e19f4086e0c7 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -35,6 +35,7 @@ import { enableSuspenseCallback, enableScopeAPI, enableStrictEffects, + enableStrongMemoryCleanup, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -60,6 +61,7 @@ import { hasCaughtError, clearCaughtError, } from 'shared/ReactErrorUtils'; +import {unmountNode} from './ReactFiberHostConfig'; import { NoFlags, ContentReset, @@ -1212,9 +1214,24 @@ function detachFiberMutation(fiber: Fiber) { fiber.return = null; } -export function detachFiberAfterEffects(fiber: Fiber): void { +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; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index d50095b3bebb1..8d187832e4d84 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -35,6 +35,7 @@ import { enableSuspenseCallback, enableScopeAPI, enableStrictEffects, + enableStrongMemoryCleanup, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -60,6 +61,7 @@ import { hasCaughtError, clearCaughtError, } from 'shared/ReactErrorUtils'; +import {unmountNode} from './ReactFiberHostConfig'; import { NoFlags, ContentReset, @@ -1212,9 +1214,24 @@ function detachFiberMutation(fiber: Fiber) { fiber.return = null; } -export function detachFiberAfterEffects(fiber: Fiber): void { +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; diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index a2f065102d05f..8e34f5119eee9 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -73,6 +73,7 @@ export const preparePortalMount = $$$hostConfig.preparePortalMount; export const prepareScopeUpdate = $$$hostConfig.preparePortalMount; export const getInstanceFromScope = $$$hostConfig.getInstanceFromScope; export const getCurrentEventPriority = $$$hostConfig.getCurrentEventPriority; +export const unmountNode = $$$hostConfig.unmountNode; // ------------------- // Microtasks diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index a787d31840af3..babb0c2437215 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -360,3 +360,7 @@ export function prepareScopeUpdate(scopeInstance: Object, inst: Object): void { export function getInstanceFromScope(scopeInstance: Object): null | Object { return nodeToInstanceMap.get(scopeInstance) || null; } + +export function unmountNode(node: any): void { + // noop +} diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 75ab4b6842d90..2a3d0a337bf88 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -112,6 +112,9 @@ 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; + // -------------------------- // Future APIs to be deprecated // -------------------------- diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index b6a8016ec8b77..443c489c0063f 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -46,6 +46,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; +export const enableStrongMemoryCleanup = false; 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 cb8721af25fe9..bac13f5091b78 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; +export const enableStrongMemoryCleanup = false; 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 b504491d98de2..da03038c0211a 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; +export const enableStrongMemoryCleanup = false; 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 ddab49284e437..eda47cde1139b 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; +export const enableStrongMemoryCleanup = false; 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 7098bfed2aa5b..9b02f6408229f 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; +export const enableStrongMemoryCleanup = false; 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 a321f5ea12f92..7bb7ed424c7bd 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; +export const enableStrongMemoryCleanup = false; 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 bb6a952355657..8834238b02b14 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -45,6 +45,7 @@ export const enableLegacyFBSupport = !__EXPERIMENTAL__; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = true; +export const enableStrongMemoryCleanup = false; 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 df98012634970..f58e0da1e80b9 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -52,6 +52,7 @@ export const disableNativeComponentFrames = false; export const createRootStrictEffectsByDefault = false; export const enableStrictEffects = false; export const enableUseRefAccessWarning = __VARIANT__; +export const enableStrongMemoryCleanup = __VARIANT__; 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 893c2699b0d07..25b88a019f1da 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -33,6 +33,7 @@ export const { disableSchedulerTimeoutInWorkLoop, enableSyncMicroTasks, enableLazyContextPropagation, + enableStrongMemoryCleanup, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build. From a16ac10c514f830e461f7d8e33f10bc3e139555f Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 22 Mar 2021 20:21:32 -0500 Subject: [PATCH 2/5] Detach sibling pointers in old child list When a fiber is deleted, 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. Will use this feature flag to test the memory impact. --- .../src/ReactFiberCommitWork.new.js | 28 +++++++++++++++++++ .../src/ReactFiberCommitWork.old.js | 28 +++++++++++++++++++ packages/shared/ReactFeatureFlags.js | 1 + .../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 | 1 + 12 files changed, 66 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 1e19f4086e0c7..36472020c602e 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -36,6 +36,7 @@ import { enableScopeAPI, enableStrictEffects, enableStrongMemoryCleanup, + enableDetachOldChildList, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -2323,6 +2324,33 @@ function commitPassiveUnmountEffects_begin() { detachFiberAfterEffects(alternate); } } + + if (enableDetachOldChildList) { + // 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 = fiber.alternate; + if (previousFiber !== null) { + let detachedChild = previousFiber.child; + if (detachedChild !== null) { + previousFiber.child = null; + do { + const detachedSibling = detachedChild.sibling; + detachedChild.sibling = null; + detachedChild = detachedSibling; + } while (detachedChild !== null); + } + } + } + nextEffect = fiber; } } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 8d187832e4d84..b21ded0035759 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -36,6 +36,7 @@ import { enableScopeAPI, enableStrictEffects, enableStrongMemoryCleanup, + enableDetachOldChildList, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -2323,6 +2324,33 @@ function commitPassiveUnmountEffects_begin() { detachFiberAfterEffects(alternate); } } + + if (enableDetachOldChildList) { + // 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 = fiber.alternate; + if (previousFiber !== null) { + let detachedChild = previousFiber.child; + if (detachedChild !== null) { + previousFiber.child = null; + do { + const detachedSibling = detachedChild.sibling; + detachedChild.sibling = null; + detachedChild = detachedSibling; + } while (detachedChild !== null); + } + } + } + nextEffect = fiber; } } diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 2a3d0a337bf88..591a24c3cd317 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -114,6 +114,7 @@ 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; // -------------------------- // Future APIs to be deprecated diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 443c489c0063f..913063d587be3 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; export const enableStrongMemoryCleanup = false; +export const enableDetachOldChildList = false; 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 bac13f5091b78..8714352a17686 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -46,6 +46,7 @@ export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; export const enableStrongMemoryCleanup = false; +export const enableDetachOldChildList = false; 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 da03038c0211a..0ea1b5fda395a 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -46,6 +46,7 @@ export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; export const enableStrongMemoryCleanup = false; +export const enableDetachOldChildList = false; 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 eda47cde1139b..db62ddc1dc10e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -46,6 +46,7 @@ export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; export const enableStrongMemoryCleanup = false; +export const enableDetachOldChildList = false; 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 9b02f6408229f..46fd65d187658 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -46,6 +46,7 @@ export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; export const enableStrongMemoryCleanup = false; +export const enableDetachOldChildList = false; 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 7bb7ed424c7bd..144ce7f4dca87 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -46,6 +46,7 @@ export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = false; export const enableStrongMemoryCleanup = false; +export const enableDetachOldChildList = false; 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 8834238b02b14..188c6bdcffdfe 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -46,6 +46,7 @@ export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; export const skipUnmountedBoundaries = true; export const enableStrongMemoryCleanup = false; +export const enableDetachOldChildList = false; 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 f58e0da1e80b9..bd666333c5f72 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -53,6 +53,7 @@ export const createRootStrictEffectsByDefault = false; export const enableStrictEffects = false; export const enableUseRefAccessWarning = __VARIANT__; export const enableStrongMemoryCleanup = __VARIANT__; +export const enableDetachOldChildList = __VARIANT__; 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 25b88a019f1da..0f9e27011af06 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -34,6 +34,7 @@ export const { enableSyncMicroTasks, enableLazyContextPropagation, enableStrongMemoryCleanup, + enableDetachOldChildList, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build. From 100239e1fe787d9ec2a07b1d9a09ea2de4497516 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 22 Mar 2021 22:31:52 -0500 Subject: [PATCH 3/5] 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. From f73ae7e1aba3c7bd959aa2c4c7217075636ce695 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 22 Mar 2021 23:06:44 -0500 Subject: [PATCH 4/5] Add Flow type to new host config method --- packages/react-art/src/ReactARTHostConfig.js | 2 +- packages/react-dom/src/client/ReactDOMComponentTree.js | 7 +++---- packages/react-dom/src/client/ReactDOMHostConfig.js | 6 +----- .../react-native-renderer/src/ReactFabricHostConfig.js | 2 +- .../react-native-renderer/src/ReactNativeHostConfig.js | 2 +- packages/react-noop-renderer/src/createReactNoop.js | 2 +- packages/react-reconciler/src/ReactFiberCommitWork.new.js | 4 ++-- packages/react-reconciler/src/ReactFiberCommitWork.old.js | 4 ++-- .../src/forks/ReactFiberHostConfig.custom.js | 2 +- packages/react-test-renderer/src/ReactTestHostConfig.js | 2 +- 10 files changed, 14 insertions(+), 19 deletions(-) diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index 6a02366979a07..7ae3941c9253a 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -472,6 +472,6 @@ export function preparePortalMount(portalInstance: any): void { // noop } -export function unmountNode(node: any): void { +export function detachDeletedInstance(node: Instance): void { // noop } diff --git a/packages/react-dom/src/client/ReactDOMComponentTree.js b/packages/react-dom/src/client/ReactDOMComponentTree.js index 1c09f24931aee..2afe66035f364 100644 --- a/packages/react-dom/src/client/ReactDOMComponentTree.js +++ b/packages/react-dom/src/client/ReactDOMComponentTree.js @@ -43,12 +43,11 @@ const internalEventHandlersKey = '__reactEvents$' + randomKey; const internalEventHandlerListenersKey = '__reactListeners$' + randomKey; const internalEventHandlesSetKey = '__reactHandles$' + randomKey; -export function unmountNode( - node: Instance | TextInstance | SuspenseInstance | ReactScopeInstance, -): void { +export function detachDeletedInstance(node: Instance): void { + // TODO: This function is only called on host components. I don't think all of + // these fields are relevant. delete (node: any)[internalInstanceKey]; delete (node: any)[internalPropsKey]; - delete (node: any)[internalContainerInstanceKey]; delete (node: any)[internalEventHandlersKey]; delete (node: any)[internalEventHandlerListenersKey]; delete (node: any)[internalEventHandlesSetKey]; diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 301bd2e72ef8a..b00432d045168 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -24,8 +24,8 @@ import { getFiberFromScopeInstance, getInstanceFromNode as getInstanceFromNodeDOMTree, isContainerMarkedAsRoot, - unmountNode as unmountNodeFromDOMTree, } from './ReactDOMComponentTree'; +export {detachDeletedInstance} from './ReactDOMComponentTree'; import {hasRole} from './DOMAccessibilityRoles'; import { createElement, @@ -1210,7 +1210,3 @@ export function setupIntersectionObserver( }, }; } - -export function unmountNode(node: any): void { - unmountNodeFromDOMTree(node); -} diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index 121480051a4a5..52d1ecf9385e0 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -482,6 +482,6 @@ export function preparePortalMount(portalInstance: Instance): void { // noop } -export function unmountNode(node: any): void { +export function detachDeletedInstance(node: Instance): void { // noop } diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index f1ec3143020ed..84538532979e1 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -539,6 +539,6 @@ export function preparePortalMount(portalInstance: Instance): void { // noop } -export function unmountNode(node: any): void { +export function detachDeletedInstance(node: Instance): void { // noop } diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 80b77ff9fe256..dd304fce6d878 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -421,7 +421,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { throw new Error('Not yet implemented.'); }, - unmountNode() {}, + detachDeletedInstance() {}, }; const hostConfig = useMutation diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 41900aa28aca3..3e8de112ad512 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -61,7 +61,7 @@ import { hasCaughtError, clearCaughtError, } from 'shared/ReactErrorUtils'; -import {unmountNode} from './ReactFiberHostConfig'; +import {detachDeletedInstance} from './ReactFiberHostConfig'; import { NoFlags, ContentReset, @@ -1285,7 +1285,7 @@ function detachFiberAfterEffects(fiber: Fiber) { if (fiber.tag === HostComponent) { const hostInstance: Instance = fiber.stateNode; if (hostInstance !== null) { - unmountNode(hostInstance); + detachDeletedInstance(hostInstance); } } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 3acdd547adf02..67dfc947ef79a 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -61,7 +61,7 @@ import { hasCaughtError, clearCaughtError, } from 'shared/ReactErrorUtils'; -import {unmountNode} from './ReactFiberHostConfig'; +import {detachDeletedInstance} from './ReactFiberHostConfig'; import { NoFlags, ContentReset, @@ -1285,7 +1285,7 @@ function detachFiberAfterEffects(fiber: Fiber) { if (fiber.tag === HostComponent) { const hostInstance: Instance = fiber.stateNode; if (hostInstance !== null) { - unmountNode(hostInstance); + detachDeletedInstance(hostInstance); } } diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index 8e34f5119eee9..0c2895deda989 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -73,7 +73,7 @@ export const preparePortalMount = $$$hostConfig.preparePortalMount; export const prepareScopeUpdate = $$$hostConfig.preparePortalMount; export const getInstanceFromScope = $$$hostConfig.getInstanceFromScope; export const getCurrentEventPriority = $$$hostConfig.getCurrentEventPriority; -export const unmountNode = $$$hostConfig.unmountNode; +export const detachDeletedInstance = $$$hostConfig.detachDeletedInstance; // ------------------- // Microtasks diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index babb0c2437215..bac900c469fa7 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -361,6 +361,6 @@ export function getInstanceFromScope(scopeInstance: Object): null | Object { return nodeToInstanceMap.get(scopeInstance) || null; } -export function unmountNode(node: any): void { +export function detachDeletedInstance(node: Instance): void { // noop } From 31e19771bbf0120963bb36578b5184a64fdca95f Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 22 Mar 2021 23:45:05 -0500 Subject: [PATCH 5/5] Re-use existing recursive clean up path We already have a recursive loop that visits every deleted fiber. We can re-use that one for clean up instead of adding another one. --- .../src/ReactFiberCommitWork.new.js | 45 ++++++++++--------- .../src/ReactFiberCommitWork.old.js | 45 ++++++++++--------- 2 files changed, 50 insertions(+), 40 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 3e8de112ad512..3ae0766cdac08 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1228,8 +1228,7 @@ function detachFiberAfterEffects(fiber: Fiber) { // 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. + // This is the default branch (level 0). fiber.child = null; fiber.deletions = null; fiber.dependencies = null; @@ -1244,14 +1243,6 @@ function detachFiberAfterEffects(fiber: Fiber) { 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 @@ -2365,9 +2356,6 @@ function commitPassiveUnmountEffects_begin() { fiberToDelete, fiber, ); - - // Now that passive effects have been processed, it's safe to detach lingering pointers. - detachFiberAfterEffects(fiberToDelete); } if (deletedTreeCleanUpLevel >= 1) { @@ -2472,7 +2460,8 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( resetCurrentDebugFiberInDEV(); const child = fiber.child; - // TODO: Only traverse subtree if it has a PassiveStatic flag + // TODO: Only traverse subtree if it has a PassiveStatic flag. (But, if we + // do this, still need to handle `deletedTreeCleanUpLevel` correctly.) if (child !== null) { ensureCorrectReturnPointer(child, fiber); nextEffect = child; @@ -2489,19 +2478,35 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete( ) { while (nextEffect !== null) { const fiber = nextEffect; - if (fiber === deletedSubtreeRoot) { - nextEffect = null; - return; + 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; + } } - const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + ensureCorrectReturnPointer(sibling, returnFiber); nextEffect = sibling; return; } - nextEffect = fiber.return; + nextEffect = returnFiber; } } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 67dfc947ef79a..e020518a42f13 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -1228,8 +1228,7 @@ function detachFiberAfterEffects(fiber: Fiber) { // 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. + // This is the default branch (level 0). fiber.child = null; fiber.deletions = null; fiber.dependencies = null; @@ -1244,14 +1243,6 @@ function detachFiberAfterEffects(fiber: Fiber) { 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 @@ -2365,9 +2356,6 @@ function commitPassiveUnmountEffects_begin() { fiberToDelete, fiber, ); - - // Now that passive effects have been processed, it's safe to detach lingering pointers. - detachFiberAfterEffects(fiberToDelete); } if (deletedTreeCleanUpLevel >= 1) { @@ -2472,7 +2460,8 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( resetCurrentDebugFiberInDEV(); const child = fiber.child; - // TODO: Only traverse subtree if it has a PassiveStatic flag + // TODO: Only traverse subtree if it has a PassiveStatic flag. (But, if we + // do this, still need to handle `deletedTreeCleanUpLevel` correctly.) if (child !== null) { ensureCorrectReturnPointer(child, fiber); nextEffect = child; @@ -2489,19 +2478,35 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete( ) { while (nextEffect !== null) { const fiber = nextEffect; - if (fiber === deletedSubtreeRoot) { - nextEffect = null; - return; + 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; + } } - const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + ensureCorrectReturnPointer(sibling, returnFiber); nextEffect = sibling; return; } - nextEffect = fiber.return; + nextEffect = returnFiber; } }