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

Aggressive memory leak with the cache #644

Closed
prasannavl opened this issue Feb 25, 2016 · 8 comments
Closed

Aggressive memory leak with the cache #644

prasannavl opened this issue Feb 25, 2016 · 8 comments

Comments

@prasannavl
Copy link
Contributor

Velocity is aggressive about caching, and for each element it is run on, its values are cached in the cache variable with an id tag. But the problem is, this cache is never removed.

So, for single page apps, and apps that remove and add elements dynamically, velocity ends up leaking memory quite aggressively.

There should be a way to delete the cache, atleast manually. I use Velocity with react for high-performance scenarios where react idiomatic way of animation has too much impact. However, I'd like to remove the cache on Component lifecycle methods.

PS: I haven't given the source a thorough read, however, it also concerns me that, if VirtualDOM recycles existing DOM elements, and velocity uses the internal cache of elements, it would end up breaking animation and all kinds of horrible things.

If not an automated strategy, there definitely should be a way to atleast take over manual control of the cache.

@ydaniv
Copy link
Contributor

ydaniv commented Feb 25, 2016

How about using $.remoteData()?

@prasannavl
Copy link
Contributor Author

Yup. That's what I'm using, however, I saw that it was never referenced anywhwere else in the source code. And it seems most of the default caches go into "velocity" key. However, is there any other key that velocity uses?

It seems quite fragile. There should be a way to clean up everything related to an element in a more robust way.

@ydaniv
Copy link
Contributor

ydaniv commented Feb 25, 2016

The 'velocity' key is used to store Velocity related data on the object that's namespaced by the node's id.
If you're using jQuery it should clean it automatically for you.
If not you should remove data for every node you remove from the DOM.

@prasannavl
Copy link
Contributor Author

@ydaniv - No, I don't use JQuery. And yes, that's what I'm trying to do. The problem is that there's no way to actually identify a cache node correctly, outside of Velocity. The cache id seems to be referred in by "velocity" + (new Date().getTime()); - No way to retrive without looping through every object in the DOM node, or by reading the private variable of $.expando.

@prasannavl
Copy link
Contributor Author

removeData seems to handle all the internal details, and remove keys, if the "key" is known. But it never cleans up the cache itself. (i.e, the entire store).

I am also curious as to why the cache isn't a part of the DOM node itself. It already uses a property inside the DOM node to reference back to the cache, but why not have the cache directly in the node. It should simply it, and gets automatically de-referenced when removed.

prasannavl added a commit to prasannavl/velocity that referenced this issue Feb 25, 2016
There seems to be no way to cleanup the entire cache efficiently. Modify the removeData function to optionally do so. 

Ref: julianshapiro#644 (comment)
@Rycochet Rycochet added the Bug label May 5, 2016
@Rycochet
Copy link
Collaborator

Rycochet commented May 5, 2016

I've been making extensive use of in-element data for a work project, and it might be worthwhile considering getting away from the jQuery.data side of it and storing the cache in our own magic ID instead - would certainly fix this issue, and might be worth doing - needs discussion before anything is done though.

eirikurn added a commit to eirikurn/velocity-react that referenced this issue Jul 3, 2016
Without jQuery, velocity implements it's own .data() utilities which
store element (animation) data in one big cache.

Data in this cache is never cleared which is an issue because it keeps
references to all animated dom elements as well as whole React trees
through velocity-react's `complete` handler.

This commit crudely clears the most important velocity data when an
animated component is unmounted.

See julianshapiro/velocity#644
@Rycochet Rycochet added this to the Velocity 2 milestone Oct 24, 2017
@Rycochet
Copy link
Collaborator

In Velocity 2 the per-Element data is stored in a WeakMap, so it will be freed when the Element is removed from the DOM and released. THe list of animations is also no longer a list so frees each one as they finish.

@Rycochet Rycochet self-assigned this Oct 24, 2017
@Rycochet
Copy link
Collaborator

Changed to storing the data against the element itself, without any internal references (was slightly faster and made it easier to manage).

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

No branches or pull requests

3 participants