Open marcin-ol opened 1 year ago
Remaining actions for this PR:
I reviewed most of it.
Issues/problems I spot:
- Profilers still need to specify
default_mode
andpossible_modes
, which no longer makes sense as each profiler class now represents a single profiler and a single mode. IMO this is one of the problems we were trying to solve in this PR.
That would be okay but I see a few opportunities:
SystemProfiler
supports 3 possible on-modes: smart, dwarf, and fp. Either we refactor it with this PR (but that wouldn't align with the intention of this PR) or keep possible_modes for now, but add a new issue to solve it.And issue with default_mode
:
DotnetProfiler
is disabled by default, with use of default_mode
parameter; I think we might still need it for newly introduced profilers. I propose to keep this argument but at the Runtime level, renamed to: enabled_by_default
- As you mentioned, we lack the concept of "runtime arguments". This is visible for Python which has 2 profilers, and will be visible in others as well if we ever add more profielrs (e.g in Java, some of the arguments are truly async-profiler specific and some are not and will be relevant to any java profiler, e.g
--java-collect-jvm-flags
). "Runtime arguments" is where we could have placed thedefault_mode
andpossible_mode
arguments.My proposal:
- let's add the concept of a runtime object, and profilers will be attached to their runtime object. When generating commandline arguments, they will be taken into account separately (i.e there will be a section "Java arguments" and then "Java async-profiler arguments").
I'm all for it. I wanted to make it clear (and convince myself too) that it's really needed, that's why I paused implementation at this stage.
possible_modes
will be the default ones + the different registered runtime profilers for this runtime.
I agree we still need default mode - for the likes of DotnetProfiler and experimental ones.
- At the same time we can rename
JavaProfiler
toJavaAsyncProfiler
And register with profiler_name=ap
to support auto-generated possible_mode=ap
?
- This simplifies
get_profiler_arguments
Sure.
As discussed earlier, I have introduced runtime object in form of ProfilingRuntime
.
Runtime defines shared attributes of profilers: default_mode, mode_help, disablement_help and common_arguments.
InternalArgument
to handle arguments that are set in the code, not by arg parser, i.e.: perf_node_attach
, perf_inject
.I only briefly reviewed now.
Refactor py-spy & pyperf to separate ProfilerInterface classes.
Description
Group profilers by common runtime (e.g.: Python, Java, etc.). Support automatic selection of best available profiler for each runtime.
Related Issue
500
Motivation and Context
We have PythonProfiler which is a class that hides the fact that we have 2 profiler types behind - py-spy or pyperf, and decides itself which one is used. There was no mechanism to support multiple profiler options and keep them separate. More profilers will arrive in future that will benefit from this.
Also: #448
How Has This Been Tested?
Additional tests developed for new mechanism, added tests to ensure correct behavior of Python profiling.
Checklist: