Refactor py-spy & pyperf to separate ProfilerInterface. #805
Conversation
marcin-ol
changed the title
|
Remaining actions for this PR:
|
marcin-ol
force-pushed
the
refactor-decouple-pyperf-and-pyspy
branch
from
165292f to
c11b6a1
Compare
tests/conftest.py
Outdated
|
|
||
| # Reuse gProfiler flow to initialize specific profiler instance - one selected by runtime fixture. | ||
| @contextmanager | ||
| def _make_profiler_instance() -> Iterator[ProfilerBase]: |
There was a problem hiding this comment.
What's the reason to create profiler instances in such implicit way and not directly as we did before?
Personally I prefer the explicit way. This new flow is harder to understand and is more error prone :/
There was a problem hiding this comment.
Yes, in hindsight that could be simpler.
tests/test_python.py
Outdated
| ) | ||
| wait_for_log(gprofiler, "gProfiler initialized and ready to start profiling", 0, timeout=7) | ||
| assert f"Initialized {profiler_class_name}".encode() in gprofiler.logs() | ||
| gprofiler.remove(force=True) |
There was a problem hiding this comment.
force=True uses SIGKILL, preventing gprofiler from doing proper cleanup. Why?
There was a problem hiding this comment.
No specific reason, I think I made a shortcut here.
Will replace it.
gprofiler/profilers/python_ebpf.py
Outdated
| "Python", | ||
| profiler_name="PyPerf", | ||
| is_preferred=True, | ||
| # py-spy is like pyspy, it's confusing and I mix between them |
There was a problem hiding this comment.
Comment not relevant here anymore
gprofiler/profilers/registry.py
Outdated
| def get_runtime_possible_modes(runtime: str) -> List[str]: | ||
| possible_modes: List[str] = [ProfilerConfig.ENABLED_MODE] if len(profilers_config[runtime]) > 1 else [] | ||
| for config in profilers_config[runtime]: | ||
| possible_modes += [m for m in config.get_active_modes() if m not in possible_modes] |
There was a problem hiding this comment.
Instead of checking for existence you can build it as a set and then convert to a list.
gprofiler/profilers/registry.py
Outdated
| arch = get_arch() | ||
| profiler_configs = sorted( | ||
| profilers_config[runtime], | ||
| key=lambda c: (arch in c.get_supported_archs(), c.is_preferred, c.profiler_name), |
There was a problem hiding this comment.
unsupported arches should be filtered out, not sorted away, no?
There was a problem hiding this comment.
Yes, they should be.
This was needed for some earlier revision of this PR and will be removed.
There was a problem hiding this comment.
On a second thought, this has one implication - we won't be able to explain to the user why profiler wasn't selected:
factory.get_profilers()is walking through this list of sorted profilers and will warn if some profilers are unavailable (on given architecture),- if we get an already-filtered list, user will only learn that
no profilers were selected.
To be on the safe side we should filter available profilers when building list of command-line arguments. Therefore another function needs changing - _add_profilers_arguments(), to do this filtering.
gprofiler/profilers/factory.py
Outdated
| runtime_args_prefix = runtime.lower().replace("-", "_") | ||
| runtime_mode = user_args.get(f"{runtime_args_prefix}_mode") |
There was a problem hiding this comment.
- Why do you need
replace(-, _)on the runtime name? - This logic (these 2 lines) repeat more than once, extract to a function?
There was a problem hiding this comment.
Not needed now. Was used when exploring different approaches.
gprofiler/profilers/factory.py
Outdated
| return system_profiler, process_profilers_instances | ||
| arch = get_arch() |
There was a problem hiding this comment.
I'd add a blank line between these 2 lines for clarity
gprofiler/profilers/factory.py
Outdated
| logger.critical( | ||
| f"Couldn't create the {profiler_name} profiler for runtime {runtime}, not continuing." | ||
| f" Request different profiler for runtime with --{runtime_args_prefix}-mode, or disable" | ||
| f" {runtime} profiling with --no-{runtime_args_prefix} to disable this profiler", |
There was a problem hiding this comment.
| f" {runtime} profiling with --no-{runtime_args_prefix} to disable this profiler", | |
| f" {runtime} profiling with --{runtime_args_prefix}-mode=none to disable this profiler", |
I prefer this way of expressing it (--no-... is legacy to me)
| process_profilers_instances.append(profiler_instance) | ||
|
|
||
| if len(requested_configs) == 1: | ||
| logger.critical( |
There was a problem hiding this comment.
This is a new fatal error. In which situations can it happen? When I force --python-mode=pyperf but PyPerf can't be used?
There was a problem hiding this comment.
Yes, that's the case.
Previously it was c-tor that could raise an Exception (i.e.: EbpfProfiler.test() called from PythonProfiler.__init__()).
Now the test is wrapped in check_readiness().
If it was the only available profiler (by the size of requested_configs), we raise an Exception, as previously.
Backward compatibility is retained.
gprofiler/profilers/python.py
Outdated
| "Python", | ||
| profiler_name="PySpy", | ||
| # py-spy is like pyspy, it's confusing and I mix between them | ||
| possible_modes=["auto", "pyspy", "py-spy"], |
There was a problem hiding this comment.
What does "auto" mean? This profiler no longer supports any "modes", it's just py-spy.
There was a problem hiding this comment.
It works the same as enabled and is listed here to support the previous behavior.
There was a problem hiding this comment.
I reviewed most of it.
Issues/problems I spot:
- Profilers still need to specify
default_modeandpossible_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. - 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_modeandpossible_modearguments.
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").
possible_modeswill be the default ones + the different registered runtime profilers for this runtime.- At the same time we can rename
JavaProfilertoJavaAsyncProfiler - This simplifies
get_profiler_arguments
…-pyperf-and-pyspy
That would be okay but I see a few opportunities:
And issue with
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.
I agree we still need default mode - for the likes of DotnetProfiler and experimental ones.
And register with
Sure. |
|
As discussed earlier, I have introduced runtime object in form of
|
gprofiler/profilers/python.py
Outdated
| "PySpy", | ||
| runtime_class=PythonRuntime, | ||
| possible_modes=["auto", "pyspy", "py-spy"], | ||
| # we build pyspy for both,. |
There was a problem hiding this comment.
| # we build pyspy for both,. | |
| # we build pyspy for both |
tests/conftest.py
Outdated
| @@ -626,7 +626,6 @@ def python_version(in_container: bool, application_docker_container: Container) | |||
| else: | |||
| # If not running in a container the test application runs on host | |||
| output = subprocess.check_output("python3 --version", stderr=subprocess.STDOUT, shell=True) | |||
|
|
|||
|
I only briefly reviewed now. |
…-pyperf-and-pyspy
…-pyperf-and-pyspy
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: