Skip to content

Teuchos: VerboseObject fix filestreams

James Willenbring requested to merge jjellio:jje/fix_VerboseObject into develop

Created by: jjellio

@trilinos/teuchos

Description

Currently, VerboseObject provides an option to allow output to be redirected to a file. Unfortunately, this feature results in every MPI process opening/writing the file, which is not typically what an OStream is used to accomplish. For file output, this implementation leads to extreme performance degradation with large parallel jobs.

This patch accomplishes two things:

  1. It restricts file opening/writing to a single MPI process. The implementation of this is problematic, because VerboseObject's sublist reader has no access to any Comm. Ideally, the interface should be refactored to allow a comm to be passed, so that the logic can use the com to enforce which rank writes output.

For now, the implementation guards the use of Teuchos::GlobalMPISession to obtain the process' global rank, and uses this to restrict std::ofstream construction.

  1. This patch provides a means to configure a large file buffer for the ofstream. Implementing this is problematic as well. The question is where and how to store the allocated buffer. Extending the FancyOStream class to add storage for a buffer would imply that other OStreams could support buffer modifications (which is a much broader impact than configuring a file stream buffer - it isn't clear that any arbitrary OStream can support this.)

To handle construction and deallocation of the buffer, a static std::vector of std::vector<char> is stored in the associated source files' object code. This is not ideal, but it should result in the memory being deallocated at program exit (not FancyOStream deconstruction though).

Moreover, this implementation is not thread safe w.r.t. to creating this OStream object. Concurrency could be guarded using C++11's concurrency features (or possibly Kokkos if using Kokkos inside this package is allowed).

I am open to suggestions to handle any of the above issues better. GlobalMPISession is guarded by the Teuchos macro, HAVE_TEUCHOS_MPI, and there is an implicit assumption that MPI has been initialized and that calling GlobalMPISession::getRank is safe.

I suppose if this patch isn't accepted, then this code path should really be eradicated, because it presented a very hard to identify scaling problem, simply due to a change in Stratimikos parameters in an XML file.

How Has This Been Tested?

Full sems/std testing suite (release/gcc-7.3)

Checklist

  • My commit messages mention the appropriate GitHub issue numbers.
  • My code follows the code style of the affected package(s).
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the code contribution guidelines for this project.
  • I have added tests to cover my changes. - It is not obvious how to test this. The code has been verified by using the system tool 'strace' to ensure single process opening/writing of the file
  • All new and existing tests passed.
  • No new compiler warnings were introduced.
  • These changes break backwards compatibility.

closes #4731 (closed)

Merge request reports