Revert "Convert HeapProfiler JSON params to regular FeatureParams"

This reverts commit 1c0d6593cb.

Reason for revert: Caused crash on launch in Canary b/382002928

Original change's description:
> Convert HeapProfiler JSON params to regular FeatureParams
>
> Now that the heap profiler is controlled centrally from the browser
> process, many params are redundant. This simplifies the params that
> were in per-process JSON dicts:
>
> * `is_supported` is no longer needed. To disable in a subprocess, set
>   its snapshot probability (eg. `gpu-prob-pct`, `renderer-proc-pct`)
>   to 0. To disable in the browser process, disable the
>   "HeapProfilerReporting" feature.
> * `stable_probability`, `nonstable_probability` and
>   `collection_interval` now only affect the browser process, so make
>   them global FeatureParams.
> * `sampling_rate_bytes` is split into separate FeatureParams for each
>   process (eg. `gpu-sampling-rate-bytes`, `renderer-sampling-rate-
>   bytes`).
>
> Also removes the test that enables profiler reports in a child process
> but not the browser process, since there's no longer any combination
> of feature params that can do this. (Since HeapProfilerCentralControl
> launched, the test only checked that the giving params that enabled a
> child process without enabling the browser did nothing.)
>
> Bug: 40840943
> Change-Id: I070d8ec1035ab6c21fe676e3a44c15e199508b9c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6056270
> Commit-Queue: Joe Mason <joenotcharles@google.com>
> Reviewed-by: Sean Maher <spvw@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1390539}

Bug: 40840943, 382002928
Change-Id: I5a0bd185d48620983348a13944ff7cc212b29e13
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6062973
Reviewed-by: Ben Mason <benmason@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Daniel Yip <danielyip@google.com>
Commit-Queue: Daniel Yip <danielyip@google.com>
Owners-Override: Ben Mason <benmason@chromium.org>
Auto-Submit: Daniel Yip <danielyip@google.com>
Cr-Commit-Position: refs/heads/main@{#1390998}

components/heap_profiling/in_process/BUILD.gn

@@ -55,7 +55,10 @@ source_set("unit_tests") {
   # HeapProfilerController's dependencies are not compiled on iOS unless
   # use_allocator_shim is true.
   if (!is_ios || use_allocator_shim) {
-    sources = [ "heap_profiler_controller_unittest.cc" ]
+    sources = [
+      "heap_profiler_controller_unittest.cc",
+      "heap_profiler_parameters_unittest.cc",
+    ]
     deps = [
       ":in_process",
       ":mojom",

components/heap_profiling/in_process/browser_process_snapshot_controller.cc

@@ -7,12 +7,14 @@
 #include <memory>
 #include <utility>
 
-#include "base/check.h"
+#include "base/check_op.h"
 #include "base/containers/flat_map.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/metrics/field_trial_params.h"
+#include "base/notreached.h"
 #include "base/rand_util.h"
 #include "base/sequence_checker.h"
 #include "base/task/sequenced_task_runner.h"
@@ -97,8 +99,25 @@ void BrowserProcessSnapshotController::TakeSnapshotsOnSnapshotSequence() {
       // processes to measure.
       continue;
     }
-    const int snapshot_probability_pct =
-        GetSnapshotProbabilityForProcess(process_type);
+    int snapshot_probability_pct;
+    switch (process_type) {
+      case sampling_profiler::ProfilerProcessType::kGpu:
+        snapshot_probability_pct = kGpuSnapshotProbability.Get();
+        break;
+      case sampling_profiler::ProfilerProcessType::kNetworkService:
+        snapshot_probability_pct = kNetworkSnapshotProbability.Get();
+        break;
+      case sampling_profiler::ProfilerProcessType::kRenderer:
+        snapshot_probability_pct = kRendererSnapshotProbability.Get();
+        break;
+      case sampling_profiler::ProfilerProcessType::kUtility:
+        snapshot_probability_pct = kUtilitySnapshotProbability.Get();
+        break;
+      default:
+        NOTREACHED();
+    }
+    CHECK_GE(snapshot_probability_pct, 0);
+    CHECK_LE(snapshot_probability_pct, 100);
     if (snapshot_probability_pct == 0) {
       // No need to test each process since none will be chosen.
       continue;

components/heap_profiling/in_process/heap_profiler_controller.cc

@@ -17,12 +17,10 @@
 #include "base/allocator/dispatcher/reentry_guard.h"
 #include "base/check_op.h"
 #include "base/command_line.h"
-#include "base/feature_list.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
-#include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/metrics_hashes.h"
 #include "base/notreached.h"
@@ -45,7 +43,6 @@
 #include "components/metrics/call_stacks/call_stack_profile_builder.h"
 #include "components/sampling_profiler/process_type.h"
 #include "components/services/heap_profiling/public/cpp/merge_samples.h"
-#include "components/variations/variations_switches.h"
 #include "components/version_info/channel.h"
 #include "third_party/abseil-cpp/absl/cleanup/cleanup.h"
 
@@ -110,18 +107,19 @@ std::string ProcessHistogramName(std::string_view base_name,
   }
 }
 
-double GetChannelProbability(version_info::Channel channel) {
+double GetChannelProbability(version_info::Channel channel,
+                             const HeapProfilerParameters& params) {
   switch (channel) {
     case version_info::Channel::STABLE:
     case version_info::Channel::UNKNOWN:
       // If the channel can't be determined, treat it as `stable` for safety.
       // Don't disable heap profiling completely so that developers can still
       // enable it with --enable-feature flags.
-      return kStableProbability.Get();
+      return params.stable_probability;
     case version_info::Channel::BETA:
     case version_info::Channel::DEV:
     case version_info::Channel::CANARY:
-      return kNonStableProbability.Get();
+      return params.nonstable_probability;
   }
   NOTREACHED();
 }
@@ -142,19 +140,15 @@ std::pair<bool, std::optional<std::string>> DecideIfCollectionIsEnabled(
     return {is_enabled, std::nullopt};
   }
 
-  // Never profile during benchmarking.
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          variations::switches::kEnableBenchmarking)) {
-    return {false, std::nullopt};
-  }
-
-  if (!base::FeatureList::IsEnabled(kHeapProfilerReporting)) {
-    return {false, std::nullopt};
-  }
-
   // Randomly determine whether profiling is enabled.
+  HeapProfilerParameters params =
+      GetHeapProfilerParametersForProcess(process_type);
+  if (!params.is_supported) {
+    return {false, std::nullopt};
+  }
+
   const double seed = base::RandDouble();
-  const double probability = GetChannelProbability(channel);
+  const double probability = GetChannelProbability(channel, params);
   if (seed < probability) {
     return {true, "Enabled"};
   }
@@ -263,9 +257,13 @@ bool HeapProfilerController::StartIfEnabled() {
   if (!profiling_enabled_) {
     return false;
   }
-  const size_t sampling_rate_bytes = GetSamplingRateForProcess(process_type_);
-  if (sampling_rate_bytes > 0) {
-    base::SamplingHeapProfiler::Get()->SetSamplingInterval(sampling_rate_bytes);
+  HeapProfilerParameters profiler_params =
+      GetHeapProfilerParametersForProcess(process_type_);
+  // DecideIfCollectionIsEnabled() should return false if not supported.
+  DCHECK(profiler_params.is_supported);
+  if (profiler_params.sampling_rate_bytes > 0) {
+    base::SamplingHeapProfiler::Get()->SetSamplingInterval(
+        profiler_params.sampling_rate_bytes);
   }
   base::SamplingHeapProfiler::Get()->Start();
 
@@ -274,10 +272,9 @@ bool HeapProfilerController::StartIfEnabled() {
     return true;
   }
 
-  const base::TimeDelta collection_interval = kCollectionInterval.Get();
-  CHECK(collection_interval.is_positive());
+  DCHECK(profiler_params.collection_interval.is_positive());
   SnapshotParams params(
-      collection_interval,
+      profiler_params.collection_interval,
       /*use_random_interval=*/!suppress_randomness_for_testing_, stopped_,
       process_type_, creation_time_, std::move(on_first_snapshot_callback_));
   params.trigger_child_process_snapshot_closure = base::BindRepeating(
@@ -371,7 +368,7 @@ void HeapProfilerController::AppendCommandLineSwitchInternal(
     BrowserProcessSnapshotController* snapshot_controller) {
   CHECK_NE(child_process_type, ProcessType::kBrowser);
   if (snapshot_controller &&
-      GetSnapshotProbabilityForProcess(child_process_type) > 0) {
+      GetHeapProfilerParametersForProcess(child_process_type).is_supported) {
     command_line->AppendSwitch(switches::kSubprocessHeapProfiling);
     snapshot_controller->BindRemoteForChildProcess(child_process_id,
                                                    child_process_type);

components/heap_profiling/in_process/heap_profiler_controller_unittest.cc

@@ -19,12 +19,16 @@
 #include "base/auto_reset.h"
 #include "base/barrier_closure.h"
 #include "base/command_line.h"
+#include "base/containers/contains.h"
+#include "base/containers/enum_set.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/functional/callback_helpers.h"
+#include "base/json/json_writer.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/metrics_hashes.h"
+#include "base/notreached.h"
 #include "base/process/launch.h"
 #include "base/process/process.h"
 #include "base/sampling_heap_profiler/poisson_allocation_sampler.h"
@@ -42,6 +46,7 @@
 #include "base/test/test_timeouts.h"
 #include "base/time/time.h"
 #include "base/types/optional_util.h"
+#include "base/values.h"
 #include "build/build_config.h"
 #include "components/heap_profiling/in_process/browser_process_snapshot_controller.h"
 #include "components/heap_profiling/in_process/child_process_snapshot_controller.h"
@@ -113,12 +118,21 @@ namespace {
 #define ENABLE_MULTIPROCESS_TESTS 1
 #endif
 
+using FeatureRef = base::test::FeatureRef;
+using FeatureRefAndParams = base::test::FeatureRefAndParams;
+using ProcessType = sampling_profiler::ProfilerProcessType;
+using ProcessTypeSet =
+    base::EnumSet<ProcessType, ProcessType::kUnknown, ProcessType::kMax>;
+using ProfileCollectorCallback =
+    base::RepeatingCallback<void(base::TimeTicks, metrics::SampledProfile)>;
 using base::allocator::dispatcher::AllocationNotificationData;
 using base::allocator::dispatcher::AllocationSubsystem;
 using base::allocator::dispatcher::FreeNotificationData;
-using base::test::FeatureRef;
-using base::test::FeatureRefAndParams;
-using sampling_profiler::ProfilerProcessType;
+using ScopedMuteHookedSamplesForTesting =
+    base::PoissonAllocationSampler::ScopedMuteHookedSamplesForTesting;
+using ScopedSuppressRandomnessForTesting =
+    base::PoissonAllocationSampler::ScopedSuppressRandomnessForTesting;
+
 using ::testing::_;
 using ::testing::AllOf;
 using ::testing::Conditional;
@@ -131,13 +145,6 @@ using ::testing::Property;
 using ::testing::ResultOf;
 using ::testing::UnorderedElementsAreArray;
 
-using ProfileCollectorCallback =
-    base::RepeatingCallback<void(base::TimeTicks, metrics::SampledProfile)>;
-using ScopedMuteHookedSamplesForTesting =
-    base::PoissonAllocationSampler::ScopedMuteHookedSamplesForTesting;
-using ScopedSuppressRandomnessForTesting =
-    base::PoissonAllocationSampler::ScopedSuppressRandomnessForTesting;
-
 constexpr size_t kSamplingRate = 1024;
 constexpr size_t kAllocationSize = 42 * kSamplingRate;
 
@@ -393,9 +400,8 @@ class MultiprocessTestChild final : public mojom::TestConnector,
 
     // Start the heap profiler and wait for TakeSnapshot() messages from the
     // parent.
-    HeapProfilerController controller(
-        version_info::Channel::STABLE,
-        static_cast<ProfilerProcessType>(process_type));
+    HeapProfilerController controller(version_info::Channel::STABLE,
+                                      static_cast<ProcessType>(process_type));
     controller.SuppressRandomnessForTesting();
     ASSERT_TRUE(controller.IsEnabled());
     controller.StartIfEnabled();
@@ -517,7 +523,7 @@ class MultiprocessTestParent {
   // `should_profile` is false, simulate the embedder refusing to profile the
   // child process.
   void LaunchTestChild(HeapProfilerController* controller,
-                       ProfilerProcessType process_type,
+                       ProcessType process_type,
                        int num_allocations,
                        bool should_profile) {
     // `should_profile` will apply during next call to BindTestConnector().
@@ -599,13 +605,14 @@ struct FeatureTestParams {
   };
   // Whether HeapProfilerReporting is enabled.
   bool feature_enabled = true;
+  const ProcessTypeSet supported_processes;
   ChannelParams stable;
   ChannelParams nonstable;
   // Probabilities for snapshotting child processes.
-  int gpu_snapshot_prob = 0;
-  int network_snapshot_prob = 0;
-  int renderer_snapshot_prob = 0;
-  int utility_snapshot_prob = 0;
+  int gpu_snapshot_prob = 100;
+  int network_snapshot_prob = 100;
+  int renderer_snapshot_prob = 100;
+  int utility_snapshot_prob = 100;
 
   base::FieldTrialParams ToFieldTrialParams() const;
 
@@ -616,32 +623,58 @@ struct FeatureTestParams {
 // Converts the test params to field trial parameters for the
 // HeapProfilerReporting feature.
 base::FieldTrialParams FeatureTestParams::ToFieldTrialParams() const {
-  base::FieldTrialParams field_trial_params;
+  base::FieldTrialParams field_trial_params{
+      {"gpu-prob-pct", base::NumberToString(gpu_snapshot_prob)},
+      {"network-prob-pct", base::NumberToString(network_snapshot_prob)},
+      {"renderer-prob-pct", base::NumberToString(renderer_snapshot_prob)},
+      {"utility-prob-pct", base::NumberToString(utility_snapshot_prob)},
+  };
 
-  // Global parameters.
-  field_trial_params["stable-probability"] =
-      base::NumberToString(stable.probability);
-  field_trial_params["nonstable-probability"] =
-      base::NumberToString(nonstable.probability);
+  // Add the default params.
+  base::Value::Dict dict;
+  if (!supported_processes.empty()) {
+    // Explicitly disable profiling by default, so that only the processes
+    // given in `supported_processes` will be enabled.
+    dict.Set("is-supported", false);
+  }
+  dict.Set("stable-probability", stable.probability);
+  dict.Set("nonstable-probability", nonstable.probability);
+  dict.Set("sampling-rate-bytes", static_cast<int>(kSamplingRate));
+  std::string param_string;
+  base::JSONWriter::WriteWithOptions(
+      dict, base::JSONWriter::OPTIONS_PRETTY_PRINT, &param_string);
+  field_trial_params["default-params"] = param_string;
 
-  // Per-process parameters.
-  field_trial_params["browser-sampling-rate-bytes"] =
-      base::NumberToString(kSamplingRate);
-  field_trial_params["gpu-sampling-rate-bytes"] =
-      base::NumberToString(kSamplingRate);
-  field_trial_params["gpu-prob-pct"] = base::NumberToString(gpu_snapshot_prob);
-  field_trial_params["network-sampling-rate-bytes"] =
-      base::NumberToString(kSamplingRate);
-  field_trial_params["network-prob-pct"] =
-      base::NumberToString(network_snapshot_prob);
-  field_trial_params["renderer-sampling-rate-bytes"] =
-      base::NumberToString(kSamplingRate);
-  field_trial_params["renderer-prob-pct"] =
-      base::NumberToString(renderer_snapshot_prob);
-  field_trial_params["utility-sampling-rate-bytes"] =
-      base::NumberToString(kSamplingRate);
-  field_trial_params["utility-prob-pct"] =
-      base::NumberToString(utility_snapshot_prob);
+  // Add a field trial param that enables each process type in
+  // `supported_processes`.
+  base::Value::Dict is_supported_dict;
+  is_supported_dict.Set("is-supported", true);
+  std::string is_supported_string;
+  base::JSONWriter::WriteWithOptions(is_supported_dict,
+                                     base::JSONWriter::OPTIONS_PRETTY_PRINT,
+                                     &is_supported_string);
+
+  for (ProcessType process_type : supported_processes) {
+    switch (process_type) {
+      case ProcessType::kBrowser:
+        field_trial_params["browser-process-params"] = is_supported_string;
+        break;
+      case ProcessType::kRenderer:
+        field_trial_params["renderer-process-params"] = is_supported_string;
+        break;
+      case ProcessType::kGpu:
+        field_trial_params["gpu-process-params"] = is_supported_string;
+        break;
+      case ProcessType::kUtility:
+        field_trial_params["utility-process-params"] = is_supported_string;
+        break;
+      case ProcessType::kNetworkService:
+        field_trial_params["network-process-params"] = is_supported_string;
+        break;
+      default:
+        NOTREACHED();
+    }
+  }
 
   return field_trial_params;
 }
@@ -729,18 +762,18 @@ class HeapProfilerControllerTest
   // The test must call StartIfEnabled() after this to start profiling.
   void CreateHeapProfiler(
       version_info::Channel channel,
-      ProfilerProcessType process_type,
+      ProcessType process_type,
       bool expect_enabled,
       base::OnceClosure first_snapshot_callback = base::DoNothing(),
       ProfileCollectorCallback collector_callback = base::DoNothing()) {
     ASSERT_FALSE(controller_) << "CreateHeapProfiler called twice";
     switch (process_type) {
-      case ProfilerProcessType::kBrowser:
+      case ProcessType::kBrowser:
         expected_process_ = metrics::Process::BROWSER_PROCESS;
         metrics::CallStackProfileBuilder::SetBrowserProcessReceiverCallback(
             std::move(collector_callback));
         break;
-      case ProfilerProcessType::kUtility:
+      case ProcessType::kUtility:
         expected_process_ = metrics::Process::UTILITY_PROCESS;
         metrics::CallStackProfileBuilder::
             SetParentProfileCollectorForChildProcess(
@@ -772,7 +805,7 @@ class HeapProfilerControllerTest
   // profiling.
   void StartHeapProfiling(
       version_info::Channel channel,
-      ProfilerProcessType process_type,
+      ProcessType process_type,
       bool expect_enabled,
       base::OnceClosure first_snapshot_callback = base::DoNothing(),
       ProfileCollectorCallback collector_callback = base::DoNothing()) {
@@ -874,8 +907,7 @@ TEST_P(HeapProfilerControllerTest, ProfileCollectionsScheduler) {
     ++profile_count;
   };
 
-  StartHeapProfiling(version_info::Channel::STABLE,
-                     ProfilerProcessType::kBrowser,
+  StartHeapProfiling(version_info::Channel::STABLE, ProcessType::kBrowser,
                      /*expect_enabled=*/true,
                      /*first_snapshot_callback=*/base::DoNothing(),
                      base::BindLambdaForTesting(check_profile));
@@ -905,8 +937,7 @@ TEST_P(HeapProfilerControllerTest, ProfileCollectionsScheduler) {
 TEST_P(HeapProfilerControllerTest, UnhandledProcess) {
   // Starting the heap profiler in an unhandled process type should safely do
   // nothing.
-  StartHeapProfiling(version_info::Channel::STABLE,
-                     ProfilerProcessType::kUnknown,
+  StartHeapProfiling(version_info::Channel::STABLE, ProcessType::kUnknown,
                      /*expect_enabled=*/false);
   // The Enabled summary histogram should not be logged for unsupported
   // processes, because they're not included in the per-process histograms that
@@ -918,10 +949,10 @@ TEST_P(HeapProfilerControllerTest, EmptyProfile) {
   // Should save an empty profile even though no memory is allocated.
   ScopedCallbacks callbacks = CreateScopedCallbacks(
       /*expect_take_snapshot=*/true, /*expect_sampled_profile=*/true);
-  StartHeapProfiling(
-      version_info::Channel::STABLE, ProfilerProcessType::kBrowser,
-      /*expect_enabled=*/true, callbacks.first_snapshot_callback(),
-      callbacks.collector_callback());
+  StartHeapProfiling(version_info::Channel::STABLE, ProcessType::kBrowser,
+                     /*expect_enabled=*/true,
+                     callbacks.first_snapshot_callback(),
+                     callbacks.collector_callback());
   task_env().RunUntilQuit();
   EXPECT_TRUE(sample_received_);
 }
@@ -972,9 +1003,8 @@ TEST_P(HeapProfilerControllerChannelTest, StableChannel) {
   ScopedCallbacks callbacks = CreateScopedCallbacks(
       /*expect_take_snapshot=*/profiling_enabled,
       GetParam().stable.expect_browser_sample);
-  StartHeapProfiling(version_info::Channel::STABLE,
-                     ProfilerProcessType::kBrowser, profiling_enabled,
-                     callbacks.first_snapshot_callback(),
+  StartHeapProfiling(version_info::Channel::STABLE, ProcessType::kBrowser,
+                     profiling_enabled, callbacks.first_snapshot_callback(),
                      callbacks.collector_callback());
   histogram_tester_.ExpectUniqueSample(
       "HeapProfiling.InProcess.Enabled.Browser", profiling_enabled, 1);
@@ -990,9 +1020,8 @@ TEST_P(HeapProfilerControllerChannelTest, CanaryChannel) {
   ScopedCallbacks callbacks = CreateScopedCallbacks(
       /*expect_take_snapshot=*/profiling_enabled,
       GetParam().nonstable.expect_browser_sample);
-  StartHeapProfiling(version_info::Channel::CANARY,
-                     ProfilerProcessType::kBrowser, profiling_enabled,
-                     callbacks.first_snapshot_callback(),
+  StartHeapProfiling(version_info::Channel::CANARY, ProcessType::kBrowser,
+                     profiling_enabled, callbacks.first_snapshot_callback(),
                      callbacks.collector_callback());
   histogram_tester_.ExpectUniqueSample(
       "HeapProfiling.InProcess.Enabled.Browser", profiling_enabled, 1);
@@ -1010,9 +1039,8 @@ TEST_P(HeapProfilerControllerChannelTest, UnknownChannel) {
   ScopedCallbacks callbacks = CreateScopedCallbacks(
       /*expect_take_snapshot=*/profiling_enabled,
       GetParam().stable.expect_browser_sample);
-  StartHeapProfiling(version_info::Channel::UNKNOWN,
-                     ProfilerProcessType::kBrowser, profiling_enabled,
-                     callbacks.first_snapshot_callback(),
+  StartHeapProfiling(version_info::Channel::UNKNOWN, ProcessType::kBrowser,
+                     profiling_enabled, callbacks.first_snapshot_callback(),
                      callbacks.collector_callback());
   histogram_tester_.ExpectUniqueSample(
       "HeapProfiling.InProcess.Enabled.Browser", profiling_enabled, 1);
@@ -1026,12 +1054,22 @@ TEST_P(HeapProfilerControllerChannelTest, UnknownChannel) {
 constexpr FeatureTestParams kProcessConfigs[] = {
     // Enabled in parent process only.
     {
+        .supported_processes = {ProcessType::kBrowser},
         .stable = {.expect_browser_sample = true, .expect_child_sample = false},
     },
+    // Enabled in child process only.
+    // Central control only samples child processes when the browser process is
+    // sampled, so no samples are expected even though sampling is supported in
+    // the child process.
+    {
+        .supported_processes = {ProcessType::kUtility},
+        .stable = {.expect_browser_sample = false,
+                   .expect_child_sample = false},
+    },
     // Enabled in parent and child processes.
     {
+        .supported_processes = {ProcessType::kBrowser, ProcessType::kUtility},
         .stable = {.expect_browser_sample = true, .expect_child_sample = true},
-        .utility_snapshot_prob = 100,
     },
 };
 
@@ -1042,10 +1080,11 @@ INSTANTIATE_TEST_SUITE_P(All,
                          ::testing::ValuesIn(kProcessConfigs));
 
 TEST_P(HeapProfilerControllerProcessTest, BrowserProcess) {
-  // This test always enables profiling in the browser process. (Disabled is
-  // tested in HeapProfilerControllerChannelTest.)
+  const bool profiling_enabled =
+      base::Contains(GetParam().supported_processes, ProcessType::kBrowser);
   ScopedCallbacks callbacks = CreateScopedCallbacks(
-      /*expect_take_snapshot=*/true, GetParam().stable.expect_browser_sample,
+      /*expect_take_snapshot=*/profiling_enabled,
+      GetParam().stable.expect_browser_sample,
       /*use_other_process_callback=*/GetParam().stable.expect_child_sample);
 
   // Mock the child end of the SnapshotController mojo pipe. (Only used when
@@ -1054,27 +1093,31 @@ TEST_P(HeapProfilerControllerProcessTest, BrowserProcess) {
   mojo::Receiver<mojom::SnapshotController> mock_receiver(
       &mock_child_snapshot_controller);
 
-  StartHeapProfiling(
-      version_info::Channel::STABLE, ProfilerProcessType::kBrowser,
-      /*expect_enabled=*/true, callbacks.first_snapshot_callback(),
-      callbacks.collector_callback());
+  StartHeapProfiling(version_info::Channel::STABLE, ProcessType::kBrowser,
+                     profiling_enabled, callbacks.first_snapshot_callback(),
+                     callbacks.collector_callback());
   histogram_tester_.ExpectUniqueSample(
-      "HeapProfiling.InProcess.Enabled.Browser", true, 1);
-  histogram_tester_.ExpectUniqueSample("HeapProfiling.InProcess.Enabled", true,
-                                       1);
+      "HeapProfiling.InProcess.Enabled.Browser", profiling_enabled, 1);
+  histogram_tester_.ExpectUniqueSample("HeapProfiling.InProcess.Enabled",
+                                       profiling_enabled, 1);
 
   constexpr int kTestChildProcessId = 1;
-  ASSERT_TRUE(controller_->GetBrowserProcessSnapshotController());
+  if (profiling_enabled) {
+    ASSERT_TRUE(controller_->GetBrowserProcessSnapshotController());
 
-  // This callback should be invoked from AppendCommandLineSwitchForChildProcess
-  // to bind the child end of the mojo pipe.
-  controller_->GetBrowserProcessSnapshotController()
-      ->SetBindRemoteForChildProcessCallback(base::BindLambdaForTesting(
-          [&](int child_process_id,
-              mojo::PendingReceiver<mojom::SnapshotController> receiver) {
-            EXPECT_EQ(child_process_id, kTestChildProcessId);
-            mock_receiver.Bind(std::move(receiver));
-          }));
+    // This callback should be invoked from
+    // AppendCommandLineSwitchForChildProcess to bind the child end of the mojo
+    // pipe.
+    controller_->GetBrowserProcessSnapshotController()
+        ->SetBindRemoteForChildProcessCallback(base::BindLambdaForTesting(
+            [&](int child_process_id,
+                mojo::PendingReceiver<mojom::SnapshotController> receiver) {
+              EXPECT_EQ(child_process_id, kTestChildProcessId);
+              mock_receiver.Bind(std::move(receiver));
+            }));
+  } else {
+    EXPECT_FALSE(controller_->GetBrowserProcessSnapshotController());
+  }
 
   // Simulate a child process launch. If profiling is enabled in both browser
   // and child processes, this will bind the browser end of the mojo pipe to the
@@ -1082,7 +1125,7 @@ TEST_P(HeapProfilerControllerProcessTest, BrowserProcess) {
   // child end to `mock_child_snapshot_controller`.
   base::CommandLine child_command_line(base::CommandLine::NO_PROGRAM);
   controller_->AppendCommandLineSwitchForChildProcess(
-      &child_command_line, ProfilerProcessType::kUtility, kTestChildProcessId);
+      &child_command_line, ProcessType::kUtility, kTestChildProcessId);
 
   if (GetParam().stable.expect_child_sample) {
     EXPECT_CALL(mock_child_snapshot_controller, TakeSnapshot(100, 0))
@@ -1100,12 +1143,12 @@ TEST_P(HeapProfilerControllerProcessTest, BrowserProcess) {
 }
 
 TEST_P(HeapProfilerControllerProcessTest, ChildProcess) {
-  // TakeSnapshot() is called in the child process when the browser process
-  // triggers it, which always happens in this test when `utility_snapshot_prob`
-  // > 0.
-  const bool profiling_enabled = GetParam().utility_snapshot_prob > 0;
+  const bool profiling_enabled =
+      base::Contains(GetParam().supported_processes, ProcessType::kUtility);
+  // TakeSnapshot() is only called in the child process when the browser process
+  // triggers it.
   ScopedCallbacks callbacks = CreateScopedCallbacks(
-      /*expect_take_snapshot=*/profiling_enabled,
+      /*expect_take_snapshot=*/GetParam().stable.expect_child_sample,
       /*expect_sampled_profile=*/GetParam().stable.expect_child_sample,
       /*use_other_process_callback=*/true);
 
@@ -1130,22 +1173,25 @@ TEST_P(HeapProfilerControllerProcessTest, ChildProcess) {
 
   base::test::ScopedCommandLine scoped_command_line;
   HeapProfilerController::AppendCommandLineSwitchForTesting(
-      scoped_command_line.GetProcessCommandLine(),
-      ProfilerProcessType::kUtility, kTestChildProcessId,
-      fake_browser_snapshot_controller.get());
+      scoped_command_line.GetProcessCommandLine(), ProcessType::kUtility,
+      kTestChildProcessId, fake_browser_snapshot_controller.get());
 
-  // Simulate the browser process taking a sample after a delay.
+  // Simulate the browser process taking a sample after a delay. If profiling
+  // isn't enabled in the browser process, just quit waiting after the delay.
+  base::OnceClosure browser_snapshot_callback = base::DoNothing();
+  if (base::Contains(GetParam().supported_processes, ProcessType::kBrowser)) {
+    browser_snapshot_callback = base::BindOnce(
+        &BrowserProcessSnapshotController::TakeSnapshotsOnSnapshotSequence,
+        std::move(fake_browser_snapshot_controller));
+  }
   snapshot_task_runner->PostDelayedTask(
       FROM_HERE,
-      base::BindOnce(
-          &BrowserProcessSnapshotController::TakeSnapshotsOnSnapshotSequence,
-          std::move(fake_browser_snapshot_controller))
+      std::move(browser_snapshot_callback)
           .Then(callbacks.other_process_callback()),
       TestTimeouts::action_timeout());
 
-  StartHeapProfiling(version_info::Channel::STABLE,
-                     ProfilerProcessType::kUtility, profiling_enabled,
-                     callbacks.first_snapshot_callback(),
+  StartHeapProfiling(version_info::Channel::STABLE, ProcessType::kUtility,
+                     profiling_enabled, callbacks.first_snapshot_callback(),
                      callbacks.collector_callback());
   histogram_tester_.ExpectUniqueSample(
       "HeapProfiling.InProcess.Enabled.Utility", profiling_enabled, 1);
@@ -1189,8 +1235,8 @@ auto GetProfileMetadataFunc(std::string_view name) {
 // End-to-end test with multiple child processes.
 constexpr FeatureTestParams kMultipleChildConfigs[] = {
     {
-        .gpu_snapshot_prob = 100,
-        .network_snapshot_prob = 100,
+        .supported_processes = {ProcessType::kBrowser, ProcessType::kGpu,
+                                ProcessType::kUtility, ProcessType::kRenderer},
         .renderer_snapshot_prob = 66,
         .utility_snapshot_prob = 50,
     },
@@ -1217,19 +1263,19 @@ TEST_P(HeapProfilerControllerMultipleChildTest, EndToEnd) {
 
   // Process types to test. Each will make a different
   // number of memory allocations so their reports are all different.
-  const std::vector<std::pair<ProfilerProcessType, size_t>> kProcessesToTest{
-      {ProfilerProcessType::kBrowser, 0},
-      {ProfilerProcessType::kGpu, 1},
+  const std::vector<std::pair<ProcessType, size_t>> kProcessesToTest{
+      {ProcessType::kBrowser, 0},
+      {ProcessType::kGpu, 1},
       // 2 utility processes.
-      {ProfilerProcessType::kUtility, 2},
-      {ProfilerProcessType::kUtility, 3},
+      {ProcessType::kUtility, 2},
+      {ProcessType::kUtility, 3},
       // 5 renderer processes including one with no samples. The first one will
       // be ignored to simulate the embedder refusing to profile it.
-      {ProfilerProcessType::kRenderer, 10},
-      {ProfilerProcessType::kRenderer, 0},
-      {ProfilerProcessType::kRenderer, 4},
-      {ProfilerProcessType::kRenderer, 5},
-      {ProfilerProcessType::kRenderer, 6},
+      {ProcessType::kRenderer, 10},
+      {ProcessType::kRenderer, 0},
+      {ProcessType::kRenderer, 4},
+      {ProcessType::kRenderer, 5},
+      {ProcessType::kRenderer, 6},
   };
 
   // Expect only 1 utility process and 3 renderer processes to be sampled due
@@ -1257,10 +1303,10 @@ TEST_P(HeapProfilerControllerMultipleChildTest, EndToEnd) {
             task_runner->DeleteSoon(FROM_HERE, controller_.release());
           }));
 
-  CreateHeapProfiler(
-      version_info::Channel::STABLE, ProfilerProcessType::kBrowser,
-      /*expect_enabled=*/true, std::move(stop_after_first_snapshot_callback),
-      callbacks.collector_callback());
+  CreateHeapProfiler(version_info::Channel::STABLE, ProcessType::kBrowser,
+                     /*expect_enabled=*/true,
+                     std::move(stop_after_first_snapshot_callback),
+                     callbacks.collector_callback());
   ASSERT_TRUE(controller_);
 
   // Start all processes in `kProcessesToTest` except the browser.
@@ -1286,11 +1332,10 @@ TEST_P(HeapProfilerControllerMultipleChildTest, EndToEnd) {
 
   bool renderer_was_skipped = false;
   for (const auto [process_type, num_allocations] : kProcessesToTest) {
-    if (process_type != ProfilerProcessType::kBrowser) {
+    if (process_type != ProcessType::kBrowser) {
       // Skip the first renderer.
       bool should_profile = true;
-      if (process_type == ProfilerProcessType::kRenderer &&
-          !renderer_was_skipped) {
+      if (process_type == ProcessType::kRenderer && !renderer_was_skipped) {
         should_profile = false;
         renderer_was_skipped = true;
       }

components/heap_profiling/in_process/heap_profiler_parameters.cc

@@ -4,14 +4,19 @@
 
 #include "components/heap_profiling/in_process/heap_profiler_parameters.h"
 
-#include "base/check_op.h"
+#include <string>
+#include <string_view>
+
+#include "base/command_line.h"
 #include "base/feature_list.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_value_converter.h"
 #include "base/metrics/field_trial_params.h"
-#include "base/notreached.h"
-#include "base/numerics/safe_conversions.h"
 #include "base/time/time.h"
+#include "base/values.h"
 #include "build/build_config.h"
 #include "components/sampling_profiler/process_type.h"
+#include "components/variations/variations_switches.h"
 
 namespace heap_profiling {
 
@@ -49,60 +54,65 @@ constexpr double kDefaultStableProbability = 0.01;
 // provider if it's on a non-stable channel.
 constexpr double kDefaultNonStableProbability = 0.5;
 
-// The probability of including a child process in each snapshot that's taken,
-// as a percentage from 0 to 100. Defaults to 100, but can be set lower to
-// sub-sample process types that are very common (mainly renderers) to keep data
-// volume low. Samples from child processes are weighted in inverse proportion
-// to the snapshot probability to normalize the aggregated results. Set to 0 to
-// disable sampling a process completely.
+constexpr HeapProfilerParameters kDefaultHeapProfilerParameters{
+    .is_supported = false,
+    // If a process overrides `is_supported`, use the following defaults.
+    .stable_probability = kDefaultStableProbability,
+    .nonstable_probability = kDefaultNonStableProbability,
+    .sampling_rate_bytes = kDefaultSamplingRateBytes,
+    .collection_interval = kDefaultCollectionInterval,
+};
 
-constexpr base::FeatureParam<int> kGpuSnapshotProbability{
-    &kHeapProfilerReporting, "gpu-prob-pct", 100};
+// Feature parameters.
 
-constexpr base::FeatureParam<int> kNetworkSnapshotProbability{
-    &kHeapProfilerReporting, "network-prob-pct", 100};
+// JSON-encoded parameter map that will set the default parameters for the
+// heap profiler unless overridden by the process-specific parameters below.
+constexpr base::FeatureParam<std::string> kDefaultParameters{
+    &kHeapProfilerReporting, "default-params", ""};
 
-// Sample 10% of renderer processes by default, because last time this was
-// evaluated (2024-08) the 50th %ile of renderer process count
-// (Memory.RenderProcessHost.Count.All) ranged from 8 on Windows to 18 on Mac.
-// 10% is an easy default between 1/18 and 1/8.
-constexpr base::FeatureParam<int> kRendererSnapshotProbability{
-    &kHeapProfilerReporting, "renderer-prob-pct", 10};
+// JSON-encoded parameter map that will override the default parameters for the
+// browser process.
+constexpr base::FeatureParam<std::string> kBrowserProcessParameters{
+    &kHeapProfilerReporting, "browser-process-params", ""};
 
-// Sample 50% of utility processes by default, because last time this was
-// evaluated (2024-08) the profiler collected 1.8x as many snapshots on Mac and
-// 2.4x as many snapshots on Windows for each browser process snapshot.
-constexpr base::FeatureParam<int> kUtilitySnapshotProbability{
-    &kHeapProfilerReporting, "utility-prob-pct", 50};
+// JSON-encoded parameter map that will override the default parameters for
+// renderer processes.
+constexpr base::FeatureParam<std::string> kRendererProcessParameters{
+    &kHeapProfilerReporting, "renderer-process-params", ""};
 
-// The sampling rates of each process type, in bytes.
+// JSON-encoded parameter map that will override the default parameters for the
+// GPU process.
+constexpr base::FeatureParam<std::string> kGPUProcessParameters{
+    &kHeapProfilerReporting, "gpu-process-params", ""};
 
-constexpr base::FeatureParam<int> kBrowserSamplingRateBytes{
-    &kHeapProfilerReporting, "browser-sampling-rate-bytes",
-    kDefaultSamplingRateBytes};
+// JSON-encoded parameter map that will override the default parameters for
+// utility processes.
+constexpr base::FeatureParam<std::string> kUtilityProcessParameters{
+    &kHeapProfilerReporting, "utility-process-params", ""};
 
-// Use half the threshold used in the browser process, because last time it was
-// validated the GPU process allocated a bit over half as much memory at the
-// median.
-constexpr base::FeatureParam<int> kGpuSamplingRateBytes{
-    &kHeapProfilerReporting, "gpu-sampling-rate-bytes",
-    kDefaultSamplingRateBytes / 2};
+// JSON-encoded parameter map that will override the default parameters for the
+// network process.
+constexpr base::FeatureParam<std::string> kNetworkProcessParameters{
+    &kHeapProfilerReporting, "network-process-params", ""};
 
-constexpr base::FeatureParam<int> kNetworkSamplingRateBytes{
-    &kHeapProfilerReporting, "network-sampling-rate-bytes",
-    kDefaultSamplingRateBytes};
-
-constexpr base::FeatureParam<int> kRendererSamplingRateBytes{
-    &kHeapProfilerReporting, "renderer-sampling-rate-bytes",
-    kDefaultSamplingRateBytes};
-
-// Use 1/10th the threshold used in the browser process, because last time it
-// was validated with the default sampling rate (2024-08) the sampler collected
-// 6% to 11% as many samples per snapshot in the utility process, depending on
-// platform.
-constexpr base::FeatureParam<int> kUtilitySamplingRateBytes{
-    &kHeapProfilerReporting, "utility-sampling-rate-bytes",
-    kDefaultSamplingRateBytes / 10};
+// Interprets `value` as a positive number of minutes, and writes the converted
+// value to `result`. If `value` contains anything other than a positive
+// integer, returns false to indicate a conversion failure.
+bool ConvertCollectionInterval(const base::Value* value,
+                               base::TimeDelta* result) {
+  if (!value) {
+    // Missing values are ok, so report success without updating `result`.
+    return true;
+  }
+  if (value->is_int()) {
+    const int minutes = value->GetInt();
+    if (minutes > 0) {
+      *result = base::Minutes(minutes);
+      return true;
+    }
+  }
+  return false;
+}
 
 }  // namespace
 
@@ -110,65 +120,141 @@ BASE_FEATURE(kHeapProfilerReporting,
              "HeapProfilerReporting",
              base::FEATURE_ENABLED_BY_DEFAULT);
 
-const base::FeatureParam<double> kStableProbability{
-    &kHeapProfilerReporting, "stable-probability", kDefaultStableProbability};
+// TODO(crbug.com/40840943): The process sampling probabilities are separate
+// FeatureParams, but other per-process parameters are parsed from a JSON string
+// in a single FeatureParam. The JSON parameters are too complicated: split them
+// up into separate FeatureParams.
 
-const base::FeatureParam<double> kNonStableProbability{
-    &kHeapProfilerReporting, "nonstable-probability",
-    kDefaultNonStableProbability};
+const base::FeatureParam<int> kGpuSnapshotProbability{&kHeapProfilerReporting,
+                                                      "gpu-prob-pct", 100};
 
-const base::FeatureParam<base::TimeDelta> kCollectionInterval{
-    &kHeapProfilerReporting, "collection-interval", kDefaultCollectionInterval};
+const base::FeatureParam<int> kNetworkSnapshotProbability{
+    &kHeapProfilerReporting, "network-prob-pct", 100};
 
-size_t GetSamplingRateForProcess(
-    sampling_profiler::ProfilerProcessType process_type) {
-  int sampling_rate_bytes;
-  switch (process_type) {
-    case sampling_profiler::ProfilerProcessType::kBrowser:
-      sampling_rate_bytes = kBrowserSamplingRateBytes.Get();
-      break;
-    case sampling_profiler::ProfilerProcessType::kRenderer:
-      sampling_rate_bytes = kRendererSamplingRateBytes.Get();
-      break;
-    case sampling_profiler::ProfilerProcessType::kGpu:
-      sampling_rate_bytes = kGpuSamplingRateBytes.Get();
-      break;
-    case sampling_profiler::ProfilerProcessType::kUtility:
-      sampling_rate_bytes = kUtilitySamplingRateBytes.Get();
-      break;
-    case sampling_profiler::ProfilerProcessType::kNetworkService:
-      sampling_rate_bytes = kNetworkSamplingRateBytes.Get();
-      break;
-    case sampling_profiler::ProfilerProcessType::kUnknown:
-    default:
-      // Profiler should not be enabled for these process types.
-      NOTREACHED();
-  }
-  return base::saturated_cast<size_t>(sampling_rate_bytes);
+// Sample 10% of renderer processes by default, because last time this was
+// evaluated (2024-08) the 50th %ile of renderer process count
+// (Memory.RenderProcessHost.Count.All) ranged from 8 on Windows to 18 on Mac.
+// 10% is an easy default between 1/18 and 1/8.
+const base::FeatureParam<int> kRendererSnapshotProbability{
+    &kHeapProfilerReporting, "renderer-prob-pct", 10};
+
+// Sample 50% of utility processes by default, because last time this was
+// evaluated (2024-08) the profiler collected 1.8x as many snapshots on Mac and
+// 2.4x as many snapshots on Windows for each browser process snapshot.
+const base::FeatureParam<int> kUtilitySnapshotProbability{
+    &kHeapProfilerReporting, "utility-prob-pct", 50};
+
+// static
+void HeapProfilerParameters::RegisterJSONConverter(
+    base::JSONValueConverter<HeapProfilerParameters>* converter) {
+  converter->RegisterBoolField("is-supported",
+                               &HeapProfilerParameters::is_supported);
+  converter->RegisterDoubleField("stable-probability",
+                                 &HeapProfilerParameters::stable_probability);
+  converter->RegisterDoubleField(
+      "nonstable-probability", &HeapProfilerParameters::nonstable_probability);
+  converter->RegisterIntField("sampling-rate-bytes",
+                              &HeapProfilerParameters::sampling_rate_bytes);
+  converter->RegisterCustomValueField(
+      "collection-interval-minutes",
+      &HeapProfilerParameters::collection_interval, &ConvertCollectionInterval);
 }
 
-int GetSnapshotProbabilityForProcess(
+bool HeapProfilerParameters::UpdateFromJSON(std::string_view json_string) {
+  if (json_string.empty())
+    return true;
+
+  base::JSONValueConverter<HeapProfilerParameters> converter;
+  std::optional<base::Value> value =
+      base::JSONReader::Read(json_string, base::JSON_ALLOW_TRAILING_COMMAS |
+                                              base::JSON_ALLOW_COMMENTS);
+  if (value && converter.Convert(*value, this))
+    return true;
+
+  // Error reading JSON params. Disable the heap sampler. This will be reported
+  // when HeapProfilerController logs HeapProfiling.InProcess.Enabled.
+  is_supported = false;
+  return false;
+}
+
+HeapProfilerParameters GetDefaultHeapProfilerParameters() {
+  HeapProfilerParameters params = kDefaultHeapProfilerParameters;
+  params.UpdateFromJSON(kDefaultParameters.Get());
+  return params;
+}
+
+HeapProfilerParameters GetHeapProfilerParametersForProcess(
     sampling_profiler::ProfilerProcessType process_type) {
-  int snapshot_probability_pct;
+  using Process = sampling_profiler::ProfilerProcessType;
+
+  HeapProfilerParameters params = kDefaultHeapProfilerParameters;
+
+  // Apply per-process defaults.
   switch (process_type) {
-    case sampling_profiler::ProfilerProcessType::kGpu:
-      snapshot_probability_pct = kGpuSnapshotProbability.Get();
+    case Process::kBrowser:
+      params.is_supported = true;
       break;
-    case sampling_profiler::ProfilerProcessType::kNetworkService:
-      snapshot_probability_pct = kNetworkSnapshotProbability.Get();
+    case Process::kNetworkService:
+      params.is_supported = true;
       break;
-    case sampling_profiler::ProfilerProcessType::kRenderer:
-      snapshot_probability_pct = kRendererSnapshotProbability.Get();
+    case Process::kGpu:
+      params.is_supported = true;
+      // Use half the threshold used in the browser process, because last time
+      // it was validated the GPU process allocated a bit over half as much
+      // memory at the median.
+      params.sampling_rate_bytes = params.sampling_rate_bytes / 2;
       break;
-    case sampling_profiler::ProfilerProcessType::kUtility:
-      snapshot_probability_pct = kUtilitySnapshotProbability.Get();
+    case Process::kRenderer:
+      params.is_supported = true;
       break;
+    case Process::kUtility:
+      params.is_supported = true;
+      // Use 1/10th the threshold used in the browser process, because last time
+      // it was validated with the default sampling rate (2024-08) the sampler
+      // collected 6% to 11% as many samples per snapshot in the utility
+      // process, depending on platform.
+      params.sampling_rate_bytes = params.sampling_rate_bytes / 10;
+      break;
+    case Process::kUnknown:
     default:
-      NOTREACHED();
+      // Do nothing. Profiler hasn't been tested in these process types.
+      break;
   }
-  CHECK_GE(snapshot_probability_pct, 0);
-  CHECK_LE(snapshot_probability_pct, 100);
-  return snapshot_probability_pct;
+
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          variations::switches::kEnableBenchmarking) ||
+      !base::FeatureList::IsEnabled(kHeapProfilerReporting)) {
+    params.is_supported = false;
+    return params;
+  }
+
+  // Override with field trial parameters if any are set.
+  if (!params.UpdateFromJSON(kDefaultParameters.Get())) {
+    // After an error is detected don't alter `params` further.
+    return params;
+  }
+  switch (process_type) {
+    case Process::kBrowser:
+      params.UpdateFromJSON(kBrowserProcessParameters.Get());
+      break;
+    case Process::kRenderer:
+      params.UpdateFromJSON(kRendererProcessParameters.Get());
+      break;
+    case Process::kGpu:
+      params.UpdateFromJSON(kGPUProcessParameters.Get());
+      break;
+    case Process::kUtility:
+      params.UpdateFromJSON(kUtilityProcessParameters.Get());
+      break;
+    case Process::kNetworkService:
+      params.UpdateFromJSON(kNetworkProcessParameters.Get());
+      break;
+    case Process::kUnknown:
+    default:
+      // Do nothing. Profiler hasn't been tested in these process types.
+      break;
+  }
+  return params;
 }
 
 }  // namespace heap_profiling

components/heap_profiling/in_process/heap_profiler_parameters.h

@@ -5,7 +5,10 @@
 #ifndef COMPONENTS_HEAP_PROFILING_IN_PROCESS_HEAP_PROFILER_PARAMETERS_H_
 #define COMPONENTS_HEAP_PROFILING_IN_PROCESS_HEAP_PROFILER_PARAMETERS_H_
 
+#include <string_view>
+
 #include "base/feature_list.h"
+#include "base/json/json_value_converter.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/time/time.h"
 #include "components/sampling_profiler/process_type.h"
@@ -20,24 +23,54 @@ namespace heap_profiling {
 // reporting is enabled.
 BASE_DECLARE_FEATURE(kHeapProfilerReporting);
 
-// Chance that this client will report heap samples through a metrics
-// provider if it's on the stable channel.
-extern const base::FeatureParam<double> kStableProbability;
+// The probability of including a child process in each snapshot that's taken,
+// as a percentage from 0 to 100. Defaults to 100, but can be set lower to
+// sub-sample process types that are very common (mainly renderers) to keep data
+// volume low. Samples from child processes are weighted in inverse proportion
+// to the snapshot probability to normalize the aggregated results.
+extern const base::FeatureParam<int> kGpuSnapshotProbability;
+extern const base::FeatureParam<int> kNetworkSnapshotProbability;
+extern const base::FeatureParam<int> kRendererSnapshotProbability;
+extern const base::FeatureParam<int> kUtilitySnapshotProbability;
 
-// Chance that this client will report heap samples through a metrics
-// provider if it's on a non-stable channel.
-extern const base::FeatureParam<double> kNonStableProbability;
+// Parameters to control the heap profiler.
+struct HeapProfilerParameters {
+  // True if heap profiling is supported, false otherwise.
+  bool is_supported = false;
 
-// Mean time between snapshots.
-extern const base::FeatureParam<base::TimeDelta> kCollectionInterval;
+  // Chance that this client will report heap samples through a metrics
+  // provider if it's on the stable channel.
+  double stable_probability = 0.0;
 
-// Returns the sampling rate in bytes to use for `process_type`.
-size_t GetSamplingRateForProcess(
-    sampling_profiler::ProfilerProcessType process_type);
+  // Chance that this client will report heap samples through a metrics
+  // provider if it's on a non-stable channel.
+  double nonstable_probability = 0.0;
 
-// Returns the probability of sampling a `process_type` process in each
-// snapshot, from 0 to 100.
-int GetSnapshotProbabilityForProcess(
+  // Mean heap sampling interval in bytes.
+  int sampling_rate_bytes = 0;
+
+  // Mean time between snapshots.
+  base::TimeDelta collection_interval;
+
+  // Invoked by JSONValueConverter to parse parameters from JSON.
+  static void RegisterJSONConverter(
+      base::JSONValueConverter<HeapProfilerParameters>* converter);
+
+  // Overwrites this object's fields with parameters parsed from `json_string`.
+  // Missing parameters will not be touched. If parsing fails, returns false and
+  // sets `is_supported` to false to ensure heap profiling doesn't run with
+  // invalid parameters.
+  bool UpdateFromJSON(std::string_view json_string);
+};
+
+// Returns a default set of parameters to use if not overridden for a
+// specific process.
+HeapProfilerParameters GetDefaultHeapProfilerParameters();
+
+// Returns the set of process parameters to use for `process_type`. This will be
+// identical to the result of GetDefaultHeapProfilerParameters() unless
+// overridden by a field trial.
+HeapProfilerParameters GetHeapProfilerParametersForProcess(
     sampling_profiler::ProfilerProcessType process_type);
 
 }  // namespace heap_profiling

components/heap_profiling/in_process/heap_profiler_parameters_unittest.cc

@@ -0,0 +1,233 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/heap_profiling/in_process/heap_profiler_parameters.h"
+
+#include "base/command_line.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/time/time.h"
+#include "components/sampling_profiler/process_type.h"
+#include "components/variations/variations_switches.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace heap_profiling {
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::Field;
+
+// Can't define operator== because gmock has a conflicting operator== in an
+// internal namespace.
+auto MatchesParameters(const HeapProfilerParameters& expected) {
+  return AllOf(
+      Field("is_supported", &HeapProfilerParameters::is_supported,
+            expected.is_supported),
+      Field("stable_probability", &HeapProfilerParameters::stable_probability,
+            expected.stable_probability),
+      Field("nonstable_probability",
+            &HeapProfilerParameters::nonstable_probability,
+            expected.nonstable_probability),
+      Field("sampling_rate_bytes", &HeapProfilerParameters::sampling_rate_bytes,
+            expected.sampling_rate_bytes),
+      Field("collection_interval", &HeapProfilerParameters::collection_interval,
+            expected.collection_interval));
+}
+
+TEST(HeapProfilerParametersTest, ParseEmptyParameters) {
+  constexpr char kJSONParams[] = R"({})";
+  HeapProfilerParameters params;
+  EXPECT_TRUE(params.UpdateFromJSON(kJSONParams));
+  EXPECT_THAT(params, MatchesParameters({}));
+}
+
+TEST(HeapProfilerParametersTest, ParseParameters) {
+  constexpr char kJSONParams[] = R"({
+    "is-supported": true,
+    "stable-probability": 0.1,
+    // Comments should be allowed.
+    // Double parameters should convert from integers.
+    "nonstable-probability": 1,
+    "sampling-rate-bytes": 1000,
+    "collection-interval-minutes": 30,
+  })";
+  HeapProfilerParameters params;
+  EXPECT_TRUE(params.UpdateFromJSON(kJSONParams));
+  EXPECT_THAT(params, MatchesParameters({
+                          .is_supported = true,
+                          .stable_probability = 0.1,
+                          .nonstable_probability = 1.0,
+                          .sampling_rate_bytes = 1000,
+                          .collection_interval = base::Minutes(30),
+                      }));
+}
+
+TEST(HeapProfilerParametersTest, ParsePartialParameters) {
+  constexpr char kJSONParams[] = R"({
+    "is-supported": false,
+    "stable-probability": 0.5,
+    "collection-interval-minutes": 60,
+  })";
+  // Only the parameters that are included in the JSON should be overwritten.
+  HeapProfilerParameters params{
+      .is_supported = true,
+      .stable_probability = 0.1,
+      .nonstable_probability = 0.2,
+      .sampling_rate_bytes = 1000,
+      .collection_interval = base::Minutes(30),
+  };
+  EXPECT_TRUE(params.UpdateFromJSON(kJSONParams));
+  EXPECT_THAT(params, MatchesParameters({
+                          .is_supported = false,
+                          .stable_probability = 0.5,
+                          .nonstable_probability = 0.2,
+                          .sampling_rate_bytes = 1000,
+                          .collection_interval = base::Minutes(60),
+                      }));
+}
+
+TEST(HeapProfilerParametersTest, ParseInvalidParameters) {
+  constexpr char kJSONParams[] = R"({
+    "collection-interval-minutes": -1,
+  })";
+  HeapProfilerParameters params;
+  EXPECT_FALSE(params.UpdateFromJSON(kJSONParams));
+  EXPECT_FALSE(params.is_supported);
+}
+
+// Test that heap profiling is not supported for any process type when
+// --enable-benchmarking is specified on the command line.
+TEST(HeapProfilerParametersTest, EnableBenchmarking) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      variations::switches::kEnableBenchmarking);
+
+  using Process = sampling_profiler::ProfilerProcessType;
+  EXPECT_FALSE(GetDefaultHeapProfilerParameters().is_supported);
+  EXPECT_FALSE(
+      GetHeapProfilerParametersForProcess(Process::kBrowser).is_supported);
+  EXPECT_FALSE(GetHeapProfilerParametersForProcess(Process::kGpu).is_supported);
+  EXPECT_FALSE(
+      GetHeapProfilerParametersForProcess(Process::kRenderer).is_supported);
+  EXPECT_FALSE(
+      GetHeapProfilerParametersForProcess(Process::kUtility).is_supported);
+  EXPECT_FALSE(GetHeapProfilerParametersForProcess(Process::kNetworkService)
+                   .is_supported);
+}
+
+TEST(HeapProfilerParametersTest, ApplyParameters) {
+  constexpr char kDefaultParams[] = R"({
+    "is-supported": false,
+    "stable-probability": 0.1,
+    "nonstable-probability": 0.2,
+    "sampling-rate-bytes": 1000,
+    "collection-interval-minutes": 15,
+  })";
+  constexpr char kBrowserParams[] = R"({
+    "sampling-rate-bytes": 1001,
+  })";
+  constexpr char kGPUParams[] = R"({
+    "is-supported": true,
+    "sampling-rate-bytes": 1002,
+    "collection-interval-minutes": 60,
+  })";
+  constexpr char kRendererParams[] = R"({
+    "is-supported": false,
+    "sampling-rate-bytes": 1003,
+  })";
+
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeatureWithParameters(
+      kHeapProfilerReporting, {
+                                  {"default-params", kDefaultParams},
+                                  {"browser-process-params", kBrowserParams},
+                                  {"gpu-process-params", kGPUParams},
+                                  {"renderer-process-params", kRendererParams},
+                                  {"utility-process-params", "{}"},
+                              });
+
+  EXPECT_THAT(GetDefaultHeapProfilerParameters(),
+              MatchesParameters({
+                  .is_supported = false,
+                  .stable_probability = 0.1,
+                  .nonstable_probability = 0.2,
+                  .sampling_rate_bytes = 1000,
+                  .collection_interval = base::Minutes(15),
+              }));
+
+  using Process = sampling_profiler::ProfilerProcessType;
+  EXPECT_THAT(GetHeapProfilerParametersForProcess(Process::kBrowser),
+              MatchesParameters({
+                  .is_supported = false,
+                  .stable_probability = 0.1,
+                  .nonstable_probability = 0.2,
+                  .sampling_rate_bytes = 1001,
+                  .collection_interval = base::Minutes(15),
+              }));
+
+  EXPECT_THAT(GetHeapProfilerParametersForProcess(Process::kGpu),
+              MatchesParameters({
+                  .is_supported = true,
+                  .stable_probability = 0.1,
+                  .nonstable_probability = 0.2,
+                  .sampling_rate_bytes = 1002,
+                  .collection_interval = base::Minutes(60),
+              }));
+
+  EXPECT_THAT(GetHeapProfilerParametersForProcess(Process::kRenderer),
+              MatchesParameters({
+                  .is_supported = false,
+                  .stable_probability = 0.1,
+                  .nonstable_probability = 0.2,
+                  .sampling_rate_bytes = 1003,
+                  .collection_interval = base::Minutes(15),
+              }));
+
+  EXPECT_THAT(GetHeapProfilerParametersForProcess(Process::kUtility),
+              MatchesParameters({
+                  .is_supported = false,
+                  .stable_probability = 0.1,
+                  .nonstable_probability = 0.2,
+                  .sampling_rate_bytes = 1000,
+                  .collection_interval = base::Minutes(15),
+              }));
+
+  EXPECT_THAT(GetHeapProfilerParametersForProcess(Process::kNetworkService),
+              MatchesParameters({
+                  .is_supported = false,
+                  .stable_probability = 0.1,
+                  .nonstable_probability = 0.2,
+                  .sampling_rate_bytes = 1000,
+                  .collection_interval = base::Minutes(15),
+              }));
+}
+
+TEST(HeapProfilerParametersTest, ApplyInvalidParameters) {
+  constexpr char kDefaultParams[] = R"({
+    "is-supported": true,
+    "collection-interval-minutes": "unexpected string",
+  })";
+  // Ensure that valid per-process params don't overwrite invalid default
+  // params.
+  constexpr char kBrowserParams[] = R"({
+    "is-supported": true,
+    "collection-interval-minutes": 1,
+  })";
+
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeatureWithParameters(
+      kHeapProfilerReporting, {
+                                  {"default-params", kDefaultParams},
+                                  {"browser-process-params", kBrowserParams},
+                              });
+
+  EXPECT_FALSE(GetDefaultHeapProfilerParameters().is_supported);
+  EXPECT_FALSE(GetHeapProfilerParametersForProcess(
+                   sampling_profiler::ProfilerProcessType::kBrowser)
+                   .is_supported);
+}
+
+}  // namespace
+
+}  // namespace heap_profiling