[Sampling profiler] Factor out platform config 4/4

Fourth in a series factoring the ThreadProfiler platform specific
configuration state from the code that takes action on the state.
Defines a GetChildProcessEnableFraction function that returns how
frequently the profiler should be enabled within a given process.

The end goal is to reduce the configuration complexity, to support
enabling per-thread on Android.

Bug: 1129939
Change-Id: I936c671cd30f1750cf87d6ec8ce9aee45c051be4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2424672
Commit-Queue: Mike Wittman <wittman@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812248}

chrome/browser/chrome_content_browser_client.cc

@@ -2421,7 +2421,7 @@ void ChromeContentBrowserClient::AppendExtraCommandLineSwitches(
 #endif
 
   ThreadProfilerConfiguration::Get()->AppendCommandLineSwitchForChildProcess(
-      process_type, command_line);
+      command_line);
 
 #if defined(OS_LINUX) || defined(OS_CHROMEOS)
   // Processes may only query perf_event_open with the BPF sandbox disabled.

chrome/common/profiler/thread_profiler_configuration.cc

@@ -4,30 +4,17 @@
 
 #include "chrome/common/profiler/thread_profiler_configuration.h"
 
+#include "base/check.h"
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
+#include "base/notreached.h"
 #include "base/rand_util.h"
 #include "build/branding_buildflags.h"
-#include "build/build_config.h"
 #include "chrome/common/channel_info.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/profiler/thread_profiler_platform_configuration.h"
 #include "components/version_info/version_info.h"
 #include "content/public/common/content_switches.h"
-#include "extensions/buildflags/buildflags.h"
-#include "sandbox/policy/sandbox.h"
-
-#if defined(OS_WIN)
-#include "base/win/static_constants.h"
-#endif
-
-#if defined(OS_MAC)
-#include "base/mac/mac_util.h"
-#endif
-
-#if BUILDFLAG(ENABLE_EXTENSIONS)
-#include "extensions/common/switches.h"
-#endif
 
 namespace {
 
@@ -43,15 +30,6 @@ bool IsBrowserProcess() {
   return process_type.empty();
 }
 
-// True if the command line corresponds to an extension renderer process.
-bool IsExtensionRenderer(const base::CommandLine& command_line) {
-#if BUILDFLAG(ENABLE_EXTENSIONS)
-  return command_line.HasSwitch(extensions::switches::kExtensionProcess);
-#else
-  return false;
-#endif
-}
-
 // Allows the profiler to be run in a special browser test mode for testing that
 // profiles are collected as expected, by providing a switch value. The test
 // mode reduces the profiling duration to ensure the startup profiles complete
@@ -63,15 +41,6 @@ bool IsBrowserTestModeEnabled() {
          switches::kStartStackProfilerBrowserTest;
 }
 
-bool ShouldEnableProfilerForNextRendererProcess() {
-  // Ensure deterministic behavior for testing the profiler itself.
-  if (IsBrowserTestModeEnabled())
-    return true;
-
-  // Enable for every N-th renderer process, where N = 5.
-  return base::RandInt(0, 4) == 0;
-}
-
 }  // namespace
 
 ThreadProfilerConfiguration::ThreadProfilerConfiguration()
