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

Clean libmints new/delete #2346

Merged
merged 2 commits into from
Nov 11, 2021
Merged

Conversation

JonathonMisiewicz
Copy link
Contributor

Description

There's a memory leak somewhere in the SCF code. This PR replaces some of the manual memory management with new/delete in libmints to cut down the number of places where it could be coming from. (I'd like to start from a zero-leak baseline for DIIS profiling.)

I'll probably need to use valgrind to track this down further...

Checklist

  • Quicktests pass

Status

  • Ready for review
  • Ready for merge

@JonathonMisiewicz JonathonMisiewicz added libmints For things that are wrong with libmints (but not wavefunction). cleanup For issues where the goal is to make Psi4 a little cleaner. efficiency For issues about code in Psi needing a disturbing amount of time and/or memory. labels Nov 8, 2021
Copy link
Contributor

@PeterKraus PeterKraus left a comment

Choose a reason for hiding this comment

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

I'm by no means a C++ expert, but the changes look sensible and the code's a lot more readable. The comments are just a couple of clarifications.

psi4/src/psi4/libmints/basisset.cc Outdated Show resolved Hide resolved
psi4/src/psi4/libmints/basisset.cc Show resolved Hide resolved
psi4/src/psi4/libmints/basisset.cc Show resolved Hide resolved
psi4/src/psi4/libmints/irrep.cc Show resolved Hide resolved
psi4/src/psi4/libmints/molecule.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@PeterKraus PeterKraus left a comment

Choose a reason for hiding this comment

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

Thanks Jonathon!

Copy link
Member

@jturney jturney left a comment

Choose a reason for hiding this comment

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

Always nice to see updates to modern C++ 😄

@jturney
Copy link
Member

jturney commented Nov 9, 2021

Yeah, using valgrind will make it easier to find the leak.

At one point, we did have it leak free.

@JonathonMisiewicz
Copy link
Contributor Author

Yeah, using valgrind will make it easier to find the leak.

At one point, we did have it leak free.

Good to know. Unfortunately, it may be a bit before I can get valgrind working - there are some issues with Francesco's cluster.

@JonathonMisiewicz JonathonMisiewicz merged commit 8aa91f7 into psi4:master Nov 11, 2021
@JonathonMisiewicz JonathonMisiewicz added this to the Psi4 1.5 milestone Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup For issues where the goal is to make Psi4 a little cleaner. efficiency For issues about code in Psi needing a disturbing amount of time and/or memory. libmints For things that are wrong with libmints (but not wavefunction).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants