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

Moves to C++14 and fixes a SAPT memory bug #1398

Merged
merged 15 commits into from Dec 5, 2018

Conversation

Projects
None yet
5 participants
@robertodr
Copy link
Contributor

robertodr commented Dec 3, 2018

Description

Set the minimum required C++ standard to C++14. Lower bounds on compilers can be found here. Also addresses a bug in the SAPT code that resulted in a double free.

About the SAPT bug

The Iterator class in SAPT contained an int * array that was cleaned up by the class's destructor. Iterator objects are constructed by a builder routine that first creates a local Iterator object, fills it, then returns it by value. The C++98 behavior of this design is problematic; a copy of the local temp Iterator is made and that copy is returned to the caller. When that copy is made, both the local temp and its copy have int * pointers that point to the same pool of memory because no deep-copy copy constructor exists for Iterator. Upon returning, the local temp object is destroyed, triggering the memory pointed to by the int * to be freed, causing the returned object to point to freed memory which is undefined behavior. When that returned object eventually goes out of scope, it will try to free the memory again, leading to the double free memory error we observed. Because we use C++11 most compilers appear to be able to correctly elide the copy, by implementing move semantics, so we haven't seen this before. The switch to C++14 with GCC5.4 caused consistent segfaults, revealing the issue. The fix is simple; don't use raw int *, but std::vector<int> instead; the lifetime of these is correctly managed automatically and the various move constructor/copy constructor/destructor can be generated correctly by the compiler. RAII for the win!

Todos

Notable points (developer or user-interest) that this PR has or will accomplish.

  • Fix C++14 compliance CMake-side
  • Fix memory bug in SAPT code
  • Move Travis to Xenial

Status

  • Ready for review
  • Ready for merge

robertodr added some commits Nov 16, 2018

cd "$TRAVIS_BUILD_DIR"
rm -f "$HOME"/Downloads/miniconda.sh
fi
echo "-- Done with latest Miniconda"

This comment has been minimized.

@loriab

loriab Dec 3, 2018

Member

has this done a bout with the latest molssi conda downloader to see who advances? https://github.com/MolSSI/QCFractal/blob/master/devtools/travis-ci/before_install.sh

This comment has been minimized.

@robertodr

robertodr Dec 3, 2018

Contributor

Nope. I was not aware of the MolSSI script. It looks better than my concoction, should I swap the two?

This comment has been minimized.

@loriab

loriab Dec 3, 2018

Member

may as well, unless you see something that should be merged into it

This comment has been minimized.

@robertodr

robertodr Dec 3, 2018

Contributor

The cache check is missing from the MolSSI script. I'll merge the two.

This comment has been minimized.

@dgasmith

dgasmith Dec 5, 2018

Member

Overall we found caching was more complex than it was worth. Conda takes about 1.5 minutes to download and install at current.

This comment has been minimized.

@loriab

loriab Dec 5, 2018

Member

oh, and when we switch to AM8 libint, that's also an issue (though probably could arrange AM6 for CI).

This comment has been minimized.

@dgasmith

dgasmith Dec 5, 2018

Member

MKL install which does take ~40 seconds on its own. Installing base miniconda is about 40 seconds as well.

This comment has been minimized.

@robertodr

robertodr Dec 5, 2018

Contributor

I agree that seeing the cache in action doesn't show much improvement in build times. What's your recommendation? Pull it out and revisit later?

This comment has been minimized.

@loriab

loriab Dec 5, 2018

Member

i'm indifferent

This comment has been minimized.

@dgasmith

dgasmith Dec 5, 2018

Member

Historically caching has provided an array of issues, but it might be worth seeing if the above causes any instabilities as these things do evolve quickly. So I would leave it in for now and simply be watchful.

robertodr added some commits Dec 3, 2018

@loriab

This comment has been minimized.

Copy link
Member

loriab commented Dec 4, 2018

Well, SAPT (and maybe SAPT Disp) is the common theme to the errors. Strange!

@robertodr

This comment has been minimized.

Copy link
Contributor

robertodr commented Dec 4, 2018

Annoying 😡 Any idea what to try out?

andysim and others added some commits Dec 4, 2018

Additional clean up in SAPT
First use of std::make_unique in Psi4! (I think)
@andysim

This comment has been minimized.

Copy link
Member

andysim commented Dec 4, 2018

It looks like those changes did the trick. The tests passed fairly quickly on travis, but i'm not sure what the timings were like before so i don't know how much impact the header refactoring had. When you think this one's good, could you ping us on Slack, please? The seg fault seems to be hitting a few people so it would be good to get it in quickly.

@robertodr

This comment has been minimized.

Copy link
Contributor

robertodr commented Dec 4, 2018

I'm still figuring out the caching of Conda-installed dependencies.

robertodr added some commits Dec 4, 2018

@loriab loriab added this to the Psi4 1.3 milestone Dec 5, 2018

@loriab

loriab approved these changes Dec 5, 2018

Copy link
Member

loriab left a comment

conda-build approves this PR; therefore, so do I.

I do think "fix double free in SAPT" should be added to the PR summary. And posting Andy's explanation from core-dev channel could be valuable breadcrumbs in future.

@andysim andysim changed the title Psi4 is now a C++14 code Moves to C++14 and fixes a SAPT memory bug Dec 5, 2018

@andysim

This comment has been minimized.

Copy link
Member

andysim commented Dec 5, 2018

I updated the title and posted some blurb describing the issue. I approve the PR, but I'm a part author so maybe another reviewer would be good.

@andysim

andysim approved these changes Dec 5, 2018

@robertodr

This comment has been minimized.

Copy link
Contributor

robertodr commented Dec 5, 2018

Thanks @andysim! The caching of conda is currently a bit idiotic: I am nuking p4env every time after a build succeeds. Probably we want to keep the conda environment in cache and just activate and update it. @loriab does that sound like a reasonable proposition? I'll tackle it in a subsequent PR.

@loriab

This comment has been minimized.

Copy link
Member

loriab commented Dec 5, 2018

For cacheing, keeping “ miniconda/packages” dir around is what I expect to be the key part. So long as the mkl dir or tarball is in there, making the conda env is trivial.

@robertodr

This comment has been minimized.

Copy link
Contributor

robertodr commented Dec 5, 2018

As I said, my current take is idiotic. The only thing kept around is the Conda installation itself, not the environment.

robertodr added some commits Dec 5, 2018

@loriab

This comment has been minimized.

Copy link
Member

loriab commented Dec 5, 2018

What I meant was that that may be just fine. If what's being cached is $HOME/miniconda/bin and $HOME/miniconda/packages, then you've preserved all the heavy stuff (mkl w/i the packages dir). A new conda create -n p4dev that goes into $HOME/miniconda/envs/p4dev is trivial compared to downloading all the packages. I'd prefer the env to be re-solved each time, so I'm not advocating caching p4dev env.

Adjust ERI holder and buffer
Thanks @dgasmith for the suggestions
@dgasmith
Copy link
Member

dgasmith left a comment

Looks good. We should wait for one more review here however.

@dgasmith dgasmith requested a review from jturney Dec 5, 2018

@dgasmith

This comment has been minimized.

Copy link
Member

dgasmith commented Dec 5, 2018

Let's wait on a review from @jturney before emerging just to give it a final once over.

@jturney

jturney approved these changes Dec 5, 2018

Copy link
Member

jturney left a comment

Seems good to me.

@jturney jturney merged commit 16b8532 into psi4:master Dec 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
psi4.psi4 #20181205.14 succeeded
Details

@robertodr robertodr deleted the robertodr:cpp14 branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment