kenwheeler / slick

the last carousel you'll ever need
kenwheeler.github.io/slick
MIT License
28.47k stars 5.89k forks source link

Unslick causes memory leaks #1380

Open recser opened 9 years ago

recser commented 9 years ago

Having a slick that is recreated multiple times causes the memory used to grow pretty fast.

http://jsfiddle.net/uyav5o4x/2/

Just record the heap allocations and you will see that the slick 'rows' remain referenced somewhere. Is there anything that can be done to avoid this leaks ? Or maybe my example has some big issues and there are different ways of recreating slick without causing leaks

superted17 commented 9 years ago

I'm working on an JS app at the moment that uses Slick.

We're doing some profiling in the app and we can see that Slick isn't properly being destroyed when unslick is called.

Here's a JS fiddle that demos the problem:

http://jsfiddle.net/n7r4y2h2/4/

Using Chrome's Heap profiler and taking snapshots between Slicking and Unslicking, I can see that there are a few detached dom elements being held on to by references. E.g, In the above fiddle there are two divs: div.slick-track and div.slick-list.draggable. This will obviously change depending on how you have configured Slick (dots, etc).

I'm currently playing around with the unslick function and I'll update this if I find anything.

utkuturunc commented 8 years ago

+1. It seems like ~140kb leaks everytime I unslick and slick the carousel in my react app. There are a few slick html elements in the heap records.

kenwheeler commented 8 years ago

@Simeydotme is this resolved?

simeydotme commented 8 years ago

Nope, apologies if you wanted to get it fixed up before 2.0 :), I took this as a performance enhancement instead of a major bug. can reopen if needed.

kenwheeler commented 8 years ago

Memory leaks are no fun

joshuagonz commented 8 years ago

I was experiencing this same issue. I had to use slick and unslick more than 100 times on my app and sadly am getting the same issue above. I had to dig into the slick code to fix this. I hope my fix helps anyone out there who ran into memory leak issues. So far i tried it with 7000 unslicks using the sample above before the fix i went from 10MB - 120MB, with the fix i went from 10MB-12MB and it doesn't increase more than that.

Here's what i did:

  1. I added this code at the end of this function to clear all events

Slick.prototype.cleanUpEvents _.$slider.off(); _.$list.off(); $(_.$slideTrack).off(); $(_.$slideTrack).children().off();

  1. I change all the .detach() to .remove() in the Slick.prototype.destroy function
denis-andrikevich commented 6 years ago

@joshuagonz thx a lot, works like a charm

denis-andrikevich commented 6 years ago

I suggest implementing Joshua fix. Memory leak is awful.

elgwiedo commented 6 years ago

@joshuagonz thx for the nice fix. Please implement.

JPustkuchen commented 5 years ago

Could someone please create a clean pull request from the fix to test and speed things up a bit for the maintainer? Thank you all!