search

metafizzy / outlayer

:construction_worker: the brains & guts of a layout library
163 stars 63 forks source link

Signed-off-by: Matt Harker <thundercloud@theflux.co.uk> #21

Closed TeaSeaLancs closed 10 years ago

TeaSeaLancs commented 10 years ago

It seems that Outlayer maintains an internal list of instances created, referenced by the element's outlayerGUID, but those references aren't removed when the outlayer instance is destroyed.

Because of this, and the outlayer's track of the element that an instance was created on, these elements are never able to be garbage collected, which, over time in single page apps, will massively inflate the heap size of the page.

This pull request contains a small fix to clear the outlayer instance from the instances object on destruction.

TeaSeaLancs commented 10 years ago

(Also sorry for the silly commit message, i'm still getting to grips with git)

desandro commented 10 years ago

Hi! Thank you for this contribution. Yes, this is a valid concern. I like it. Let me think it over before I merge. There might be a use case where you want to keep the instance.

joews commented 10 years ago

:+1:

If destroy has been called explicitly I think it can be assumed that the instances is no longer required. If it is still required, the developer can handle the "destroyed" instance explicitly. It is wrong to prevent an object from being garbage collected by keeping a reference under closure - the developer has literally no way to free the memory retained by that instance.

desandro commented 10 years ago

Thanks for following up! let's do this.

joews commented 10 years ago

Great, thanks! Will there be updated builds of Isotope, Packery and Masonry that include this fix?

desandro commented 10 years ago

Yes. First I need to cut a release for Outlayer, v1.2.1. Then it will be included in future releases of Masonry, Packery, and Isotope.

maimairel commented 10 years ago

I just discovered this today, tons of Detached DOM elements are being kept by Outlayer. This causes a massive memory leak in a full AJAX website I'm working on.

Luckily this PR fixes it!

Thank you!

maimairel commented 10 years ago

Hi,

There is a critical bug here:

It shouldn't be delete this.instances[id] but delete instances[id] instances is not a property of Outlayer but a variable scoped inside the outlayerDefinition.

Thanks!

TeaSeaLancs commented 10 years ago

Hi,

Ah, good catch! I think quite possibly what happened here is that I made the change to the local copy of Masonry that my project was using, and I ended up doing a bad transcribe into the Outlayer project.

I'll fix it tomorrow morning and submit a fresh pull request. On 7 Nov 2014 23:06, "maimairel" notifications@github.com wrote:

Hi,

There is a critical bug here:

It shouldn't be delete this.instances[id] but delete instances[id] instances is not a property of Outlayer but a variable scoped inside the outlayerDefinition.

Thanks!

— Reply to this email directly or view it on GitHub https://github.com/metafizzy/outlayer/pull/21#issuecomment-62228856.