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

Detach sibling pointers in old child list #21041

Closed
wants to merge 1 commit into from

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Mar 20, 2021

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.

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.
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 20, 2021
@sizebot
Copy link

sizebot commented Mar 20, 2021

Comparing: 6d3ecb7...8066dbe

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 = 122.77 kB 122.77 kB = 39.49 kB 39.49 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 129.22 kB 129.22 kB = 41.52 kB 41.52 kB
facebook-www/ReactDOM-prod.classic.js +0.17% 407.02 kB 407.70 kB +0.16% 75.43 kB 75.54 kB
facebook-www/ReactDOM-prod.modern.js +0.17% 395.28 kB 395.96 kB +0.17% 73.49 kB 73.62 kB
facebook-www/ReactDOMForked-prod.classic.js +0.17% 407.03 kB 407.71 kB +0.16% 75.43 kB 75.55 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.77% 9.75 kB 9.82 kB +0.72% 2.65 kB 2.67 kB
facebook-www/ReactIs-dev.modern.js +0.75% 10.01 kB 10.09 kB +0.63% 2.70 kB 2.72 kB
facebook-www/SchedulerTracing-dev.modern.js +0.68% 11.04 kB 11.11 kB +0.86% 2.43 kB 2.45 kB
facebook-www/SchedulerTracing-dev.classic.js +0.68% 11.04 kB 11.12 kB +0.86% 2.43 kB 2.45 kB
facebook-www/ReactART-prod.modern.js +0.27% 255.34 kB 256.02 kB +0.25% 45.37 kB 45.48 kB
facebook-www/ReactART-prod.classic.js +0.26% 262.70 kB 263.38 kB +0.26% 46.60 kB 46.71 kB

Generated by 🚫 dangerJS against 8066dbe

@acdlite acdlite requested review from sebmarkbage and bvaughn March 20, 2021 02:51
@acdlite acdlite force-pushed the detach-old-child-list branch 3 times, most recently from 3aa7ea5 to 8066dbe Compare March 20, 2021 03:09
@bgirard
Copy link
Contributor

bgirard commented Mar 20, 2021

Here's one the leak report were look at that seem to point at this problem:

  --child (property)--->  [FiberNode react.offscreen] (object) @525503 [484 bytes]
  --alternate (property)--->  [FiberNode react.offscreen] (object) @525519 [180 bytes]
  --sibling (property)--->  [Detached FiberNode] (object) @978585 [140 bytes]

We have an attached FiberNode, with it's attached alternate ref, but the sibling ref points to a FiberNode that's been unmounted. We will investigate if this code cleans up this case.

@bgirard
Copy link
Contributor

bgirard commented Mar 21, 2021

I'm able to confirm that the above mentioned leak is fixed by this PR. The drop in memory doesn't appear to be large however, but that would need further testing. This PR is promising, we should try it!

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 23, 2021

Closed by #21039

@acdlite acdlite closed this Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants