Enable idle rate counter on demand #1489
Conversation
… will be activated as soon as one of the counters is created.
| @@ -49,7 +51,7 @@ | |||
| #include <boost/atomic.hpp> | |||
|
|
|||
| #if defined(HPX_HAVE_SWAP_CONTEXT_EMULATION) | |||
| extern "C" void switch_to_fiber(void* lpFiber) throw(); | |||
| extern "C" void HPX_EXPORT switch_to_fiber(void* lpFiber) throw(); | |||
There was a problem hiding this comment.
Is this change related to the PR? If so, how?
There was a problem hiding this comment.
Probably not, but for some weird reason I was not able to make it work without this change :/ It should probably a separate bug-fix, but I think it does not harm if it is left as a part of this PR.
|
Are there any objections to merge this? |
|
I would have liked to see a proper review here, as this was already changed in the opposite direction (about a year or two ago), and concerns about it were raised on IRC. I am not familiar with the implementation of counters nor with the rationale for disabling idle-rate (overheads?) to do that review, but the general code looks ok and the pattern seems to be already used for other counters (incidentally, access to those This is not an objection. |
No, it never was implemented this way, it was always a compile time switch. @pagrubel was doing some analysis a while back. She said that it had no noticeable effect on performance. But ok, let's do a full review here.
Yah, it's what's called a benign data race. |
I'm not asking for a full review. The code looks sane, and anyone interested had plenty enough time to be made aware of the proposed change. I would have just liked the PR to be signed off by those who have expressed concern.
They actually look like undefined behavior, access to them is still potentially concurrent regardless of the architecture and whatever codegen they produce. I'm not asking for this to change either, I know it to work and be portable (for now) despite not being standard conformant. |
| // We control whether to collect idle rates using this global bool. | ||
| // It will be set by any of the related performance counters. Once set it | ||
| // stays set, thus no race conditions will occur. | ||
| extern bool maintain_idle_rates; |
There was a problem hiding this comment.
Why does the bool have to be defined here as well? Especially since it is not used in that header anywhere?
|
I was hoping to see some comments from @pagrubel regarding the performance impact (if any) of those changes. |
|
Yes, I’d like to run some tests. I haven’t had time as of yet. Now, I see hermione is having problems. Can you wait a bit? Pat |
| @@ -551,8 +551,8 @@ system and application performance. | |||
| is `worker-thread#*` the counter will return the average time spent | |||
| executing one __hpx__-thread for all worker threads separately. | |||
| This counter is available only if the configuration time constants | |||
| `HPX_THREAD_MAINTAIN_CUMULATIVE_COUNTS` (default: ON) and | |||
| `HPX_THREAD_MAINTAIN_IDLE_RATES` are set to `ON` (default: OFF).] | |||
There was a problem hiding this comment.
These names look old, shouldn't these have been HPX_WITH_... already?
There was a problem hiding this comment.
OTOH, this branch might be old enough to still have the old names.
There was a problem hiding this comment.
The new names are used in the cmake_variables file above
|
Note to self: make sure this counter interacts with the existence of RTDSC or similar. |
|
@pagrubel do you still need to run your performance regression tests? Could you share your scripts so that someone could take over that duty if you lack the time at the moment? |
|
I ran the data and will look at it by tomorrow Pat |
|
Doesn't lgtm |
|
Ok, thanks Pat! I'll close this without merging, then. |
This removes the need for compile time configuration of the idle-rate counters. Those will be enabled by default, but activated only when needed.