7 Replies - 384 Views - Last Post: 11 January 2013 - 05:11 PM Rate Topic: -----

#1 Mr_Cloud  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 3
  • Joined: 09-April 12

Memory not being freed as expected when using pointers

Posted 10 January 2013 - 08:37 PM

Hello everyone,

I am trying to write a template class but I get a memory leak. The class is as follows:

template <class T> class Cvector{

    public:
	Cvector(){//pass initial parameters
	    sizeOfStream=0;
	    oldstream=new T;
	    stream=new T;
	}

	void addData(T *dataIn, unsigned int size){//main function of class
	    stream=new T[sizeOfStream+size];
	    for (unsigned int i=0;i<sizeOfStream;i++) stream[i]=oldstream[i];
	    for (unsigned int i=0;i<size;i++) stream[sizeOfStream+i]=dataIn[i];
	    sizeOfStream+=size;
	    oldstream=new T[sizeOfStream];
	    for (unsigned int i=0;i<sizeOfStream;i++) oldstream[i]=stream[i];
	}

	unsigned int size(){//return the size of the array
	    return sizeOfStream;
	}

	void clear(){//flush values
	    delete[] stream;
	    delete[] oldstream;
	    sizeOfStream=0;
	}

	T *data(){//retrieve the array
	    return stream;
	}

    private:
	T *stream;
	T *oldstream;
	unsigned int sizeOfStream;
};



Now, when I do something along the lines of

Cvector<double> derp;
double *hurr=new double[1024];
for (int i=0;i<1024;i++) hurr[i]=i;
for (int i=0;i<1000;i++) derp.addData(hurr,1024);



it crashes because it runs out of RAM.

As you can see, what I am trying to do is make something similar to std::vector, but make it so that it accepts arrays of data, as opposed to the simple push_back(). If I try to call Cvector::clear() when I am done adding data, it doesn't free the memory.

How can I fix this memory leak problem?

Thank you.


Regards,
Mr_Cloud

Is This A Good Question/Topic? 0
  • +

Replies To: Memory not being freed as expected when using pointers

#2 jjl  Icon User is offline

  • Engineer
  • member icon

Reputation: 1072
  • View blog
  • Posts: 4,532
  • Joined: 09-June 09

Re: Memory not being freed as expected when using pointers

Posted 10 January 2013 - 08:48 PM

Quote

it crashes because it runs out of RAM.

You are most definitely not "running out of RAM".

Quote

Cvector::clear() when I am done adding data, it doesn't free the memory.

What have you done to verify this? Did you profile your code via valgrind?

Have you stepped through your code with a debugger?
Was This Post Helpful? 0
  • +
  • -

#3 Mr_Cloud  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 3
  • Joined: 09-April 12

Re: Memory not being freed as expected when using pointers

Posted 10 January 2013 - 09:21 PM

jjl, I checked in Task Manager the Memory tab. It starts hogging about 2-2.5GB of rams before it "terminates in an unusual way".

I am thinking that if I somehow manage to trigger the Cvector destructor, I could make this go away and then just declare another Cvector derp. Because I realise that declaring one double and calling a new instance repeatedly is causing this (as per line 11 of quoted code above), but for what I need, the delete[] call is way too slow (takes seconds to perform). I would need an instantaneous freeing of memory, which I don't know how to implement.

Thoughts?

Can't edit, I meant to say that if I add delete[] in my class, the code at line 4 of the second snippet is what takes seconds to perform.
Was This Post Helpful? 0
  • +
  • -

#4 jjl  Icon User is offline

  • Engineer
  • member icon

Reputation: 1072
  • View blog
  • Posts: 4,532
  • Joined: 09-June 09

Re: Memory not being freed as expected when using pointers

Posted 10 January 2013 - 09:53 PM

Quote

the delete[] call is way too slow (takes seconds to perform).

I find that hard to believe. "new" may be time time consuming (not in the magnitude of seconds however), but deleting the memory simply involves marking it as fair game on the run time heap. If you are finding your delete calls to be overly time consuming, it most likely is due to some time consuming operations with your deconstructor.

The fact that you have two dynamic arrays for a single vector object seems strange. I would implement this with a single array.

i.e.
#define DEF_VEC_CAP 20

template <typename T>
class MyVector {
   T *data;
   int size; //Number of elements currently occupying the vector
   int capacity; // Number of elements the vector can hold

public:
   MyVector(int cap = DEF_VEC_CAP) 
      : data(new T[cap]), size(0), capacity(cap) {
   }

   ~MyVector() {
      delete []data;
   }

   void addData(T *data, int n) {
      //if you cannot fit data into vector,
      //increase vector capacity by 2 times requested capacity
      if(n + size >= capacity) 
         recap(2 * (n + size));

      /* add data to vector */

      size += n;
   }

protected:
   void recap(int new_cap) {
      T *tmp = new T[new_cap]; //create a larger array to hold everything

      for(int i=0; i<capacity; i++)
         tmp[i] = data[i]; //copy old data over to new array

      delete []data; //delete old array
      data = tmp; //set data to the new array
      capacity = new_cap;
   }
};


This post has been edited by jjl: 10 January 2013 - 09:59 PM

Was This Post Helpful? 0
  • +
  • -

#5 DoNotWant  Icon User is offline

  • D.I.C Head

Reputation: 11
  • View blog
  • Posts: 59
  • Joined: 03-November 11

Re: Memory not being freed as expected when using pointers

Posted 10 January 2013 - 10:17 PM

Every time you use addData, you create 2 new objects on the heap. If you at the end of the program use your clear function after adding a lot of stuff, you will only delete the last allocations of new memory.
Someone please correct me here if I'm wrong.

This post has been edited by DoNotWant: 10 January 2013 - 10:22 PM

Was This Post Helpful? 0
  • +
  • -

#6 Skydiver  Icon User is online

  • Code herder
  • member icon

Reputation: 3459
  • View blog
  • Posts: 10,666
  • Joined: 05-May 12

Re: Memory not being freed as expected when using pointers

Posted 11 January 2013 - 07:10 AM

Actually it is possible for him to run out of RAM if he was running in a 16-bit environment. In the OP, his addData() method never frees up any memory and therefore leaks. So consider:
Cvector<double> derp;

double *hurr=new double[1024];
// 8000 bytes: allocate 1000 doubles (assuming IEEE double)
// let's just call this 8KB

for (int i=0;i<1024;i++)
    hurr[i]=i;

for (int i=0;i<1000;i++)
{
    derp.addData(hurr,1024);
}

// iter stream oldStream hurr leaked_memory total_used
// 0    8KB    8KB       8KB  0             24KB
// 1    16KB   16KB      8KB  8KB+8KB       56KB
// 2    24KB   24KB      8KB  16KB+16KB     82KB

// !!! 16-bit C can only access 64KB, so out of RAM.


Was This Post Helpful? 0
  • +
  • -

#7 NickDMax  Icon User is offline

  • Can grep dead trees!
  • member icon

Reputation: 2250
  • View blog
  • Posts: 9,245
  • Joined: 18-February 07

Re: Memory not being freed as expected when using pointers

Posted 11 January 2013 - 11:59 AM

being able to create your own vector class is a really good excersize and I don't want to steer you away from that from an educational point of view, BUT in real-world application programming you would probably want to stick with std::vector unless there were compelling reasons not to.

So your problem seems to be adding an array of values to the vector all at once. This is easy enough to do with vectors:
#include <iostream>
#include <vector>

using namespace std;

static const int ARRAY_SIZE = 1024;

int main() {
    vector<double> derp;
    double *hurr = new double[ARRAY_SIZE]; //Why not just use a vector here rather than an array?
    for (int i=0;i<1024;i++) { hurr[i]=i; }
    for (int i=0;i<1000;i++) { 
        derp.insert(derp.end(), hurr, hurr+ARRAY_SIZE); //add all elements of the array
    } 
    cout << "Size of derp: " << derp.size() << endl;
    
    
    delete[] hurr;
}


so you could make that into a little utility function if you wanted:
#include <iostream>
#include <vector>

using namespace std;

static const int ARRAY_SIZE = 1024;

template<typename CONTAINER_TYPE>
CONTAINER_TYPE& addData(CONTAINER_TYPE& container, typename CONTAINER_TYPE::value_type* array, size_t arraySize) {
    container.insert(container.end(), array, array+arraySize);
    return container;
}

int main() {
    vector<double> derp;
    double *hurr = new double[ARRAY_SIZE]; //Why not just use a vector here rather than an array?
    for (int i=0;i<1024;i++) { hurr[i]=i; }
    for (int i=0;i<1000;i++) { 
        //derp.insert(derp.end(), hurr, hurr+ARRAY_SIZE); //add all elements of the array
        addData(derp, hurr, ARRAY_SIZE);
    } 
    cout << "Size of derp: " << derp.size() << endl;
    
    
    delete[] hurr;
}


