Skip to content

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.

@changeset-bot
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

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.

3 participants