Copying memory, pointers and the like.

I've come accross something in my program that has me curious.

Suppose I have this scenerio:

Code:

class CLASS_1{
    public:
    DataClass *data;

    CLASS_1(){
        data = new DataClass();
    }
    ~CLASS_1(){
        delete data;
        data = NULL;
    }
};

class CLASS_2{
    public:
    DataClass *data;
    CLASS_1 *class1;

    CLASS_2(){
        this->data = new DataClass();

        class1 = new CLASS_1();
        class1->data = this->data;
    }
    ~CLASS_2(){
        delete data;
        data = NULL;

        delete class1;
        class1 = NULL;
    }
};

Class 2 creates an instance of class 1. class 1 creates and manages an instance of DataClass, as does class 2. When class2 is created, class1->data is set to be a copy of class2->data;

So, my understanding is that there will be memory allocated for CLASS_1, CLASS_2, and two instances of DataClass contained in these classes. Both instances of DataClass will be the same.

Now, if I delete class1 before class2's deconstructor, is it's memory freed? In my program, class2 would be the main window, and class1 would be a child window. So, if I close the child window before the main window, I still want to free it's resources. DataClass of each window is made the same, so that the main window can keep a "master copy", and pass it to each child window as necessary.

What I've noticed though, by monitoring the applications memory usage in the task manager, is that when I close the child window, the applications memory usage doesn't go down. What i'm wondering then, is if the memory isn't being freed like I assumed it should be, perhaps due to the copying (the data is made equal at other points at runtime too, not just at initialization)?

Originally Posted by psychopath

Class 2 creates an instance of class 1. class 1 creates and manages an instance of DataClass, as does class 2. When class2 is created, class1->data is set to be a copy of class2->data;

Sort of. class1->data is a pointer that is set equal to class2->data. The two pointers are actually the same value, which means they are the address of the same memory. The assignment does not copy the memory blocks.

Originally Posted by psychopath

So, my understanding is that there will be memory allocated for CLASS_1, CLASS_2, and two instances of DataClass contained in these classes. Both instances of DataClass will be the same.

The memory is allocated, yes. But the assignment "class1->data = this->data" loses any reference to the memory that was originally stored in class1->data. Your code does not store that original address anywhere else, so that original memory is leaked (it is not cleaned up, and you have no reference to it so cannot access it or clean it up).

Originally Posted by psychopath

Now, if I delete class1 before class2's deconstructor, is it's memory freed?

Yes it will. Problem is, class1->data contains the same address as class2->data. So class1's destructor (deconstructor is the wrong word) will delete class2->data. class2->data then becomes a dangling reference: it holds the same address as it before class1's destructor was invoked, but the memory at that address no longer exists (as far as your code is concerned). Any attempt to dereference class2->data (including deleting it) yields undefined behaviour.

Originally Posted by psychopath

What I've noticed though, by monitoring the applications memory usage in the task manager, is that when I close the child window, the applications memory usage doesn't go down. What i'm wondering then, is if the memory isn't being freed like I assumed it should be, perhaps due to the copying (the data is made equal at other points at runtime too, not just at initialization)?