And there are several other ways to go about this

Next lets talk about your Cvector class:

First in your constructor:
	    oldstream= new T;
	    stream= new T;
-- Your vector will be managing things of an array type and will need to use delete[] so you CAN NOT assign stream or oldstream and new T, it must be of type T[]. So if there is no data it is best to set these to null (or an initial size) and deal with them later.

Since we know that oldstream and stream will have data we should probably work on our deconstructor, copy constructor, and assignment operator (remember if you need one of these three then you need all three).

so the deconstructor would look like this
	~Cvector() {
        if (oldstream != NULL) {
            delete[] oldstream;
            oldstream = NULL;
        }
        if (stream != null) {
            delete[] stream;
            stream = NULL;
        }
	    sizeOfStream = 0;
	}{/code]

and for now it is fine to just create a private copy constructor and assignment operator with nothing in them (this disables clients from using those functions and will result in wonderful cryptic error messages when someone tries):[code]    Cvector(const Cvector& other) { } // do not allow copy
    Cvector& operator= (const Cvector &other) { }


Ok -- now we are ready to add data! I don't really understand your addData function so I am just going to do it my own way that makes sense to me and you can tell me what you think.


So the first error in your code I see is this:

stream=new T[sizeOfStream+size]; -- you didn't save or delete the old array from stream -- memory leak right there!

that should probably look something like:
T* temp = stream;
stream = new T[sizeOfStream+size];


So I rewrote your function as:
	void addData(T *dataIn, unsigned int size){//main function of class
        //first create a temp to hold the new data;
        T* temp = stream;

        
	    stream=new T[sizeOfStream+size];
        
        //note that if temp = null then this is std::copy(null, null, stream) -- and should result in no copy so
	    T* insertPosition = std::copy(temp, temp+sizeOfStream, stream);
        std::copy(dataIn, dataIn+size, insertPosition);
        //the above two lines could be written as:
        //std::copy(dataIn, dataIn+size,
        //    std::copy(temp, temp+sizeOfStream, stream));
        // which avoids the use of the var insertPosition but is less clear. 
        
	    //update our size
        sizeOfStream += size;
        
        //Release the old temp
        if (temp != NULL) {
            delete[] temp;
        }
	}


we now want clear to work like our deconstructor:
	void clear(){
        if (oldstream != NULL) {
            delete[] oldstream;
            oldstream = NULL;
        }
        if (stream != null) {
            delete[] stream;
            stream = NULL;
        }
	    sizeOfStream = 0;
	}


the data() function works the same I guess so we have:
#include <iostream>
#include <algorithm> //std::copy

template <class T> class Cvector {
    typedef T value_type;

    Cvector(const Cvector& other) { } // do not allow copy
    Cvector& operator= (const Cvector &other) { }
    
    
    public:
	Cvector() { //pass initial parameters
	    sizeOfStream=0;
	    oldstream= NULL;
	    stream= NULL;
	}
    
    
	~Cvector() {
        if (oldstream != NULL) {
            delete[] oldstream;
            oldstream = NULL;
        }
        if (stream != NULL) {
            delete[] stream;
            stream = NULL;
        }
	    sizeOfStream = 0;
	}
    
    

	void addData(T *dataIn, unsigned int size){//main function of class
        //first create a temp to hold the new data;
        T* temp = stream;

        
	    stream=new T[sizeOfStream+size];
        
        //note that if temp = null then this is std::copy(null, null, stream) -- and should result in no copy so
	    T* insertPosition = std::copy(temp, temp+sizeOfStream, stream);
        std::copy(dataIn, dataIn+size, insertPosition);
        //the above two lines could be written as:
        //std::copy(dataIn, dataIn+size,
        //    std::copy(temp, temp+sizeOfStream, stream));
        // which avoids the use of the var insertPosition but is less clear. However this is how you are likely to see this.
        
	    //update our size
        sizeOfStream += size;
        
        //Release the old temp
        if (temp != NULL) {
            delete[] temp;
        }
	}

	unsigned int size(){//return the size of the array
	    return sizeOfStream;
	}

	void clear(){
        if (oldstream != NULL) {
            delete[] oldstream;
            oldstream = NULL;
        }
        if (stream != NULL) {
            delete[] stream;
            stream = NULL;
        }
	    sizeOfStream = 0;
	}

	T *data(){//retrieve the array
	    return stream;
	}

    private:
	T *stream;
	T *oldstream;
	unsigned int sizeOfStream;
};

using namespace std;

static const int ARRAY_SIZE = 1024;

int main() {
    Cvector<double> derp;
    double *hurr = new double[ARRAY_SIZE]; //Why not just use a vector here rather than an array?
    for (int i=0;i<1024;i++) { hurr[i]=i; }
    for (int i=0;i<1000;i++) { 
        derp.addData(hurr, ARRAY_SIZE);
    } 
    cout << "Size of derp: " << derp.size() << endl;
    
    
    delete[] hurr;
    return 0;
}


I didn't end up using oldstream at all, I suspect that I used temp where your thinking was to use oldstream.

Now this is MUCH MUCH MUCH slower than vector! So the question is why?

Well vectors don't allocate "just the memory needed" -- they buffer ahead and allocate more memory than is needed, they also grow by a factor of roughly 2 every time they reallocate memory. So this might be something that we could implement to look for a boost in performance.
#include <iostream>
#include <algorithm> //std::copy

template <class T> class Cvector {
    typedef T value_type;

    Cvector(const Cvector& other) { } // do not allow copy
    Cvector& operator= (const Cvector &other) { }
    
    
    public:
	Cvector() { //pass initial parameters
	    sizeOfStream = 0;
        sizeOfData = 0;
	    stream = NULL;
	}
    
    
	~Cvector() {
        if (stream != NULL) {
            delete[] stream;
            stream = NULL;
        }
	    sizeOfStream = 0;
        sizeOfData = 0;
	}
    
    

	void addData(T *dataIn, unsigned int size){//main function of class
        // first determine if we need to grow in size:
        if (sizeOfData+size <= sizeOfStream) { // no need to grow we can fit with what we have!
            std::copy(dataIn, dataIn+size, stream + sizeOfData);
            sizeOfData += size;
            return; //done
        }
        //ok so we need to determine the size we need, if we can double our size we will do that:
        size_t newSize = sizeOfStream * 2;
        if (sizeOfData + size > newSize) {
            newSize = sizeOfData + size;
        }       
        T* temp = stream;
	    stream=new T[newSize];
        
        //note that if temp = null then this is std::copy(null, null, stream) -- and should result in no copy so
	    T* insertPosition = std::copy(temp, temp+sizeOfStream, stream);
        std::copy(dataIn, dataIn+size, insertPosition);
        //the above two lines could be written as:
        //std::copy(dataIn, dataIn+size,
        //    std::copy(temp, temp+sizeOfStream, stream));
        // which avoids the use of the var insertPosition but is less clear. However this is how you are likely to see this.
        
	    //update our size
        sizeOfData += size;
        sizeOfStream = newSize;
        
        //Release the old temp
        if (temp != NULL) {
            delete[] temp;
        }
	}

	unsigned int size(){//return the size of the array
	    return sizeOfData;
	}

	void clear(){
        if (oldstream != NULL) {
            delete[] oldstream;
            oldstream = NULL;
        }
        if (stream != NULL) {
            delete[] stream;
            stream = NULL;
        }
	    sizeOfStream = 0;
	}

	T *data(){//retrieve the array
	    return stream;
	}

    private:
	T* stream;
	size_t sizeOfStream;
    size_t sizeOfData;
};

using namespace std;

static const int ARRAY_SIZE = 1024;

int main() {
    Cvector<double> derp;
    double *hurr = new double[ARRAY_SIZE]; //Why not just use a vector here rather than an array?
    for (int i=0;i<1024;i++) { hurr[i]=i; }
    for (int i=0;i<1000;i++) { 
        derp.addData(hurr, ARRAY_SIZE);
    } 
    cout << "Size of derp: " << derp.size() << endl;
    
    
    delete[] hurr;
    return 0;
}


This preforms much the same as the vector class in terms of time. It is still not a perfect vector and there are some very good articles on how the std::vector class works so you might want to do a little deeper research.

This post has been edited by NickDMax: 11 January 2013 - 12:00 PM

Was This Post Helpful? 1
  • +
  • -

#8 Mr_Cloud  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 3
  • Joined: 09-April 12

Re: Memory not being freed as expected when using pointers

Posted 11 January 2013 - 05:11 PM

Thank you so much for the throrough explanation, NickDMax! That was really really helpful and you helped me find the answer I was looking for.

And thank you to everyone else that replied in this thread, I appreciate you taking the time to respond.


Regards,
Mr_Cloud
Was This Post Helpful? 0
  • +
  • -

Page 1 of 1