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

Fix memory leak issue with UseEffect #1506

Merged
merged 3 commits into from
Feb 18, 2020
Merged

Commits on Jan 21, 2020

  1. 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.
    larrylin28 committed Jan 21, 2020
    Configuration menu
    Copy the full SHA
    b58f4c7 View commit details
    Browse the repository at this point in the history
  2. Drop this comment for now.

    It's historic, not related to the code as-is. That can be represented in the git history.
    timdorr committed Jan 21, 2020
    Configuration menu
    Copy the full SHA
    eb79ce3 View commit details
    Browse the repository at this point in the history

Commits on Jan 22, 2020

  1. Configuration menu
    Copy the full SHA
    54c65df View commit details
    Browse the repository at this point in the history