Clean up host pointers in level 2 of clean-up flag (#21112)
The host tree is a cyclical structure. Leaking a single DOM node can retain a large amount of memory. React-managed DOM nodes also point back to a fiber tree. Perf testing suggests that disconnecting these fields has a big memory impact. That suggests leaks in non-React code but since it's hard to completely eliminate those, it may still be worth the extra work to clear these fields. I'm moving this to level 2 to confirm whether this alone is responsible for the memory savings, or if there are other fields that are retaining large amounts of memory. In our plan for removing the alternate model, DOM nodes would not be connected to fibers, except at the root of the whole tree, which is easy to disconnect on deletion. So in that world, we likely won't have to do any additional work.
Andrew Clark committed
Mar 27, 2021 at 15:26 UTC
5f21a9fca455069bc1e986e1528963a5055a8f21
2 files changed
+24
-18
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+12
-9
@@ -1251,6 +1251,18 @@ function detachFiberAfterEffects(fiber: Fiber) {
1251
fiber.deletions = null;
1252
fiber.sibling = null;
1253
1254
+ // The `stateNode` is cyclical because on host nodes it points to the host
1255
+ // tree, which has its own pointers to children, parents, and siblings.
1256
+ // The other host nodes also point back to fibers, so we should detach that
1257
+ // one, too.
1258
+ if (fiber.tag === HostComponent) {
1259
+ const hostInstance: Instance = fiber.stateNode;
1260
+ if (hostInstance !== null) {
1261
+ detachDeletedInstance(hostInstance);
1262
+ }
1263
+ }
1264
+ fiber.stateNode = null;
1265
+
1266
// I'm intentionally not clearing the `return` field in this level. We
1267
// already disconnect the `return` pointer at the root of the deleted
1268
// subtree (in `detachFiberMutation`). Besides, `return` by itself is not
@@ -1269,15 +1281,6 @@ function detachFiberAfterEffects(fiber: Fiber) {
1281
// The purpose of this branch is to be super aggressive so we can measure
1282
// if there's any difference in memory impact. If there is, that could
1283
// indicate a React leak we don't know about.
1272
-
1273
- // For host components, disconnect host instance -> fiber pointer.
1274
- if (fiber.tag === HostComponent) {
1275
- const hostInstance: Instance = fiber.stateNode;
1276
- if (hostInstance !== null) {
1277
- detachDeletedInstance(hostInstance);
1278
- }
1279
- }
1280
-
1284
fiber.return = null;
1285
fiber.dependencies = null;
1286
fiber.memoizedProps = null;
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+12
-9
@@ -1251,6 +1251,18 @@ function detachFiberAfterEffects(fiber: Fiber) {
1251
fiber.deletions = null;
1252
fiber.sibling = null;
1253
1254
+ // The `stateNode` is cyclical because on host nodes it points to the host
1255
+ // tree, which has its own pointers to children, parents, and siblings.
1256
+ // The other host nodes also point back to fibers, so we should detach that
1257
+ // one, too.
1258
+ if (fiber.tag === HostComponent) {
1259
+ const hostInstance: Instance = fiber.stateNode;
1260
+ if (hostInstance !== null) {
1261
+ detachDeletedInstance(hostInstance);
1262
+ }
1263
+ }
1264
+ fiber.stateNode = null;
1265
+
1266
// I'm intentionally not clearing the `return` field in this level. We
1267
// already disconnect the `return` pointer at the root of the deleted
1268
// subtree (in `detachFiberMutation`). Besides, `return` by itself is not
@@ -1269,15 +1281,6 @@ function detachFiberAfterEffects(fiber: Fiber) {
1281
// The purpose of this branch is to be super aggressive so we can measure
1282
// if there's any difference in memory impact. If there is, that could
1283
// indicate a React leak we don't know about.
1272
-
1273
- // For host components, disconnect host instance -> fiber pointer.
1274
- if (fiber.tag === HostComponent) {
1275
- const hostInstance: Instance = fiber.stateNode;
1276
- if (hostInstance !== null) {
1277
- detachDeletedInstance(hostInstance);
1278
- }
1279
- }
1280
-
1284
fiber.return = null;
1285
fiber.dependencies = null;
1286
fiber.memoizedProps = null;