Your code has a problem that contributes to this; the original memory, allocated by CLASS1's constructor is leaked, and will never be released. CLASS2's destructor actually deletes its own data twice (once in the destructor of CLASS1, and once via "delete data"). That is undefined behaviour, and may cause intermittent (or, if you're lucky, continual) crashes of your program.

Even if you fix that problem, you will not necessarily see memory usage go down when you delete an object. The reason is that memory management schemes used by compilers and libraries (eg implementation of operators new and delete) are often smarter than that. The act of obtaining memory from the operating system (or physically from the machine) is relatively expensive. So, implementation of dynamic memory allocation schemes often use a cache internally: when operator delete is called, the memory is released as far as your code is concerned, but not released back to the operating system. On the next usage of operator new might obtain memory from the internal cache, negating the need to do more expensive operations such as asking the operating system to provide memory. This is a performance optimisation for your program: a measurable effect is that speed of your program increases. Another observable side effect is that memory allocation for your program does not reduce when it invokes operator delete (and may not increase on subsequent usages of operator new).

OK, thanks, that all makes sense.

Now, what if I use something like memcpy() to set the data, rather than the operator assignment.

Code:

memcpy(class1->data, this->data, sizeof(DataClass));

rather than;

Code:

class1->data = this->data;

Would that fix the memory leak? Since memcpy() copies the memory block, not the address (I think :/)

I don't see any memory leak in that code.

Class_1's data data member is a separate instance as you can see from the Class_1 constructor. When Class_1 destructor is ran, that instance gets deleted. Class_2 remains valid and its class1 data member is deallocated.

Now... how are you deleting class1? I'm assuming from within some Class_2 method, since this is the class that manages it... When you deallocate this pointer (that was created through Class_2 constructor) you should also set it to NULL. The problem being that if your Class_2 destructor is ran with class1 deallocated you will be trying to delete the pointer twice and bang! Run-Time error.

Originally Posted by Mario F.

I don't see any memory leak in that code.

You're blind Look at CLASS_2's constructor.

Code:

   CLASS_2(){
        this->data = new DataClass();

        class1 = new CLASS_1();
        class1->data = this->data;
    }

CLASS_1's constructor initialises it's data member using operator new, and then CLASS_2's constructor reassigns class1->data to something else. The original dynamically allocated memory pointed to by class1->data is therefore leaked.

>> Would that fix the memory leak? Since memcpy() copies the memory block, not the address (I think :/)

Yes. And it would fix the undefined behavior of deleting the memory twice. Unfortunately, it is still a bad idea, since you should not be copying a class with memcpy. Only if DataClass is a POD type could that work, although it still wouldn't be as great idea.

A better solution would be:

Code:

*(class1->data) = *(this->data);

This assumes DataClass has a valid copy assignment operator, but it should if it is a well designed class. A well designed class might also disable copying, in which case you could not do that.

The only other option I can think of is to replace the raw pointers with smart pointers. For example, if the data pointers in both classes were std::tr1::shared_ptr's, I believe there would be no memory leak, and the pointers would point to the same memory location.

> You're blind Look at CLASS_2's constructor.[...] CLASS_1's constructor initialises it's data member using operator new, and then CLASS_2's constructor reassigns class1->data to something else.

Doh! You are right.

Awesome, thanks guys!

I've got it cleaned up now, and everything seems to be fine. I made note of my available memory before launching the app, and it returned to those numbers after closing it. So I guess it's not leaking memory. Seems a bit faster in the areas where it copies the data, but that could be my imagination.

Still a couple of questions though, regarding some things Daved said.

Originally Posted by Daved

This assumes DataClass has a valid copy assignment operator

I'm just using the default copy assignment operator that the compiler defines. Is this OK, or should I write my own?

Originally Posted by Daved

The only other option I can think of is to replace the raw pointers with smart pointers. For example, if the data pointers in both classes were std::tr1::shared_ptr's, I believe there would be no memory leak, and the pointers would point to the same memory location.

Would this be any better (faster, more efficient or reliable) than the other method [*(ptr) = *(ptr)]?

Originally Posted by psychopath

I'm just using the default copy assignment operator that the compiler defines. Is this OK, or should I write my own?

Depends what your class is doing. The rule is that, if your class directly holds a handle of some resource (eg a mutex, a file handle, or a pointer to dynamically allocated memory) and the destructor has to free up that resource, then you will need to roll your own copy assignment operator.

The reason I underlined directly in the above is that the rule is not recursive. For example, a std::string has it's own hand-rolled copy assignment operator. But, because std::string does that, a class which contains a std::string does not have to worry about freeing up the memory used internally be a std::string.

Originally Posted by psychopath

Would this be any better (faster, more efficient or reliable) than the other method [*(ptr) = *(ptr)]?

Use of a shared_ptr involves some trade-offs. In terms of overhead, it will be slower than a "*a = *b" approach but will be safer depending on the nature of the objects being played with. The basic rule is that you pay a price for safety, unless you take steps to ensure unsafe conditions are never reached: if you can identify cases where you don't need that safety (eg your code avoids doing something unsafe) then you can reduce the need for safety nets like shared_ptr.

Note that there is a real semantic difference between use of a shared_ptr and actually making a copy of the object. Which one you use should be determined by the behavior which you need, not by performance or reliability.

In your original code, you were copying the pointer. Using the shared_ptr is like that. There is only one instance of the object, and that data is shared between the pointers that point to it. If you change class1->data, that change can be seen from class2->data because they both point to the same object.

On the other hand, if you make a copy with *(ptr) = *(ptr), then the two objects are different. There are two instances, one for class1 and one for class2. Immediatly after the copy they have the same values, but when you change one, the other will not be changed.

So choose between the options based on whether you want two separate copies or you want both pointers pointing to a single instance.

OK, I don't want to actual copies, I want a single instance with multiple pointers to it. AFAIK, I cleared up the memory leak, by removing the 'new' and 'delete' calls for data in class1. Everything loads up fine, and it seems to work. class2 creates a new instance of DataClass, some functions are called from that instance, and a few variables set, then the address is copied to class1. However, if I try to call functions from class2's instance of DataClass at runtime, I get errors like "Attempted to read or write protected memory. This is often an indication that other memory is corrupt." The debugger shows that values that are trying to be accessed by the function are invalid.

EDIT: the same thing worked when I was copying memory, and not the address. but I don't see how the function calls should be affected by this anyway.

What have I done now?

Why don't you try something like this?

Code:

class CLASS_1{
    public:
    DataClass *data;

    CLASS_1(){
        data = NULL;
    }
    ~CLASS_1(){
        delete data;
        data = NULL;
    }
};

class CLASS_2{
    public:
    DataClass *data;
    CLASS_1 *class1;

    CLASS_2(){
        this->data = new DataClass();

        class1 = new CLASS_1();
        class1->data = this->data;
    }
    ~CLASS_2(){
        delete data;
        data = NULL;

        delete class1;
        class1 = NULL;
    }
};

If you leave that line unchanged and allocate a new DataClass in CLASS_1's constructor, then use it instead of allocating a new one in CLASS_2's constructor.

Code:

class CLASS_1{
    public:
    DataClass *data;

    CLASS_1(){
        data = new DataClass();
    }
    ~CLASS_1(){
        delete data;
        data = NULL;
    }
};

class CLASS_2{
    public:
    DataClass *data;
    CLASS_1 *class1;

    CLASS_2(){
        this->data = new DataClass();

        //class1 = new CLASS_1();
        this->data = class1->data;
    }
    ~CLASS_2(){
        delete data;
        data = NULL;

        delete class1;
        class1 = NULL;
    }
};

In short, to fix your problem, you have to allocate only one DataClass.

Is that what you tried?

class2 has to copy to class1, unfortunately. Not calling "class1 = new CLASS_1()" on CLASS_2's constructor is out of the question too.

EDIT:
What I did try is this (which causes the runtime errors):

Code:

class CLASS_1{
    public:
    DataClass *data;

    CLASS_1(){
        //data = new DataClass();
    }
    ~CLASS_1(){
        //delete data;
        //data = NULL;
    }
};

class CLASS_2{
    public:
    DataClass *data;
    CLASS_1 *class1;

    CLASS_2(){
        this->data = new DataClass();

        class1 = new CLASS_1();
        class1->data = this->data;
    }
    ~CLASS_2(){
        delete data;
        data = NULL;

        delete class1;
        class1 = NULL;
    }

    void somefunction(){
        data->subclass->vector[data->var].function(); //ERROR
        class1->data = this->data;
    }
};

>> data->subclass->vector[data->var].function(); //ERROR
That error is due to something else in your code.

The way you implemented CLASS_1 looks correct. It does not own the pointer, so it never allocates or deallocates memory for it. You should initialize it to null and check for null before you use it, though, for safety.

Basically what you're trying is this, which works, so as Daved said the problem must lie elsewhere.

Code:

int *p = new int, *q = p;
*p = 3;
std::cout << *q;
delete p;