@samitouri / QOS-React / commits / c6702656ff

Revert "Clean up host pointers in level 2 of clean-up flag (#21112)"

This reverts commit 8ed0c85bf174ce6e501be62d9ccec1889bbdbce1. 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 Apr 23, 2021 at 11:53 UTC c6702656ff312fda4da9f442c99fe3931745c80d

2 files changed +18 -24

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

+9 -12

@@ -1382,18 +1382,6 @@ function detachFiberAfterEffects(fiber: Fiber) {

1382 fiber.deletions = null;

1383 fiber.sibling = null;

1384

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

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

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

1388 - // one, too.

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

1390 - const hostInstance: Instance = fiber.stateNode;

1391 - if (hostInstance !== null) {

1392 - detachDeletedInstance(hostInstance);

1393 - }

1394 - }

1395 - fiber.stateNode = null;

1396 -

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

1386 // already disconnect the `return` pointer at the root of the deleted

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

@@ -1412,6 +1400,15 @@ function detachFiberAfterEffects(fiber: Fiber) {

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

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

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

1403 +

1404 + // For host components, disconnect host instance -> fiber pointer.

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

1406 + const hostInstance: Instance = fiber.stateNode;

1407 + if (hostInstance !== null) {

1408 + detachDeletedInstance(hostInstance);

1409 + }

1410 + }

1411 +

1412 fiber.return = null;

1413 fiber.dependencies = null;

1414 fiber.memoizedProps = null;

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