fix: Avoid unnecessary memory leaks due to prevExports #766
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
naruaway
force-pushed
the
fix-fast-refresh-memory-leak
branch
from
f2a6111 to
971f99c
Compare
naruaway
force-pushed
the
fix-fast-refresh-memory-leak
branch
from
971f99c to
e956de9
Compare
timneutkens
pushed a commit
to vercel/next.js
that referenced
this pull request
Sep 21, 2023
…53797) This fixes memory leaks caused by `prevExports` in react-refresh-utils. It happens in code like the following: ```tsx const DATA = Array.from({ length: 100000 }, (_, i) => Math.random()); export const App = () => { return ( <div> <div>REWRITE_HERE</div> <div>{DATA.length}</div> </div> ); }; ``` After we edit this file to trigger fast refresh, previous `DATA` will be still retained in the memory since it forms `App(new) -> prevExports -> App(old) -> DATA` reference chain (there is some screenshots [here](pmmmwh/react-refresh-webpack-plugin#766)). I believe there is no reason to retain the whole exports as `prevExports`. We can just retain "signature" (`string[]`). By only holding this, we no longer create reference to the old exports, which fixes the memory leak here. Note that I filed a similar PR in pmmmwh/react-refresh-webpack-plugin#766 and also https://github.com/naruaway-sandbox/fast-refresh-hmr-memory-leak-demo is a reproducible example of this issue, which also explains that interestingly this issue is not easily solved for Vite. ## Should we fix it? I think yes, as long as there is no unintended side effect, it's better to fix it since we cannot predict whether users would load large payload AND does Fast Refresh many times without reloading the browser or not. In [this extreme case](https://github.com/naruaway-sandbox/fast-refresh-hmr-memory-leak-demo), it eats several hundred mega bytes of RAM. ## Verification I confirmed that the memory leak is gone with this change by running https://github.com/naruaway-sandbox/fast-refresh-hmr-memory-leak-demo with the change. I am not sure whether new tests are needed but my concern is to accidentally break Fast Refresh behavior somehow. I believe we have enough existing test cases 🙏 and I also tested manually.
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.
When fast-refreshing the following code:
each refresh will not purge the previous DATA since we chain
prevExports, which referencesApp, which closes overDATA. For reproducible example and more detailed explanation, please check https://github.com/naruaway-sandbox/fast-refresh-hmr-memory-leak-demoHistorical list of DATA is retained like the following via prevExports:
This PR breaks the chain by only passing absolutely necessary data across fast-refreshes, which is
signature: string[]andisReactRefreshBoundary: boolean. So it will be impossible to accidentally form this kind of reference chain.Note that I also tried to fix the same issue in Next.js vercel/next.js#53797
Verification
I verified using this PR with https://github.com/naruaway-sandbox/fast-refresh-hmr-memory-leak-demo and confirmed that Fast Refresh itself is working well while not causing the memory leak