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

Proposed fix for std::cout sync bug in output.cpp #2122

Merged
merged 2 commits into from
Jul 19, 2022
Merged

Proposed fix for std::cout sync bug in output.cpp #2122

merged 2 commits into from
Jul 19, 2022

Conversation

richmorrison
Copy link
Contributor

When a non-MPI version of the code is compiled and run, print to std::cout is corrupted. Carriage returns appear in the wrong places, and data columns appear as a single line. This behaviour has not been seen when compiled for MPI.

One possible fix presented here replaces the offending stream directs of the form std::cout << std::endl with calls to fmt::print. The problem appears to be mixing the two output methods. Alternative fixes are probably possible.

I've not been able to explain his behaviour, or why compiling with MPI corrects the issue. Presumably fmtlib maintains its own print buffer for performance which is flushed at a different rate to std::cout. A possible complicating factor is that std::endl both introduces a carriage return and flushes the buffer. Perhaps compiling with MPI overrides the default std::cout behaviour.

I've left references to std::cout in other areas of the code. It may be prudent to remove them.

I've been unable to test this code since I can't get the Dockerfile to compile. However, since these are trivial changes I've decided to submit these changes for your review.

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

@richmorrison Thanks for your contribution and congrats on your first pull request! We had deliberately changed fmt::print('\n') to std::cout << std::endl at some point to force output to be flushed, but perhaps that was not the right approach. I think what we should do is adopt your change here (using fmt::print) but in each place where we had a endl before, also add std::fflush(stdout). For good measure, we should #include <cstdio> for stdout.

@richmorrison
Copy link
Contributor Author

@paulromano If fmtlib does maintain a separate buffer, and contains a flush function for the library user, you can possibly discard my proposed changes for a simpler solution.

Best,
Rich

@paulromano
Copy link
Contributor

As far as I'm aware, fmtlib does not maintain a separate buffer -- it simply provides a nicer API on top of normal printf/etc calls. My suggestion to call fflush(stdout) was actually stolen from the fmtlib maintainer 😄

@richmorrison
Copy link
Contributor Author

Cool. (One afternoon when I get more time I think I'd like to figure out why MPI changes things.)

@richmorrison
Copy link
Contributor Author

@paulromano I've added your proposed additional changes.

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Thanks again @richmorrison!

@paulromano paulromano merged commit caf68cf into openmc-dev:develop Jul 19, 2022
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.

2 participants