TSE GFXPrimitiveBuffer Memory leak fix

#1 @Justin,

A destructor is usually declared as virtual so that inherited object's destructor's are also called upon destruction - e.g. if GFXCubeBuffer is derived from GFXPrimitive buffer, and you put one in your list of GFXPrimitive objects, then try and delete it, only the GFXPrimitive destructor will be called - unless of course GFXPrimitive's destructor is virtual, then of course GFXCubeBuffer's destructor will be called.

At least, thats how i remember it :)

#2 @ James

Any function declared virtual just means that a subclass should implement it. You use virtual a lot when creating interface classes or abstract classes. In this case, nothing is subclassing the GFXPrimitiveBuffer in TSE, it is instantiated directly, and it has no destructor declared, so it leaks.

I'm not aware of any special rules for virtual destructors, but even if that's the case, the GFXPrimitiveBuffer isn't subclassed, and thus will never have a destructor made for it. It's my opinion that any class with dynamic private data ought to be able to destroy that data.

#3 This is not fixing it for me...

Still losing around 20MB evertime i run the mission. If i loaded mission 20 times, i would be using well into 512mb..

#4 @ Westy:

Keep in mind that TSE has a number of leaks, it's still a work in progress, this only fixes one of them. I'm not even sure what some of the other ones were, we just sort of inadvertently fixed them at some point when porting our code in or modifying what was there. This is just one of the ones I consciously had to hunt down and eliminate because it was killing progress. No one could stay in-engine for more than 10 minutes without taking their machine down.

In any event, I highly recommend using the Torque Memory Profiler. Go into platform/plaformMemory.cpp and put a #define DEBUG_GUARD at the top somewhere. Then, rebuild and start the engine. When you get the leak to start (Windows' Task Manager's Memory Delta field is great for finding this out), let it run for a bit, then execute dumpMemSnapshot("memory.txt"); from the console, exit the game, and read the memory.txt file you'll find in the same folder as your executable. This should tell you what code is allocating a bunch of memory, and you should be able to track your leak down from there. Good luck!

#5 Yes, GFXPrimitiveBuffer should have a virtual destructor because it may need to be subclassed for API specific implementations. But these fixes aren't incompatible.

Anyway, I'll see about getting this into TSE HEAD later today. Doing my morning forum chores from home, on my laptop, so I don't have the horsepower for TSE atm. :)

#6 If i define DEBUG_GUARD then a Platform::debugBreak(); gets called in checkGuard() and it exits.

The condition is if header->preguard[0] != guardVal

header->preguard[0] = 1431699200

while

guardVal = 1431699455

#7 @ Westy:

Interesting. You made me think, I wonder if I ever defined DEBUG_GUARD or not. Have you tried doing the rest of what I said without DEBUG_GUARD on?

#8 :o) Virtual destrutors are used to prevent memory leaks in derived classes.

Symptoms of not using them

if you have a class like

class B {~B();};
class D : B {~D();};
B* b = new D;
delete b;

1. delete passed incorrect size_t (oops memory leak).
2. delete may get passed incorrect pointer address.
3. delete does not call derived classes destructor.

Who didn't read their C++ book then ;o)

Incidentally virtual destuctors are extended not overriden by derived classes.

#9 @Peter:

Personal comments and pastes from http://cpptips.hyperformix.com/cpptips/why_virt_dtor aside, you'll note that GFXPrimitiveBuffer, as I've already said, is NEVER inherited from inside TSE, and therefore has NO destructor. My solution was to create one for it because I have no intention of inheriting from it. In addition, any class that declares private dynamic data should NOT be relying on some other class to destroy that data. GFXPrimitiveBuffer allocates a buffer, and should be able to destroy it as well.

#10 Justin dumpMemSnapshot("memory.txt"); cannot be found.I cant find a dumpMemSnapshot() function anywhere in the source.

#11 See platform/plaformMemory.cpp. It's in there. In my copy (which is slightly modified from the HEAD version), it's at line 1030:

ConsoleFunction(dumpMemSnapshot,void,2,2,"(string fileName) Dump a snapshot of current memory to a file.")

Something I noticed when I went looking for it is that it is indeed wrapped in a DEBUG_GUARD block. I know you had some trouble with that, but I'd direct questions about it towards the GG guys or someone on the forums who is more familiar with the Memory namespace than I am. I don't use the torque Memory stuff unless I am debugging a leak. Come to think of it, I should make a up a patch at some point that reflects the changes I made such that defining TORQUE_DISABLE_MEMORY_MANAGER actually does that instead of breaking your build.

Update: That patch is available here as soon as it's moderated.

#12 OK, a fix is checked in to the head. Thanks for catching this and letting us know Justin!

#13 Ah yes, just need to remove the ifdef for DEBUG_GUARD.

#14

Quote:
any class that declares private dynamic data should NOT be relying on some other class to destroy that data. GFXPrimitiveBuffer allocates a buffer, and should be able to destroy it as well.

GFXPrimitiveBuffer can still destroy the allocated buffer if the destructor is virtual. I don't get what you're saying here.

virtual ~GFXPrimitiveBuffer()
{
if (mPrimitiveArray != NULL)
delete mPrimitiveArray;
}

If there's any chance at all that the class will be extended, this *needs* to be virtual. For your own local copy of Torque, no biggie (unil months down the road when you forget and subclass it anyway). But for general use the virtualness is a requirement.

#15 @Mike:

Then let me quote myself:
[quote]
you

#16 If you get "check guard" errors, it basically means that something is whomping memory. The DEBUG_GUARD macro basically adds a header and a footer at the start and end of any allocations, which it then sets to a known value. At different times during the code (although plenty of those are commented out), it checks the guard bytes for the known value. If they arent the known value, then its likely you have a memory stomp (where something is overwriting memory, usually past the bounds of an array, or writing to a pointer thats been mangled).

I had this when I enabled the debug guard checking and finally found out it was a simobjectptr in one of my AI Action classes (which wasnt simobject derived).