I have a lot of code so I am going to try to do this with as little as possible to show you guys.
I am writing a program that is leaking memory, my efforts of cleaning up memory is causing my program to crash (only in Visual Studio, not using MinGw). I am using Visual Studio 2015 to debug my code, and see how much memory I am using. However, when adding the delete
keyword to try to free up some memory Visual Studio triggers a breakpont. When following the breakpoint to try and figure out whats wrong, VS takes me to a page that says 'No Source Available'.
Compiling this same code with MinGw gcc works find and executes fine, however I need Visual Studio's debugger so I can see my memory usage so that I can determine if the leak is fixed or not.
I am creating a lot of objects dynamically and re assigning new objects to them, I need help figuring out how to delete the old memory so I can only keep in memory the newly created object.
Here is the code that I am concerned with
StateNode *initState = nullptr; // Pointer to the initial state
StateNode *finishState = nullptr; // Pointer to the final state
bool finished = false; // Flag for checking if the puzzle has completed
size = getNumQueens();
// Make dynamic 2D array of the specified size
char** init = new char*[size];
for (int i = 0; i < size; i++)
init[i] = new char[size];
// Puzzle main loop
while (!finished)
{
// Randomize the queens placement on the board
randomizeGame(init, size);
// Make the initial state with the current game board
initState = new StateNode(init, size);
// Run the hillclimbing algo
finishState = HillClimbing<StateNode>::Run(initState, size);
// Check to see if the algo returned a valid end state
if (finishState->getHeuristic() == 0)
finished = true;
else
{
// Try to clean up memory to prevent memory leak
delete initState; // This is where Visual Studio throws breakpoint
delete finishState;
}
}
As you can see, this while loop constantly creates new StateNode
objects by assigning them to initState. Also, the HillClimbing::Run()
method returns a dynamically created StateNode and assigns it to finishState.
Without this code:
else
{
// Try to clean up memory to prevent memory leak
delete initState; // This is where Visual Studio throws breakpoint
delete finishState;
}
My program leaks a lot of memory, approaching 2GB when the program crashes. With those lines VS throws breakpoints, but MinGw gcc does not, and the program works a lot faster.
My main question: How can I correctly manage the memory of initState
and finishState
to fix memory leaks.
i.e. How can I only keep in memory one StateNode
object, while deleting all other instances as I go.
EDIT This is what is in the VS output window
The thread 0x4244 has exited with code 1857355776 (0x6eb50000).
HEAP[N-Queens.exe]: Invalid address specified to RtlValidateHeap( 01230000, 0126B540 )
N-Queens.exe has triggered a breakpoint.
When going into the dissasembly and pressing F11 to keep going through the code, eventually this happens:
EDIT 2
StateNode.h
class StateNode
{
private:
char** state;
int heuristic;
int size;
public:
StateNode(char** state, int size);
int getHeuristic();
void printState();
char** getState();
};
Here is the code for StateNode.cpp
#include <iostream>
#include "state-node.h"
#include "heuristic.h"
/* Constructor, accepts a state and a size (the number of queens) */
StateNode::StateNode(char ** state, int size)
{
this->state = state;
this->size = size;
this ->heuristic = NQueens::CalcHeuristic(state, size);
}
/* Returns the heuristic value of the node */
int StateNode::getHeuristic()
{
return this->heuristic;
}
/* Prints the state with a nice like board for better visualization */
void StateNode::printState()
{
for (int i = 0; i < this->size; i++)
std::cout << " ____";
std::cout << std::endl;
for (int i = 0; i < this->size; i++)
{
for (int j = 0; j < this->size; j++)
{
if (j < this->size - 1)
{
std::cout << "| " << state[i][j] << " ";
}
else
{
std::cout << "| " << state[i][j] << " |";
}
}
std::cout << std::endl;
for (int k = 0; k < this->size; k++)
std::cout << "|____";
std::cout << "|\n";
}
}
/* Returns a copy of the nodes state */
char ** StateNode::getState()
{
return state;
}
Your current code allocates dynamically allocated memory, but doesn't have a coherent sense of who owns what pointer(s). It then becomes cumbersome to figure out when, where, and who is responsible for freeing the memory. To fix such code may require more error-prone logic to figure to attempt to straighten out the mess.
Instead of this, using C++ and "new-less" code, the following is more or less the equivalent of your current code:
#include <vector>
typedef std::vector<std::vector<char>> Char2D;
class StateNode
{
private:
char2D state;
int size;
int heuristic;
public:
StateNode(const Char2D& theState, int theSize);
int getHeuristic();
void printState();
Char2D& getState() { return state; }
};
Then your constructor lools like this:
StateNode::StateNode(const Char2D& theState, int theSize) :
state(theState),
size(theSize),
heuristic(NQueens::CalcHeuristic(state, size)) {}
Of course, your NQueens::CalcHeuristic
has to take a Char2D
(by reference) instead of a char**
.
Then the rest of the implementation can look like this:
bool finished = false;
size = getNumQueens();
// Make dynamic 2D array of the specified size
Char2D init(size, std::vector<char>(size));
// Puzzle main loop
while (!finished)
{
// Randomize the queens placement on the board
randomizeGame(init, size);
// Make the initial state with the current game board
StateNode initState(init, size);
// Run the hillclimbing algo
finishState = HillClimbing<StateNode>::Run(initState, size);
// Check to see if the algo returned a valid end state
if (finishState.getHeuristic() == 0)
finished = true;
}
The initState
and finishState
are two distinct objects. Also, no need for the else
block.
I know this is somewhat different than your original code, but the goal should be to use value
types and if need be, smart pointers (which I didn't see a need here). Using types as the aforementioned ones is one way to not have issues as your facing now.
If you still want to go the pointer route, I would still leave the vector
alone, and make the following changes:
#include <memory>
//...
std::unique_ptr<StateNode> finishState;
// Puzzle main loop
while (!finished)
{
// Randomize the queens placement on the board
randomizeGame(init, size);
// Make the initial state with the current game board
std::unique_ptr<StateNode> initState = std::make_unique<StateNode>(init, size);
// Run the hillclimbing algo
finishState.reset(HillClimbing<StateNode>::Run(initState, size));
// Check to see if the algo returned a valid end state
if (finishState->getHeuristic() == 0)
finished = true;
}
There is no leak in this code, as we are using std::unique_ptr
, which automatically deallocates the memory for you when either the pointer goes out of scope, or reset
is called.
Collected from the Internet
Please contact [email protected] to delete if infringement.
Comments