Enhance Worker Coordinator Async Profiler #710
Conversation
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
IanHoang
requested review from
gkamat,
beaioun,
cgchinmay,
rishabh6788,
VijayanB and
OVI3D0
as code owners
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
…pace Signed-off-by: Ian Hoang <[email protected]>
IanHoang
changed the title
| f"Available sort types: {worker_coordinator.AsyncProfiler.SORT_TYPES}. Default is None.", | ||
| default=None) |
There was a problem hiding this comment.
May help to add a check to warn if this argument is used but profiling is not enabled.
Also, does it make sense to provide an option for sort order (asc/desc)?
Finally, it may be useful to be able to specify only specific tasks for profiling.
There was a problem hiding this comment.
May help to add a check to warn if this argument is used but profiling is not enabled.
Rishabh made a call out earlier in this PR that suggests moving the responsibility of checking if sort types exists to the argparser. This is doable and requires adding more classes and leveraging the action field in the arguments. This does add more complexity to benchmark.py though.
does it make sense to provide an option for sort order (asc/desc)?
I initially wanted to minimize the number of arguments for AsyncProfiler since it exists in the execute-test parser, which already has a lot of arguments. I could look to incorporate it still.
Finally, it may be useful to be able to specify only specific tasks for profiling.
This is a fair call out but similar to the question above, I'm concerned about the number of arguments in execute-test and wonder if users will be able to spot that these are for the AsyncProfiler. Still, both of these are arguments that users would actually want. Can aim to have this as a draft and see if there's other ways we can achieve this.
|
|
||
| stats = yappi.get_func_stats() | ||
|
|
||
| if self.sort_type: |
There was a problem hiding this comment.
Enforce this check while accepting the params.
e.g.
parser.add_argument(
'--mode',
type=str,
choices=['normal', 'advanced', 'expert'], # List of valid options
required=True, # Make this argument mandatory
help="Choose a mode: 'normal', 'advanced', or 'expert'"
)
This way argparser will throw exception.
There was a problem hiding this comment.
This would work if --enable-worker-coordinator-profiling was required in execute-test but it's currently optional. --sort-type is not needed unless the user wants to specify the a column for the Yappi library to sort on.
Looking at the argparse documentation, we'll need to create additional classes that inherit from argparse.Actions and some additional if statements in benchmark.py to enforce Argparser to throw the exception, which requires new changes as oppose to this if statement within the AsyncProfiler. @rishabh6788 Let me know if you have other thoughts on this?
If we find an approach with argparse that we like and doesn't add too much to the existing benchmark.py, I can open a separate PR to address this as well so that there is less changes per PR.
There was a problem hiding this comment.
Even when you set a default value if flag not provided?
| profiler = worker_coordinator.AsyncProfiler(f) | ||
| client_id = 2 | ||
| task = "queries" | ||
| sort_type = None |
There was a problem hiding this comment.
One more test to verify any other sort type option?
There was a problem hiding this comment.
I can but this test is more for seeing if the wrapper is called
|
Moving to draft because haven't had time to address this and since several conflicts exists |
Description
OSB comes with a profiler, called Async Profiler, that can analyze performance calls in OSB's worker coordinator. In order to use the Async Profiler, users can run the tests with
--enable-worker-coordinator-profilingand profiled results will be outputted to~/.benchmark/logs/profile.logonce the benchmark completes. Since custom runners and param sources are on a performance critical path, the profiler will come in handy to determine if they are adding any bottlenecks to OSB.The profiler currently outputs all profiled lines to
~/.benchmark/logs/profile.log. This PR makes it easier to interpret the profile.log by labeling which output pertains to which task and client (if using more than one client) and gives users the options to sort the outputted lines by sort types (or column names). This will also help users who are investigating performance bottlenecks in the code.Example profile.log that was not sorted
Example of profile.log ouput sorted on ncalls field
Issues Resolved
#709
Testing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.