How to free memory in my Class destructor?

Question

OK, I suppose this question may sound too silly, but memory-management (especially in C/C++) has never been my strong point and since it's usually not a noticeable thing, I tend to overlook it. So, forgive me if all this sounds stupid. However, since my current project involves A LOT of data and processing, memory consumption easily goes above 2GB in a matter of seconds and it definitely slows the whole thing down, so... it's time I started thinking of ways how to fix it.

---

So, here is my situation...

My principal class (of which I'm creating some millions of instances (which hopefully are automatically deleted, since they stop being used), so supposedly this is the culprit) is (roughly) this :

class MyClass
{
    public:
        // My Constructors
        MyClass ();
        MyClass (std::string param);

        // My Destructor (still empty)
        virtual ~MyClass ();

        // Some methods
        void methodA(std::string moves);
        void methodB();

        //----------------------
        // My Variables
        //----------------------
        boost::array<int,64> arrA;
        boost::array<unsigned int,13> arrB;
        unsigned int key;

        boost::array<int,3> arrC;       
        int argA;
        int argB;
};

And this is (roughly - actual code has been altered) how the instances of the above class are created :

vector<MyClass*> SomeOtherClass::getListOfObjects()
{   
    vector<MyClass*> objects;

    for (int i=0; i<MAX_OBJS; i++)
    {
          // Do some preparatory work
          objects += new MyClass();
    }

    return objects;
}

And here's how the results of the above function are being used :

void SomeOtherClass::doSth()
{
    vector<MyClass*> objs = this->getListOfObjects();
    int objsSize = objs.size();

    for (int i=0; i<objsSize; i++)
    {
        MyClass* obj = objs[i];

        // Do sth with obj

        delete objs[i];
    }
}

So, my question is :

• What should I do in my destructor, so that when the object is not needed anymore and thus released, all of its "subcomponents" are released too? (e.g. the 2 boost::arrays)

• Is there anything wrong you notice with the approach above?

---

Please let me know if there's anything else you need to know about my implementation...

Solution 1

Your class members do not look to be dynamically allocated, in which case you don't need to explicitly delete anything in the destructor. If you happened to leave out some pointers to allocated memory, in this question, which you allocate with new, you would need to delete these in the destructor.

Remember, if you new you need to delete, similarly with new[]-delete[]. Barring the case of allocation to a std::unique_ptr.

If your MyClass objects themselves are allocated on the heap with new, then you would have to delete them.

p.s. if you are using C++11, you should probably use std::array now.

---

From your new code, it is obvious that whoever keeps the list returned from getListOfObjects(), needs to call delete on each element when its to be destroyed. It is likely the destructor of SomeOtherClass.

Alternatively you can wrap the MyClass* pointers within a std::unique_ptr or std::shared_ptr (or any of the boost smart pointers which might be relevant here) which would then delete automatically when the vector holding them goes out of scope and is destroyed.

---

If doSth is accurate in its representation, and ensures that ALL instances of MyClass get deleted, then this code seems fine, from a memory leak standpoint.

Solution 2

All I see is a class that correctly manages it's memory. boost::array is a fancy wrapper over plain old array (boost::array<int,64> arrA is just int arrA[64] with methods added to make it a random-access read-only collection for standard library algorithms) and as such is allocated as part of the object.

Now if you say you have a leak, you apparently do mismanage memory somewhere, but it's not here.

---

In modern C++, it's trend to avoid writing the deletes yourself, leaving it up to specialized destructors instead. Given the allocating code above, the Boost.Pointer Container might be the right tool.

Solution 3

What I see is all allocated on stack. so they are being properly managed and well deleted in destructor. If there is a memory leak, may be you are calling new MyClass but not releasing them. or something elsewhere.

as I see in edited code there is a vector are you deleting the items of objects returned by SomeOtherClass::getListOfObjects ?

also you may use boost::shared_ptr<MyClass> instead of raw pointers MyClass*.

Solution 4

This sounds a bit odd

My principal class (of which I'm creating some millions of instances (which hopefully are automatically deleted, since they stop being used)

If you are creating instances of MyClass and have not done it using smart pointers of some kind then they will not magically be deleted, you need to do the job.

View more solutions

Comments

[Seng Cheong]

"it's usually not a noticeable thing, I tend to overlook it." -- Worst attitude ever. Are you familiar with the delete keyword?

[BЈовић]

you are missing copy/move constructors

[Dr.Kameleon]

@JonathonReinhart I know, I know; but you know how these things are...

[johnbakers]

Doesn't the question depend on whether your components are allocated on the stack or the heap?

[Seng Cheong]

@Dr.Kameleon Right, and that's the reason modern software hogs huge amounts of memory and crashes. "If something is worth doing, it's worth doing right."

[Dr.Kameleon]

@JonathonReinhart How's delete applicable to boost::array?

[Jesse Good]

There is no memory to be managed in your class. How many of these MyClass objects are you making?

[Dr.Kameleon]

@JesseGood Around 100 millions per second (lol). True.

[Jan Hudec]

@Dr.Kameleon: I don't think any current memory allocator on current hardware could achieve that speed. Not to mention that the object has at least 336 bytes, so 100 millions are 33.6 GB and that does not fit to memory on common computer today.

[Dr.Kameleon]

@JanHudec On a common computer. Not one with a 64GB memory on it. ;-)

[billz]

@Dr.Kameleon what's value of MAX_OBJS? and do you call getListOfObjects() a lot or just once?

[Dr.Kameleon]

@billz I don't think the answer is relevant (since the code above is an example of what the original code is), but if it matters at all it is somewhere near 12.

[juanchopanza]

And how do you delete the objects in the vector<MyClass*>? BTW std::vector has no operator +=.

[Dr.Kameleon]

@juanchopanza Boost supports it (#include <boost/assign/std/vector.hpp>). After the function has been called, and looped through all of the results, I either tried relooping and deleting or deleting after each iteration, but still keep getting the same error : malloc: *** error for object 0xSOME_MEMORY_ADDRESS: pointer being freed was not allocated.

[Bernhard Barker]

You probably don't need a vector of pointers (use vector<MyClass> instead), just be careful since it's a lot more to copy (pass it by reference to getListOfObjects rather than returning it, which is probably a good idea in either case).

[juanchopanza]

@Dukeling return by value is fine, the copy would get optimised out by named return value optimisation (NRVO). And the code would be cleaner and safer.

[billz]

@Dr.Kameleon after the function is called, you are copying pointers to other vector, if you iterate through it and delete again, which is UB.

[Dr.Kameleon]

@billz The initial post has been re-edited to also show how the function is called and how I'm attempting to delete the objects

[Dr.Kameleon]

Hmmm... I see your point. Perhaps, you're right. Give me a sec to test it out.

[johnbakers]

I assume that you could tell they were dynamically allocated because the class members would be pointers instead, correct?

[Karthik T]

@SebbyJohanns Yes thats what I was using.. let me add the caveat

[Najzero]

this + since boost is already used... maybe using boost.org/doc/libs/1_52_0/libs/smart_ptr/smart_ptr.htm

[juanchopanza]

@SebbyJohanns Pointer data members does not imply dynamic allocation. You have to check whether these pointers are made to point to some memory dynamically allocated with new or new[]. Even then it isn't 100% clear who's responsibility it is to call delete. Best to avoid raw pointers in these cases and use the appropriate smart pointer.

[Karthik T]

@juanchopanza Another check would be that this class would have to own it, so ideally, this class would need to have allocated it.

[Tony Delroy]

"What I see is all allocated on stack" - not true... the members will be embedded where the object is created, which could be static, on the stack or on the heap. The crucial point is that whatever memory management is applied to the object will automatically handle the members... that's where you correctly warn against new Myclass sans release.

[Dr.Kameleon]

OK, I've been trying to see what's going on with the whole thing and this the case : actually, the MyClass objects are created with a new, and then put in a vector. Now, whenever I try deleteing a MyClass object, once I'm done with it, I'm getting a malloc: *** error for object 0xSOME_MEMORY_ADDRESS: pointer being freed was not allocated. Any ideas?

[juanchopanza]

@KarthikT Right, but it isn't something you can check easily from looking at a data member declaration. You need to know it (which you should, if you desinged the class), or have the documentation from the class, or make an educated guess from looking at the code.

[Karthik T]

@Dr.Kameleon you might want to edit the question to show the code where you allocate and deallocate the objects and the definition of the vector, and add these details there

[Karthik T]

@juanchopanza having no pointers is good enough to infer that the destructor is free though.