My memory is not free

Question

I have a pretty large program that usually works wonderfully, but uses crazy amounts of memory to run. This is a machine learning method that collects a lot of data, and it’s usually normal, but the memory grows and grows very fast even after collecting all the data, so I used the valgrind array to find out what is going wrong. The top of the array of the array of the array is as follows:

99.52% (60,066,179B) (heap allocation functions) malloc/new/new[], --alloc-fns, etc. ->43.50% (26,256,000B) 0x439785: Image::Image(SDL_Surface*) (Image.cpp:95) | ->43.50% (26,256,000B) 0x437277: EncodedFeature::forwardPass() (EncodedFeature.cpp:65) .... 

So, I thought, hmm, maybe the constructed image is not free, but no:

 void EncodedFeature::forwardPass() { // Get image: Image* img = new Image(screen); // Preprocess: if(preprocessor) preprocessor->process(img); // Do forward pass: encoder->encode(img, features); delete img; } 

So, for the Image constructor:

 Image::Image(SDL_Surface* surface) { this->width = surface->w; this->height = surface->h; pixels = new int*[width]; for(int i = 0; i < width; i++) pixels[i] = new int[height]; for(int x = 0; x < surface->w; x++) for(int y = 0; y < surface->h; y++) pixels[x][y] = getPixelFromSDLSurface(surface, x, y); } 

Just allocates an array of pixels, which is later freed up in the destructor:

 Image::~Image() { if(!pixels) return; for(int x = 0 ; x < width; x++) delete[] pixels[x]; delete[] pixels; } 

So, the last culprit:

 Uint32 Image::getPixelFromSDLSurface(SDL_Surface *surface, int x, int y) { if(!surface) return 0; // Got this method from http://www.libsdl.org/cgi/docwiki.fcg/Pixel_Access int bpp = surface->format->BytesPerPixel; /* Here p is the address to the pixel we want to retrieve */ Uint8 *p = (Uint8 *)surface->pixels + y * surface->pitch + x * bpp; switch(bpp) { case 1: return *p; break; case 2: return *(Uint16 *)p; break; case 3: if(SDL_BYTEORDER == SDL_BIG_ENDIAN) return p[0] << 16 | p[1] << 8 | p[2]; else return p[0] | p[1] << 8 | p[2] << 16; break; case 4: return *(Uint32 *)p; break; default: return 0; /* shouldn't happen, but avoids warnings */ } } 

As mentioned in the commentary, I got this version from the wiki of the SDL file, so I hope that there is nothing there. bpp in my case is actually always 1, so it just returns an int at the address p, which doesn’t sound unpleasant to me.

I am at the end of my mind. Can anyone think about where the memory went? I mean, massive dots specifically for the Image constructor, but I don't see anything bad there ...

Thanks for looking at my problem!

Max

---

Responding to your comments:

You are right, I do not need img as a pointer. I come from a Java background, so I just want everything to be a pointer :) Changed but didn't help.

Line 95 is inside the first loop of the constructor loop: pixels[i] = new int[height];

In one of the preprocessors, I resize the image, but I call this reset function, which should make sure that the old array is deleted:

 void Image::reset(int width, int height) { if(pixels) { // Delete old image: for(int x = 0 ; x < width; x++) delete[] pixels[x]; delete[] pixels; } this->width = width; this->height = height; pixels = new int*[width]; for(int i = 0; i < width; i++) pixels[i] = new int[height]; } 

After that I replenish the pixel values ​​...

No exceptions are thrown anywhere.

Where do you suggest using smart pointers?

Thanks for all the answers!

Answer 1

I do not think that you correctly represent pixels in your class of images. I think you can use a simple 1-dimensional vector correct unsigned type ( uint32_t ?).

(in the title):

 class Image { protected: std::vector<uint32_t> pixels; ... }; 

(in the implementation file)

 size_t Image::offset(unsigned x, unsigned y) { return (y * width) + x; } Image::Image(const SDL_Surface* surface) { width = surface->w; height = surface->h; pixels.reserve(width * height); for(unsigned x = 0; x < width; x++) for(unsigned y = 0; y < height; y++) pixels[offset(x, y)] = getPixelFromSDLSurface(surface, x, y); } 

Then your destructor will be completely empty, since it does not need to do anything:

 Image::~Image() { } 

Note that you need to use the offset() method to get the correct index in vector for any x / y pair.

Answer 2

The most likely scenario is that somehow your reset or destructor is not called in some code path, this memory leak.

Fortunately, C ++ has a built-in ability to prevent such leaks: it is called vector .

So, first you create pixels as follows:

 typedef std::vector<int> PixelCol; typedef std::vector<PixelCol> Pixels; Pixels pixels; 

Now we put it in the constructor:

 Image::Image(SDL_Surface* surface) : pixels(surface->w, PixelCol(surface->h)) { this->width = surface->w; this->height = surface->h; for(int x = 0; x < surface->w; x++) for(int y = 0; y < surface->h; y++) pixels[x][y] = getPixelFromSDLSurface(surface, x, y); } 

Finally, we update reset :

 void Image::reset(int width, int height) { Pixels(width, PixelCol(height)).swap(pixels); } 

By letting the language manage your memory, you completely eliminate any such memory management error in your code.