Fix memory leak issue with UseEffect
#1506
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There's a memory leak in `react-redux`'s usage of UseEffect, here's the detail: In the last useIsomorphicLayoutEffect usage in connectAdvanced.js, it returns a funcion, `unsubscribeWrapper`, which will be retained by React as `destroy` function of the effect. Since this `useEffect` only subscribes to `store`, `subscription`, `childPropsSelector`, which in most case won't change when store state updates. So this `useEffect` is never called again in following updates of `connected` component. So the effect in the `connected` component will always keep a reference to the `unsubscribeWrapper` created in first call. But this will lead to a memory leak in modern JS VM. In modern JS VM such as V8(Chrome), the instance of function `unsubscribeWrapper` will retain a closure for context when it's created. Which means, all local variables in first call of each `connected` component will be retained by this instance of `unsubscribeWrapper`, even though they are not used by it at all. In this case, the context includes `actualChildProps`. Which means, every connected component, will in the end retain 2 copy of its props in the memory, one as it's current prop, another is the first version of its props. It can be huge impact of memory if a connected component has props retaining a reference to big chunk of data in store state that can be fully updated to another version(e.g. data parsed from cache, network repsonse, etc). It will end up always retaining 2 copy of that data in memory, or more if there're more other `connected` compoents. A better JS VM should optimize to not include unused variable in the closure, but as I tested in V8 and Hermes, they both doesn't have such optimisation to avoid this case. This can be easy reproduced: 1. Have a connected component, reference one object in the store state. 2. Update the store state(add a version maker in the object to help identify the issue) 3. Use Chrome dev tools to take a heap snapshot. 4. Search for the object in the heap snapshot, you will find 2 version of it in the heap, one retained by connected wrapped component's props, one retained by a `destroy` in lastEffect of conneted compoents. By communicating with React community, a good solution suggested is to lift `useEffect` outside of the hook component in such kind of case. And this is how this PR solve the problem.
|
cc @gaearon |
|
Deploy preview for react-redux-docs ready! Built with commit 54c65df |
gaearon
approved these changes
Jan 21, 2020
It's historic, not related to the code as-is. That can be represented in the git history.
|
Ah, closures strike again! Always making a fool out of the reference counter... I think this is A-OK. Let me look through some things. |
timdorr
reviewed
Jan 21, 2020
|
@timdorr Applied the suggested change. |
timdorr
approved these changes
Jan 22, 2020
|
LGTM. Let's get another set of eyes on it and then we can see about merging it in. |
|
I'll try to look through it more closely this evening. |
|
Seems plausible enough. I don't suppose there's any kind of a test we could add to cover this internal implementation change? Also, does |
|
Not really, since it's hard to parse through heap snapshots without a whole bunch of other stuff going on. |
|
I figured as much. Can we at least have some kind of sandbox or something set up that shows the before and after behavior on this PR, just to confirm it really does make a difference? |
|
Before we go any further on this PR, @Jessidhia reported a bug in Reactiflux, which I've filed as #1508 . We should try to investigate that one first, because she offered up a suggested refactoring of |
|
@markerikson how does that goes? |
|
Awright, let's get this in. @larrylin28 just to check, does |
This was referenced Mar 8, 2020
This was referenced Mar 22, 2020
This was referenced Mar 30, 2020
This was referenced Apr 9, 2020
This was referenced Apr 28, 2020
webguru7
pushed a commit
to webguru7/react-redux
that referenced
this pull request
Dec 8, 2022
* Fix memory leak issue with `UseEffect` There's a memory leak in `react-redux`'s usage of UseEffect, here's the detail: In the last useIsomorphicLayoutEffect usage in connectAdvanced.js, it returns a funcion, `unsubscribeWrapper`, which will be retained by React as `destroy` function of the effect. Since this `useEffect` only subscribes to `store`, `subscription`, `childPropsSelector`, which in most case won't change when store state updates. So this `useEffect` is never called again in following updates of `connected` component. So the effect in the `connected` component will always keep a reference to the `unsubscribeWrapper` created in first call. But this will lead to a memory leak in modern JS VM. In modern JS VM such as V8(Chrome), the instance of function `unsubscribeWrapper` will retain a closure for context when it's created. Which means, all local variables in first call of each `connected` component will be retained by this instance of `unsubscribeWrapper`, even though they are not used by it at all. In this case, the context includes `actualChildProps`. Which means, every connected component, will in the end retain 2 copy of its props in the memory, one as it's current prop, another is the first version of its props. It can be huge impact of memory if a connected component has props retaining a reference to big chunk of data in store state that can be fully updated to another version(e.g. data parsed from cache, network repsonse, etc). It will end up always retaining 2 copy of that data in memory, or more if there're more other `connected` compoents. A better JS VM should optimize to not include unused variable in the closure, but as I tested in V8 and Hermes, they both doesn't have such optimisation to avoid this case. This can be easy reproduced: 1. Have a connected component, reference one object in the store state. 2. Update the store state(add a version maker in the object to help identify the issue) 3. Use Chrome dev tools to take a heap snapshot. 4. Search for the object in the heap snapshot, you will find 2 version of it in the heap, one retained by connected wrapped component's props, one retained by a `destroy` in lastEffect of conneted compoents. By communicating with React community, a good solution suggested is to lift `useEffect` outside of the hook component in such kind of case. And this is how this PR solve the problem. * Drop this comment for now. It's historic, not related to the code as-is. That can be represented in the git history. * Apply suggestions on parameters naming Co-authored-by: Tim Dorr <timdorr@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's a memory leak in
react-redux's usage ofuseEffect, here's the detail:In the last
useIsomorphicLayoutEffectusage inconnectAdvanced.js, it returns a function,unsubscribeWrapper, which will be retained by React asdestroyfunction of the effect. Since thisuseEffectonly subscribes tostore,subscriptionandchildPropsSelector, which in most case won't change when store state updates. So thisuseEffectis never called again in following updates ofconnectedcomponent. So the effect in theconnectedcomponent will always keep a reference to theunsubscribeWrappercreated in first call. But this will lead to a memory leak in modern JS VM.In modern JS VM such as V8(Chrome), the instance of function
unsubscribeWrapperwill retain a closure for context when it's created. Which means, all local variables in first call of eachconnectedcomponent will be retained by this instance ofunsubscribeWrapper, even though they are not used by it at all. In this case, the context includesactualChildProps. Which means, every connected component, will in the end retain 2 copy of its props in the memory, one as its current prop, another is the first version of its props.It can be huge impact of memory if a connected component has props retaining a reference to big chunk of data in store state that can be fully updated to another version(e.g. data parsed from cache, network response, etc). It will end up always retaining 2 copy of that data in memory, or more if there're more other
connectedcomponents.A better JS VM should optimize to not include unused variable in the closure, but as I tested in V8 and Hermes, they both doesn't have such optimisation to avoid this case.
This can be easy reproduced:
destroyin lastEffect of connected components.By communicating with React community, a good solution suggested is to lift
useEffectoutside of the hook component in such kind of case.And this is how this PR solve the problem.