content: Move AndroidBatteryMetrics to thread pool

Reporting metrics on the UI thread causes jank. Apart from the dark /
light mode metrics, they don't need to be tracked or reported on the UI
thread, so this patch moves most of the work to the thread pool.

This also requires making the observers for process visibility thread
safe.

Fixed: 1331596
Change-Id: I0f7d1b3784b0aca557ce837998ed4ef23f4304ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3782889
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Oksana Zhuravlova <oksamyt@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033010}

content/app/content_main_runner_impl.cc

@@ -67,6 +67,7 @@
 #include "content/common/content_constants_internal.h"
 #include "content/common/mojo_core_library_support.h"
 #include "content/common/partition_alloc_support.h"
+#include "content/common/process_visibility_tracker.h"
 #include "content/common/url_schemes.h"
 #include "content/gpu/in_process_gpu_thread.h"
 #include "content/public/app/content_main_delegate.h"
@@ -1154,11 +1155,14 @@ int ContentMainRunnerImpl::RunBrowser(MainFunctionParams main_params,
     base::PowerMonitor::Initialize(
         std::make_unique<base::PowerMonitorDeviceSource>());
 
+    // Ensure the visibility tracker is created on the main thread.
+    ProcessVisibilityTracker::GetInstance();
+
 #if BUILDFLAG(IS_ANDROID)
     SetupCpuTimeMetrics();
 
     // Requires base::PowerMonitor to be initialized first.
-    AndroidBatteryMetrics::GetInstance();
+    AndroidBatteryMetrics::CreateInstance();
 #endif
 
     if (start_minimal_browser)

content/browser/android/battery_metrics.cc

@@ -10,16 +10,21 @@
 #include "base/feature_list.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "base/metrics/histogram_base.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/no_destructor.h"
 #include "base/power_monitor/power_monitor.h"
+#include "base/task/sequenced_task_runner.h"
+#include "base/task/task_runner_util.h"
+#include "base/task/thread_pool.h"
 #include "base/time/time.h"
 #include "base/trace_event/application_state_proto_android.h"
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/typed_macros.h"
 #include "base/tracing/protos/chrome_track_event.pbzero.h"
 #include "content/browser/web_contents/web_contents_impl.h"
+#include "content/public/browser/browser_thread.h"
 #include "net/android/network_library.h"
 #include "net/android/traffic_stats.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
@@ -28,6 +33,13 @@
 const base::Feature kForegroundRadioStateCountWakeups{
     "ForegroundRadioStateCountWakeups", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Keeps reporting of the battery metrics on the UI thread, where it may cause
+// jank. This is used for a holdback experiment to estimate the jank reduction
+// won by moving reporting to the thread pool.
+// TODO(eseckler): Remove once holdback experiment is complete.
+const base::Feature kAndroidBatteryMetricsReportOnUIThread{
+    "AndroidBatteryMetricsReportOnUIThread", base::FEATURE_DISABLED_BY_DEFAULT};
+
 namespace content {
 namespace {
 
@@ -131,17 +143,12 @@ base::HistogramBase* GetAvgBatteryDrainHistogram(const char* suffix) {
       base::HistogramBase::kUmaTargetedHistogramFlag);
 }
 
-void ReportAveragedDrain(int capacity_consumed,
-                         bool is_exclusive_measurement,
-                         int num_sampling_periods) {
-  // Averaged drain over 30 second intervals in uAh. We assume a max current of
-  // 10A which translates to a little under 100mAh capacity drain over 30
-  // seconds.
-  auto capacity_consumed_avg = capacity_consumed / num_sampling_periods;
-
-  GetAvgBatteryDrainHistogram("")->AddCount(capacity_consumed_avg,
-                                            num_sampling_periods);
-
+// Dark mode histograms are reported on the UI thread, because they depend on
+// the current darkening state of the web contents -- which we can only inspect
+// on the UI thread.
+void ReportDarkModeDrains(int capacity_consumed_avg,
+                          bool is_exclusive_measurement,
+                          int num_sampling_periods) {
   size_t no_darkening_count = 0, user_agent_darkening_count = 0,
          web_page_or_user_agent_darkening_count = 0,
          web_page_darkening_count = 0;
@@ -196,12 +203,35 @@ void ReportAveragedDrain(int capacity_consumed,
   DCHECK(exclusive_dark_mode_histogram);
 
   dark_mode_histogram->AddCount(capacity_consumed_avg, num_sampling_periods);
+  if (is_exclusive_measurement) {
+    exclusive_dark_mode_histogram->AddCount(capacity_consumed_avg,
+                                            num_sampling_periods);
+  }
+}
+
+void ReportAveragedDrain(int capacity_consumed,
+                         bool is_exclusive_measurement,
+                         int num_sampling_periods) {
+  // Averaged drain over 30 second intervals in uAh. We assume a max current of
+  // 10A which translates to a little under 100mAh capacity drain over 30
+  // seconds.
+  auto capacity_consumed_avg = capacity_consumed / num_sampling_periods;
 
+  GetAvgBatteryDrainHistogram("")->AddCount(capacity_consumed_avg,
+                                            num_sampling_periods);
   if (is_exclusive_measurement) {
     GetAvgBatteryDrainHistogram(".Exclusive")
         ->AddCount(capacity_consumed_avg, num_sampling_periods);
-    exclusive_dark_mode_histogram->AddCount(capacity_consumed_avg,
-                                            num_sampling_periods);
+  }
+
+  // TODO(eseckler): Remove conditional once
+  // kAndroidBatteryMetricsReportOnUIThread is gone.
+  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    GetUIThreadTaskRunner({base::TaskPriority::BEST_EFFORT})
+        ->PostTask(
+            FROM_HERE,
+            base::BindOnce(&ReportDarkModeDrains, capacity_consumed_avg,
+                           is_exclusive_measurement, num_sampling_periods));
   }
 }
 
@@ -212,23 +242,41 @@ constexpr base::TimeDelta AndroidBatteryMetrics::kMetricsInterval;
 constexpr base::TimeDelta AndroidBatteryMetrics::kRadioStateInterval;
 
 // static
-AndroidBatteryMetrics* AndroidBatteryMetrics::GetInstance() {
+void AndroidBatteryMetrics::CreateInstance() {
   static base::NoDestructor<AndroidBatteryMetrics> instance;
-  return instance.get();
 }
 
 AndroidBatteryMetrics::AndroidBatteryMetrics()
-    : app_visible_(false),
-      on_battery_power_(base::PowerMonitor::IsOnBatteryPower()) {
-  base::PowerMonitor::AddPowerStateObserver(this);
-  base::PowerMonitor::AddPowerThermalObserver(this);
-  content::ProcessVisibilityTracker::GetInstance()->AddObserver(this);
-  UpdateMetricsEnabled();
+    : task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
+          {base::TaskPriority::BEST_EFFORT,
+           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})) {
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+  // TODO(eseckler): Remove conditional once
+  // kAndroidBatteryMetricsReportOnUIThread is gone.
+  if (base::FeatureList::IsEnabled(kAndroidBatteryMetricsReportOnUIThread)) {
+    // Initializing on the current (UI) thread registers all observers on the UI
+    // thread, such that all notifications will be received on the UI thread,
+    // too.
+    InitializeOnSequence();
+  } else {
+    task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(&AndroidBatteryMetrics::InitializeOnSequence,
+                                  base::Unretained(this)));
+  }
 }
 
 AndroidBatteryMetrics::~AndroidBatteryMetrics() {
-  base::PowerMonitor::RemovePowerThermalObserver(this);
-  base::PowerMonitor::RemovePowerStateObserver(this);
+  // Never called, this is a no-destruct singleton.
+  NOTREACHED();
+}
+
+void AndroidBatteryMetrics::InitializeOnSequence() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  on_battery_power_ =
+      base::PowerMonitor::AddPowerStateObserverAndReturnOnBatteryState(this);
+  base::PowerMonitor::AddPowerThermalObserver(this);
+  content::ProcessVisibilityTracker::GetInstance()->AddObserver(this);
+  UpdateMetricsEnabled();
 }
 
 void AndroidBatteryMetrics::OnVisibilityChanged(bool visible) {

content/browser/android/battery_metrics.h

@@ -9,6 +9,7 @@
 #include "base/no_destructor.h"
 #include "base/power_monitor/power_observer.h"
 #include "base/sequence_checker.h"
+#include "base/task/sequenced_task_runner.h"
 #include "base/timer/timer.h"
 #include "content/common/process_visibility_tracker.h"
 
@@ -22,19 +23,21 @@ class AndroidBatteryMetrics
       public base::PowerThermalObserver,
       public ProcessVisibilityTracker::ProcessVisibilityObserver {
  public:
-  static AndroidBatteryMetrics* GetInstance();
+  static void CreateInstance();
 
   AndroidBatteryMetrics(const AndroidBatteryMetrics&) = delete;
   AndroidBatteryMetrics& operator=(const AndroidBatteryMetrics&) = delete;
 
-  // ProcessVisibilityTracker::ProcessVisibilityObserver implementation:
-  void OnVisibilityChanged(bool visible) override;
-
  private:
   friend class base::NoDestructor<AndroidBatteryMetrics>;
   AndroidBatteryMetrics();
   ~AndroidBatteryMetrics() override;
 
+  void InitializeOnSequence();
+
+  // ProcessVisibilityTracker::ProcessVisibilityObserver implementation:
+  void OnVisibilityChanged(bool visible) override;
+
   // base::PowerStateObserver implementation:
   void OnPowerStateChange(bool on_battery_power) override;
 
@@ -59,8 +62,10 @@ class AndroidBatteryMetrics
   // Radio state is polled with this interval to count radio wakeups.
   static constexpr base::TimeDelta kRadioStateInterval = base::Seconds(1);
 
-  bool app_visible_;
-  bool on_battery_power_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
+  bool app_visible_ = false;
+  bool on_battery_power_ = false;
   int last_remaining_capacity_uah_ = 0;
   int64_t last_tx_bytes_ = -1;
   int64_t last_rx_bytes_ = -1;

content/child/child_process.cc

@@ -22,6 +22,7 @@
 #include "content/child/child_thread_impl.h"
 #include "content/common/android/cpu_time_metrics.h"
 #include "content/common/mojo_core_library_support.h"
+#include "content/common/process_visibility_tracker.h"
 #include "content/public/common/content_switches.h"
 #include "mojo/public/cpp/system/dynamic_library_support.h"
 #include "sandbox/policy/sandbox_type.h"
@@ -125,6 +126,9 @@ ChildProcess::ChildProcess(base::ThreadType io_thread_type,
   tracing::InitTracingPostThreadPoolStartAndFeatureList(
       /* enable_consumer */ false);
 
+  // Ensure the visibility tracker is created on the main thread.
+  ProcessVisibilityTracker::GetInstance();
+
 #if BUILDFLAG(IS_ANDROID)
   SetupCpuTimeMetrics();
 #endif

content/common/android/cpu_time_metrics_internal.cc

@@ -450,8 +450,6 @@ ProcessCpuTimeMetrics::ProcessCpuTimeMetrics(
     reporting_interval_ = kReportAfterEveryNTasksOtherProcess;
   }
 
-  ProcessVisibilityTracker::GetInstance()->AddObserver(this);
-
   task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&ProcessCpuTimeMetrics::InitializeOnThreadPool,
                                 base::Unretained(this)));
@@ -471,6 +469,7 @@ ProcessCpuTimeMetrics::~ProcessCpuTimeMetrics() {
 }
 
 void ProcessCpuTimeMetrics::InitializeOnThreadPool() {
+  ProcessVisibilityTracker::GetInstance()->AddObserver(this);
   arbiter_->AddObserver(this);
   PerformFullCollectionOnThreadPool();
 }
@@ -498,14 +497,6 @@ void ProcessCpuTimeMetrics::DidProcessTask(
 
 // ProcessVisibilityTracker::ProcessVisibilityObserver implementation:
 void ProcessCpuTimeMetrics::OnVisibilityChanged(bool visible) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(main_thread_);
-  task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(&ProcessCpuTimeMetrics::OnVisibilityChangedOnThreadPool,
-                     base::Unretained(this), visible));
-}
-
-void ProcessCpuTimeMetrics::OnVisibilityChangedOnThreadPool(bool visible) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(thread_pool_);
   // Collect high-level metrics that include a visibility breakdown and
   // attribute them to the old value of |is_visible_| before updating it.

content/common/android/cpu_time_metrics_internal.h

@@ -114,7 +114,6 @@ class CONTENT_EXPORT ProcessCpuTimeMetrics
   explicit ProcessCpuTimeMetrics(power_scheduler::PowerModeArbiter* arbiter);
 
   void InitializeOnThreadPool();
-  void OnVisibilityChangedOnThreadPool(bool visible);
   void PerformFullCollectionOnThreadPool();
   void CollectHighLevelMetricsOnThreadPool();
   void ReportAverageCpuLoad(base::TimeDelta cumulative_cpu_time);

content/common/android/cpu_time_metrics_unittest.cc

@@ -27,6 +27,8 @@ void WorkForOneCpuSec(base::WaitableEvent* event) {
 }
 
 TEST(CpuTimeMetricsTest, RecordsMetricsForeground) {
+  // Ensure the visibility tracker is created on the test runner thread.
+  ProcessVisibilityTracker::GetInstance();
   base::test::TaskEnvironment task_environment;
 
   base::HistogramTester histograms;
@@ -109,6 +111,8 @@ TEST(CpuTimeMetricsTest, RecordsMetricsForeground) {
 }
 
 TEST(CpuTimeMetricsTest, RecordsMetricsBackground) {
+  // Ensure the visibility tracker is created on the test runner thread.
+  ProcessVisibilityTracker::GetInstance();
   base::test::TaskEnvironment task_environment;
 
   base::HistogramTester histograms;

content/common/process_visibility_tracker.cc

@@ -5,7 +5,6 @@
 #include "content/common/process_visibility_tracker.h"
 
 #include "base/no_destructor.h"
-#include "base/observer_list.h"
 #include "components/power_scheduler/power_mode_arbiter.h"
 
 namespace content {
@@ -17,7 +16,9 @@ ProcessVisibilityTracker* ProcessVisibilityTracker::GetInstance() {
 }
 
 ProcessVisibilityTracker::ProcessVisibilityTracker()
-    : power_mode_visibility_voter_(
+    : observers_(base::MakeRefCounted<
+                 base::ObserverListThreadSafe<ProcessVisibilityObserver>>()),
+      power_mode_visibility_voter_(
           power_scheduler::PowerModeArbiter::GetInstance()->NewVoter(
               "PowerModeVoter.Visibility")) {}
 
@@ -27,33 +28,44 @@ ProcessVisibilityTracker::~ProcessVisibilityTracker() {
 
 void ProcessVisibilityTracker::AddObserver(
     ProcessVisibilityObserver* observer) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(main_thread_);
-
-  observers_.AddObserver(observer);
-  if (is_visible_.has_value())
-    observer->OnVisibilityChanged(*is_visible_);
+  absl::optional<bool> is_visible;
+  {
+    // Synchronize access to |observers_| and |is_visible_| to ensure
+    // consistency in notifications to observers (in case of concurrent
+    // modification of the visibility state).
+    base::AutoLock lock(lock_);
+    observers_->AddObserver(observer);
+    is_visible = is_visible_;
+  }
+  // Notify outside the lock to allow the observer to call back into
+  // ProcessVisibilityTracker.
+  if (is_visible.has_value())
+    observer->OnVisibilityChanged(*is_visible);
 }
 
 void ProcessVisibilityTracker::RemoveObserver(
     ProcessVisibilityObserver* observer) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(main_thread_);
-
-  observers_.RemoveObserver(observer);
+  base::AutoLock lock(lock_);
+  observers_->RemoveObserver(observer);
 }
 
 void ProcessVisibilityTracker::OnProcessVisibilityChanged(bool visible) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(main_thread_);
-  if (is_visible_.has_value() && *is_visible_ == visible)
-    return;
 
-  is_visible_ = visible;
+  {
+    base::AutoLock lock(lock_);
+    if (is_visible_.has_value() && *is_visible_ == visible)
+      return;
 
-  power_mode_visibility_voter_->VoteFor(
-      *is_visible_ ? power_scheduler::PowerMode::kIdle
-                   : power_scheduler::PowerMode::kBackground);
+    is_visible_ = visible;
+
+    observers_->Notify(
+        FROM_HERE, &ProcessVisibilityObserver::OnVisibilityChanged, visible);
+  }
 
-  for (ProcessVisibilityObserver& observer : observers_)
-    observer.OnVisibilityChanged(*is_visible_);
+  power_mode_visibility_voter_->VoteFor(
+      visible ? power_scheduler::PowerMode::kIdle
+              : power_scheduler::PowerMode::kBackground);
 }
 
 }  // namespace content