-
Notifications
You must be signed in to change notification settings - Fork 144
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
Add timer for walker log #5259
Add timer for walker log #5259
Conversation
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.
This PR adds only 1 walker_log timer covering all kinds of walker_log operations. Only the total time can be interpreted and time per call is not really meaningful.
Could you put fine timers into functions of WalkerLogCollector and WalkerLogManager? In this way, we will have better timer resolution and zero timer impact if walker_log is not activated.
The timer gets listed in different enclosing timer scopes in the output. It's quite easy to tell what is what from the number of calls for each timer output. But I get what you mean, will do when I have time. |
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.
Agreeing with Ye's suggestions: Better to have the logging within the log manager etc. Also nicer to have finer timers in case any one operation is particularly slow.
@PDoakORNL could you relocate timers into respective classes and functions? |
fc0379f
to
0c2ee0e
Compare
0c2ee0e
to
99f9370
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.
Very minor nit, but it looks like clangformat needs to be rerun on at least WalkerLogCollector.cpp
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 but note I have not run it.
Test this please |
Proposed changes
I was curious about the cost of walker logging so I added a timer.
Running the batched energy density with it on its 100 times more expensive than the energy denisty but still just ~15% of the total dmc time.
This is wth 2400 walkers across 8 ranks with 3 crowds each. 8 v100's
diamond 1x1x1
What type(s) of changes does this code introduce?
Delete the items that do not apply
Does this introduce a breaking change?
What systems has this change been tested on?
dgx server
Checklist
Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.