Measure and report executor starvation time #322
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The executor is considered to be "starving" when it is out of instructions to process, and the scheduler is currently busy. This means that phases of the user program that do not interact with the Celerity API do not count as starvation periods. If the total starvation time exceeds a percentage threshold of the time spent doing actual work (i.e., processing instructions), we print a warning indicating that the application might be scheduler-bound.
psalz
force-pushed
the
report-executor-starvation
branch
from
cf2be90 to
26b9c3f
Compare
|
Check-perf-impact results: (b640933924085610d15d7a9597b095a1) ✔️ No significant performance change in the microbenchmark set. You are good to go! Relative execution time per category: (mean of relative medians)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -287,3 +287,76 @@ TEST_CASE("scheduler(lookahead::automatic) avoids reallocations in the RSim patt | |||
| CHECK(iq.count<free_instruction_record>() == num_devices); | |||
| }); | |||
| } | |||
|
|
|||
| TEST_CASE("scheduler reports idle and busy phases") { // | |||
| struct test_delegate : scheduler::delegate { | |||
There was a problem hiding this comment.
destructor of test_delegate is public and non-virtual
Pull Request Test Coverage Report for Build 12432570567Details
💛 - Coveralls |
fknorr
requested changes
Jan 16, 2025
There was a problem hiding this comment.
Nice, another simple but very useful debugging tool!
I agree on how starvation time is measured, but I believe that the design can be improved by moving the actual tracking from live_executor to runtime.
Proposal:
- Move
total_starvation_timeandtotal_active_timeintoclass runtime::impl - Have
on_executor_busyandon_executor_idleinexecutor_delegate, mirroringscheduler_delegate - If either scheduler or executor change between busy/idle states, take a lock and record which of the 4 possible states we are in (each event also means suspending / resuming a thread, so I suppose the overhead is manageable)
- Remove getters (and dry-run dummy implementations) from executors
This would have the following advantages:
- executor would not need to concern itself with scheduler state conceptually (they are fully separated atm)
- executor public API would not expose any query functions, which makes reasoning about its behavior in the multithreaded context easier
- Cross-thread counter updates would be centralized in runtime (precedence:
runtime::impl::m_latest_epoch_reached) idle_state_changewould not need to traverse the executor queue, so its reporting would be more accurate in time.
The only downside I see in this is that the Tracy integration will not be able to distinguish between executor starvation and idleness.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We previously had a rudimentary Tracy zone for executor starvation, however this was only semi-correct, because if the scheduler is not currently producing any instructions (for example if the user is doing something other than submitting tasks to Celerity), then the executor is not really starving.
I've extended the scheduler to report its idle state to the runtime, which in turn feeds it to the executor for measuring starvation. If the total starvation time exceeds a percentage threshold of the time spent doing actual work (i.e., processing instructions), we print a warning indicating that the application might be scheduler-bound.
Since the scheduler looks at task and command queue sizes to decide whether it is busy or idle, the lookahead mechanism can cause the scheduler to remain busy even if it is not actually producing any instructions. I consider this a feature rather than a bug, because it means that applications that attempt to interleave Celerity kernels with other work will produce a starvation warning if the kernels aren't actually being executed. I've opted to include a hint for this in the warning itself.