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

[16.8.6] Aggressive cleanup to assist garbage collection #1

Open
wants to merge 4 commits into
base: 16.8.6
Choose a base branch
from

Conversation

paulshen
Copy link

@paulshen paulshen commented Jun 19, 2019

At Discord, we were experiencing serious memory leaks when upgrading to React with Fiber internals. We spent a lot of time investigating but ultimately ended on this fork to unblock us. These changes are made on top of the tagged 16.8.6 release.

If you want to try this build, you can use use "react-dom": "discordapp/react#cfda84f6b3c49a1398709cf43b3b959366f7e01a" to your package.json.

I still don't know whether the root cause of the memory leak is in userland or framework design. I do know the node-to-node pointers and fiber <-> host pointers make big memory leaks possible. See associated discussion in facebook#15157.

Changes

b0194a0
Clear the Fiber node's nextEffect pointers when finishing effect processing. This helps alleviate the long fiber chains that we saw in the profiler.

d28ee48
This clears the stateNode (pointer to DOM node and class instances) described above when unmounting. It appears it's easy to keep references to detached DOM nodes from leaking fiber nodes.

2e5ee2f and c31cc03
At this point, we're desperate. On unmount, I clear the host node's pointer to the Fiber node. This is most definitely a hack but with this change, we are no longer seeing memory leaks.

@johanneslumpe
Copy link

@paulshen We're seeing basically the same issues you guys at discord are observing! Will give this patch a shot tomorrow. Just here to thank you for digging into this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants