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

Old references retained by memoizedProps in fiber #14380

Closed
emilschutte opened this issue Dec 3, 2018 · 11 comments
Closed

Old references retained by memoizedProps in fiber #14380

emilschutte opened this issue Dec 3, 2018 · 11 comments
Labels
Resolution: Stale Automatically closed due to inactivity Type: Needs Investigation

Comments

@emilschutte
Copy link

Do you want to request a feature or report a bug?

Bug (or misunderstanding on my part)

What is the current behavior?

Fibers seem to retain stale references to old data via memoizedProps / memoizedState

Demo:

https://jsfiddle.net/0tfq4p5r/

This demo just puts some big strings in the state (and into the props of a child component). If you click the button it will replace the array of strings with an empty array. At this point if you inspect the memory usage you will see that both memoizedState and memoizedProps are still holding references to the original array of strings and that memory can't be collected.

What is the expected behavior?

I was expecting no references to the old data to be retained, so that the memory could be reclaimed immediately.

I ran into this when testing an app with a large dataset that is near my browser's heap limit. If I want to replace that with a different, equally large dataset, and I attempt to make the first large dataset GC-able by removing it from my Redux store before putting the second dataset in the store, I wind up with the old dataset still resident in memory due to being retained by memoizedProps of a fiber.

I noticed this PR that sounds possibly related:

#14276

and tested against current master (f1bf281) but still had the issue with memoizedProps.

Right now I'm working around this by clearing the props twice consecutively before loading the new data. Is that the intended solution for this situation?

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

macOS High Sierra 10.13.6 (17G65)
Chrome Version 70.0.3538.110 (Official Build) (64-bit)
React 16.6.3
Don't know about previous versions of React.

@gaearon
Copy link
Collaborator

gaearon commented Dec 3, 2018

cc @trueadm

@trueadm
Copy link
Contributor

trueadm commented Dec 3, 2018

This seems like the exact same problem as #14276. However, we didn't resolve that issue by removing the memory leak in all cases. Instead we ensured the memory was fixed when the component gets unmounted.

If I'm not mistaken, the component in the example example is not getting unmounted here, it's just being updated with an empty array. A single update will mean React switches to the alternate fiber, which will not clean the old state. It should only clean the state of the original fiber on a subsequent update (so two updates). @emilschutte it sounds like you've already found the fix for this. I believe this is an intention design decision in React Fiber after discussions with @sebmarkbage.

@emilschutte
Copy link
Author

OK, thanks @trueadm. If I'm understanding right, the need for the alternate fiber to be one step behind is just a "cost of doing business" the Fiber way. I understand that and I think there are ways to work around the memory implications when necessary.

I'm trying to think about when this might be an issue, so I can understand whether my use case is just pathological. It seems like this is only a problem when you swap a large dataset in one shot without unmounting and without doing any subsequent state updates, in which case your app would be using a lot more memory than you might expect. That seems uncommon to me, so we're probably back to my scenario, with a dataset so large you really need it cleared before you can even load the new dataset. Also probably uncommon.

@richie-south
Copy link

I would say it's also an issue when doing memory leak detection in the (chrome) profiler. When comparing two snapshots or viewing memory graphs we will always see detached dom nodes which makes it hard to know if we are leaking memory somewhere or if it's just React that keeps it in memory.

It's also common that users don't know about or expects this behaviour to happen, and believes it to be a memory leak.
#13702 #14732 #12420 #14630.

@alkemir
Copy link

alkemir commented May 17, 2019

First timer here:
I am actually tracking down an issue where apparently many fibers reside in memory,
apparently memory is returned only on DOM GC events, not Major GC.

But I am unsure, because I also have event listeners, which also seem a common source
of issues (plus webpack, so memory profiles is getting hard to understand).

Is there any way that the GC is getting lazy about fibers? I have seen my app take up to a gigabyte after hours of running, and then go down by itself (it usually takes just 100 Mb, and datasets are of the size of kb)

@alkemir
Copy link

alkemir commented May 17, 2019

Actually, it seems like our issue is some kind of starvation. We update a component every second, and this causes a leak. But if we update every 10 seconds, exactly the same code will not increase the amount of memory consumed. If we stop the updates, the GC is able to reclaim the memory. Should I open a new issue about this?

@ajsuvarna6
Copy link

ajsuvarna6 commented Aug 8, 2019

We are also facing this issue of unmounted components still being retained in the React Fiber. Sometimes the memory reference grows on a large scale, even after that the unmounted components are not Garbage Collected.

Screenshot from 2019-08-08 15-14-42

@alkemir
Copy link

alkemir commented Aug 8, 2019

We changed the component from being a PureComponent to a regular Component and that solved it for us (although, to our understanding, the component was in fact Pure). You might try that.

@stale
Copy link

stale bot commented Jan 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@stale
Copy link

stale bot commented Jan 17, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@stale stale bot closed this as completed Jan 17, 2020
@FezVrasta
Copy link

👋 could someone help me understand if the memoizedProps object is a clone/deep-clone of the original properties or if the individual memoized properties are just pointers to the original ones?

We are seeing double the memory allocation and it makes us think the properties are being somehow cloned rather than simply referenced, any reason why this is happening?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Stale Automatically closed due to inactivity Type: Needs Investigation
Projects
None yet
Development

No branches or pull requests

7 participants