Skip to content

Fix memory leak in radTInteraction::Setup #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
per-gron wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix memory leak in radTInteraction::Setup #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 7 additions & 10 deletions cpp/src/core/radintrc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ radTInteraction::radTInteraction()

NewMagnArray = NULL;
NewFieldArray = NULL;
IdentTransPtr = NULL;

@ochubar ochubar Dec 30, 2019

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is probably a ton of places in the code that may lead to some memory leak(s) when a method fails. In our previous / current interfaces (Mathematica. Python) this is not so important, because entire simulation is usually re-started if some method fails. But all such places could indeed de analyzed more thoroughly, and eliminated.
In this particular case, instead of a pointer to a radIdentTrans object, we can keep the object itself (since it does not require a lot of memory) as a member variable, in that or in the base class.
I am working now on parallelizing (via MPI) this and other functions, so will postpone this eventual change until the corresponding update.

@per-gron per-gron Dec 30, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, code with manual memory management tends to not do the right thing around errors, it's just very very hard to get it right. This is where things like unique_ptr, vector and other higher level constructs really shine. In fact, there is a wide consensus that since C++11 there is essentially never a need to use new or delete at all in application level code*, and it's considered best practice to avoid them.

Cool that you are working on parallelizing this code! Let me warmly recommend ThreadSanitizer. It detects and pinpoints data races and other threading issues, which can be nearly impossible to fix or even find. If you write code that has any mutable state that is shared across threads you need this tool. It is one of the closest things to magic I have seen in the world of software.

*: The one exception to this is when creating unique_ptr objects, where you still need new. C++14 introduces std::make_unique to fix this little wart.


RelaxSubIntervArray = NULL; // New
mKeepTransData = 0;
Expand All @@ -65,7 +64,6 @@ int radTInteraction::Setup(const radThg& In_hg, const radThg& In_hgMoreExtSrc, c

NewMagnArray = NULL;
NewFieldArray = NULL;
IdentTransPtr = NULL;

RelaxSubIntervArray = NULL; // New
AmOfRelaxSubInterv = 0; // New
Expand All @@ -79,7 +77,7 @@ int radTInteraction::Setup(const radThg& In_hg, const radThg& In_hgMoreExtSrc, c

MemAllocTotAtOnce = InMemAllocTotAtOnce;

IdentTransPtr = new radIdentTrans();
IdentTransPtr.reset(new radIdentTrans());

radTlphgPtr NewListOfTransPtr;
CountMainRelaxElems((radTg3d*)(SourceHandle.rep), &NewListOfTransPtr);
Expand Down Expand Up @@ -156,7 +154,6 @@ radTInteraction::~radTInteraction()
DestroyMainTransPtrArray();
EmptyVectOfPtrToListsOfTrans();
}
if(IdentTransPtr != NULL) delete IdentTransPtr; //required by EmptyVectOfPtrToListsOfTrans();
}

//-------------------------------------------------------------------------
Expand Down Expand Up @@ -423,7 +420,7 @@ void radTInteraction::FillInMainTransPtrArray()
{
MainTransPtrArray[i] = new radTrans(*(TransPtrVect[0]));
}
else MainTransPtrArray[i] = IdentTransPtr;
else MainTransPtrArray[i] = IdentTransPtr.get();
EmptyTransPtrVect();
}
FillInMainTransOnly = 0;
Expand Down Expand Up @@ -1043,7 +1040,7 @@ void radTInteraction::DumpBin(CAuxBinStrVect& oStr, vector<int>& vElemKeysOut, m
//radVectPtr_lphgPtr ExtVectOfPtrToListsOfTransPtr; //required
DumpBinVectOfPtrToListsOfTransPtr(oStr, ExtVectOfPtrToListsOfTransPtr, gMapOfHandlers);

//radIdentTrans* IdentTransPtr; //required, but doesn't need to be saved
//std::unique_ptr<radIdentTrans> IdentTransPtr; //required, but doesn't need to be saved
//radTCast Cast; //no members?
//radTSend Send; //no members?

Expand Down Expand Up @@ -1145,8 +1142,8 @@ void radTInteraction::DumpBinParseVectOfPtrToListsOfTransPtr(CAuxBinStrVect& inS

radTInteraction::radTInteraction(CAuxBinStrVect& inStr, map<int, int>& mKeysOldNew, radTmhg& gMapOfHandlers)
{
//radIdentTrans* IdentTransPtr; //required
IdentTransPtr = new radIdentTrans();
//std::unique_ptr<radIdentTrans> IdentTransPtr; //required
IdentTransPtr.reset(new radIdentTrans());

//int AmOfMainElem;
inStr >> AmOfMainElem;
Expand Down Expand Up @@ -1190,7 +1187,7 @@ radTInteraction::radTInteraction(CAuxBinStrVect& inStr, map<int, int>& mKeysOldN
{
radThg hg;
int oldKey = DumpBinParseSourceHandle(inStr, mKeysOldNew, gMapOfHandlers, false, false, hg);
if(oldKey < 0) TransPtrVect.push_back(IdentTransPtr);
if(oldKey < 0) TransPtrVect.push_back(IdentTransPtr.get());
else if(hg.rep != 0) TransPtrVect.push_back(new radTrans(*((radTrans*)hg.rep))); //will be deleted at distraction
}

Expand Down Expand Up @@ -1372,7 +1369,7 @@ radTInteraction::radTInteraction(CAuxBinStrVect& inStr, map<int, int>& mKeysOldN
{
radThg hg;
int oldKey = DumpBinParseSourceHandle(inStr, mKeysOldNew, gMapOfHandlers, false, false, hg);
if(oldKey < 0) MainTransPtrArray[i] = IdentTransPtr;
if(oldKey < 0) MainTransPtrArray[i] = IdentTransPtr.get();
else if(hg.rep != 0) MainTransPtrArray[i] = new radTrans(*((radTrans*)hg.rep)); //will be deleted at distraction
}
}
Expand Down
7 changes: 4 additions & 3 deletions cpp/src/core/radintrc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "gmtrans.h"
#include "radg3d.h"

#include <memory>
#include <sstream>
#include <vector>

Expand Down Expand Up @@ -171,7 +172,7 @@ class radTInteraction : public radTg {

radTCast Cast;
radTSend Send;
radIdentTrans* IdentTransPtr;
std::unique_ptr<radIdentTrans> IdentTransPtr;
short FillInMainTransOnly;
char mKeepTransData;

Expand Down Expand Up @@ -290,8 +291,8 @@ inline void radTInteraction::FillInTransPtrVectForElem(int ElemLocInd, char I_or
if(I_or_E == 'I') PtrToListOfPtrToTrans = IntVectOfPtrToListsOfTransPtr[ElemLocInd];
else PtrToListOfPtrToTrans = ExtVectOfPtrToListsOfTransPtr[ElemLocInd];

if(PtrToListOfPtrToTrans->empty()) TransPtrVect.push_back(IdentTransPtr);
else NestedFor_Trans(IdentTransPtr, PtrToListOfPtrToTrans->begin(), ElemLocInd, I_or_E);
if(PtrToListOfPtrToTrans->empty()) TransPtrVect.push_back(IdentTransPtr.get());
else NestedFor_Trans(IdentTransPtr.get(), PtrToListOfPtrToTrans->begin(), ElemLocInd, I_or_E);
}

//-------------------------------------------------------------------------
Expand Down