Clean up ReduceCpuUtilization2 and associated code

Bug: None
Change-Id: I0ff25e0ee32afff37f55262cbc423bccb9a6bbc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6202281
Owners-Override: Francois Pierre Doray <fdoray@chromium.org>
Commit-Queue: Anthony Vallée-Dubois <anthonyvd@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@google.com>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1415749}

base/BUILD.gn

@@ -249,8 +249,6 @@ component("base") {
     "containers/unique_ptr_adapters.h",
     "containers/util.h",
     "containers/vector_buffer.h",
-    "cpu_reduction_experiment.cc",
-    "cpu_reduction_experiment.h",
     "critical_closure.h",
     "dcheck_is_on.h",
     "debug/alias.cc",

base/cpu_reduction_experiment.cc

@@ -1,78 +0,0 @@
-// 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 "base/cpu_reduction_experiment.h"
-
-#include <atomic>
-
-#include "base/check.h"
-#include "base/dcheck_is_on.h"
-#include "base/feature_list.h"
-#include "base/rand_util.h"
-
-namespace base {
-
-namespace {
-
-// Whether to enable a series of optimizations that reduce total CPU
-// utilization.
-BASE_FEATURE(kReduceCpuUtilization,
-             "ReduceCpuUtilization2",
-             FEATURE_ENABLED_BY_DEFAULT);
-
-class CpuReductionExperimentSubSampler {
- public:
-  CpuReductionExperimentSubSampler() : counter_(base::RandUint64()) {}
-
-  bool ShouldLogHistograms() {
-    // Relaxed memory order since there is no dependent memory access.
-    uint64_t val = counter_.fetch_add(1, std::memory_order_relaxed);
-    return val % 1000 == 0;
-  }
-
- private:
-  std::atomic<uint64_t> counter_{0};
-};
-
-// Singleton instance of CpuReductionExperimentSubSampler. This is only set when
-// the ReduceCpuUtilization experiment is enabled -- as a result, it's ok to
-// assume that the experiment is disabled when this is not set.
-CpuReductionExperimentSubSampler* g_subsampler = nullptr;
-
-#if DCHECK_IS_ON()
-// Atomic to support concurrent writes from IsRunningCpuReductionExperiment().
-std::atomic_bool g_accessed_subsampler = false;
-#endif
-
-}  // namespace
-
-bool IsRunningCpuReductionExperiment() {
-#if DCHECK_IS_ON()
-  // Relaxed memory order since there is no dependent memory access.
-  g_accessed_subsampler.store(true, std::memory_order_relaxed);
-#endif
-  return !!g_subsampler;
-}
-
-void InitializeCpuReductionExperiment() {
-#if DCHECK_IS_ON()
-  // TSAN should generate an error if InitializeCpuReductionExperiment() races
-  // with IsRunningCpuReductionExperiment().
-  //
-  // Relaxed memory order since there is no dependent memory access.
-  DCHECK(!g_accessed_subsampler.load(std::memory_order_relaxed));
-#endif
-  if (FeatureList::IsEnabled(kReduceCpuUtilization)) {
-    g_subsampler = new CpuReductionExperimentSubSampler();
-  }
-}
-
-bool ShouldLogHistogramForCpuReductionExperiment() {
-  if (!IsRunningCpuReductionExperiment()) {
-    return true;
-  }
-  return g_subsampler->ShouldLogHistograms();
-}
-
-}  // namespace base

base/cpu_reduction_experiment.h

@@ -1,29 +0,0 @@
-// 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.
-
-#ifndef BASE_CPU_REDUCTION_EXPERIMENT_H_
-#define BASE_CPU_REDUCTION_EXPERIMENT_H_
-
-#include "base/base_export.h"
-
-namespace base {
-
-// Returns whether the cpu cycle reduction experiment is running.
-// The goal of this experiment is to better understand the relationship between
-// total CPU cycles used across the fleet and top-line chrome metrics.
-BASE_EXPORT bool IsRunningCpuReductionExperiment();
-
-// Must be called after FeatureList initialization and while chrome is still
-// single-threaded.
-BASE_EXPORT void InitializeCpuReductionExperiment();
-
-// Returns true if the next sample should be recorded to an histogram
-// sub-sampled under the CPU reduction experiment. Returns true randomly for
-// ~1/1000 calls when the experiment is enabled, or always returns true when the
-// experiment is disabled.
-BASE_EXPORT bool ShouldLogHistogramForCpuReductionExperiment();
-
-}  // namespace base
-
-#endif  // BASE_CPU_REDUCTION_EXPERIMENT_H_

base/features.cc

@@ -4,7 +4,6 @@
 
 #include "base/features.h"
 
-#include "base/cpu_reduction_experiment.h"
 #include "base/task/sequence_manager/sequence_manager_impl.h"
 #include "base/threading/platform_thread.h"
 #include "build/buildflag.h"
@@ -125,7 +124,6 @@ BASE_FEATURE(kPostGetMyMemoryStateToBackground,
 
 void Init(EmitThreadControllerProfilerMetadata
               emit_thread_controller_profiler_metadata) {
-  InitializeCpuReductionExperiment();
   sequence_manager::internal::SequenceManagerImpl::InitializeFeatures();
   sequence_manager::internal::ThreadController::InitializeFeatures(
       emit_thread_controller_profiler_metadata);

base/rand_util.cc

@@ -27,6 +27,11 @@ namespace {
 std::atomic<bool> g_subsampling_always_sample = false;
 std::atomic<bool> g_subsampling_never_sample = false;
 
+MetricsSubSampler* GetSharedSubsampler() {
+  static thread_local MetricsSubSampler g_shared_subsampler;
+  return &g_shared_subsampler;
+}
+
 }  // namespace
 
 uint64_t RandUint64() {
@@ -182,6 +187,10 @@ bool MetricsSubSampler::ShouldSample(double probability) const {
   return generator_.RandDouble() < probability;
 }
 
+void MetricsSubSampler::Reseed() {
+  generator_ = InsecureRandomGenerator();
+}
+
 MetricsSubSampler::ScopedAlwaysSampleForTesting::
     ScopedAlwaysSampleForTesting() {
   DCHECK(!g_subsampling_always_sample.load(std::memory_order_relaxed));
@@ -208,4 +217,12 @@ MetricsSubSampler::ScopedNeverSampleForTesting::~ScopedNeverSampleForTesting() {
   g_subsampling_never_sample.store(false, std::memory_order_relaxed);
 }
 
+bool ShouldRecordSubsampledMetric(double probability) {
+  return GetSharedSubsampler()->ShouldSample(probability);
+}
+
+void ReseedSharedMetricsSubsampler() {
+  GetSharedSubsampler()->Reseed();
+}
+
 }  // namespace base

base/rand_util.h

@@ -288,6 +288,8 @@ class BASE_EXPORT MetricsSubSampler {
   MetricsSubSampler();
   bool ShouldSample(double probability) const;
 
+  void Reseed();
+
   // Make any call to ShouldSample for any instance of MetricsSubSampler
   // return true for testing. Cannot be used in conjunction with
   // ScopedNeverSampleForTesting.
@@ -310,6 +312,19 @@ class BASE_EXPORT MetricsSubSampler {
   InsecureRandomGenerator generator_;
 };
 
+// Returns true with `probability` using a pseudo-random number generator (or
+// always/never returns true if a `ScopedAlwaysSampleForTesting` or
+// `ScopedNeverSampleForTesting` is in scope).  This function is intended for
+// sub-sampled metric recording only. Do not use it for any other purpose,
+// especially where cryptographic randomness is required.
+// Uses a thread local MetricsSubSampler.
+BASE_EXPORT bool ShouldRecordSubsampledMetric(double probability);
+
+// Reseeds the MetricsSubsampler used by ShouldRecordSubsampledMetric. Used
+// after forking a zygote to avoid having multiple processes sharing initial
+// RNG state.
+BASE_EXPORT void ReseedSharedMetricsSubsampler();
+
 }  // namespace base
 
 #endif  // BASE_RAND_UTIL_H_

cc/metrics/compositor_frame_reporter.cc

@@ -16,10 +16,10 @@
 #include <utility>
 
 #include "base/check.h"
-#include "base/cpu_reduction_experiment.h"
 #include "base/debug/alias.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/notreached.h"
+#include "base/rand_util.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
@@ -361,7 +361,7 @@ void ReportTopControlsMetric(
         base::Histogram::FactoryMicrosecondsTimeGet(
             versioned_name, bucketing->min, bucketing->max, bucketing->count,
             base::HistogramBase::kUmaTargetedHistogramFlag));
-  } else if (base::ShouldLogHistogramForCpuReductionExperiment()) {
+  } else if (base::ShouldRecordSubsampledMetric(0.001)) {
     // We want to sub-sample the reports with top controls not moving. As they
     // dominate in volume.
     std::string versioned_name = name + kTopControlsDidNotMoveName;
@@ -1016,8 +1016,8 @@ void CompositorFrameReporter::EndCurrentStage(base::TimeTicks end_time) {
 }
 
 void CompositorFrameReporter::ReportCompositorLatencyMetrics() const {
-  // Subsampling these metrics reduced CPU utilization (crbug.com/1295441).
-  if (!base::ShouldLogHistogramForCpuReductionExperiment()) {
+  // Subsampling these metrics to reduce CPU utilization.
+  if (!base::ShouldRecordSubsampledMetric(0.001)) {
     return;
   }
 

components/viz/service/display/frame_interval_decider.cc

@@ -8,10 +8,10 @@
 
 #include <utility>
 
-#include "base/cpu_reduction_experiment.h"
 #include "base/functional/overloaded.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_functions.h"
+#include "base/rand_util.h"
 #include "base/trace_event/trace_event.h"
 #include "components/viz/common/quads/frame_interval_inputs.h"
 #include "components/viz/service/surfaces/surface.h"
@@ -89,7 +89,7 @@ void FrameIntervalDecider::Decide(
     }
   }
 
-  if (base::ShouldLogHistogramForCpuReductionExperiment()) {
+  if (base::ShouldRecordSubsampledMetric(0.001)) {
     base::UmaHistogramEnumeration("Viz.FrameIntervalDecider.ResultMatcherType",
                                   matcher_type);
     if (match_result &&

components/viz/service/frame_sinks/external_begin_frame_source_mac.cc

@@ -9,8 +9,8 @@
 #include <utility>
 
 #include "base/containers/contains.h"
-#include "base/cpu_reduction_experiment.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/rand_util.h"
 #include "base/trace_event/trace_event.h"
 
 namespace viz {
@@ -56,10 +56,6 @@ void RecordDisplayLinkCreateStatus(DisplayLinkResult result) {
 // Record the delay from the system CVDisplayLink or CADisplaylink source to
 // VizCompositorThread OnDisplayLinkCallback().
 void RecordVSyncCallbackDelay(base::TimeDelta delay) {
-  if (!base::ShouldLogHistogramForCpuReductionExperiment()) {
-    return;
-  }
-
   UMA_HISTOGRAM_CUSTOM_MICROSECONDS_TIMES(
       "Viz.BeginFrameSource.VSyncCallbackDelay", delay,
       /*min=*/base::Microseconds(10),
@@ -279,7 +275,9 @@ void ExternalBeginFrameSourceMac::OnDisplayLinkCallback(
                "callback_timebase_to_display",
                callback_timebase_to_display.InMicroseconds(), "callback_delay",
                callback_delay.InMicroseconds());
-  RecordVSyncCallbackDelay(callback_delay);
+  if (base::ShouldRecordSubsampledMetric(0.001)) {
+    RecordVSyncCallbackDelay(callback_delay);
+  }
 
   bool display_link_frame_interval_changed =
       !AlmostEqual(nominal_refresh_period_, interval);

content/app/content_main_runner_impl.cc

@@ -41,6 +41,7 @@
 #include "base/process/memory.h"
 #include "base/process/process.h"
 #include "base/process/process_handle.h"
+#include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/task/single_thread_task_runner.h"
@@ -624,6 +625,10 @@ NO_STACK_PROTECTOR int RunZygote(ContentMainDelegate* delegate) {
         base::BindOnce(&base::SetStackSmashingEmitsDebugMessage));
   }
 
+  // Reseed the shared subsampler used to subsample UMA histograms, to avoid
+  // having the forked child share the parent process' RNG state.
+  base::ReseedSharedMetricsSubsampler();
+
   // The zygote sets up base::GlobalDescriptors with all of the FDs passed to
   // the new child, so populate base::FileDescriptorStore with a subset of the
   // FDs currently stored in base::GlobalDescriptors.

content/browser/scheduler/responsiveness/watcher.cc

@@ -6,7 +6,6 @@
 
 #include <variant>
 
-#include "base/cpu_reduction_experiment.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
 #include "base/functional/overloaded.h"
@@ -50,25 +49,16 @@ void Watcher::WillRunTaskOnUIThread(const base::PendingTask* task,
 void Watcher::DidRunTaskOnUIThread(const base::PendingTask* task) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  if (base::IsRunningCpuReductionExperiment()) {
-    // Capturing `this` is safe because the callback is invoked synchronously by
-    // `DidRunTask()`.
-    auto lambda = [this](base::TimeTicks queue_time,
-                         base::TimeTicks execution_start_time,
-                         base::TimeTicks execution_finish_time) {
-      calculator_->TaskOrEventFinishedOnUIThread(
-          queue_time, execution_start_time, execution_finish_time);
-    };
-    DidRunTask(task, &currently_running_metadata_ui_,
-               &mismatched_task_identifiers_ui_, lambda);
-  } else {
-    // Unretained() is safe because the callback is invoked synchronously by
-    // `DidRunTask()`.
-    auto callback = base::BindOnce(&Calculator::TaskOrEventFinishedOnUIThread,
-                                   base::Unretained(calculator_.get()));
-    DidRunTask(task, &currently_running_metadata_ui_,
-               &mismatched_task_identifiers_ui_, std::move(callback));
-  }
+  // Capturing `this` is safe because the callback is invoked synchronously by
+  // `DidRunTask()`.
+  auto lambda = [this](base::TimeTicks queue_time,
+                       base::TimeTicks execution_start_time,
+                       base::TimeTicks execution_finish_time) {
+    calculator_->TaskOrEventFinishedOnUIThread(queue_time, execution_start_time,
+                                               execution_finish_time);
+  };
+  DidRunTask(task, &currently_running_metadata_ui_,
+             &mismatched_task_identifiers_ui_, lambda);
 }
 
 void Watcher::WillRunTaskOnIOThread(const base::PendingTask* task,
@@ -82,25 +72,16 @@ void Watcher::WillRunTaskOnIOThread(const base::PendingTask* task,
 void Watcher::DidRunTaskOnIOThread(const base::PendingTask* task) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  if (base::IsRunningCpuReductionExperiment()) {
-    // Capturing `this` is safe because the callback is invoked synchronously by
-    // `DidRunTask()`.
-    auto lambda = [this](base::TimeTicks queue_time,
-                         base::TimeTicks execution_start_time,
-                         base::TimeTicks execution_finish_time) {
-      calculator_io_->TaskOrEventFinishedOnIOThread(
-          queue_time, execution_start_time, execution_finish_time);
-    };
-    DidRunTask(task, &currently_running_metadata_io_,
-               &mismatched_task_identifiers_io_, lambda);
-  } else {
-    // Unretained() is safe because the callback is invoked synchronously by
-    // `DidRunTask()` below.
-    auto callback = base::BindOnce(&Calculator::TaskOrEventFinishedOnIOThread,
-                                   base::Unretained(calculator_io_));
-    DidRunTask(task, &currently_running_metadata_io_,
-               &mismatched_task_identifiers_io_, std::move(callback));
-  }
+  // Capturing `this` is safe because the callback is invoked synchronously by
+  // `DidRunTask()`.
+  auto lambda = [this](base::TimeTicks queue_time,
+                       base::TimeTicks execution_start_time,
+                       base::TimeTicks execution_finish_time) {
+    calculator_io_->TaskOrEventFinishedOnIOThread(
+        queue_time, execution_start_time, execution_finish_time);
+  };
+  DidRunTask(task, &currently_running_metadata_io_,
+             &mismatched_task_identifiers_io_, lambda);
 }
 
 void Watcher::WillRunTask(const base::PendingTask* task,
@@ -165,18 +146,7 @@ void Watcher::DidRunTask(const base::PendingTask* task,
   DCHECK_LE(queue_time, metadata.execution_start_time);
   DCHECK_LE(metadata.execution_start_time, execution_finish_time);
 
-  absl::visit(
-      base::Overloaded{
-          [&](base::FunctionRef<TaskOrEventFinishedSignature>& function_ref) {
-            function_ref(queue_time, metadata.execution_start_time,
-                         execution_finish_time);
-          },
-          [&](base::OnceCallback<TaskOrEventFinishedSignature>& base_callback) {
-            std::move(base_callback)
-                .Run(queue_time, metadata.execution_start_time,
-                     execution_finish_time);
-          }},
-      callback);
+  callback(queue_time, metadata.execution_start_time, execution_finish_time);
 }
 
 void Watcher::WillRunEventOnUIThread(const void* opaque_identifier) {

content/browser/scheduler/responsiveness/watcher.h

@@ -104,8 +104,7 @@ class CONTENT_EXPORT Watcher : public base::RefCounted<Watcher>,
                                             base::TimeTicks,
                                             base::TimeTicks);
   using TaskOrEventFinishedCallback =
-      absl::variant<base::OnceCallback<TaskOrEventFinishedSignature>,
-                    base::FunctionRef<TaskOrEventFinishedSignature>>;
+      base::FunctionRef<TaskOrEventFinishedSignature>;
   // |callback| will either be synchronously invoked, or else never invoked.
   void DidRunTask(const base::PendingTask* task,
                   std::vector<Metadata>* currently_running_metadata,

gpu/ipc/client/command_buffer_proxy_impl.cc

@@ -8,7 +8,6 @@
 #include <optional>
 
 #include "base/command_line.h"
-#include "base/cpu_reduction_experiment.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/location.h"
@@ -16,6 +15,7 @@
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/observer_list.h"
+#include "base/rand_util.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/timer/elapsed_timer.h"
 #include "base/trace_event/trace_event.h"
@@ -442,7 +442,7 @@ void CommandBufferProxyImpl::EnsureWorkVisible() {
   TRACE_EVENT_NESTABLE_ASYNC_END0("gpu,login", kEnsureWorkVisible,
                                   TRACE_ID_LOCAL(kEnsureWorkVisible));
 
-  if (base::ShouldLogHistogramForCpuReductionExperiment()) {
+  if (base::ShouldRecordSubsampledMetric(0.001)) {
     GetUMAHistogramEnsureWorkVisibleDuration()->Add(
         elapsed_timer.Elapsed().InMicroseconds());
 

gpu/ipc/service/image_transport_surface_overlay_mac.mm

@@ -11,10 +11,10 @@
 #include <sstream>
 
 #include "base/command_line.h"
-#include "base/cpu_reduction_experiment.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/rand_util.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "components/viz/common/features.h"
@@ -54,10 +54,6 @@ BASE_FEATURE(kPresentationDelayForInteractiveFrames,
 // Record the delay from the system CVDisplayLink or CADisplaylink source to
 // CrGpuMain OnVSyncPresentation().
 void RecordVSyncCallbackDelay(base::TimeDelta delay) {
-  if (!base::ShouldLogHistogramForCpuReductionExperiment()) {
-    return;
-  }
-
   UMA_HISTOGRAM_CUSTOM_MICROSECONDS_TIMES(
       "GPU.Presentation.VSyncCallbackDelay", delay,
       /*min=*/base::Microseconds(10),
@@ -342,7 +338,8 @@ void ImageTransportSurfaceOverlayMacEGL::OnVSyncPresentation(
     frame_interval_ = params.display_interval;
   }
 
-  if (params.callback_times_valid) {
+  if (params.callback_times_valid &&
+      base::ShouldRecordSubsampledMetric(0.001)) {
     RecordVSyncCallbackDelay(base::TimeTicks::Now() - params.callback_timebase);
   }
 

mojo/core/channel_posix.cc

@@ -17,7 +17,6 @@
 #include <memory>
 #include <tuple>
 
-#include "base/cpu_reduction_experiment.h"
 #include "base/functional/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ -76,11 +75,9 @@ class MessageView {
   MessageView& operator=(const MessageView&) = delete;
 
   ~MessageView() {
-    if (message_) {
-      if (base::ShouldLogHistogramForCpuReductionExperiment()) {
-        UMA_HISTOGRAM_TIMES("Mojo.Channel.WriteMessageLatency",
-                            base::TimeTicks::Now() - start_time_);
-      }
+    if (message_ && base::ShouldRecordSubsampledMetric(0.001)) {
+      UMA_HISTOGRAM_TIMES("Mojo.Channel.WriteMessageLatency",
+                          base::TimeTicks::Now() - start_time_);
     }
   }
 
@@ -166,8 +163,9 @@ void ChannelPosix::Write(MessagePtr message) {
     if (reject_writes_)
       return;
     if (outgoing_messages_.empty()) {
-      if (!WriteNoLock(MessageView(std::move(message), 0)))
+      if (!WriteNoLock(MessageView(std::move(message), 0))) {
         reject_writes_ = write_error = true;
+      }
     } else {
       outgoing_messages_.emplace_back(std::move(message), 0);
     }

ui/base/prediction/prediction_metrics_handler.cc

@@ -7,7 +7,6 @@
 #include <string_view>
 #include <utility>
 
-#include "base/cpu_reduction_experiment.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/strcat.h"
 

url/url_canon_host.cc

@@ -8,7 +8,6 @@
 #endif
 
 #include "base/check.h"
-#include "base/cpu_reduction_experiment.h"
 #include "url/url_canon.h"
 #include "url/url_canon_internal.h"
 #include "url/url_features.h"