Help fixing a memory leak in C++ app

[loktar]

Hey, any C++ programmers willing to dig through some source code in the house?

In my Model Optimizer and Converter tool, there's a huge memory leak. AFAIK it only happens when optimizing/sanitizing, not when only converting (or doing nothing with the files). Here's the source code, and here's the source code for the MDLX parser. They require Qt5 and MinGW 7.3.0 to build.

Here is the source code for the optimizer, where I believe the problem lies, but I'm not sure.

Thanks for any help, and I'll certainly add a thank you to the description for anyone who helps me fix this problem

 

[Retera]

Given 15 minutes on my breakfast break reading your code, I have a hunch about the problem. it appears that your MDLX parser is using a scaffolding on top of the original parsed data called "Edit" in many places. This includes "GeosetEdit" who has shared pointers to "Vertex" who have shared pointers to "MatrixGroup" edit who has shared pointers to "GenericObjectEdit." In those cases, then "GenericObjectEdit" is subclassed by "BoneEdit" who in turn has a "GeosetEdit" shared pointer.

You may have noticed, if you ever looked, that the code of Retera Model Studio does this also. However, that code is Java with an MDL parser written over a dozen years ago when I was a high school kid, and Java is sort of for dummies. When we make a pointer cycle like this in Java, and then throw it away, the garbage collector identifies that the pointer cycle is no longer connected to the running program and frees the memory of the whole thing.

However, when you make a pointer cycle like this with std::shared_ptr the result is that the reference counter of everything in the cycle is 1 or greater, and so the std::shared_ptr never automatically will free the memory.


The above example is not necessarily an isolated case. I gave this a couple of minutes and saw this paradigm/style of bug in your code, but I think that the one I pasted above is by no means the only instance of the bug if I am correct about this.

Edit:
From some further reading, it appears that your "edit" scaffolding might also be inspired by Ghostwolf rather than anything in Retera Model Studio. However, I think my claim about garbage collectors applies to JavaScript also -- so copying from that source would introduce similar issues.

This type of discrepancy, where the Java / JavaScript ends up being much more brainless to create than C++, is one example that I can make such fun parodies of Warcraft in java but the actual company running the actual game struggles so greatly to keep it running in C++.

 

[loktar]

Given 15 minutes on my breakfast break reading your code, I have a hunch about the problem. it appears that your MDLX parser is using a scaffolding on top of the original parsed data called "Edit" in many places. This includes "GeosetEdit" who has shared pointers to "Vertex" who have shared pointers to "MatrixGroup" edit who has shared pointers to "GenericObjectEdit." In those cases, then "GenericObjectEdit" is subclassed by "BoneEdit" who in turn has a "GeosetEdit" shared pointer.

View attachment 549969

You may have noticed, if you ever looked, that the code of Retera Model Studio does this also. However, that code is Java with an MDL parser written over a dozen years ago when I was a high school kid, and Java is sort of for dummies. When we make a pointer cycle like this in Java, and then throw it away, the garbage collector identifies that the pointer cycle is no longer connected to the running program and frees the memory of the whole thing.

However, when you make a pointer cycle like this with std::shared_ptr the result is that the reference counter of everything in the cycle is 1 or greater, and so the std::shared_ptr never automatically will free the memory.


The above example is not necessarily an isolated case. I gave this a couple of minutes and saw this paradigm/style of bug in your code, but I think that the one I pasted above is by no means the only instance of the bug if I am correct about this.

Edit:
From some further reading, it appears that your "edit" scaffolding might also be inspired by Ghostwolf rather than anything in Retera Model Studio. However, I think my claim about garbage collectors applies to JavaScript also -- so copying from that source would introduce similar issues.

This type of discrepancy, where the Java / JavaScript ends up being much more brainless to create than C++, is one example that I can make such fun parodies of Warcraft in java but the actual company running the actual game struggles so greatly to keep it running in C++.

Thank you, I think you're probably right! I'm not sure I fully understand this, it's been so long since I worked on this that I don't remember my own logic. I think it was based on both GhostWolf's code and yours, IIRC, but primarily GhostWolf's. I will have to read up on `shared_ptr`s if I want to figure out how to fix this. Maybe I can just bruteforce it and delete all the shared ptrs in the destructors, though that's probably not a good idea.

 

[Retera]

Maybe I can just bruteforce it and delete all the shared ptrs in the destructors, though that's probably not a good idea.

I would imagine the destructors will never be called. But perhaps you could add a method that can be called on any edit object prior to erasing it from a vector, when you wish for it to no longer be used, wherein the implementation of that method clears all vectors of shared pointers on the object, and calls .reset() on all shared pointers on the object.

 

[loktar]

I would imagine the destructors will never be called. But perhaps you could add a method that can be called on any edit object prior to erasing it from a vector, when you wish for it to no longer be used, wherein the implementation of that method clears all vectors of shared pointers on the object, and calls .reset() on all shared pointers on the object.

Thanks, I'll try that.

 

[loktar]

Hmm I added xx.reset() to all the destructors (which do appear to be called when the Model object goes out of scope) and no luck. It does seem to be less now, but I'm not 100% sure, it could just be that I didn't clear out my dump test folder for a while last time I tested it and looked at the memory usage.

Edit: if you wait long enough the memory does get cleared, but that's probably because the thread has quit/finished, or something. It really should be cleared immediately when done with a model. Imagine someone batch optimizing a large number of reforged model, it could crash their computer.

 

[Retera]

xx.reset() to all the destructors (which do appear to be called when the Model object goes out of scope) and no luck

I would not expect destructors to work for this. If you log when destructors are called you will see it happening sometimes, because not every class/struct is a component of a loop.

But as long as there is a loop, nothing in the loop will have its destructor automatically called. This is the nature of shared_ptr reference counting

(It does not matter when something outside pointing in is destructed, a shared pointer loop will always be a loop until you intentionally break the loop, which by definition cannot happen in a destructor since no destructor in the loop is called)

 

[loktar]

I would not expect destructors to work for this. If you log when destructors are called you will see it happening sometimes, because not every class/struct is a component of a loop.

But as long as there is a loop, nothing in the loop will have its destructor automatically called. This is the nature of shared_ptr reference counting

(It does not matter when something outside pointing in is destructed, a shared pointer loop will always be a loop until you intentionally break the loop, which by definition cannot happen in a destructor since no destructor in the loop is called)

Thanks for the explanation. I added an unload function everywhere where needed (where I reset the pointers), and then calling them from the Model destructor, but now it's crashing lol. Probably referring an already out of memory object somewhere. Think I'm gonna call it a night ^^

Edit: ok I fixed the crash, but it's still leaking and now most of the models are getting infinite passes
Edit2: fixed the infinite loops, and the memory leak is gone :O
Edit3: damn it, spoke too soon, there was another infinite loop and memory usage ballooned... but it seems like if I can fix that the memory leak is fixed

Edit4: ok seems like the infinite loops were mostly unrelated to the changes made for the memory leak (an earlier change I didn't test properly), it's still eating up a little bit of memory, but nothing like before! Thanks a lot @Retera!