Only optionally run pprof for profiling - Set pprof to None by default for profiling probe - If None, auto-infer the best choice (e.g. pprof where applicable). - Add optional platform to env.check_installed Change-Id: Iffc7986efb7bcfb1f72411ea0a797aa181cf8674 Reviewed-on: https://chromium-review.googlesource.com/c/crossbench/+/7780808 Reviewed-by: Patrick Thier <pthier@chromium.org> Commit-Queue: Camillo Bruni <cbruni@chromium.org>
diff --git a/crossbench/env/base.py b/crossbench/env/base.py index aec0e72..e7c8a37 100644 --- a/crossbench/env/base.py +++ b/crossbench/env/base.py
@@ -78,10 +78,12 @@ def check_installed(self, binaries: Iterable[str], - message: str = "Missing binaries: {}") -> None: + message: str = "Missing binaries: {}", + platform: Optional[Platform] = None) -> None: assert not isinstance(binaries, str), "Expected iterable of strings." + target_platform = platform or self._platform missing_binaries = [ - binary for binary in binaries if not self._platform.which(binary) + binary for binary in binaries if not target_platform.which(binary) ] if missing_binaries: self.handle_validation_warning(message.format(missing_binaries))
diff --git a/crossbench/probes/profiling/context/linux.py b/crossbench/probes/profiling/context/linux.py index 5ab716d..82eb1d0 100644 --- a/crossbench/probes/profiling/context/linux.py +++ b/crossbench/probes/profiling/context/linux.py
@@ -24,6 +24,7 @@ if TYPE_CHECKING: import crossbench.path as pth + from crossbench.probes.profiling.system_profiling import ProfilingProbe from crossbench.probes.results import ProbeResult from crossbench.runner.run import Run @@ -39,6 +40,10 @@ JIT_DUMP_PATTERN, ) + def __init__(self, probe: ProfilingProbe, run: Run) -> None: + super().__init__(probe, run) + self._run_pprof: Optional[bool] = self.probe.run_pprof(run.browser) + @override def get_default_result_path(self) -> pth.AnyPath: result_dir = super().get_default_result_path() @@ -49,6 +54,11 @@ def has_perf_prof_path(self) -> bool: return self.browser.version > V8_PERF_PROF_PATH_FLAG_MIN_VERSION + @property + def run_pprof(self) -> bool: + assert self._run_pprof is not None, "pprof status not initialized" + return self._run_pprof + @override def setup(self) -> None: self.setup_v8_log_path() @@ -110,11 +120,11 @@ try: if self.probe.sample_js: perf_files = self._inject_v8_symbols(self.run, perf_files) - if self.probe.run_pprof: + if self.run_pprof: urls = self._export_to_pprof(self.run, perf_files) finally: self._clean_up_temp_files(self.run) - if self.probe.run_pprof: + if self.run_pprof: logging.debug("Profiling results: %s", urls) return self.browser_result(url=urls, file=raw_perf_files) if self.browser_platform.which("pprof"): @@ -177,7 +187,7 @@ def _export_to_pprof(self, run: Run, perf_files: list[pth.AnyPath]) -> list[str]: - assert self.probe.run_pprof + assert self.run_pprof run_details_json = json.dumps(run.get_browser_details_json()) with run.actions( f"Probe {self.probe.name}: " @@ -216,7 +226,7 @@ logging.debug("%s: skipping cleanup", self.probe) return if self.probe.cleanup_mode == CleanupMode.AUTO: - if not self.probe.run_pprof: + if not self.run_pprof: logging.debug("%s: skipping auto cleanup without pprof upload", self.probe) return
diff --git a/crossbench/probes/profiling/system_profiling.py b/crossbench/probes/profiling/system_profiling.py index 879102e..0eb9e53 100644 --- a/crossbench/probes/profiling/system_profiling.py +++ b/crossbench/probes/profiling/system_profiling.py
@@ -4,6 +4,7 @@ from __future__ import annotations +import functools import logging import shlex from typing import TYPE_CHECKING, Any, ClassVar, Final, Iterable, Optional, \ @@ -97,7 +98,6 @@ parser.add_argument( "pprof", type=bool, - default=True, help="linux-only: process collected samples with pprof.") parser.add_argument( "cleanup", @@ -199,7 +199,7 @@ self, js: bool = True, v8_interpreted_frames: Optional[bool] = None, - pprof: bool = True, + pprof: Optional[bool] = None, cleanup: CleanupMode = CleanupMode.AUTO, browser_process: bool = False, spare_renderer_process: bool = False, @@ -218,7 +218,7 @@ self._sample_js: bool = js self._sample_browser_process: bool = browser_process self._spare_renderer_process: bool = spare_renderer_process - self._run_pprof: bool = pprof + self._run_pprof: Optional[bool] = pprof self._cleanup_mode = cleanup if v8_interpreted_frames is None: v8_interpreted_frames = js @@ -270,9 +270,14 @@ def sample_browser_process(self) -> bool: return self._sample_browser_process - @property - def run_pprof(self) -> bool: - return self._run_pprof + @functools.lru_cache(maxsize=None) + def run_pprof(self, browser: Browser) -> bool: + if self._run_pprof is not None: + return self._run_pprof + if not browser.platform.is_linux: + return False + return (browser.platform.which("pprof") is not None and + browser.platform.which("gcert") is not None) @property def cleanup_mode(self) -> CleanupMode: @@ -344,7 +349,7 @@ if browser.attributes().is_chromium_based: chromium = cast(ChromiumBased, browser) self._validate_chromium_based(chromium) - if self.run_pprof: + if self.run_pprof(browser): self._validate_pprof(env, browser) # Check that certain Android-only options are # not provided by on other platforms. @@ -387,7 +392,8 @@ f"{repr(name)} is currently only supported on {platforms}") def _validate_linux(self, env: RunnerEnv, browser: Browser) -> None: - env.check_installed(binaries=["pprof"]) + if self.run_pprof(browser): + env.check_installed(binaries=["pprof"], platform=browser.platform) assert browser.platform.which("perf"), "Please install linux-perf" def _validate_macos(self, env: RunnerEnv, browser: Browser) -> None: @@ -420,7 +426,7 @@ assert self._run_pprof host_platform = browser.host_platform self._run_pprof = host_platform.which("gcert") is not None - if not self.run_pprof: + if not self.run_pprof(browser): logging.warning( "Disabled automatic pprof uploading for non-googler machine.") return
diff --git a/tests/crossbench/probes/profiling/test_system_profiling.py b/tests/crossbench/probes/profiling/test_system_profiling.py index 009b91f..112816a 100644 --- a/tests/crossbench/probes/profiling/test_system_profiling.py +++ b/tests/crossbench/probes/profiling/test_system_profiling.py
@@ -6,13 +6,16 @@ import argparse import pathlib import unittest +from unittest import mock from typing_extensions import override +from crossbench.browsers.chromium.version import ChromiumVersion from crossbench.browsers.settings import Settings from crossbench.probes import all as all_probes from crossbench.probes.profiling.context.android import \ generate_simpleperf_command_line +from crossbench.probes.profiling.context.linux import LinuxProfilingContext from crossbench.probes.profiling.system_profiling import RENDERER_CMD_PATH, \ CallGraphMode, CleanupMode, ProfilingProbe, TargetMode from tests import test_helper @@ -253,6 +256,41 @@ probe = ProfilingProbe.parse_str("renderer_main_only") self.assertEqual(probe.target, TargetMode.RENDERER_MAIN_ONLY) + def test_pprof_default_none(self): + probe = ProfilingProbe() + self.assertIsNone(probe._run_pprof) + + def test_run_pprof_method(self): + probe = ProfilingProbe(pprof=True) + mock_browser1 = mock.Mock() + self.assertTrue(probe.run_pprof(mock_browser1)) + + probe = ProfilingProbe(pprof=False) + mock_browser2 = mock.Mock() + self.assertFalse(probe.run_pprof(mock_browser2)) + + probe = ProfilingProbe(pprof=None) + + mock_browser3 = mock.Mock() + mock_browser3.platform.is_linux = False + self.assertFalse(probe.run_pprof(mock_browser3)) + + mock_browser4 = mock.Mock() + mock_browser4.platform = LinuxMockPlatform(fake_fs=self.fs) + mock_browser4.platform.install_mock_binary("pprof", "/usr/bin/pprof4") + mock_browser4.platform.install_mock_binary("gcert", "/usr/bin/gcert4") + self.assertTrue(probe.run_pprof(mock_browser4)) + + mock_browser5 = mock.Mock() + mock_browser5.platform = LinuxMockPlatform(fake_fs=self.fs) + mock_browser5.platform.install_mock_binary("gcert", "/usr/bin/gcert5") + self.assertFalse(probe.run_pprof(mock_browser5)) + + mock_browser6 = mock.Mock() + mock_browser6.platform = LinuxMockPlatform(fake_fs=self.fs) + mock_browser6.platform.install_mock_binary("pprof", "/usr/bin/pprof6") + self.assertFalse(probe.run_pprof(mock_browser6)) + def test_resolve_target_mode(self): probe = ProfilingProbe() self.assertEqual(probe.target, TargetMode.AUTO) @@ -300,7 +338,7 @@ self.assertTrue(probe.key) self.assertFalse(probe.sample_js) self.assertTrue(probe.sample_browser_process) - self.assertFalse(probe.run_pprof) + self.assertFalse(probe._run_pprof) self.assertTrue(probe.cleanup_mode, CleanupMode.NEVER) self.assertEqual(probe.target, TargetMode.RENDERER_PROCESS_ONLY) self.assertTrue(probe.start_profiling_after_setup(probe.target)) @@ -381,6 +419,55 @@ settings=Settings(platform=linux_platform)).attach_probe(probe) +class LinuxProfilingContextTestCase(GenericProbeTestCase): + + @override + def setUp(self): + super().setUp() + self.probe = ProfilingProbe(pprof=None) + self.platform = LinuxMockPlatform(fake_fs=self.fs) + self.run = mock.Mock() + self.run.browser = mock.Mock() + self.run.browser.platform = self.platform + self.run.browser.version = ChromiumVersion((120, 0, 0, 0)) + self.run.result_path = pathlib.Path("/tmp/test_result") + self.run.session = mock.Mock() + self.run.session.extra_js_flags = {} + self.run.get_default_probe_result_path = mock.Mock( + return_value=pathlib.Path("/tmp/test_result")) + self.platform.absolute = mock.Mock( + return_value=pathlib.Path("/tmp/test_result")) + + def test_auto_infer_pprof_true(self): + self.platform.install_mock_binary("pprof", "/usr/bin/pprof") + self.platform.install_mock_binary("gcert", "/usr/bin/gcert") + context = LinuxProfilingContext(self.probe, self.run) + context.setup_v8_log_path = mock.Mock() + context.setup() + self.assertTrue(context.run_pprof) + + def test_auto_infer_pprof_false_missing_pprof(self): + self.platform.install_mock_binary("gcert", "/usr/bin/gcert") + context = LinuxProfilingContext(self.probe, self.run) + context.setup_v8_log_path = mock.Mock() + context.setup() + self.assertFalse(context.run_pprof) + + def test_auto_infer_pprof_false_missing_gcert(self): + self.platform.install_mock_binary("pprof", "/usr/bin/pprof") + context = LinuxProfilingContext(self.probe, self.run) + context.setup_v8_log_path = mock.Mock() + context.setup() + self.assertFalse(context.run_pprof) + + def test_explicit_pprof_true(self): + self.probe = ProfilingProbe(pprof=True) + context = LinuxProfilingContext(self.probe, self.run) + context.setup_v8_log_path = mock.Mock() + context.setup() + self.assertTrue(context.run_pprof) + + class EnumTestCase(unittest.TestCase): def test_cleanup_mode(self):