Sign in to follow this  
silverphyre673

Huge memory leak issues.

Recommended Posts

I have a vector of a vector of pointers to a "TerrainCell" class as part of another "Mesh" class. This Mesh class basically holds the data for the terrain vertices and such for a chunk of a level. It has two major functions - InitializeMemory() and DeinitializeMemory(). It also has three main components - a width and height, and "mesh", which is: std::vector<std::vector<TerrainCell*> > mesh; I don't want to have to use up all the stack space. Anyways, "mesh" is supposed to have flat edges - the inner vectors should all have the same Size(). DeinitializeMemory() is supposed to delete all the pointers in mesh, and InitializeMemory() first calls DeinitializeMemory, then allocates new memory according to the width and height specified. Unfortunately, I made a simple memory-leak check, and it seems that the memory never gets deallocated. I was wondering how I am supposed to deallocate the memory in the vector successfully. Here is the relevant code: Mesh class declaration
//Declaration of TerrainCell (just a non-pointer array of 4 3d vertices)
#include "cell.h"

class Mesh
{
    public:
    Mesh();
    Mesh(unsigned int init_width, unsigned int init_height);
    ~Mesh();
    
    //Allocates memory for "mesh" according to "width" and "height"
    void InitializeMemory();
    //Deallocates all the memory in "mesh"
    void DeinitializeMemory();

    private:
    //Terrain vertices
    std::vector< std::vector<TerrainCell*> > mesh;
    //height: the number of outer vectors for "mesh"
    //width: the size of the inner vectors of "mesh"
    unsigned int height, width;
};


InitializeMemory()
void Mesh::InitializeMemory()
{
    DeinitializeMemory();
    unsigned int i = 0;
    for ( unsigned int out_count = 0; out_count < height; ++out_count )
    {
        std::vector<TerrainCell*> v_cells;
        for ( unsigned int in_count = 0; in_count < width; ++in_count )
        {
            TerrainCell * t_cell = new TerrainCell;
            v_cells.push_back(t_cell);
            ++i;
            //Thought the lack of "delete t_cell" might be the cause of the
            //memory leak.  Leaving it in causes a double free error.
            //delete t_cell;
        }
        mesh.push_back(v_cells);
    }
}


DeinitializeMemory()
void Mesh::DeinitializeMemory()
{
    //For each vector of vectors of TerrainCells
    for ( std::vector<std::vector<TerrainCell*> >::iterator iter_out = mesh.begin();
          iter_out != mesh.end();
          ++iter_out )
    {
        //Cycle through each cell in the vector
        for ( std::vector<TerrainCell*>::iterator iter_in = (*iter_out).begin();
              iter_in != (*iter_out).end();
              ++iter_in )
        {
            //And delete it... apparently unsuccessfully :(
            delete (*iter_in);
        }
        //Clear the row (neccessary?)
        (*iter_out).clear();
    }
    //Clear the mesh
    mesh.clear();
}


Obviously, in a terrain editor, this is a very big problem :) Thanks for any help you can provide. [Edited by - silverphyre673 on June 5, 2005 8:45:28 PM]

Share this post


Link to post
Share on other sites
I doubt there is actually that much memory at stake, to be honest. The vector holds onto the memory in case you want to use it again. If you will re-use the vector, then this probably isn't a problem. If you won't re-use the vector, just destroy it and again you should be ok.

But if you insist on keeping the vector but losing the memory, I believe that this function will work:
template < class T >
void shrinkCapacity(std::vector< T > & v) {
std::vector< T > tmp(v); // copy elements
v.swap(tmp); // then swap the refs
}


Call it on every vector you need 'shrinking'.

Share this post


Link to post
Share on other sites
Hmmm... I have been thinking, and I actually don't need the Mesh to be resizable. Thanks though. Basically the goal was to create a mesh of these TerrainCells that would represent one chunk of a level. If we moved to another chunk, then we would destroy the old chunk's data (which was originally loaded from a level info file) and then give it new memory. Thinking on it now, the way I had it was a very stupid way to do it, since each chunk would be the same size, and I was just going to fill in the data cell-by-cell as I loaded it.

Thanks, though!

Share this post


Link to post
Share on other sites
actually, I changed it, and it is now giving me "invalid pointer" errors at runtime on my linux box, where I am testing the memory leaks, and is, I believe, using up a bunch of system resources on the windows box because it gets progressively slower. Here is the revised code (the declaration is now free of DeinitializeMemory and InitializeMemory, and the height and width - its now just a constructor, destructor and "mesh", and the memory allocation is done in the constructor and destructor.

This is the new "mesh":

TerrainCell ** mesh;


Mesh::Mesh()
{
mesh = new TerrainCell * [MESH_HEIGHT];
for ( unsigned int i = 0; i < MESH_HEIGHT; ++i)
mesh[i] = new TerrainCell;
}

Mesh::~Mesh()
{
for ( unsigned int i = 0; i < MESH_HEIGHT; ++i )
{
delete [] mesh[i];
}
delete mesh;
}



Thanks again.

Share this post


Link to post
Share on other sites
Quote:
actually, I changed it, and it is now giving me "invalid pointer" errors at runtime on my linux box, where I am testing the memory leaks.


Then that means you are doing Bad Things™ with the memory, writing where you shouldn't be writing, clobbering data structures that you shouldn't be clobbering, overwriting pointers with non-address data.

That's might be what caused the problems before: you might be doing Bad Things™ with the outer vector and be corrupting the inner vectors, which then don't end up releasing anything.

Or it could be something else entirely. This is mere speculation. Your mileage may vary. Pay no attention to the programmer behind the keyboard. Based on a 1125 calorie diet.

Share this post


Link to post
Share on other sites
actually, all I am doing ( and the operation gives successful results,although they may be the Bad Things you speak of ) is, in the test main function that gets called like 100 times during leak testing is creating a new TerrainCell on the stack, setting its parameters, and using two functions that I hadn't mentioned to put it into the mesh and retrieve it. Here they are:


bool Mesh::SetCell( unsigned int x, unsigned int y, TerrainCell new_cell)
{
if ( x < MESH_WIDTH && y < MESH_HEIGHT )
{
mesh[x][y] = new_cell;
return true;
}
return false;

}

bool Mesh::GetCell( unsigned int x, unsigned int y, TerrainCell & get_cell)const
{
if ( x < MESH_WIDTH && y < MESH_HEIGHT )
{
get_cell = mesh[x][y];
return true;
}
return false;
}



And they are called here:

int mymain(int argc, char *argv[])
{
Mesh terrain_mesh;
TerrainCell new_cell;

for ( unsigned int i = 0; i < 3; ++i )
{
new_cell.position[i].x = rand()%10;
new_cell.position[i].y = rand()%10;
new_cell.position[i].z = rand()%10;
}
if ( terrain_mesh.SetCell(9, 4, new_cell) && terrain_mesh.GetCell(9, 4, new_cell) )
{
std::cout << "Finished successfuly\n";
}
else
std::cout << "Failed!\n";

return 1;
}

Share this post


Link to post
Share on other sites
Your access functions do not use the same width/height order as your constructor and destructor.

Constructor: M[HEIGHT][WIDTH]
Access: M[WIDTH][HEIGHT]

That's a Bad Thing™

Share this post


Link to post
Share on other sites
shouldn't matter, although thanks - since the width and height are the same. I think the problem is that I am making it get a pointer to an object on the stack,so it gets deleted twice - once at the end of the function automatically, and once through delete in the destructor of the Mesh. Let me test it.

[EDIT]Nope. :( Removing the body of mymain didn't affect the program, either. I think its either the constructor or destructor of Mesh.


int mymain(int argc, char *argv[])
{
Mesh terrain_mesh;
std::cout << "Done\n";
return EXIT_SUCCESS;
}



Share this post


Link to post
Share on other sites
Quote:
Original post by Fruny
delete[] mesh;


Well, I fixed that particular issue, but I think that the Get/Set routines aren't causing the problem - I'm pretty sure its in the contructor and destructor, since removing anything about Get/Set TerrainCell didn't affect the programs performance - allocating 30,000 doubles (1 Mesh of size 100x100) didn't affect performance visibly, but after creating and, in theory, destroying around 17 of them, my system came to almost a halt, and that is where I wound up using up all the system resources. I'm 100% certain its the destructor's fault.

Share this post


Link to post
Share on other sites
Quote:
Original post by Fruny
delete[] mesh;


You mean like this? Tried it, and it didn't change anything, unfortunately


Mesh::~Mesh()
{
for ( unsigned int i = 0; i < MESH_HEIGHT; ++i )
{
delete[] mesh[i];
}
delete[] mesh;
}

Share this post


Link to post
Share on other sites
OK, I can't find it either, nobody. Actually, thanks alot for the help, Fruny. I tried something interesting: increased the size of MESH_WIDTH and MESH_HEIGHT to 1000 (basically made the size of a Mesh ten times bigger). And guess what: it ran exactly the same as when it was 1/10th the size, petering out after 17 destructor calls. So I guess the destructor is working. And after 17 tries, it slows down considerably, but it doesn't stop. I think if I left it running long enough, it would eventually finish. Anyone ever experianced this?

[EDIT] nevermind - instead it crashed after creating two Meshes, as I would have expected if the Mesh wasn't being created - having 1,000,000 must be no fun for the computer. I think I accidentally didn't do a full build, just compiled the wrong files. Any help, guys?

Share this post


Link to post
Share on other sites
Quote:
Original post by silverphyre673
Quote:
Original post by Fruny
delete[] mesh;


You mean like this? Tried it, and it didn't change anything, unfortunately

*** Source Snippet Removed ***

no, like this:


Mesh::~Mesh()
{
for ( unsigned int i = 0; i < MESH_HEIGHT; ++i )
{
delete mesh[i]; // delete single
}
delete[] mesh; // delete array
}




why obfuscate things, I avoid nD dynamic arrays whenever I can
(it usually saves me time somewhere else *cough*debugging*cough*)
you can change it over when you're sure the code works.

struct MeshPointer {
Mesh *p;
};

MeshPointer *meshes;

for each M in meshes
delete M.p;
delete[] meshes;

Share this post


Link to post
Share on other sites
Is there any particular reason why your storing pointers to TerrainCell instead of TerrainCell, which you have done in both cases with std::vector & C-style dynamic array.

Do you know std::vector separates allocation/deallocation & construction/destruction, so you can do things like allocate a chunk of uninitialized memory and then at a later time incrementally initialize each element (you can do this with C-style dynamic arrays but its low-level code)

Share this post


Link to post
Share on other sites
Quote:
Original post by silvermace
Quote:
Original post by silverphyre673
Quote:
Original post by Fruny
delete[] mesh;


You mean like this? Tried it, and it didn't change anything, unfortunately

*** Source Snippet Removed ***

no, like this:

*** Source Snippet Removed ***

why obfuscate things, I avoid nD dynamic arrays whenever I can
(it usually saves me time somewhere else *cough*debugging*cough*)
you can change it over when you're sure the code works.

struct MeshPointer {
Mesh *p;
};

MeshPointer *meshes;

for each M in meshes
delete M.p;
delete[] meshes;


I think I'll try this at home. I hate multidimentional arrays too :)
Not sure if it'll work, and I'm still not sure why the program is using all the resources, but I'll give it a shot.

Jingo: it's not crashing, just using all the system resources (memory).

snk_kid: I want to keep the memory off the stack to increase the amount of memory I can utilize. Tell me if this is false thinking.


Share this post


Link to post
Share on other sites
Quote:
Original post by silverphyre673
snk_kid: I want to keep the memory off the stack to increase the amount of memory I can utilize. Tell me if this is false thinking.


It is. The vector already "keeps the memory off the stack"; the members of vector<T> look something like


int capacity; // how much storage space is allocated
int size; // how much of it actually corresponds to valid elements
// possibly a couple other little things I forgot
T* storage; // since it's variable, it would be kind of hard to put on the stack!

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this