Skip to content

Minor refactoring of BoutInitialise, BoutFinalise#1651

Merged
ZedThree merged 32 commits intonextfrom
split-up-bout-init-final
Apr 2, 2019
Merged

Minor refactoring of BoutInitialise, BoutFinalise#1651
ZedThree merged 32 commits intonextfrom
split-up-bout-init-final

Conversation

@ZedThree
Copy link
Member

  • Split up BoutInitialise into smaller functions, and then call all of them
    • New functions are in bout::experimental namespace, as there's some potential for a lot of flux on the signatures/location, and they need to be "always visible"
  • Add unit tests for most of these functions
  • Add optional argument to BoutFinalise to write out settings file
  • Clang-format bout++.cxx and bout.hxx

Splitting up BoutInitialise makes it easier to read and test, as well as potentially be advantageous for the Python API and the integrated tests (when we don't need to do all the setup)

There's also a bug fix in here, that we might want to pull out and get into master:

  // Save the PID of this process to file, so it can be shut down by user signal
  {	
    std::string filename;
    std::stringstream(filename) << data_dir << "/.BOUT.pid." << MYPE;	
    std::ofstream pid_file;	
    pid_file.open(filename, std::ios::out);

std::stringstream(filename) is a temporary, so the filename is always "" and the file never gets created.

ZedThree added 27 commits March 25, 2019 15:36
Allows gtest death test to check usage message is printed
Consistent handling of errors -- but do we want to rethrow or just
return 1?
Also make savePIDtoFile throw if file could not be opened
Some small tidying with minor functions
@ZedThree
Copy link
Member Author

After way too long debugging, I finally worked out that the boutcore tests were failing because they were not calling boutcore.finalise (i.e. BoutFinalise), which meant that the global msg_stack was getting destroyed before bout::globals::dump (or rather, in some arbitrary order). Unfortunately, the Datafile destructor calls Datafile::close which has a TRACE macro -- which tries to use the now deleted msg_stack.

I've done the quick fix which is to explicitly call boutcore.finalise in the tests. It might be better if MsgStackItem actually had a static member MsgStack which it pushed to. This is plausibly backwards-incompatible as it would mean the global msg_stack would be pointless (and therefore could be removed).

I did notice that there is a commented-out call to atexist.register(boutcore.finalise) in boutcore.init, which would presumably ensure that finalise would always be called on exit. @dschwoerer was this causing problems?


For future debugging reference: Python 3.6 introduced the environment variable PYTHONMALLOC which can be set to malloc, which in combination with the suppression file that comes with the source, makes valgrind a lot less noisy. You possibly also need to combine it with a suppression file for your MPI implementation which you can do with the --gen-suppressions=all --log-file=<file> arguments to valgrind on a simple MPI hello world executable.

AddressSanitizer turned out to not be so useful, because it doesn't take a blacklist (they claim no false positives!)

@dschwoerer
Copy link
Contributor

The python library has checks in various places to sure that the bout library is currently initialised. Thus after finalise, several calls are invalid.

I think the issue arose as Python doesn't have RAII, so the destruction of several object caused issues.

I'll have a look again ...

@ZedThree
Copy link
Member Author

The latest reason the tests failed was due to a call to MPI_Comm_free after MPI_Finalize. This appears to be coming from the global mesh getting destroyed at the same time as or before the call to BoutFinalise. This only happens in the one boutcore test that makes non-global meshes.

I've worked around it by getting the global mesh and explicitly del-ing it. Not a great solution, but I don't understand the boutcore stuff well enough to implement a better fix right now.

@dschwoerer Uncommenting the atexit.register call (and import atexit) leads to an error: "The BOUT++ library was not initialised - please call boutcore.init(args) first", so I guess it's still a problem.

@ZedThree ZedThree merged commit 687919a into next Apr 2, 2019
@ZedThree ZedThree deleted the split-up-bout-init-final branch April 2, 2019 09:03
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.

3 participants