[Sampling profiler] Decouple browser and child process configs

Changes the internal configuration representation within
ThreadProfilerConfiguration to cleanly separate the configuration
representation for the browser process from the one for child
processes. The configuration state for the two does not overlap
conceptually so it's not desirable for them to share the same
representation.

Also explicitly separates the browser process configuration state
that governs whether to register a synthetic field trial from the
state that says what group to use when registering.

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

chrome/common/profiler/BUILD.gn

@@ -32,6 +32,7 @@ source_set("profiler") {
     "//components/version_info:version_info",
     "//content/public/common",
     "//extensions/buildflags",
+    "//third_party/abseil-cpp:absl",
   ]
 
   if (is_android) {

chrome/common/profiler/thread_profiler_configuration.cc

@@ -47,12 +47,11 @@ base::Optional<version_info::Channel> GetReleaseChannel() {
 }  // namespace
 
 ThreadProfilerConfiguration::ThreadProfilerConfiguration()
-    : current_process_(
-          GetProfileParamsProcess(*base::CommandLine::ForCurrentProcess())),
-      platform_configuration_(ThreadProfilerPlatformConfiguration::Create(
+    : platform_configuration_(ThreadProfilerPlatformConfiguration::Create(
           IsBrowserTestModeEnabled())),
-      configuration_(
-          GenerateConfiguration(current_process_, *platform_configuration_)) {}
+      configuration_(GenerateConfiguration(
+          GetProfileParamsProcess(*base::CommandLine::ForCurrentProcess()),
+          *platform_configuration_)) {}
 
 ThreadProfilerConfiguration::~ThreadProfilerConfiguration() = default;
 
@@ -72,36 +71,38 @@ ThreadProfilerConfiguration::GetSamplingParams() const {
 }
 
 bool ThreadProfilerConfiguration::IsProfilerEnabledForCurrentProcess() const {
-  if (current_process_ == metrics::CallStackProfileParams::BROWSER_PROCESS) {
-    return configuration_ == PROFILE_ENABLED ||
-           configuration_ == PROFILE_CONTROL;
+  if (const ChildProcessConfiguration* child_process_configuration =
+          absl::get_if<ChildProcessConfiguration>(&configuration_)) {
+    return *child_process_configuration == CHILD_PROCESS_PROFILE_ENABLED;
   }
 
-  DCHECK_EQ(PROFILE_FROM_COMMAND_LINE, configuration_);
-  // This is a child process. The |kStartStackProfiler| switch passed by the
-  // browser process determines whether the profiler is enabled for the process.
-  return base::CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kStartStackProfiler);
+  const base::Optional<VariationGroup>& variation_group =
+      absl::get<BrowserProcessConfiguration>(configuration_);
+
+  return EnableForVariationGroup(variation_group);
 }
 
 bool ThreadProfilerConfiguration::IsProfilerEnabledForCurrentProcessAndThread(
     metrics::CallStackProfileParams::Thread thread) const {
   return IsProfilerEnabledForCurrentProcess() &&
-         platform_configuration_->IsEnabledForThread(current_process_, thread);
+         platform_configuration_->IsEnabledForThread(
+             GetProfileParamsProcess(*base::CommandLine::ForCurrentProcess()),
+             thread);
 }
 
 bool ThreadProfilerConfiguration::GetSyntheticFieldTrial(
     std::string* trial_name,
     std::string* group_name) const {
-  DCHECK_EQ(metrics::CallStackProfileParams::BROWSER_PROCESS, current_process_);
+  DCHECK(absl::holds_alternative<BrowserProcessConfiguration>(configuration_));
+  const base::Optional<VariationGroup>& variation_group =
+      absl::get<BrowserProcessConfiguration>(configuration_);
 
-  if (!platform_configuration_->IsSupported(GetReleaseChannel())) {
+  if (!variation_group.has_value())
     return false;
-  }
 
   *trial_name = "SyntheticStackProfilingConfiguration";
   *group_name = std::string();
-  switch (configuration_) {
+  switch (*variation_group) {
     case PROFILE_DISABLED:
       *group_name = "Disabled";
       break;
@@ -117,20 +118,18 @@ bool ThreadProfilerConfiguration::GetSyntheticFieldTrial(
     case PROFILE_ENABLED:
       *group_name = "Enabled";
       break;
-
-    case PROFILE_FROM_COMMAND_LINE:
-      NOTREACHED();
-      break;
   }
 
-  return !group_name->empty();
+  return true;
 }
 
 void ThreadProfilerConfiguration::AppendCommandLineSwitchForChildProcess(
     base::CommandLine* child_process_command_line) const {
-  DCHECK_EQ(metrics::CallStackProfileParams::BROWSER_PROCESS, current_process_);
+  DCHECK(absl::holds_alternative<BrowserProcessConfiguration>(configuration_));
+  const base::Optional<VariationGroup>& variation_group =
+      absl::get<BrowserProcessConfiguration>(configuration_);
 
-  if (!(configuration_ == PROFILE_ENABLED || configuration_ == PROFILE_CONTROL))
+  if (!EnableForVariationGroup(variation_group))
     return;
 
   const metrics::CallStackProfileParams::Process child_process =
@@ -156,9 +155,18 @@ ThreadProfilerConfiguration* ThreadProfilerConfiguration::Get() {
 }
 
 // static
-ThreadProfilerConfiguration::ProfileConfiguration
-ThreadProfilerConfiguration::ChooseConfiguration(
-    const std::vector<Variation>& variations) {
+bool ThreadProfilerConfiguration::EnableForVariationGroup(
+    base::Optional<VariationGroup> variation_group) {
+  // Enable if assigned to a variation group, and the group is one of the groups
+  // that are to be enabled.
+  return variation_group.has_value() && (*variation_group == PROFILE_ENABLED ||
+                                         *variation_group == PROFILE_CONTROL);
+}
+
+// static
+ThreadProfilerConfiguration::VariationGroup
+ThreadProfilerConfiguration::ChooseVariationGroup(
+    std::initializer_list<Variation> variations) {
   int total_weight = 0;
   for (const Variation& variation : variations)
     total_weight += variation.weight;
@@ -169,7 +177,7 @@ ThreadProfilerConfiguration::ChooseConfiguration(
   for (const auto& variation : variations) {
     if (chosen >= cumulative_weight &&
         chosen < cumulative_weight + variation.weight) {
-      return variation.config;
+      return variation.group;
     }
     cumulative_weight += variation.weight;
   }
@@ -178,17 +186,14 @@ ThreadProfilerConfiguration::ChooseConfiguration(
 }
 
 // static
-ThreadProfilerConfiguration::ProfileConfiguration
-ThreadProfilerConfiguration::GenerateConfiguration(
-    metrics::CallStackProfileParams::Process process,
+ThreadProfilerConfiguration::BrowserProcessConfiguration
+ThreadProfilerConfiguration::GenerateBrowserProcessConfiguration(
     const ThreadProfilerPlatformConfiguration& platform_configuration) {
-  if (process != metrics::CallStackProfileParams::BROWSER_PROCESS)
-    return PROFILE_FROM_COMMAND_LINE;
-
   const base::Optional<version_info::Channel> release_channel =
       GetReleaseChannel();
+
   if (!platform_configuration.IsSupported(release_channel))
-    return PROFILE_DISABLED;
+    return base::nullopt;
 
   using RuntimeModuleState =
       ThreadProfilerPlatformConfiguration::RuntimeModuleState;
@@ -209,9 +214,33 @@ ThreadProfilerConfiguration::GenerateConfiguration(
           platform_configuration.GetEnableRates(release_channel);
 
   CHECK_EQ(0, relative_populations.experiment % 2);
-  return ChooseConfiguration({
+  return ChooseVariationGroup({
       {PROFILE_ENABLED, relative_populations.enabled},
       {PROFILE_CONTROL, relative_populations.experiment / 2},
       {PROFILE_DISABLED, relative_populations.experiment / 2},
   });
 }
+
+// static
+ThreadProfilerConfiguration::ChildProcessConfiguration
+ThreadProfilerConfiguration::GenerateChildProcessConfiguration(
+    const base::CommandLine& command_line) {
+  // In a child process the |kStartStackProfiler| switch passed by the
+  // browser process determines whether the profiler is enabled for the
+  // process.
+  return command_line.HasSwitch(switches::kStartStackProfiler)
+             ? CHILD_PROCESS_PROFILE_ENABLED
+             : CHILD_PROCESS_PROFILE_DISABLED;
+}
+
+// static
+ThreadProfilerConfiguration::Configuration
+ThreadProfilerConfiguration::GenerateConfiguration(
+    metrics::CallStackProfileParams::Process process,
+    const ThreadProfilerPlatformConfiguration& platform_configuration) {
+  if (process == metrics::CallStackProfileParams::BROWSER_PROCESS)
+    return GenerateBrowserProcessConfiguration(platform_configuration);
+
+  return GenerateChildProcessConfiguration(
+      *base::CommandLine::ForCurrentProcess());
+}

chrome/common/profiler/thread_profiler_configuration.h

@@ -5,12 +5,15 @@
 #ifndef CHROME_COMMON_PROFILER_THREAD_PROFILER_CONFIGURATION_H_
 #define CHROME_COMMON_PROFILER_THREAD_PROFILER_CONFIGURATION_H_
 
+#include <initializer_list>
 #include <string>
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/profiler/stack_sampling_profiler.h"
 #include "components/metrics/call_stack_profile_params.h"
+#include "third_party/abseil-cpp/absl/types/variant.h"
 
 namespace base {
 class CommandLine;
@@ -53,50 +56,77 @@ class ThreadProfilerConfiguration {
   static ThreadProfilerConfiguration* Get();
 
  private:
-  // Configuration to use for this Chrome instance.
-  enum ProfileConfiguration {
-    // Chrome-wide configurations set in the browser process.
+  // The variation groups that represent the Chrome-wide profiling
+  // configurations.
+  enum VariationGroup {
+    // Disabled within the experiment.
     PROFILE_DISABLED,
+
+    // Disabled because the required module is not installed, and outside the
+    // experiment.
     PROFILE_DISABLED_MODULE_NOT_INSTALLED,
+
+    // Enabled within the experiment (and paired with equal-sized
+    // PROFILE_DISABLED group).
     PROFILE_CONTROL,
+
+    // Enabled outside of the experiment.
     PROFILE_ENABLED,
+  };
+
+  // The configuration state for the browser process. If !has_value() profiling
+  // is disabled and no variations state is reported. Otherwise profiling is
+  // enabled based on the VariationGroup and the variation state is reported.
+  using BrowserProcessConfiguration = base::Optional<VariationGroup>;
 
-    // Configuration set in the child processes, which receive their enable
-    // state on the command line from the browser process.
-    PROFILE_FROM_COMMAND_LINE
+  // The configuration state in child processes.
+  enum ChildProcessConfiguration {
+    CHILD_PROCESS_PROFILE_DISABLED,
+    CHILD_PROCESS_PROFILE_ENABLED,
   };
 
+  // The configuration state for the current process, browser or child.
+  using Configuration =
+      absl::variant<BrowserProcessConfiguration, ChildProcessConfiguration>;
+
   // Configuration variations, along with weights to use when randomly choosing
   // one of a set of variations.
   struct Variation {
-    ProfileConfiguration config;
+    VariationGroup group;
     int weight;
   };
 
-  // Randomly chooses a configuration from the weighted variations. Weights are
+  // True if the profiler is to be enabled for |variation_group|.
+  static bool EnableForVariationGroup(
+      base::Optional<VariationGroup> variation_group);
+
+  // Randomly chooses a variation from the weighted variations. Weights are
   // expected to sum to 100 as a sanity check.
-  static ProfileConfiguration ChooseConfiguration(
-      const std::vector<Variation>& variations);
+  static VariationGroup ChooseVariationGroup(
+      std::initializer_list<Variation> variations);
+
+  // Generates a configuration for the browser process.
+  static BrowserProcessConfiguration GenerateBrowserProcessConfiguration(
+      const ThreadProfilerPlatformConfiguration& platform_configuration);
 
-  // Generates sampling profiler configurations for all processes.
-  static ProfileConfiguration GenerateConfiguration(
+  // Generates a configuration for a child process.
+  static ChildProcessConfiguration GenerateChildProcessConfiguration(
+      const base::CommandLine& command_line);
+
+  // Generates a configuration for the current process.
+  static Configuration GenerateConfiguration(
       metrics::CallStackProfileParams::Process process,
       const ThreadProfilerPlatformConfiguration& platform_configuration);
 
   // NOTE: all state in this class must be const and initialized at construction
   // time to ensure thread-safe access post-construction.
 
-  // The currently-executing process.
-  const metrics::CallStackProfileParams::Process current_process_;
-
   // Platform-dependent configuration upon which |configuration_| is based.
   const std::unique_ptr<ThreadProfilerPlatformConfiguration>
       platform_configuration_;
 
-  // In the browser process this represents the configuration to use across all
-  // Chrome processes. In the child processes it is always
-  // PROFILE_FROM_COMMAND_LINE.
-  const ProfileConfiguration configuration_;
+  // Represents the configuration to use in the current process.
+  const Configuration configuration_;
 
   DISALLOW_COPY_AND_ASSIGN(ThreadProfilerConfiguration);
 };