Skip to content

Commit

Permalink
[cleanup] remove deletedTreeCleanUpLevel feature flag (#25529)
Browse files Browse the repository at this point in the history
I noticed this was an experiment concluded 16 months ago (#21679) that
this extra work is beneficial
to break up cycles leaking memory in product code.
  • Loading branch information
kassens authored Jan 17, 2023
1 parent 4f8ffec commit ee85098
Show file tree
Hide file tree
Showing 11 changed files with 65 additions and 133 deletions.
175 changes: 65 additions & 110 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ import {
enableSchedulingProfiler,
enableSuspenseCallback,
enableScopeAPI,
deletedTreeCleanUpLevel,
enableUpdaterTracking,
enableCache,
enableTransitionTracing,
Expand Down Expand Up @@ -1658,74 +1657,43 @@ function detachFiberAfterEffects(fiber: Fiber) {
detachFiberAfterEffects(alternate);
}

// Note: Defensively using negation instead of < in case
// `deletedTreeCleanUpLevel` is undefined.
if (!(deletedTreeCleanUpLevel >= 2)) {
// This is the default branch (level 0).
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;
// 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;

// The `stateNode` is cyclical because on host nodes it points to the host
// tree, which has its own pointers to children, parents, and siblings.
// The other host nodes also point back to fibers, so we should detach that
// one, too.
if (fiber.tag === HostComponent) {
const hostInstance: Instance = fiber.stateNode;
if (hostInstance !== null) {
detachDeletedInstance(hostInstance);
}
} else {
// 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;

// The `stateNode` is cyclical because on host nodes it points to the host
// tree, which has its own pointers to children, parents, and siblings.
// The other host nodes also point back to fibers, so we should detach that
// one, too.
if (fiber.tag === HostComponent) {
const hostInstance: Instance = fiber.stateNode;
if (hostInstance !== null) {
detachDeletedInstance(hostInstance);
}
}
fiber.stateNode = 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.
}
fiber.stateNode = null;

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.
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;
}
if (__DEV__) {
fiber._debugOwner = null;
}

// 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.
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;
}

function emptyPortalContainer(current: Fiber) {
Expand Down Expand Up @@ -3986,31 +3954,29 @@ export function commitPassiveUnmountEffects(finishedWork: Fiber): void {
}

function detachAlternateSiblings(parentFiber: Fiber) {
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 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 = parentFiber.alternate;
if (previousFiber !== null) {
let detachedChild = previousFiber.child;
if (detachedChild !== null) {
previousFiber.child = null;
do {
// $FlowFixMe[incompatible-use] found when upgrading Flow
const detachedSibling = detachedChild.sibling;
// $FlowFixMe[incompatible-use] found when upgrading Flow
detachedChild.sibling = null;
detachedChild = detachedSibling;
} while (detachedChild !== null);
}
// 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 = parentFiber.alternate;
if (previousFiber !== null) {
let detachedChild = previousFiber.child;
if (detachedChild !== null) {
previousFiber.child = null;
do {
// $FlowFixMe[incompatible-use] found when upgrading Flow
const detachedSibling = detachedChild.sibling;
// $FlowFixMe[incompatible-use] found when upgrading Flow
detachedChild.sibling = null;
detachedChild = detachedSibling;
} while (detachedChild !== null);
}
}
}
Expand Down Expand Up @@ -4196,8 +4162,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin(
resetCurrentDebugFiberInDEV();

const child = fiber.child;
// TODO: Only traverse subtree if it has a PassiveStatic flag. (But, if we
// do this, still need to handle `deletedTreeCleanUpLevel` correctly.)
// TODO: Only traverse subtree if it has a PassiveStatic flag.
if (child !== null) {
child.return = fiber;
nextEffect = child;
Expand All @@ -4217,23 +4182,13 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete(
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;
}
// 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;
}

if (sibling !== null) {
Expand Down
13 changes: 0 additions & 13 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,19 +101,6 @@ export const enableHostSingletons = true;

export const enableFloat = true;

// 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 = 3;

export const enableUseHook = true;

// Enables unstable_useMemoCache hook, intended as a compilation target for
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableNativeComponentFrames = false;
export const skipUnmountedBoundaries = false;
export const deletedTreeCleanUpLevel = 3;
export const enableGetInspectorDataForInstanceInProduction = true;
export const deferRenderPhaseUpdateToNextBatch = false;

Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableNativeComponentFrames = false;
export const skipUnmountedBoundaries = false;
export const deletedTreeCleanUpLevel = 3;
export const enableGetInspectorDataForInstanceInProduction = false;
export const deferRenderPhaseUpdateToNextBatch = false;

Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableNativeComponentFrames = false;
export const skipUnmountedBoundaries = false;
export const deletedTreeCleanUpLevel = 3;
export const enableGetInspectorDataForInstanceInProduction = false;
export const deferRenderPhaseUpdateToNextBatch = false;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableNativeComponentFrames = false;
export const skipUnmountedBoundaries = false;
export const deletedTreeCleanUpLevel = 3;
export const enableGetInspectorDataForInstanceInProduction = false;
export const deferRenderPhaseUpdateToNextBatch = false;
export const enableSuspenseAvoidThisFallback = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableNativeComponentFrames = false;
export const skipUnmountedBoundaries = false;
export const deletedTreeCleanUpLevel = 3;
export const enableGetInspectorDataForInstanceInProduction = false;
export const deferRenderPhaseUpdateToNextBatch = false;

Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.testing.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableNativeComponentFrames = false;
export const skipUnmountedBoundaries = false;
export const deletedTreeCleanUpLevel = 3;
export const enableGetInspectorDataForInstanceInProduction = false;
export const deferRenderPhaseUpdateToNextBatch = false;

Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.testing.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ export const enableLegacyFBSupport = !__EXPERIMENTAL__;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableNativeComponentFrames = false;
export const skipUnmountedBoundaries = true;
export const deletedTreeCleanUpLevel = 3;
export const enableGetInspectorDataForInstanceInProduction = false;
export const deferRenderPhaseUpdateToNextBatch = false;

Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ export const enableFilterEmptyStringAttributesDOM = __VARIANT__;
export const enableLegacyFBSupport = __VARIANT__;
export const skipUnmountedBoundaries = __VARIANT__;
export const enableUseRefAccessWarning = __VARIANT__;
export const deletedTreeCleanUpLevel: number = __VARIANT__ ? 3 : 1;
export const enableProfilerNestedUpdateScheduledHook = __VARIANT__;
export const disableSchedulerTimeoutInWorkLoop = __VARIANT__;
export const enableLazyContextPropagation = __VARIANT__;
Expand Down
2 changes: 0 additions & 2 deletions packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,6 @@ export const disableTextareaChildren = __EXPERIMENTAL__;

export const allowConcurrentByDefault = true;

export const deletedTreeCleanUpLevel = 3;

export const consoleManagedByDevToolsDuringStrictMode = true;
export const enableServerContext = true;

Expand Down

0 comments on commit ee85098

Please sign in to comment.