Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cleanup] remove deletedTreeCleanUpLevel feature flag #25529

Merged
merged 2 commits into from
Jan 17, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.)
kassens marked this conversation as resolved.
Show resolved Hide resolved
// 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 @@ -61,7 +61,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 @@ -51,7 +51,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 @@ -51,7 +51,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 @@ -43,7 +43,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 @@ -51,7 +51,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 @@ -51,7 +51,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 @@ -51,7 +51,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 @@ -97,8 +97,6 @@ export const disableTextareaChildren = __EXPERIMENTAL__;

export const allowConcurrentByDefault = true;

export const deletedTreeCleanUpLevel = 3;

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

Expand Down