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

[WIP] Fix for fiber root scheduling memory leak #11644

Merged
merged 2 commits into from
Nov 28, 2017

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Nov 23, 2017

Not to be merged

After some investigation work for a memory leak on React Native, we found that nextRoot was never set to null and left in that state upon the view fulling being unmounted. I believe that is has to do with the fact that we set nextRoot and nextUnitOfWork to null, then immediately call requestWork() which sets them back again.

This results in a memory leak as the node tree of nextRoot never gets garbage collected.

This is more of a temporary research fix, rather than an explicit fix. I'm sure @acdlite will have a concrete fix based from this.

!isWorking &&
root === nextRoot &&
expirationTime < nextRenderExpirationTime
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea what my brain was thinking then. Sorry, pushed the code.

@trueadm trueadm changed the title Fix for fiber root scheduling memory leak [WIP] Fix for fiber root scheduling memory leak Nov 23, 2017
@gaearon
Copy link
Collaborator

gaearon commented Nov 23, 2017

I don't have enough context on which variables should be reset when but my hunch is that they don't all need to be reset twice. Let's leave this to @acdlite.

@acdlite
Copy link
Collaborator

acdlite commented Nov 27, 2017

Thanks for tracking this down. I think I have a better idea for how to fix it. What's the best way to reproduce the error, and confirm that it's fixed?

@trueadm
Copy link
Contributor Author

trueadm commented Nov 27, 2017

@acdlite I used create-react-app and then did ReactDOM.render(null, ...) in a setTimeout(...) after the initial render – unmounting the entire UI. I then used the Chrome dev tools to see the memory trace hadn't really decreased that much as nextRoot still held a reference to the initial fiber tree.

@clemmy clemmy merged commit 363f4f1 into facebook:master Nov 28, 2017
@clemmy
Copy link
Contributor

clemmy commented Nov 28, 2017

We'll come back to this with a more concrete implementation from @acdlite later. Merging so that 16.2 doesn't ship with memory leak.

@gaearon gaearon deleted the memory-leak branch December 4, 2017 16:42
@Rebsos
Copy link

Rebsos commented Aug 24, 2018

Thx for the fix. My React Native App needs now less memory. Just wanted to let some real world data here:

React Native React Total (average)
0.49.5 16.0.0-beta.5 164 MB
0.51.1 16.0.0 199 MB
0.52.3 16.2.0 173 MB

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.

6 participants