Skip to content

fix: prevent numerous transition/animation memory leaks #12759

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

Merged
merged 3 commits into from
Aug 11, 2024

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Aug 7, 2024

Whilst digging into #12719, I discovered several memory leaks where we retain DOM elements longer than we should. We also frustratingly retain the animation even after the DOM element has been detached and the animation is cancelled – but this seems to happen on the MDN site too, so maybe WAAPI has issues on Chromium?

I can't say for sure if this solves all the issues, but it definitely solves a whole load for me and memory usage now doesn't grow at a large rate when toggling the DOM elements in and out in the example.

Test case

  • Go to the dev tools memory tab, record a snapshot
  • Toggle the animation on and off
  • Take another snapshot
  • Toggle the animation on and off one last time
  • Take a third snapshot

If you compare the 2nd and 3rd snapshots, we have no retained DOM elements leaking.

Copy link

changeset-bot bot commented Aug 7, 2024

🦋 Changeset detected

Latest commit: 9dbd96a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris
Copy link
Member

Do you have a repro that demonstrates the leak on main being fixed with this PR? Since this stuff is hard to test for, it'd be good to link to something in the comments or we're liable to regress

@trueadm
Copy link
Contributor Author

trueadm commented Aug 7, 2024

Do you have a repro that demonstrates the leak on main being fixed with this PR? Since this stuff is hard to test for, it'd be good to link to something in the comments or we're liable to regress

I added the test case in the OP.

@trueadm
Copy link
Contributor Author

trueadm commented Aug 8, 2024

I updated the PR. @Rich-Harris it seems that we don't need to null those fields, the reason for the retainment was the unmount function being stored on the window. The other fixes are still valid

@Rich-Harris Rich-Harris merged commit d2efca0 into main Aug 11, 2024
9 checks passed
@Rich-Harris Rich-Harris deleted the improve-animation-memory-retaining branch August 11, 2024 02:46
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 this pull request may close these issues.

2 participants