Skip to content

Commit 100239e

Browse files
acdliteacdlite
committed
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.
1 parent a16ac10 commit 100239e

12 files changed

+193
-103
lines changed

‎packages/react-reconciler/src/ReactFiberCommitWork.new.js‎

Lines changed: 86 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,7 @@ import {
3535
enableSuspenseCallback,
3636
enableScopeAPI,
3737
enableStrictEffects,
38-
enableStrongMemoryCleanup,
39-
enableDetachOldChildList,
38+
deletedTreeCleanUpLevel,
4039
} from 'shared/ReactFeatureFlags';
4140
import {
4241
FunctionComponent,
@@ -1212,40 +1211,93 @@ function detachFiberMutation(fiber: Fiber) {
12121211
// Don't reset the alternate yet, either. We need that so we can detach the
12131212
// alternate's fields in the passive phase. Clearing the return pointer is
12141213
// sufficient for findDOMNode semantics.
1214+
const alternate = fiber.alternate;
1215+
if (alternate !== null) {
1216+
alternate.return = null;
1217+
}
12151218
fiber.return = null;
12161219
}
12171220

1218-
export function detachFiberAfterEffects(
1219-
fiber: Fiber,
1220-
recurseIntoSibbling: ?boolean,
1221-
): void {
1222-
// Null out fields to improve GC for references that may be lingering (e.g. DevTools).
1223-
// Note that we already cleared the return pointer in detachFiberMutation().
1224-
if (enableStrongMemoryCleanup) {
1225-
if (fiber.child) {
1226-
detachFiberAfterEffects(fiber.child, true);
1227-
}
1228-
if (fiber.sibling && recurseIntoSibbling === true) {
1229-
detachFiberAfterEffects(fiber.sibling, true);
1230-
}
1231-
if (fiber.stateNode) {
1232-
unmountNode(fiber.stateNode);
1233-
}
1234-
fiber.return = null;
1235-
}
1236-
fiber.alternate = null;
1237-
fiber.child = null;
1238-
fiber.deletions = null;
1239-
fiber.dependencies = null;
1240-
fiber.memoizedProps = null;
1241-
fiber.memoizedState = null;
1242-
fiber.pendingProps = null;
1243-
fiber.sibling = null;
1244-
fiber.stateNode = null;
1245-
fiber.updateQueue = null;
1221+
function detachFiberAfterEffects(fiber: Fiber) {
1222+
const alternate = fiber.alternate;
1223+
if (alternate !== null) {
1224+
fiber.alternate = null;
1225+
detachFiberAfterEffects(alternate);
1226+
}
1227+
1228+
// Note: Defensively using negation instead of < in case
1229+
// `deletedTreeCleanUpLevel` is undefined.
1230+
if (!(deletedTreeCleanUpLevel >= 2)) {
1231+
// This is the default branch (level 0). We do not recursively clear all the
1232+
// fiber fields. Only the root of the deleted subtree.
1233+
fiber.child = null;
1234+
fiber.deletions = null;
1235+
fiber.dependencies = null;
1236+
fiber.memoizedProps = null;
1237+
fiber.memoizedState = null;
1238+
fiber.pendingProps = null;
1239+
fiber.sibling = null;
1240+
fiber.stateNode = null;
1241+
fiber.updateQueue = null;
12461242

1247-
if (__DEV__) {
1248-
fiber._debugOwner = null;
1243+
if (__DEV__) {
1244+
fiber._debugOwner = null;
1245+
}
1246+
} else {
1247+
// Recursively traverse the entire deleted tree and clean up fiber fields.
1248+
// This is more aggressive than ideal, and the long term goal is to only
1249+
// have to detach the deleted tree at the root.
1250+
let child = fiber.child;
1251+
while (child !== null) {
1252+
detachFiberAfterEffects(child);
1253+
child = child.sibling;
1254+
}
1255+
// Clear cyclical Fiber fields. This level alone is designed to roughly
1256+
// approximate the planned Fiber refactor. In that world, `setState` will be
1257+
// bound to a special "instance" object instead of a Fiber. The Instance
1258+
// object will not have any of these fields. It will only be connected to
1259+
// the fiber tree via a single link at the root. So if this level alone is
1260+
// sufficient to fix memory issues, that bodes well for our plans.
1261+
fiber.child = null;
1262+
fiber.deletions = null;
1263+
fiber.sibling = null;
1264+
1265+
// I'm intentionally not clearing the `return` field in this level. We
1266+
// already disconnect the `return` pointer at the root of the deleted
1267+
// subtree (in `detachFiberMutation`). Besides, `return` by itself is not
1268+
// cyclical — it's only cyclical when combined with `child`, `sibling`, and
1269+
// `alternate`. But we'll clear it in the next level anyway, just in case.
1270+
1271+
if (__DEV__) {
1272+
fiber._debugOwner = null;
1273+
}
1274+
1275+
if (deletedTreeCleanUpLevel >= 3) {
1276+
// Theoretically, nothing in here should be necessary, because we already
1277+
// disconnected the fiber from the tree. So even if something leaks this
1278+
// particular fiber, it won't leak anything else
1279+
//
1280+
// The purpose of this branch is to be super aggressive so we can measure
1281+
// if there's any difference in memory impact. If there is, that could
1282+
// indicate a React leak we don't know about.
1283+
1284+
// For host components, disconnect host instance -> fiber pointer.
1285+
if (fiber.tag === HostComponent) {
1286+
const hostInstance: Instance = fiber.stateNode;
1287+
if (hostInstance !== null) {
1288+
unmountNode(hostInstance);
1289+
}
1290+
}
1291+
1292+
fiber.return = null;
1293+
fiber.dependencies = null;
1294+
fiber.memoizedProps = null;
1295+
fiber.memoizedState = null;
1296+
fiber.pendingProps = null;
1297+
fiber.stateNode = null;
1298+
// TODO: Move to `commitPassiveUnmountInsideDeletedTreeOnFiber` instead.
1299+
fiber.updateQueue = null;
1300+
}
12491301
}
12501302
}
12511303

@@ -1637,11 +1689,8 @@ function commitDeletion(
16371689
renderPriorityLevel,
16381690
);
16391691
}
1640-
const alternate = current.alternate;
1692+
16411693
detachFiberMutation(current);
1642-
if (alternate !== null) {
1643-
detachFiberMutation(alternate);
1644-
}
16451694
}
16461695

16471696
function commitWork(current: Fiber | null, finishedWork: Fiber): void {
@@ -2318,14 +2367,10 @@ function commitPassiveUnmountEffects_begin() {
23182367
);
23192368

23202369
// Now that passive effects have been processed, it's safe to detach lingering pointers.
2321-
const alternate = fiberToDelete.alternate;
23222370
detachFiberAfterEffects(fiberToDelete);
2323-
if (alternate !== null) {
2324-
detachFiberAfterEffects(alternate);
2325-
}
23262371
}
23272372

2328-
if (enableDetachOldChildList) {
2373+
if (deletedTreeCleanUpLevel >= 1) {
23292374
// A fiber was deleted from this parent fiber, but it's still part of
23302375
// the previous (alternate) parent fiber's list of children. Because
23312376
// children are a linked list, an earlier sibling that's still alive

‎packages/react-reconciler/src/ReactFiberCommitWork.old.js‎

Lines changed: 86 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,7 @@ import {
3535
enableSuspenseCallback,
3636
enableScopeAPI,
3737
enableStrictEffects,
38-
enableStrongMemoryCleanup,
39-
enableDetachOldChildList,
38+
deletedTreeCleanUpLevel,
4039
} from 'shared/ReactFeatureFlags';
4140
import {
4241
FunctionComponent,
@@ -1212,40 +1211,93 @@ function detachFiberMutation(fiber: Fiber) {
12121211
// Don't reset the alternate yet, either. We need that so we can detach the
12131212
// alternate's fields in the passive phase. Clearing the return pointer is
12141213
// sufficient for findDOMNode semantics.
1214+
const alternate = fiber.alternate;
1215+
if (alternate !== null) {
1216+
alternate.return = null;
1217+
}
12151218
fiber.return = null;
12161219
}
12171220

1218-
export function detachFiberAfterEffects(
1219-
fiber: Fiber,
1220-
recurseIntoSibbling: ?boolean,
1221-
): void {
1222-
// Null out fields to improve GC for references that may be lingering (e.g. DevTools).
1223-
// Note that we already cleared the return pointer in detachFiberMutation().
1224-
if (enableStrongMemoryCleanup) {
1225-
if (fiber.child) {
1226-
detachFiberAfterEffects(fiber.child, true);
1227-
}
1228-
if (fiber.sibling && recurseIntoSibbling === true) {
1229-
detachFiberAfterEffects(fiber.sibling, true);
1230-
}
1231-
if (fiber.stateNode) {
1232-
unmountNode(fiber.stateNode);
1233-
}
1234-
fiber.return = null;
1235-
}
1236-
fiber.alternate = null;
1237-
fiber.child = null;
1238-
fiber.deletions = null;
1239-
fiber.dependencies = null;
1240-
fiber.memoizedProps = null;
1241-
fiber.memoizedState = null;
1242-
fiber.pendingProps = null;
1243-
fiber.sibling = null;
1244-
fiber.stateNode = null;
1245-
fiber.updateQueue = null;
1221+
function detachFiberAfterEffects(fiber: Fiber) {
1222+
const alternate = fiber.alternate;
1223+
if (alternate !== null) {
1224+
fiber.alternate = null;
1225+
detachFiberAfterEffects(alternate);
1226+
}
1227+
1228+
// Note: Defensively using negation instead of < in case
1229+
// `deletedTreeCleanUpLevel` is undefined.
1230+
if (!(deletedTreeCleanUpLevel >= 2)) {
1231+
// This is the default branch (level 0). We do not recursively clear all the
1232+
// fiber fields. Only the root of the deleted subtree.
1233+
fiber.child = null;
1234+
fiber.deletions = null;
1235+
fiber.dependencies = null;
1236+
fiber.memoizedProps = null;
1237+
fiber.memoizedState = null;
1238+
fiber.pendingProps = null;
1239+
fiber.sibling = null;
1240+
fiber.stateNode = null;
1241+
fiber.updateQueue = null;
12461242

1247-
if (__DEV__) {
1248-
fiber._debugOwner = null;
1243+
if (__DEV__) {
1244+
fiber._debugOwner = null;
1245+
}
1246+
} else {
1247+
// Recursively traverse the entire deleted tree and clean up fiber fields.
1248+
// This is more aggressive than ideal, and the long term goal is to only
1249+
// have to detach the deleted tree at the root.
1250+
let child = fiber.child;
1251+
while (child !== null) {
1252+
detachFiberAfterEffects(child);
1253+
child = child.sibling;
1254+
}
1255+
// Clear cyclical Fiber fields. This level alone is designed to roughly
1256+
// approximate the planned Fiber refactor. In that world, `setState` will be
1257+
// bound to a special "instance" object instead of a Fiber. The Instance
1258+
// object will not have any of these fields. It will only be connected to
1259+
// the fiber tree via a single link at the root. So if this level alone is
1260+
// sufficient to fix memory issues, that bodes well for our plans.
1261+
fiber.child = null;
1262+
fiber.deletions = null;
1263+
fiber.sibling = null;
1264+
1265+
// I'm intentionally not clearing the `return` field in this level. We
1266+
// already disconnect the `return` pointer at the root of the deleted
1267+
// subtree (in `detachFiberMutation`). Besides, `return` by itself is not
1268+
// cyclical — it's only cyclical when combined with `child`, `sibling`, and
1269+
// `alternate`. But we'll clear it in the next level anyway, just in case.
1270+
1271+
if (__DEV__) {
1272+
fiber._debugOwner = null;
1273+
}
1274+
1275+
if (deletedTreeCleanUpLevel >= 3) {
1276+
// Theoretically, nothing in here should be necessary, because we already
1277+
// disconnected the fiber from the tree. So even if something leaks this
1278+
// particular fiber, it won't leak anything else
1279+
//
1280+
// The purpose of this branch is to be super aggressive so we can measure
1281+
// if there's any difference in memory impact. If there is, that could
1282+
// indicate a React leak we don't know about.
1283+
1284+
// For host components, disconnect host instance -> fiber pointer.
1285+
if (fiber.tag === HostComponent) {
1286+
const hostInstance: Instance = fiber.stateNode;
1287+
if (hostInstance !== null) {
1288+
unmountNode(hostInstance);
1289+
}
1290+
}
1291+
1292+
fiber.return = null;
1293+
fiber.dependencies = null;
1294+
fiber.memoizedProps = null;
1295+
fiber.memoizedState = null;
1296+
fiber.pendingProps = null;
1297+
fiber.stateNode = null;
1298+
// TODO: Move to `commitPassiveUnmountInsideDeletedTreeOnFiber` instead.
1299+
fiber.updateQueue = null;
1300+
}
12491301
}
12501302
}
12511303

@@ -1637,11 +1689,8 @@ function commitDeletion(
16371689
renderPriorityLevel,
16381690
);
16391691
}
1640-
const alternate = current.alternate;
1692+
16411693
detachFiberMutation(current);
1642-
if (alternate !== null) {
1643-
detachFiberMutation(alternate);
1644-
}
16451694
}
16461695

16471696
function commitWork(current: Fiber | null, finishedWork: Fiber): void {
@@ -2318,14 +2367,10 @@ function commitPassiveUnmountEffects_begin() {
23182367
);
23192368

23202369
// Now that passive effects have been processed, it's safe to detach lingering pointers.
2321-
const alternate = fiberToDelete.alternate;
23222370
detachFiberAfterEffects(fiberToDelete);
2323-
if (alternate !== null) {
2324-
detachFiberAfterEffects(alternate);
2325-
}
23262371
}
23272372

2328-
if (enableDetachOldChildList) {
2373+
if (deletedTreeCleanUpLevel >= 1) {
23292374
// A fiber was deleted from this parent fiber, but it's still part of
23302375
// the previous (alternate) parent fiber's list of children. Because
23312376
// children are a linked list, an earlier sibling that's still alive

‎packages/shared/ReactFeatureFlags.js‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,18 @@ export const disableNativeComponentFrames = false;
112112
// If there are no still-mounted boundaries, the errors should be rethrown.
113113
export const skipUnmountedBoundaries = false;
114114

115-
// When a node is unmounted, recurse into the Fiber subtree and clean out references.
116-
export const enableStrongMemoryCleanup = false;
117-
export const enableDetachOldChildList = false;
115+
// When a node is unmounted, recurse into the Fiber subtree and clean out
116+
// references. Each level cleans up more fiber fields than the previous level.
117+
// As far as we know, React itself doesn't leak, but because the Fiber contains
118+
// cycles, even a single leak in product code can cause us to retain large
119+
// amounts of memory.
120+
//
121+
// The long term plan is to remove the cycles, but in the meantime, we clear
122+
// additional fields to mitigate.
123+
//
124+
// It's an enum so that we can experiment with different levels of
125+
// aggressiveness.
126+
export const deletedTreeCleanUpLevel = 1;
118127

119128
// --------------------------
120129
// Future APIs to be deprecated

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,7 @@ export const enableLegacyFBSupport = false;
4646
export const enableFilterEmptyStringAttributesDOM = false;
4747
export const disableNativeComponentFrames = false;
4848
export const skipUnmountedBoundaries = false;
49-
export const enableStrongMemoryCleanup = false;
50-
export const enableDetachOldChildList = false;
49+
export const deletedTreeCleanUpLevel = 1;
5150

5251
export const enableNewReconciler = false;
5352
export const deferRenderPhaseUpdateToNextBatch = true;

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,7 @@ export const enableLegacyFBSupport = false;
4545
export const enableFilterEmptyStringAttributesDOM = false;
4646
export const disableNativeComponentFrames = false;
4747
export const skipUnmountedBoundaries = false;
48-
export const enableStrongMemoryCleanup = false;
49-
export const enableDetachOldChildList = false;
48+
export const deletedTreeCleanUpLevel = 1;
5049

5150
export const enableNewReconciler = false;
5251
export const deferRenderPhaseUpdateToNextBatch = true;

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,7 @@ export const enableLegacyFBSupport = false;
4545
export const enableFilterEmptyStringAttributesDOM = false;
4646
export const disableNativeComponentFrames = false;
4747
export const skipUnmountedBoundaries = false;
48-
export const enableStrongMemoryCleanup = false;
49-
export const enableDetachOldChildList = false;
48+
export const deletedTreeCleanUpLevel = 1;
5049

5150
export const enableNewReconciler = false;
5251
export const deferRenderPhaseUpdateToNextBatch = true;

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,7 @@ export const enableLegacyFBSupport = false;
4545
export const enableFilterEmptyStringAttributesDOM = false;
4646
export const disableNativeComponentFrames = false;
4747
export const skipUnmountedBoundaries = false;
48-
export const enableStrongMemoryCleanup = false;
49-
export const enableDetachOldChildList = false;
48+
export const deletedTreeCleanUpLevel = 1;
5049

5150
export const enableNewReconciler = false;
5251
export const deferRenderPhaseUpdateToNextBatch = true;

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,7 @@ export const enableLegacyFBSupport = false;
4545
export const enableFilterEmptyStringAttributesDOM = false;
4646
export const disableNativeComponentFrames = false;
4747
export const skipUnmountedBoundaries = false;
48-
export const enableStrongMemoryCleanup = false;
49-
export const enableDetachOldChildList = false;
48+
export const deletedTreeCleanUpLevel = 1;
5049

5150
export const enableNewReconciler = false;
5251
export const deferRenderPhaseUpdateToNextBatch = true;

0 commit comments

Comments
 (0)