@@ -148,32 +117,23 @@ bool ThreadProfilerConfiguration::GetSyntheticFieldTrial(
 }
 
 void ThreadProfilerConfiguration::AppendCommandLineSwitchForChildProcess(
-    const std::string& process_type,
     base::CommandLine* command_line) const {
   DCHECK(IsBrowserProcess());
 
-  bool enable =
-      configuration_ == PROFILE_ENABLED || configuration_ == PROFILE_CONTROL;
-  if (!enable)
+  if (!(configuration_ == PROFILE_ENABLED || configuration_ == PROFILE_CONTROL))
     return;
-  if (process_type == switches::kGpuProcess ||
-      (process_type == switches::kUtilityProcess &&
-       // The network service is the only utility process that is profiled for
-       // now.
-       sandbox::policy::SandboxTypeFromCommandLine(*command_line) ==
-           sandbox::policy::SandboxType::kNetwork) ||
-      (process_type == switches::kRendererProcess &&
-       // Do not start the profiler for extension processes since profiling the
-       // compositor thread in them is not useful.
-       !IsExtensionRenderer(*command_line) &&
-       ShouldEnableProfilerForNextRendererProcess())) {
-    if (IsBrowserTestModeEnabled()) {
-      // Propagate the browser test mode switch argument to the child processes.
-      command_line->AppendSwitchASCII(switches::kStartStackProfiler,
-                                      switches::kStartStackProfilerBrowserTest);
-    } else {
-      command_line->AppendSwitch(switches::kStartStackProfiler);
-    }
+
+  const double enable_fraction =
+      platform_configuration_->GetChildProcessEnableFraction(*command_line);
+  if (!(base::RandDouble() < enable_fraction))
+    return;
+
+  if (IsBrowserTestModeEnabled()) {
+    // Propagate the browser test mode switch argument to the child processes.
+    command_line->AppendSwitchASCII(switches::kStartStackProfiler,
+                                    switches::kStartStackProfilerBrowserTest);
+  } else {
+    command_line->AppendSwitch(switches::kStartStackProfiler);
   }
 }
 

chrome/common/profiler/thread_profiler_configuration.h

@@ -42,7 +42,6 @@ class ThreadProfilerConfiguration {
   // Add a command line switch that instructs the child process to run the
   // profiler. This should only be called from the browser process.
   void AppendCommandLineSwitchForChildProcess(
-      const std::string& process_type,
       base::CommandLine* command_line) const;
 
   // Returns the ThreadProfilerConfiguration for the process.

chrome/common/profiler/thread_profiler_platform_configuration.cc

@@ -8,11 +8,17 @@
 #include "base/profiler/stack_sampling_profiler.h"
 #include "build/build_config.h"
 #include "content/public/common/content_switches.h"
+#include "extensions/buildflags/buildflags.h"
+#include "sandbox/policy/sandbox.h"
 
 #if defined(OS_ANDROID)
 #include "chrome/android/modules/stack_unwinder/public/module.h"
 #endif
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "extensions/common/switches.h"
+#endif
+
 namespace {
 
 // The default configuration to use in the absence of special circumstances on a
@@ -31,10 +37,16 @@ class DefaultPlatformConfiguration
       bool is_chrome_branded,
       version_info::Channel channel) const override;
 
+  double GetChildProcessEnableFraction(
+      const base::CommandLine& child_process_command_line) const override;
+
  protected:
   bool IsSupportedForChannel(bool is_chrome_branded,
                              version_info::Channel channel) const override;
 
+  // True if the command line corresponds to an extension renderer process.
+  bool IsExtensionRenderer(const base::CommandLine& command_line) const;
+
   bool browser_test_mode_enabled() const { return browser_test_mode_enabled_; }
 
  private:
@@ -72,6 +84,33 @@ DefaultPlatformConfiguration::GetEnableRates(
   }
 }
 
+double DefaultPlatformConfiguration::GetChildProcessEnableFraction(
+    const base::CommandLine& child_process_command_line) const {
+  const std::string& process_type =
+      child_process_command_line.GetSwitchValueASCII(switches::kProcessType);
+
+  if (process_type == switches::kGpuProcess)
+    return 1.0;
+
+  // The network service is the only utility process that is profiled for now.
+  if (process_type == switches::kUtilityProcess &&
+      sandbox::policy::SandboxTypeFromCommandLine(child_process_command_line) ==
+          sandbox::policy::SandboxType::kNetwork) {
+    return 1.0;
+  }
+
+  // Only start the profiler for non-extension renderer processes.
+  if (process_type == switches::kRendererProcess &&
+      !IsExtensionRenderer(child_process_command_line)) {
+    // Run the profiler in all renderer processes if the browser test mode is
+    // enabled, otherwise run in 20% of the processes to collect roughly as
+    // many profiles for renderer processes as browser processes.
+    return browser_test_mode_enabled() ? 1.0 : 0.2;
+  }
+
+  return 0.0;
+}
+
 bool DefaultPlatformConfiguration::IsSupportedForChannel(
     bool is_chrome_branded,
     version_info::Channel channel) const {
@@ -85,6 +124,16 @@ bool DefaultPlatformConfiguration::IsSupportedForChannel(
          channel == version_info::Channel::DEV;
 }
 
+// True if the command line corresponds to an extension renderer process.
+bool DefaultPlatformConfiguration::IsExtensionRenderer(
+    const base::CommandLine& command_line) const {
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  return command_line.HasSwitch(extensions::switches::kExtensionProcess);
+#else
+  return false;
+#endif
+}
+
 #if defined(OS_ANDROID)
 bool IsBrowserProcess() {
   return base::CommandLine::ForCurrentProcess()
@@ -107,6 +156,9 @@ class AndroidPlatformConfiguration : public DefaultPlatformConfiguration {
 
   void RequestRuntimeModuleInstall() const override;
 
+  double GetChildProcessEnableFraction(
+      const base::CommandLine& child_process_command_line) const override;
+
  protected:
   bool IsSupportedForChannel(bool is_chrome_branded,
                              version_info::Channel channel) const override;
@@ -156,6 +208,19 @@ void AndroidPlatformConfiguration::RequestRuntimeModuleInstall() const {
   stack_unwinder::Module::RequestInstallation();
 }
 
+double AndroidPlatformConfiguration::GetChildProcessEnableFraction(
+    const base::CommandLine& child_process_command_line) const {
+  // Profile all processes in browser test mode since they're disabled
+  // otherwise.
+  if (browser_test_mode_enabled()) {
+    return DefaultPlatformConfiguration::GetChildProcessEnableFraction(
+        child_process_command_line);
+  }
+
+  // TODO(https://crbug.com/1004855): Enable for all the default processes.
+  return 0.0;
+}
+
 bool AndroidPlatformConfiguration::IsSupportedForChannel(
     bool is_chrome_branded,
     version_info::Channel channel) const {

chrome/common/profiler/thread_profiler_platform_configuration.h

@@ -9,6 +9,10 @@
 
 #include "components/version_info/version_info.h"
 
+namespace base {
+class CommandLine;
+}
+
 // Encapsulates the platform-specific configuration for the ThreadProfiler.
 //
 // The interface functions this class make a distinction between 'supported' and
@@ -75,6 +79,12 @@ class ThreadProfilerPlatformConfiguration {
       bool is_chrome_branded,
       version_info::Channel channel) const = 0;
 
+  // Returns the fraction of the time that profiling should be randomly enabled
+  // for the child process to be executed with |child_process_command_line|. The
+  // return value is in the range [0.0, 1.0].
+  virtual double GetChildProcessEnableFraction(
+      const base::CommandLine& child_process_command_line) const = 0;
+
  protected:
   // True if the profiler is to be run for the channel/chrome branding on the
   // platform. Does not need to check whether the StackSamplingProfiler is

chrome/common/profiler/thread_profiler_platform_configuration_unittest.cc

@@ -4,11 +4,22 @@
 
 #include "chrome/common/profiler/thread_profiler_platform_configuration.h"
 
+#include <utility>
+
+#include "base/command_line.h"
 #include "base/profiler/profiler_buildflags.h"
 #include "build/build_config.h"
+#include "chrome/common/chrome_switches.h"
 #include "components/version_info/version_info.h"
+#include "content/public/common/content_switches.h"
+#include "extensions/buildflags/buildflags.h"
+#include "sandbox/policy/switches.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "extensions/common/switches.h"
+#endif
+
 #if (defined(OS_WIN) && defined(ARCH_CPU_X86_64)) || \
     (defined(OS_MAC) && defined(ARCH_CPU_X86_64)) || \
     (defined(OS_ANDROID) && BUILDFLAG(ENABLE_ARM_CFI_TABLE))
@@ -41,6 +52,30 @@ class ThreadProfilerPlatformConfigurationTest : public ::testing::Test {
   const std::unique_ptr<ThreadProfilerPlatformConfiguration> config_;
 };
 
+// Utility type and function for testing various command line switches.
+struct Switch {
+  explicit Switch(const char* name, const char* value = nullptr)
+      : name(name), value(value) {}
+
+  const char* name;
+  const char* value;
+};
+
+// Second argument should be a lambda running the expectations for the given
+// switch configuration.
+template <typename Func>
+void WithSwitches(std::initializer_list<Switch> switches, const Func& func) {
+  base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
+  for (const auto& switch_pair : switches) {
+    if (!switch_pair.value)
+      command_line.AppendSwitch(switch_pair.name);
+    else
+      command_line.AppendSwitchASCII(switch_pair.name, switch_pair.value);
+  }
+
+  func(command_line);
+}
+
 }  // namespace
 
 // Glue functions to make RelativePopulations work with googletest.
@@ -175,3 +210,68 @@ MAYBE_PLATFORM_CONFIG_TEST_F(ThreadProfilerPlatformConfigurationTest,
             config()->GetEnableRates(/*is_chrome_branded=*/true,
                                      version_info::Channel::UNKNOWN));
 }
+
+MAYBE_PLATFORM_CONFIG_TEST_F(ThreadProfilerPlatformConfigurationTest,
+                             GetChildProcessEnableFraction) {
+#if defined(OS_ANDROID)
+  WithSwitches(
+      {Switch(switches::kProcessType, switches::kGpuProcess)},
+      [this](const base::CommandLine command_line) {
+        EXPECT_EQ(0.0, config()->GetChildProcessEnableFraction(command_line));
+      });
+  WithSwitches(
+      {Switch(switches::kProcessType, switches::kUtilityProcess),
+       Switch(sandbox::policy::switches::kServiceSandboxType,
+              sandbox::policy::switches::kNetworkSandbox)},
+      [this](const base::CommandLine command_line) {
+        EXPECT_EQ(0.0, config()->GetChildProcessEnableFraction(command_line));
+      });
+  WithSwitches(
+      {Switch(switches::kProcessType, switches::kUtilityProcess)},
+      [this](const base::CommandLine command_line) {
+        EXPECT_EQ(0.0, config()->GetChildProcessEnableFraction(command_line));
+      });
+  WithSwitches(
+      {Switch(switches::kProcessType, switches::kRendererProcess)},
+      [this](const base::CommandLine command_line) {
+        EXPECT_EQ(0.0, config()->GetChildProcessEnableFraction(command_line));
+      });
+  // Should be an unrecognized scenario.
+  WithSwitches({}, [this](const base::CommandLine command_line) {
+    EXPECT_EQ(0.0, config()->GetChildProcessEnableFraction(command_line));
+  });
+#else
+  WithSwitches(
+      {Switch(switches::kProcessType, switches::kGpuProcess)},
+      [this](const base::CommandLine command_line) {
+        EXPECT_EQ(1.0, config()->GetChildProcessEnableFraction(command_line));
+      });
+  WithSwitches(
+      {Switch(switches::kProcessType, switches::kUtilityProcess),
+       Switch(sandbox::policy::switches::kServiceSandboxType,
+              sandbox::policy::switches::kNetworkSandbox)},
+      [this](const base::CommandLine command_line) {
+        EXPECT_EQ(1.0, config()->GetChildProcessEnableFraction(command_line));
+      });
+  WithSwitches(
+      {Switch(switches::kProcessType, switches::kUtilityProcess)},
+      [this](const base::CommandLine command_line) {
+        EXPECT_EQ(0.0, config()->GetChildProcessEnableFraction(command_line));
+      });
+  WithSwitches(
+      {Switch(switches::kProcessType, switches::kRendererProcess),
+       Switch(extensions::switches::kExtensionProcess)},
+      [this](const base::CommandLine command_line) {
+        EXPECT_EQ(0.0, config()->GetChildProcessEnableFraction(command_line));
+      });
+  WithSwitches(
+      {Switch(switches::kProcessType, switches::kRendererProcess)},
+      [this](const base::CommandLine command_line) {
+        EXPECT_EQ(0.2, config()->GetChildProcessEnableFraction(command_line));
+      });
+  // Should be an unrecognized scenario.
+  WithSwitches({}, [this](const base::CommandLine command_line) {
+    EXPECT_EQ(0.0, config()->GetChildProcessEnableFraction(command_line));
+  });
+#endif
+}