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

More Memory Logging #15476

Closed
wants to merge 6 commits into from
Closed

Conversation

friedmud
Copy link
Contributor

Work towards satisfying #15444 . Inspired by #15446

This extends both the TimedPrint and PerfGraph capabilities to also track memory usage.

Example of new TimedPrint:

image

Example of new PerfGraph:

image

@moosebuild
Copy link
Contributor

Job Precheck on f68f728 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/15476/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format e22fd5e87442c065479350100d4dd5685781fb3e

@friedmud
Copy link
Contributor Author

Just pushed another commit. I removed the Children columns from the PerfGraph and added Mem for Self and Total. Children is really rarely used - and it's just total-self anyway. I thought it would be better to be able to see self vs. total memory.

Here is what it looks like now:

image

@friedmud
Copy link
Contributor Author

friedmud commented Jun 18, 2020

BTW - it's straightforward to get detailed memory output for all of the actions before the solve:

[Outputs]
  [pgraph]
    type = PerfGraphOutput
    level = 7
    execute_on = initial
  []
[]

image

framework/doc/content/source/utils/PerfGraph.md Outdated Show resolved Hide resolved
framework/include/utils/PerfNode.h Outdated Show resolved Hide resolved
framework/include/utils/PerfNode.h Outdated Show resolved Hide resolved
framework/include/utils/PerfNode.h Outdated Show resolved Hide resolved
framework/include/utils/PerfNode.h Outdated Show resolved Hide resolved
framework/src/utils/PerfNode.C Outdated Show resolved Hide resolved
framework/src/utils/PerfNode.C Outdated Show resolved Hide resolved
@YaqiWang
Copy link
Contributor

YaqiWang commented Jun 18, 2020

This looks pretty nice. If we can have the similar thing for initialSetup, I'll not insist to have my PR in although I do think we can let both coexist. The only problem is that initialSetup is not like action warehouse, where we have a loop to execute all actions, thus make adding this easy. Should we add a member variable in FEProblem like

std::vector<std::pair<std::string, void *>> _initial_setup_functions;

so that we can add PerfGraph and TimedPrint entries easily for all items?

@friedmud
Copy link
Contributor Author

@YaqiWang InitialSetup is already well represented within the PerfGraph. Take a closer look at the above output. Here is the relevant section:

image

@fdkong
Copy link
Contributor

fdkong commented Jun 18, 2020

Sweet

@YaqiWang
Copy link
Contributor

The list is not complete though and we can potentially miss something in a complicated model.

@loganharbour
Copy link
Member

@YaqiWang, that is an issue with both methods. In both cases, the developer is still responsible for adding entry points for logging. We strongly encourage PRs with additional timers and timed prints. We can easily move the entry points in #15446 here.

@YaqiWang
Copy link
Contributor

My way is more forgiving, users can still see something even missing items in-between the first and last message, plus the pattern created in initialSetup forces developers to follow the same pattern.

@YaqiWang
Copy link
Contributor

YaqiWang commented Jun 22, 2020

@friedmud I'v noticed that when adding print-outs in my PR. But it is not complete, which is something I worried about. If something goes wrong, we have to spend hours to figure out where and why. One example I experience in the past is that one setup has lots of materials and the sorting algorithm for materials in MOOSE is not optimized and really slow. @permcody and @rwcarlsen were involved that conversation and helped fixed the problem. We could potentially have similar issues which will waste lots of our resources to detect. With my print-out, we can simply turn on the command-line parameter and locate the hot spot really quickly. So now I am thinking, you guys should really merge my PR. It really does not hurt. We can keep improving TimedPrint and PerfGraph of course and I like them.

Note that Problem/show_initial_setup=true can be viewed as an extension of Debug/show_actions=true.

@rwcarlsen
Copy link
Contributor

@YaqiWang - if the existing time prints and perfgraph entries are missing some details you need, then we'll just add in new timed prints and perfgraph sections to cover what you need. That will be much nicer than having the disparate logging.

@YaqiWang
Copy link
Contributor

How do I know something is missing if I do not have had a problem yet?

@friedmud
Copy link
Contributor Author

@YaqiWang I don't understand what you're talking about. The TimedPrint capability prints out while the code is running - just like yours does. Explain how yours is different (other than not using the mechanism that is already in the code and is widely used).

@YaqiWang
Copy link
Contributor

TimedPrint requires CONSOLE_TIMED_PRINT and PerfGraph requires TIME_SECTION(a_timer). Mine printing the current accumulated memory and cpu time and has more samples. It shows up by a command-line parameter but not like the wait time of TimedPrint. So it is a diversified add-on in my view. Really does not hurt. TimedPrint is auto triggered so that users do not need to worry long wait without to know what is going on. I used both TimedPrint and PerfGraph. They are all good. But mine is still a great add-on.

@friedmud
Copy link
Contributor Author

@YaqiWang How does yours "have more samples"? You have to add your statements to the code in just the same way as the others. As we add code / capability we would still need to add more of your statements in just the same way as the existing perfgraph and timedprint. I don't see any difference there.

Anyway: you should know that I'm working on something even better than what's presented here. Please have a bit of patience.

Also: No, we won't be merging your PR in the interim - it is a step backward. We need something that is better than all of these solutions. Please be patient, I think you'll like the new thing.

@stale
Copy link

stale bot commented Jul 25, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale PRs that have reached or exceeded 90 days with no activity label Jul 25, 2020
@stale
Copy link

stale bot commented Jul 26, 2020

Closing due to 30 days of inactivity. See http://mooseframework.org/moose/framework_development/patch_to_code.html

@stale stale bot closed this Jul 26, 2020
@friedmud friedmud mentioned this pull request Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PRs that have reached or exceeded 90 days with no activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants