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

[Experiment] Add feature flag for more aggressive memory clean-up of deleted fiber trees #21039

Merged
merged 5 commits into from
Mar 23, 2021

Conversation

bgirard
Copy link
Contributor

@bgirard bgirard commented Mar 20, 2021

Summary

Add a new feature flag called 'enableStrongMemoryCleanup' that will recursively clean-up the FiberNode memory graph on unmount. This is to experiment on improving memory usage and/or reducing leaks after unmount.

Test Plan

Will run several memory usage scenario against this sync.

@sizebot
Copy link

sizebot commented Mar 20, 2021

Comparing: 00d4f95...31e1977

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.10% 122.83 kB 122.96 kB +0.11% 39.52 kB 39.57 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.10% 129.33 kB 129.45 kB +0.06% 41.59 kB 41.61 kB
facebook-www/ReactDOM-prod.classic.js +0.47% 408.70 kB 410.61 kB +0.35% 75.80 kB 76.07 kB
facebook-www/ReactDOM-prod.modern.js +0.48% 396.96 kB 398.87 kB +0.36% 73.86 kB 74.13 kB
facebook-www/ReactDOMForked-prod.classic.js +0.47% 408.71 kB 410.62 kB +0.35% 75.80 kB 76.07 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactIs-dev.classic.js +0.74% 9.81 kB 9.89 kB +0.82% 2.67 kB 2.69 kB
facebook-www/ReactIs-dev.modern.js +0.72% 10.08 kB 10.15 kB +0.81% 2.72 kB 2.74 kB
facebook-www/SchedulerTracing-dev.modern.js +0.66% 11.11 kB 11.18 kB +0.98% 2.44 kB 2.47 kB
facebook-www/SchedulerTracing-dev.classic.js +0.66% 11.11 kB 11.18 kB +0.98% 2.44 kB 2.47 kB
facebook-www/ReactART-prod.modern.js +0.59% 257.12 kB 258.64 kB +0.50% 45.74 kB 45.98 kB
facebook-www/ReactART-prod.classic.js +0.58% 264.48 kB 266.01 kB +0.49% 46.98 kB 47.21 kB
facebook-www/ReactART-dev.modern.js +0.53% 695.64 kB 699.36 kB +0.73% 148.11 kB 149.19 kB
facebook-www/ReactART-dev.classic.js +0.53% 705.93 kB 709.65 kB +0.72% 150.22 kB 151.30 kB
facebook-www/ReactDOM-prod.modern.js +0.48% 396.96 kB 398.87 kB +0.36% 73.86 kB 74.13 kB
facebook-www/ReactDOMForked-prod.modern.js +0.48% 396.97 kB 398.88 kB +0.37% 73.87 kB 74.14 kB
facebook-www/ReactDOM-prod.classic.js +0.47% 408.70 kB 410.61 kB +0.35% 75.80 kB 76.07 kB
facebook-www/ReactDOMForked-prod.classic.js +0.47% 408.71 kB 410.62 kB +0.35% 75.80 kB 76.07 kB
facebook-www/ReactDOM-profiling.modern.js +0.46% 418.02 kB 419.93 kB +0.38% 77.48 kB 77.77 kB
facebook-www/ReactDOMForked-profiling.modern.js +0.46% 418.03 kB 419.94 kB +0.38% 77.49 kB 77.78 kB
facebook-www/ReactDOM-profiling.classic.js +0.44% 429.80 kB 431.71 kB +0.36% 79.34 kB 79.62 kB
facebook-www/ReactDOMForked-profiling.classic.js +0.44% 429.81 kB 431.72 kB +0.36% 79.33 kB 79.62 kB
facebook-www/ReactDOMForked-dev.modern.js +0.40% 1,034.99 kB 1,039.16 kB +0.52% 230.24 kB 231.43 kB
facebook-www/ReactDOM-dev.modern.js +0.40% 1,035.59 kB 1,039.76 kB +0.52% 230.26 kB 231.45 kB
facebook-www/ReactDOMForked-dev.classic.js +0.39% 1,061.23 kB 1,065.40 kB +0.50% 235.46 kB 236.64 kB
facebook-www/ReactDOM-dev.classic.js +0.39% 1,061.84 kB 1,066.01 kB +0.50% 235.49 kB 236.66 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer.production.min.js +0.27% 13.17 kB 13.20 kB +0.24% 4.12 kB 4.13 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer.production.min.js +0.27% 13.17 kB 13.20 kB +0.24% 4.12 kB 4.13 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-persistent.production.min.js +0.27% 13.18 kB 13.22 kB +0.27% 4.13 kB 4.14 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-persistent.production.min.js +0.27% 13.18 kB 13.22 kB +0.27% 4.13 kB 4.14 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js +0.22% 241.18 kB 241.72 kB +0.19% 44.20 kB 44.29 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-profiling.js +0.21% 253.98 kB 254.52 kB +0.16% 46.52 kB 46.59 kB
oss-stable/react-art/cjs/react-art.development.js +0.21% 597.81 kB 599.07 kB +0.28% 130.98 kB 131.34 kB
oss-stable/react-test-renderer/umd/react-test-renderer.development.js +0.21% 637.89 kB 639.21 kB +0.28% 134.92 kB 135.29 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.development.js +0.21% 607.68 kB 608.94 kB +0.28% 133.33 kB 133.69 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-dev.js +0.20% 618.98 kB 620.24 kB +0.28% 134.38 kB 134.75 kB
oss-stable/react-reconciler/cjs/react-reconciler.development.js +0.20% 654.03 kB 655.35 kB +0.28% 140.36 kB 140.75 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.development.js +0.20% 654.74 kB 656.06 kB +0.27% 138.33 kB 138.71 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.development.js +0.20% 623.68 kB 624.94 kB +0.27% 136.71 kB 137.07 kB

Generated by 🚫 dangerJS against 31e1977

Comment on lines 1229 to 1233
if (fiber.stateNode) {
Object.keys(fiber.stateNode).forEach(key => {
if (key.indexOf('__react') === 0) {
delete fiber.stateNode[key];
}
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels pretty heavy. Why is it needed given that we null out fiber.stateNode below (and we null out all expensive fiber fields)?

Even if something has a reference to the fiber at this point, it wouldn't have any deep references.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. We’re starting with the shotgun approach to confirm if this kills the leak. If not it doesn’t matter, we have to keep looking and narrow down what we miss.

When the leak is fixed then we can come back and find the right way to do this clean up.

I think you are correct. Worse case keeping a ref to an unmourned / detached DOM mode will be O(1) by retaining 1 and only 1 associated fiber node. Well sort this out once we confirm the leak is gone.

if (fiber.stateNode) {
Object.keys(fiber.stateNode).forEach(key => {
if (key.indexOf('__react') === 0) {
delete fiber.stateNode[key];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already know the exact names, don't we? Can we use them directly?

const internalInstanceKey = '__reactFiber$' + randomKey;
const internalPropsKey = '__reactProps$' + randomKey;
const internalContainerInstanceKey = '__reactContainer$' + randomKey;
const internalEventHandlersKey = '__reactEvents$' + randomKey;
const internalEventHandlerListenersKey = '__reactListeners$' + randomKey;
const internalEventHandlesSetKey = '__reactHandles$' + randomKey;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added unmountNode to the HostConfig. Could use feedback if there's a better way.

Like I discussed with Brian this probably wont be necessary in the final version.

@bgirard bgirard force-pushed the master branch 2 times, most recently from 39b04e8 to 6cf1cf4 Compare March 20, 2021 21:54
@bgirard
Copy link
Contributor Author

bgirard commented Mar 21, 2021

This change and #21041 are related. I'm going to merge them into a React sync and test them with memlab to study the changes to leaks. We can then make final changes before landing these two and doing a production test.

@bgirard
Copy link
Contributor Author

bgirard commented Mar 21, 2021

Here's a leak that memlab reports as fixed with this turned on (testing with #21041 on):

<SNIP UNRELATED INTERNAL CHAIN>
  --bindings (internal)--->  [(bound arguments)] (array) @646517 [16 bytes]
  --0 (internal)--->  [Detached FiberNode CometTransientDialogProvider] (object) @583545 [2.3KB]
  --child (property)--->  [Detached FiberNode react.provider] (object) @583439 [188 bytes]
  --child (property)--->  [Detached FiberNode] (object) @583059 [188 bytes]
  --child (property)--->  [Detached FiberNode react.provider] (object) @583043 [188 bytes]
  --child (property)--->  [Detached FiberNode CometLayout] (object) @582961 [188 bytes]
  --child (property)--->  [Detached FiberNode div] (object) @582841 [188 bytes]
  --child (property)--->  [Detached FiberNode div] (object) @582691 [188 bytes]
  --stateNode (property)--->  [Detached HTMLDivElement] (native) @109319 [48 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @109321 [228 bytes]
  --7 (element)--->  [Detached HTMLDivElement] (native) @109323 [5.7KB]
  --7 (element)--->  [Detached HTMLDivElement] (native) @108191 [580 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @108189 [332 bytes]
  --7 (element)--->  [Detached HTMLDivElement] (native) @108187 [7.8KB]
  --7 (element)--->  [Detached HTMLDivElement] (native) @108185 [160 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1347749 [48 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1347751 [48 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1347753 [48 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1347791 [48 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1347793 [48 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1347795 [48 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1347801 [236 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1348887 [140 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1347567 [48 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1347345 [48 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1347347 [48 bytes]
  --7 (element)--->  [Detached HTMLDivElement] (native) @1347349 [48 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1347351 [48 bytes]
  --7 (element)--->  [Detached HTMLDivElement] (native) @1347431 [48 bytes]
  --9 (element)--->  [Detached HTMLDivElement] (native) @1348185 [48 bytes]
  --6 (element)--->  [Detached HTMLDivElement] (native) @1348187 [48 bytes]
  --7 (element)--->  [Detached HTMLDivElement] (native) @1348189 [48 bytes]
  --__reactFiber$cq9ed31lytw (property)--->  [Detached FiberNode div] (object) @1720097 [1.4KB]
  --child (property)--->  [Detached FiberNode CometFeedMatchRenderer] (object) @1718217 [1.2KB]

We would need to dig deeper to confirm if this is just making a leak pointing to within a tree that detach. This is part hard to reason about from heap snapshots and memory tooling.

This appears to reduce memory leak in the create-life-event test case by double digit megabytes of memory usage.

@JacksonGL
Copy link
Member

JacksonGL commented Mar 22, 2021

MemLab observes a reduction of detached Fiber nodes on Facebook.
The cleanup may be very useful for further releasing memory retained by video caching.

Some examples:

Profile Page

Before Fix:

· fiber node size: 4.7MB
· regular fiber node size: 1.4MB
· detached fiber node size: *2MB*
· alternate fiber node size: 1.2MB

After Fix:

· fiber node size: 2.8MB
· regular fiber node size: 1.5MB
· detached fiber node size: *144.7KB*
· alternate fiber node size: 1.2MB

Home Tab

Before Fix:

· fiber node size: 3.8MB
· regular fiber node size: 2.2MB
· detached fiber node size: *3.6KB*
· alternate fiber node size: 1.6MB

After Fix:

· fiber node size: 3.8MB
· regular fiber node size: 2.2MB
· detached fiber node size: *2.7KB*
· alternate fiber node size: 1.6MB

Marketplace Tab

Before Fix:

· fiber node size: 3.1MB
· regular fiber node size: 2.3MB
· detached fiber node size: *3.2KB*
· alternate fiber node size: 972.5KB

After Fix:

· fiber node size: 3.1MB
· regular fiber node size: 2.3MB
· detached fiber node size: *1.9KB*
· alternate fiber node size: 966.7KB

Edit: More results before and after the change
(x-axis is navigation; y-axis is JS heap size)

before:
Screen Shot 2021-04-15 at 13 13 24

After:
Screen Shot 2021-04-15 at 13 13 35

Add a feature flag that will test doing a recursive clean of an unmount
node. This will disconnect the fiber graph making leaks less severe.
When a fiber is deleted, 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.

Will use this feature flag to test the memory impact.
@acdlite acdlite force-pushed the master branch 3 times, most recently from f081a4b to a16ac10 Compare March 23, 2021 01:31
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.
@acdlite acdlite changed the title [Draft] Add feature flag: enableStrongMemoryCleanup [Experiment] Add feature flag for more aggressive memory clean-up of deleted fiber trees Mar 23, 2021
We already have a recursive loop that visits every deleted fiber. We
can re-use that one for clean up instead of adding another one.
@acdlite acdlite merged commit 25bfa28 into facebook:master Mar 23, 2021
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 6, 2021
Summary:
This sync includes the following changes:
- **[c9aab1c9d](facebook/react@c9aab1c9d )**: [email protected] //<Dan Abramov>//
- **[516b76b9a](facebook/react@516b76b9a )**: [Fast Refresh] Support callthrough HOCs ([#21104](facebook/react#21104)) //<Dan Abramov>//
- **[0853aab74](facebook/react@0853aab74 )**: Log all errors to console.error by default ([#21130](facebook/react#21130)) //<Sebastian Markbåge>//
- **[d1294c9d4](facebook/react@d1294c9d4 )**: Add global onError handler ([#21129](facebook/react#21129)) //<Sebastian Markbåge>//
- **[64983aab5](facebook/react@64983aab5 )**: Remove redundant setUpdatePriority call ([#21127](facebook/react#21127)) //<Andrew Clark>//
- **[634cc52e6](facebook/react@634cc52e6 )**: Delete dead variable: currentEventWipLanes ([#21123](facebook/react#21123)) //<Andrew Clark>//
- **[1102224bb](facebook/react@1102224bb )**: Fix: flushSync changes priority inside effect ([#21122](facebook/react#21122)) //<Andrew Clark>//
- **[dbe98a5aa](facebook/react@dbe98a5aa )**: Move sync task queue to its own module ([#21109](facebook/react#21109)) //<Andrew Clark>//
- **[3ba5c8737](facebook/react@3ba5c8737 )**: Remove Scheduler indirection ([#21107](facebook/react#21107)) //<Andrew Clark>//
- **[46b68eaf6](facebook/react@46b68eaf6 )**: Delete LanePriority type ([#21090](facebook/react#21090)) //<Andrew Clark>//
- **[dcd13045e](facebook/react@dcd13045e )**: Use Lane to track root callback priority ([#21089](facebook/react#21089)) //<Andrew Clark>//
- **[5f21a9fca](facebook/react@5f21a9fca )**: Clean up host pointers in level 2 of clean-up flag ([#21112](facebook/react#21112)) //<Andrew Clark>//
- **[32d6f39ed](facebook/react@32d6f39ed )**: [Fizz] Support special HTML/SVG/MathML tags to suspend ([#21113](facebook/react#21113)) //<Sebastian Markbåge>//
- **[a77dd13ed](facebook/react@a77dd13ed )**: Delete enableDiscreteEventFlushingChange ([#21110](facebook/react#21110)) //<Andrew Clark>//
- **[048ee4c0c](facebook/react@048ee4c0c )**: Use `act` in fuzz tester to flush expired work ([#21108](facebook/react#21108)) //<Andrew Clark>//
- **[556644e23](facebook/react@556644e23 )**: Fix plurals ([#21106](facebook/react#21106)) //<Sebastian Markbåge>//
- **[8b741437b](facebook/react@8b741437b )**: Rename SuspendedWork to Task ([#21105](facebook/react#21105)) //<Sebastian Markbåge>//
- **[38a1aedb4](facebook/react@38a1aedb4 )**: [Fizz] Add FormatContext and Refactor Work ([#21103](facebook/react#21103)) //<Sebastian Markbåge>//
- **[1b7e471b9](facebook/react@1b7e471b9 )**: React Native New Architecture: Support passing nativeViewTag to getInspectorDataForViewAtPoint callback, for React DevTools compat ([#21080](facebook/react#21080)) //<Joshua Gross>//
- **[4a99c5c3a](facebook/react@4a99c5c3a )**: Use highest priority lane to detect interruptions ([#21088](facebook/react#21088)) //<Andrew Clark>//
- **[77be52729](facebook/react@77be52729 )**: Remove LanePriority from computeExpirationTime ([#21087](facebook/react#21087)) //<Andrew Clark>//
- **[3221e8fba](facebook/react@3221e8fba )**: Remove LanePriority from getBumpedLaneForHydration ([#21086](facebook/react#21086)) //<Andrew Clark>//
- **[05ec0d764](facebook/react@05ec0d764 )**: Entangled expired lanes with SyncLane ([#21083](facebook/react#21083)) //<Andrew Clark>//
- **[03ede83d2](facebook/react@03ede83d2 )**: Use EventPriority to track update priority ([#21082](facebook/react#21082)) //<Andrew Clark>//
- **[a63f0953b](facebook/react@a63f0953b )**: Delete SyncBatchedLane ([#21061](facebook/react#21061)) //<Ricky>//
- **[fa868d6be](facebook/react@fa868d6be )**: Make opaque EventPriority type a Lane internally ([#21065](facebook/react#21065)) //<Andrew Clark>//
- **[eb58c3909](facebook/react@eb58c3909 )**: react-hooks/exhaustive-deps: Handle optional chained methods as dependency ([#20204](facebook/react#20204)) ([#20247](facebook/react#20247)) //<Ari Perkkiö>//
- **[7b84dbd16](facebook/react@7b84dbd16 )**: Fail build on deep requires in npm packages ([#21063](facebook/react#21063)) //<Dan Abramov>//
- **[2c9d8efc8](facebook/react@2c9d8efc8 )**: Add react-reconciler/constants entry point ([#21062](facebook/react#21062)) //<Dan Abramov>//
- **[d0eaf7829](facebook/react@d0eaf7829 )**: Move priorities to separate import to break cycle ([#21060](facebook/react#21060)) //<Andrew Clark>//
- **[435cff986](facebook/react@435cff986 )**: [Fizz] Expose callbacks in options for when various stages of the content is done ([#21056](facebook/react#21056)) //<Sebastian Markbåge>//
- **[25bfa287f](facebook/react@25bfa287f )**: [Experiment] Add feature flag for more aggressive memory clean-up of deleted fiber trees ([#21039](facebook/react#21039)) //<Benoit Girard>//
- **[8fe7810e7](facebook/react@8fe7810e7 )**: Remove already completed comment ([#21054](facebook/react#21054)) //<Sebastian Markbåge>//
- **[6c3202b1e](facebook/react@6c3202b1e )**: [Fizz] Use identifierPrefix to avoid conflicts within the same response ([#21037](facebook/react#21037)) //<Sebastian Markbåge>//
- **[dcdf8de7e](facebook/react@dcdf8de7e )**: Remove discrete lanes and priorities ([#21040](facebook/react#21040)) //<Andrew Clark>//
- **[ca99ae97b](facebook/react@ca99ae97b )**: Replace some flushExpired callsites ([#20975](facebook/react#20975)) //<Ricky>//
- **[1fafac002](facebook/react@1fafac002 )**: Use SyncLane for discrete event hydration ([#21038](facebook/react#21038)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions 6d3ecb7...c9aab1c

jest_e2e[run_all_tests]

Reviewed By: JoshuaGross

Differential Revision: D27436763

fbshipit-source-id: da79a41e26bffdcdacd293178062edf098e9b58a
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
…deleted fiber trees (facebook#21039)

* Add feature flag: enableStrongMemoryCleanup

Add a feature flag that will test doing a recursive clean of an unmount
node. This will disconnect the fiber graph making leaks less severe.

* Detach sibling pointers in old child list

When a fiber is deleted, 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.

Will use this feature flag to test the memory impact.

* 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.

* Add Flow type to new host config method

* Re-use existing recursive clean up path

We already have a recursive loop that visits every deleted fiber. We
can re-use that one for clean up instead of adding another one.

Co-authored-by: Andrew Clark <[email protected]>
threepointone pushed a commit to threepointone/react that referenced this pull request Oct 24, 2021
…deleted fiber trees (facebook#21039)

* Add feature flag: enableStrongMemoryCleanup

Add a feature flag that will test doing a recursive clean of an unmount
node. This will disconnect the fiber graph making leaks less severe.

* Detach sibling pointers in old child list

When a fiber is deleted, 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.

Will use this feature flag to test the memory impact.

* 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.

* Add Flow type to new host config method

* Re-use existing recursive clean up path

We already have a recursive loop that visits every deleted fiber. We
can re-use that one for clean up instead of adding another one.

Co-authored-by: Andrew Clark <[email protected]>
threepointone pushed a commit to threepointone/react that referenced this pull request Oct 24, 2021
…deleted fiber trees (facebook#21039)

* Add feature flag: enableStrongMemoryCleanup

Add a feature flag that will test doing a recursive clean of an unmount
node. This will disconnect the fiber graph making leaks less severe.

* Detach sibling pointers in old child list

When a fiber is deleted, 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.

Will use this feature flag to test the memory impact.

* 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.

* Add Flow type to new host config method

* Re-use existing recursive clean up path

We already have a recursive loop that visits every deleted fiber. We
can re-use that one for clean up instead of adding another one.

Co-authored-by: Andrew Clark <[email protected]>
@gaearon gaearon mentioned this pull request Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants