-
Notifications
You must be signed in to change notification settings - Fork 192
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
Revamp some deadlock analysis for GH #6437
Conversation
3020647
to
87666f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for doing this! A few small suggestions on printing. Please squash immediately :)
src/Parallel/GlobalCache.hpp
Outdated
} | ||
} | ||
|
||
Parallel::fprintf(file_name, "%s\n", ss.str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be easier to read the output file if you do this print outside the for_each
. Then you could do something like
============== BEGIN CALLBACKS ON NODE # =====================
...
============== END CALLBACKS ON NODE # =====================
to make the file easier to parse.
|
||
stream_points(temporal_id); | ||
|
||
Parallel::fprintf(file_name, "%s\n", ss.str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move the stream to outside the for loop. Then the block of text is guaranteed to be contiguous. You can then also delineate it with blocks like
=========== BEGIN INTERPOLATION TARGET ========
|
||
ss << difference; | ||
|
||
Parallel::fprintf(file_name, "%s\n", ss.str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move the stream to outside the loops. Then the block of text is guaranteed to be contiguous. You can then also delineate it with blocks like
=========== BEGIN INTERPOLATOR =============
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving outside the for
loop is good, but can't move outside the for_each
because I write a new file for each target tag (the output can be long).
try { | ||
if (force) { | ||
set_terminate(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious why terminate should be true. Could you add a code comment?
87666f5
to
efe3fbf
Compare
Squashed! |
Proposed changes
While debugging nodegroup deadlocks, these changes were helpful giving more, better organized, information.
This last commit was useful because sometimes after a deadlock, I'd want to run an event to get some extra information and we have a phase for doing that (
PostFailureCleanup
), but it wasn't run after a deadlock. If the method I have for implementing this needs a lot of discussion, I'm ok leaving it out for now. The other commits are more important I think.Upgrade instructions
For BBH and SingleBH executables, must add a block to the input file
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments