Skip to content

Svelte 5 memory leak after transitions #12719

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

Closed
gyzerok opened this issue Aug 3, 2024 · 16 comments · Fixed by #12796
Closed

Svelte 5 memory leak after transitions #12719

gyzerok opened this issue Aug 3, 2024 · 16 comments · Fixed by #12796
Assignees

Comments

@gyzerok
Copy link

gyzerok commented Aug 3, 2024

Describe the bug

Transitions in Svelte 5 causes memory leaks by holding onto the DOM nodes indefinitely.

image

Also I found this PR which seems to be fixing a similar problem back in 2022 and this issue, that reports something similar for version 4.

In our case we are writing highly interactive real-time SPA, which means that user will go between screens a lot and will keep tab open for days, which gets amount of detached nodes to hundreds of thousands or maybe even millions. I can get from 1k to 20k nodes in a minute.

Reproduction

Here is the repo with the smallest example: https://github.com/gyzerok/svelte-transition-memory-leak.

Clone the repo and npm i && npm run dev.

Steps:

  1. Open app in Chrome
  2. Record memory snapshot
  3. Click button twice
  4. Record second memory snapshot
  5. Compare both snapshots and filter by "detached"

Logs

No response

System Info

Chrome 127.0.6533.89

Severity

blocking all usage of svelte

@trueadm trueadm self-assigned this Aug 4, 2024
@trueadm
Copy link
Contributor

trueadm commented Aug 7, 2024

Are you sure this is related to transitions alone? If you remove the transitions do you see any memory leaks?

@gyzerok
Copy link
Author

gyzerok commented Aug 8, 2024

@trueadm yeah, if I remove the transition the leak is still there. Apparently it's really the {#key} tag that is causing the leak. Perhaps Svelte remembers previous keys even if they aren't needed anymore.

Remove the key and no leaks happen. And since key is a recommended way to start transitions in certain cases they just go hand in hand.

@trueadm
Copy link
Contributor

trueadm commented Aug 8, 2024

@gyzerok Transitions still leak separately, but I also believe the key block is leaking here too. I have a PR for the transitions leaking, I'll put another up for key blocks.

@gyzerok
Copy link
Author

gyzerok commented Aug 8, 2024

@trueadm oh, that sounds amazing! Was a bit worried about those leaks and how much attention the issue would get. Really appreciate you working out on them!

@trueadm
Copy link
Contributor

trueadm commented Aug 8, 2024

@gyzerok I'm unsure if the key block is leaking here, at least for me I can't see it is. If you remove the transition, then having the key block there or not doesn't seem to make any difference to me. Maybe you can show what you see in screenshots or with a video?

@trueadm
Copy link
Contributor

trueadm commented Aug 8, 2024

FWIW you will always have a detached element because we use template cloning and retain the template for later so we can quickly clone it. However, this won't grow past that of what is needed for a template. This is intended and this is not a memory leak.

@gyzerok
Copy link
Author

gyzerok commented Aug 8, 2024

@trueadm here is the screenshot how it looks without transition added. It seems like you are right and if I click button multiple times then the amount of detached nodes does not grow (see diff between 2 and 1 is the same as between 4 and 1). So indeed this does not seem like a leak problem.

Anyway, eagerly awaiting your transition fix then. Thank you!

image image

@gyzerok
Copy link
Author

gyzerok commented Aug 11, 2024

Hello @trueadm!

I've noticed that your fix got merged under 215 version. I've updated my reproduce repo with this version, but can still see the leak. After several button clicks I see 4 detached div nodes.

image

@trueadm
Copy link
Contributor

trueadm commented Aug 11, 2024

Hello @trueadm!

I've noticed that your fix got merged under 215 version. I've updated my reproduce repo with this version, but can still see the leak. After 4 button clickes I see 4 detached div nodes.

image

If you press it again, do you see more than 4 detached nodes? 4 detached nodes might just be templates here.

@gyzerok
Copy link
Author

gyzerok commented Aug 11, 2024

If you press it again, do you see more than 4 detached nodes? 4 detached nodes might just be templates here.

Yeah, amount of leaking nodes corresponds to the amount of toggles for me. Also in the Object graph in the middle you can see that the retainer is an animation.

image

@trueadm
Copy link
Contributor

trueadm commented Aug 11, 2024

@gyzerok Can you confirm #12796 fixes this for you too please?

@gyzerok
Copy link
Author

gyzerok commented Aug 11, 2024

@trueadm can you point me to some instructions on how to correctly install version from your branch? Would happily check it out even with my existing codebase, not just with reproduction.

@trueadm
Copy link
Contributor

trueadm commented Aug 11, 2024

The easiest way is to checkout the Svelte repo and copy packages/svelte into your project /node_modules/svelte replacing the svelte directory there. Then you can run vite with --force to make it pick up the changes.

@gyzerok
Copy link
Author

gyzerok commented Aug 11, 2024

@trueadm the fix seems to be perfectly working. Do not see any detached nodes with animation retainers now both in small repro and in my project. Great job!

@trueadm
Copy link
Contributor

trueadm commented Aug 11, 2024

Can you also sanity check that this change is needed? https://github.com/sveltejs/svelte/pull/12796/files#diff-fa124211d8f3caa3fbf8681462d4cad250a9ca42473d6f44716dc169e0571879R442

If you find the file and comment it out in your node_modules and then run again with --force. Thank you for the help!

Update: I also had to update the PR, so if you could double check if the latest changes still work for you, that would be great.

@gyzerok
Copy link
Author

gyzerok commented Aug 11, 2024

@trueadm pulled the branch to the latest commit and still seems to be fine. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants