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.

Sign up for GitHub

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

Jump to bottom

Improved timing mechanism #69

Merged
merged 21 commits into from
Aug 30, 2023
Merged

Improved timing mechanism #69

merged 21 commits into from
Aug 30, 2023

Conversation

kllrak
Copy link
Contributor

@kllrak kllrak commented Jul 18, 2023

Description of changes: Improves the timer mechanism as described in the developer docs, adds two timing categories, and saves timer counts to metadata JSON file.

Issue #, if available: Resolves #43 (although further improvements may be expected).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute
this contribution, under the terms of your choice.

@kllrak kllrak changed the title Kllrak/timing Improved timing mechanism Jul 18, 2023
@sebastiangrimberg sebastiangrimberg added the enhancement New feature or request label Jul 19, 2023
sebastiangrimberg
sebastiangrimberg reviewed Jul 19, 2023
View reviewed changes
palace/utils/timer.hpp Outdated Show resolved Hide resolved
palace/utils/timer.hpp Outdated Show resolved Hide resolved
palace/utils/timer.hpp Outdated Show resolved Hide resolved
palace/utils/timer.hpp Outdated Show resolved Hide resolved
palace/main.cpp Outdated Show resolved Hide resolved
palace/main.cpp Show resolved Hide resolved
palace/utils/timer.hpp Outdated Show resolved Hide resolved
@sebastiangrimberg
Copy link
Contributor

sebastiangrimberg commented Jul 19, 2023 •
edited
Loading

Another thought I had is to move these TimerBlock construction into the methods where they are really timing rather than outside where possible. For example, the IO timer can be placed inside of BaseSolver::PostProcessFields rather than on the outside. Likewise various POSTPRO timers can go at the top of driver's Postprocess methods. I think this will lead to more clear usage of the BlockTimer where it is constructed at the beginning of the function scope it is trying to time.

@sebastiangrimberg sebastiangrimberg marked this pull request as draft August 1, 2023 16:04
@sebastiangrimberg sebastiangrimberg force-pushed the kllrak/timing branch 2 times, most recently from 3232661 to de42147 Compare August 22, 2023 22:05
@sebastiangrimberg sebastiangrimberg marked this pull request as ready for review August 22, 2023 22:28
@sebastiangrimberg
Copy link
Contributor

sebastiangrimberg commented Aug 22, 2023 •
edited
Loading

I've made the following changes and think this is probably ready for review for merging into main:

  • Moved the initial BlockTimer construction in main to the first line, to capture startup costs
  • Added new timer categories: PROM Construction and PROM Solve for adaptive frequency sweep, Preconditioner and Coarse Solve to break down the Solve timing further, and Wave Ports. These are printed in the log file with some indentation to clarify to users that the time of Solver should be thought of as including the nested times underneath it. This was a result of the design choice to keep all timers non-overlapping (so the sum of the timing categories is equal to the total run time).

TODO from @kllrak (whenever you get the chance):

  • Final review
  • Changelog

@sebastiangrimberg sebastiangrimberg force-pushed the kllrak/timing branch 2 times, most recently from 8d98c33 to 4d9d1ba Compare August 23, 2023 01:07
@sebastiangrimberg
Copy link
Contributor

@hughcars, adding you as a reviewer here as well just in case you had any commentary on @kllrak's PR that hadn't been addressed.

@sebastiangrimberg sebastiangrimberg force-pushed the kllrak/timing branch 2 times, most recently from 7c2d021 to 974e6f1 Compare August 23, 2023 17:39
kllrak
kllrak commented Aug 30, 2023
View reviewed changes
docs/src/developer.md Outdated Show resolved Hide resolved
@kllrak @sebastiangrimberg
Timer improvements
8d6b23d 
- Prepare for breaking out adaptive driven solver timing.
- Make Timer record how many times we mark time for a particular binning.
- Clean up Timer::Print function.
@kllrak @sebastiangrimberg
Switch to a different timing model.
707abd7 
- In the previous model, a `Timer` object was passed around, and
  timing properly across function boundaries was the responsibility
  of the developer. This had the potential to make reasoning about
  timing difficult and brittle when refactoring (when was last
  `timer.Lap()` call?)
- In the new model, a `TimerBlock` object keeps track of timing for
  a category so long as it is in scope. Details in developer docs.
@kllrak @sebastiangrimberg
Save timer category counts to JSON metadata.
c13c204
@kllrak @sebastiangrimberg
Break out parameter space sampling and HDM solve timing.
b11b6a9
@kllrak @sebastiangrimberg
Use Index instead of int.
ccad554
@kllrak @sebastiangrimberg
Use inline to keep timer.hpp header only.
869ba9e
@kllrak @sebastiangrimberg
Use std::stack and rename TimedBlock -> BlockTimer.
71f5bac
@kllrak @sebastiangrimberg
Rename ParameterSpaceSampling -> FrequencySampleSelection.
563fe20
@kllrak @sebastiangrimberg
Rename BlockTimer variables.
a760157
@kllrak @sebastiangrimberg
Run source formatter.
bba3c72
@kllrak @sebastiangrimberg
Fix "declaration changes meaning" errors.
ccbe4a3
@kllrak @sebastiangrimberg
Remove initializer_list usage and rename a timing category.
db50b9e
@kllrak @sebastiangrimberg
Rename timing category.
47a5417
kllrak and others added 7 commits August 30, 2023 08:52
@kllrak @sebastiangrimberg
Introduce BlockTimer::Finalize and remove BlockTimer::GetTimer.
89c2212 
- Avoids making copy of static `Timer` object to finalize.
- Changes Timer::Now() to be static member function.
- Save printing of frequency sample selection and HDM solve timing
  until end of log file like everything else.
- Note that there's no way to access timing data anymore until it
  is saved in `BlockTimer::Finalize`.
@kllrak @sebastiangrimberg
FREQSPACESAMPLING --> FREQSAMPLESELECTION to match log output.
84dff70
@sebastiangrimberg
Minor cleanup after rebase
08bcd81
@sebastiangrimberg
Add new timing categories for linear solver preconditioning, multigri…
fd86a65 
…d coarse solve, wave ports, and clarify adaptive driven timing categories
@sebastiangrimberg
Some further cleanup: Refactor relationship between BlockTimer and Ti…
5d27339 
…mer for printing of final results, add another level of structure to timing data in metadata JSON
@sebastiangrimberg
Fix docs typo and update changelog
a667a2f
@sebastiangrimberg
Switch lambda to default capture to silence -Wunused-lambda-capture c…
5a19288 
…ompiler warning for constexpr variable
@sebastiangrimberg sebastiangrimberg merged commit 33498f1 into main Aug 30, 2023
17 checks passed
@sebastiangrimberg sebastiangrimberg deleted the kllrak/timing branch August 30, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebastiangrimberg sebastiangrimberg sebastiangrimberg approved these changes

@hughcars hughcars Awaiting requested review from hughcars

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Better detail for timing output
2 participants
@kllrak @sebastiangrimberg