deltaluca / nape

Haxe/AS3 Physics Engine
http://napephys.com
Other
542 stars 78 forks source link

Potential Nape Memory Leak #93

Closed andrew-git closed 9 years ago

andrew-git commented 9 years ago

After running the garbage collector post test, 2-6 bodies left in memory, around 20-30 Vec2 instances and several other loitering objects all created by Nape (especially ZPP_ objects)

Sample code to test https://gist.github.com/devon-o/e76c292b78fef036845d

If previously remove bodies shapes it looks better, but still some objects exists http://forum.starling-framework.org/topic/starling-nape-clicking-and-destroying-body

deltaluca commented 9 years ago

Two things:

1) Nape uses objects pools heavily (especcialy for zpp_ object types which you don't interact with in user-side code). There is a function on nape.util.Debug clearObjectPools() which will empty out this pools and leave any remaining pooled objects available for GC to dispose of.

2) It's very rare for a GC to 'actually' dispose of everything it possibly can, especcialy flash's one. In general the GC will not even attempt to dispose of data that is not trivially found to be collectable unless it needs to to allocate more objects, and won't try and do any deep cleans until it gets desperate, though in my experience it's not as bad as say, the garbage collection in V8 can be.

I'm not saying it's impossible for there to actually be a memory leak, but I would be incredibly suprised as memory usage was very, very heavily tested (all the samples on napephys.com for instance have perfectly flat memory graphs (beyond objects flash allocates itself because they don't care) once they've warmed up meaning nape isn't even allocating new memory at all and just re-using objects from the various pools at that point in time. That is true even if the samples are changed to periodically start again from scratch in the same running instance (effectively doing your startTest/endTest again and again)

deltaluca commented 9 years ago

fyi, although I doubt I have the code lying around anymore, the way memory was actually tested for nape (not to rely on fragile GC behaviours) was to actually compute the graph of 'all' reachable objects myself from any objects in scope in the test, and from every Nape class type (for static members) so that I could be 100% sure that at any point in time, no object was still reachable from some live reference following a test meaning that if there was ever any memory increase over time, it was due only to GC behaviour and not that there were actually any real memory leaks.

andrew-git commented 9 years ago

Thanks for reply, Luca.

As for 2nd point: in the gist above I changed number of items to 1000 and run test few times (runTest/endTest) and memory wastes heavily. I understand that this is pretty large quantity and real apps uses pool, but early I don't encounter problems with memory in nape, and was surprised and decide to check.

I try clearObjectPools, and it did a trick, thanks a lot! Is it available only for debug version and it's recommended to use in releas versions? And what about recommendation to remove bodie shapes before destroy body?

Images without and with clearObjectPools: without with

deltaluca commented 9 years ago

clearObjectPools is available in release too.

One way that the app-code can introduce a memory leak when working with nape is however, to do things like 'new Vec2' all the time, since this will 'always' allocate a new Vec2 that nape may release depending on how you use it into the object pool.. but since you keep allocating things with 'new' the objects don't get used and the pool just gets bigger and bigger instead of being recycled. That didn't seem to be the case from the code you showed though.

Regarding removing shapes from body first, you could argue that for the GC to work efficiently it's always a good idea to smash objects apart (null'ing references etc) so that it's easier for the GC to know an object is not being used anymore (and in some cases with Flash GC alteast in the past, it would simply give up if an object graph was far too big, though a Body + Shapes is not near big enough for that).

andrew-git commented 9 years ago

Thanks! So best way is to use Vec2.get, null objects references and if needed to clean all memory call clearObjectPools?

deltaluca commented 9 years ago

In some cases you can be even more careful and use Vec2.weak.

The difference between Vec2.weak and Vec2.get is that Nape automatically calls vec2.dispose() when you pass a 'weak' Vec2 to a nape function. eg:

// with get
var v = Vec2.get(x, y);
someNapeFunction(v);
v.dispose();

// with weak
someNapeFunction(Vec2.weak(x, y));

Of course, it's easier to introduce bugs with Vec2.weak if it get's released to object pool earlier than you realised. But yes, if there is a 'get' function you should always use it instead of 'new' (since get will in the worst-case fall back onto new itself if the object pool is empty).

andrew-git commented 9 years ago

If possible I usually use Vec2.get(x, y, true).

Thanks a lot for explanations, Luca!