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(item-set): don't leak memory when setting groups #316

Merged
merged 2 commits into from
Feb 14, 2020

Conversation

Thomaash
Copy link
Member

This is my attempt at solving #183.

There are two key differences when compared to #235:

  1. This one actually works better.
  2. This one doesn't blindly throw away all listeners.

Leaks in my tests (very rough measurements on my device):

PS: For the tests I used the JSFiddle MWE from @aphix. I just decreased delays to leak more memory in shorter amount of time.

This is my attempt at solving #183.

There are two key differences when compared to #235:
1. This one actually works better.
2. This one doesn't blindly throw away all listeners.

Leaks in my tests (very rough measurements on my device):
- Upstream: 3 MB/s
- #235: 250 KB/s
- This: about 0 KB/s

PS: For the tests I used the JSFiddle MWE from @aphix. I just decreased
delays to leak more memory in shorter amount of time.
@aphix
Copy link

aphix commented Feb 13, 2020

Great work @Thomaash! Good idea on the comments to keep people aware of some less-than-obvious interactions within the lib.

One very minor suggestion to clear up some grammar/spelling in this comment

f24b505#diff-f4d829b4f8b61a3d664fc7a5eb70924dR38-R40

I believe this:

"Anything that needs to be manually disposed off before garbage collection happens or so that garbage collection can happen should be added to this stack."

Meant to say this:

"Anything that needs to be manually disposed of before garbage collection happens (or so that garbage collection can happen) should be added to this stack."

Mostly it's the "disposed off" -> "disposed of" but I think the parenthesis help clarify the separation of the two reasons to add something to that stack.

Thanks again,
Aphix

@Thomaash
Copy link
Member Author

Thanks for pointing it out. It looks better now.

@yotamberk yotamberk merged commit c1d4f4e into master Feb 14, 2020
@yotamberk yotamberk deleted the issues/183-by-Thomaash branch February 14, 2020 16:10
@vis-bot
Copy link
Collaborator

vis-bot commented Feb 14, 2020

🎉 This PR is included in version 7.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants