-
Notifications
You must be signed in to change notification settings - Fork 65
Nested timers #2486
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
base: master
Are you sure you want to change the base?
Nested timers #2486
Conversation
|
Hi @marvinKaster, you are running into a CI issue that has been fixed in main. Please rebase and it should be gone. |
…hical_timer # Conflicts: # arbor/communication/communicator.cpp # arbor/simulation.cpp
|
Hi @marvinKaster, #2490 has a fix for the last failing test. Could you give it a review? Best, |
thorstenhater
left a comment
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.
Hi @marvinKaster,
thanks, that will make life a bit easier. I've left a few suggestions.
| bool running = false; | ||
| }; | ||
|
|
||
| struct TimerStackHash { |
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.
You could use the internal FNV hasher, which is used in label resolution, too.
| current_timer_stack.push_back(index); | ||
| auto& cur_acc = accumulators_[current_timer_stack]; | ||
| if (cur_acc.running) { | ||
| throw std::runtime_error("recorder::enter you entered the timer twice "+names[index]); |
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.
Since we are throwing here anyhow, how about adding more info? The full stack of ids maybe or even more?! See also later.
| // recorder implementation | ||
|
|
||
| const std::vector<profile_accumulator>& recorder::accumulators() const { | ||
| const std::unordered_map<timer_stack, profile_accumulator, TimerStackHash>& recorder::accumulators() const { |
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.
Give that type a name please ;)
| arb::profile::profiler_enter(region_id_); \ | ||
| } | ||
|
|
||
| // leave a profling region |
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.
Possibly a note for later updates. We could also offer a RAII macro here now, in the style of:
struct AutoProf {
AutoProf(const auto& label): lbl(label) { enter(lbl); }
~AutoProf() { leave(lbl); }
std::string lbl;
};Even some macro magick could be used to generate unique names based on __FILE__ and __LINE__.
| // This is where tasks of the task_group are actually executed. | ||
| void operator()() { | ||
| auto prev_timer_stack = arb::profile::get_current_timer_stack(); | ||
| arb::profile::thread_started(timer_stack); |
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.
task_started / _stopped might be more appropriate. Threads are running all the time and are woken from yield/sleep when tasks are pushed.
| #define PL arb::profile::profiler_leave | ||
| #define PL(name) \ | ||
| { \ | ||
| static std::size_t region_id_ = arb::profile::profiler_region_id(#name); \ |
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.
Another possible extension: we could record FILE LINE FUNCTION when we set up the region.
Introducing nested timers. You can now create timed sections within other timed sections. The spent time will be outputted in a hierarchical format.
Code example
Example output of the ring benchmark