Skip to content
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.

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

Fixups of memory leaks, double frees, etc #265

Merged
merged 70 commits into from
Apr 15, 2016
Merged

Conversation

hlkline
Copy link
Member

@hlkline hlkline commented Apr 15, 2016

Several class objects are now explicitly deleted at the end of SU2_CFD, which provides a check that their deconstructors are properly implemented.
Some memory leaks may still exist, and future work will be needed for postprocessing routines implemented, but not currently called, in driver->Postprocessing.

For future development reference some common problems:

  • pointer not initialized to NULL, causing a segfault when delete [] is called in the deconstructor. (When set to NULL an if statement avoids this segfault).
  • pointers owned by a parent class deleted in the child class. An exception could be when only the child class has information about the size of pointers to pointers, but otherwise risks double-frees (a segfault) and creates duplicated code.
  • FYI, although it does not seem to cause any error, Classname.~Classname() does not have defined behavior, and will do nothing.

economon and others added 30 commits March 20, 2014 16:52
hlkline added 22 commits March 11, 2016 17:32
… deleted in ~CSolver and NOT in child classes, eliminates repeated code and avoids double-frees
…alls to deconstructors are not valid (although they dont cause an error either)
…rface movement explicitly deleted, driver postprocessing commented out for now
@talbring
Copy link
Member

Thanks Heather for dealing with this. This is especially helpful to find future memory leaks for example with valgrind.

@economon
Copy link
Member

Agreed. This looks great, and thank you for the improvements (I know they were hard-earned). Merging in now...

Things are really starting to look better on the memory front. Keep up the good work!

@economon economon merged commit 0f3a312 into develop Apr 15, 2016
@hlkline
Copy link
Member Author

hlkline commented Apr 15, 2016

Thanks for your comments!
Have a good weekend

@aeroamit
Copy link
Contributor

Hi,
I tried the latest developer version for a large problem of 31 million cells. With SST model, initialization of jacobian is continuing for a long time (I terminated job in between).
This is not the issue with latest stable release and it goes fine.
Actually I was curious to see if memory usage cones down in the latest developer version.

Thanks and Regards


From: Thomas D. Economon [email protected]
Sent: Saturday, April 16, 2016 4:28:49 AM
To: su2code/SU2
Subject: Re: [su2code/SU2] Fixups of memory leaks, double frees, etc (#265)

Agreed. This looks great, and thank you for the improvements (I know they were hard-earned). Merging in now...

Things are really starting to look better on the memory front. Keep up the good work!

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHubhttps://github.com//pull/265#issuecomment-210675678

@hlkline
Copy link
Member Author

hlkline commented Apr 17, 2016

Thanks for your post, it is good to know about this as our regression tests use relatively small sized problems.
I have looked through some of the past regression test history, the regression tests took a bit more time after adding additional tests, but other than that have been fairly consistent - up or down by a minute or two, which seems to be well within the usual variance. Although the most recent release wasn't that long ago, there have been several changes to the develop branch, so if you continue to see these problems please feel free to open an issue.

@economon economon deleted the feature_Deallocation branch June 22, 2016 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants