Battery impact UMA

content/browser/power_usage_monitor_impl.cc

Index: content/browser/power_usage_monitor_impl.cc
diff --git a/content/browser/power_usage_monitor_impl.cc b/content/browser/power_usage_monitor_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8d28cd8817203c3797e8fd7f0171fe9aae98af24
--- /dev/null
+++ b/content/browser/power_usage_monitor_impl.cc
@@ -0,0 +1,282 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/power_usage_monitor_impl.h"
+
+#include "base/bind.h"
+#include "base/lazy_instance.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/message_loop/message_loop.h"
+#include "base/metrics/histogram.h"
+#include "base/strings/stringprintf.h"
+#include "base/time/time.h"
+#include "content/common/battery_status_messages.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_source.h"
+#include "content/public/browser/notification_types.h"
+#include "content/public/browser/power_usage_monitor.h"
+#include "content/public/browser/render_process_host.h"
+
+namespace {
+
+class PowerUsageMonitorSystemInterface
+    : public content::PowerUsageMonitor::SystemInterface {
+ public:
+  PowerUsageMonitorSystemInterface(content::PowerUsageMonitor* owner)
+      : power_usage_monitor_(owner), weak_ptr_factory_(this) {}

[Daniel Erat, 2014/10/23 19:05:34]

nit: one per line

[jeremy, 2014/10/27 08:25:12]

Sorry, clang-format did it :(

+  virtual ~PowerUsageMonitorSystemInterface() {}
+
+  virtual void ScheduleHistogramReport(base::TimeDelta delay) override {

[Daniel Erat, 2014/10/23 19:05:34]

omit 'virtual' when using 'override' (also below)

[jeremy, 2014/10/27 08:25:12]

Done.

+    content::BrowserThread::PostDelayedTask(
+        content::BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(
+            &PowerUsageMonitorSystemInterface::ReportBatteryLevelHistogram,
+            weak_ptr_factory_.GetWeakPtr(),
+            Now(),
+            delay),
+        delay);
+  }
+
+  virtual void CancelPendingHistogramReports() override {
+    weak_ptr_factory_.InvalidateWeakPtrs();
+  }
+
+  virtual base::Time Now() override { return base::Time::Now(); }
+
+ protected:
+  void ReportBatteryLevelHistogram(base::Time start_time,
+                                   base::TimeDelta discharge_time) override {
+    CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+
+    // Handle case of system suspend not cancelling histogram reports.
+    // When the system suspends, a task is posted to the IO thread to cancel all
+    // in progress calls to this function.  The delayed task could conceivably
+    // race the cancelation.
+    // If the function is executed later than expected, then cancel.
+    base::TimeDelta threshold =
+        base::TimeDelta::FromSeconds(discharge_time.InSecondsF() * 1.5);

[Daniel Erat, 2014/10/23 19:05:34]

what's the significance of 1.5 times the delay? why not just the delay plus 5
seconds or so?

[jeremy, 2014/10/27 08:25:12]

Changed to 2 minutes, added comment.

+    if ((Now() - start_time) > threshold) {
+      UMA_HISTOGRAM_BOOLEAN("Power.BatteryDischargeReportsCancelled", true);
+      return;
+    }
+
+    const std::string histogram_name = base::StringPrintf(
+        "Power.BatteryDischarge_%d", discharge_time.InMinutes());
+    base::HistogramBase* histogram =
+        base::Histogram::FactoryGet(histogram_name,
+                                    0,

[Daniel Erat, 2014/10/23 19:05:34]

the docs for Histogram::FactoryGet() say that the minimum needs to be greater
than 0

[jeremy, 2014/10/27 08:25:12]

Done.

+                                    100,
+                                    102,
+                                    base::Histogram::kUmaTargetedHistogramFlag);
+    double discharge_amount = power_usage_monitor_->DischargeAmount();
+    histogram->Add(discharge_amount * 100);
+
+    power_usage_monitor_->BatteryLevelReported();
+  }
+
+ private:
+  content::PowerUsageMonitor* power_usage_monitor_;  // Not owned.
+
+  // Used to cancel in progress timers.

[Daniel Erat, 2014/10/23 19:05:34]

s/timers/delayed tasks/

[jeremy, 2014/10/27 08:25:12]

Done.

+  base::WeakPtrFactory<PowerUsageMonitorSystemInterface> weak_ptr_factory_;
+};
+
+}  // namespace
+
+namespace content {
+
+void StartPowerUsageMonitor() {
+  base::LazyInstance<PowerUsageMonitor>::Leaky monitor =
+      LAZY_INSTANCE_INITIALIZER;
+  monitor.Get().Start();
+}
+
+PowerUsageMonitor::PowerUsageMonitor()
+    : system_interface_(new PowerUsageMonitorSystemInterface(this)),
+      started_(false),
+      on_battery_power_(false),
+      initial_battery_level_(0),
+      current_battery_level_(0) {
+  // PowerUsageMonitor gets battery level information.
+  callback_ = base::Bind(&PowerUsageMonitor::OnBatteryStatusUpdate,
+                         base::Unretained(this));
+}
+
+PowerUsageMonitor::~PowerUsageMonitor() {
+  if (started_)
+    base::PowerMonitor::Get()->RemoveObserver(this);
+}
+
+void PowerUsageMonitor::Start() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  started_ = true;
+
+  // Power Monitor used to get suspend/resume notifications.

[Daniel Erat, 2014/10/23 19:05:34]

nit: s/used/is used/

[jeremy, 2014/10/27 08:25:12]

Done.

+  base::PowerMonitor::Get()->AddObserver(this);
+
+  registrar_.Add(this,
+                 NOTIFICATION_RENDERER_PROCESS_CREATED,
+                 NotificationService::AllBrowserContextsAndSources());
+
+  registrar_.Add(this,
+                 NOTIFICATION_RENDERER_PROCESS_TERMINATED,
+                 NotificationService::AllBrowserContextsAndSources());
+  registrar_.Add(this,
+                 NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                 NotificationService::AllBrowserContextsAndSources());
+
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PowerUsageMonitor::StartOnIOThread, base::Unretained(this)));
+}
+
+void PowerUsageMonitor::StartOnIOThread() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  subscription_ = BatteryStatusService::GetInstance()->AddCallback(callback_);
+}
+
+void PowerUsageMonitor::DischargeStarted(double battery_level) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  // Cancel any in-progress ReportBatteryLevelHistogram() calls.
+  system_interface_->CancelPendingHistogramReports();
+
+  // Rate-limit power recording to once every 24 hours.

[Daniel Erat, 2014/10/23 19:05:34]

this seems a bit strange in that histograms will be reported more frequently if
chrome is restarted.

[jeremy, 2014/10/27 08:25:11]

The reason I added rate-limiting is so as not to collect more samples for users
who frequently connect/disconnect the power source.

This needs to be mitigated if frequent Chrome restarts correlate with increased
power draw, I think this is measurable so we can wait and mitigate if we do see
that users who restart Chrome more often, report different power usage from
users who report once a day.
wdyt?

+  if (!first_histogram_report_timestamp_.is_null()) {
+    base::TimeDelta time_since_last_report =
+        system_interface_->Now() - first_histogram_report_timestamp_;
+    if (time_since_last_report.InHours() < 24)
+      return;
+
+    // More than 24 hours have passed, reset timestamp.
+    first_histogram_report_timestamp_ = base::Time();
+  }
+
+  on_battery_power_ = true;
+  initial_battery_level_ = battery_level;
+  current_battery_level_ = battery_level;
+  start_discharge_time_ = system_interface_->Now();
+
+  const int kBatteryReportingIntervalMinutes[] = {5, 15, 30};
+  for (auto reporting_interval : kBatteryReportingIntervalMinutes) {
+    base::TimeDelta delay = base::TimeDelta::FromMinutes(reporting_interval);
+    system_interface_->ScheduleHistogramReport(delay);
+  }
+}
+
+void PowerUsageMonitor::WallPowerConnected(double battery_level) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  // Cancel any in-progress ReportBatteryLevelHistogram() calls.
+  system_interface_->CancelPendingHistogramReports();
+
+  if (!start_discharge_time_.is_null()) {
+    base::TimeDelta discharge_time =
+        system_interface_->Now() - start_discharge_time_;
+    double discharge_percent = initial_battery_level_ - battery_level;
+    double discharge_rate = discharge_percent / discharge_time.InMinutes();
+
+    if (discharge_time.InMinutes() > 30)
+      UMA_HISTOGRAM_PERCENTAGE("Power.BatteryDischargeRateWhenUnplugged",
+                               discharge_rate * 100.0);
+  }
+
+  on_battery_power_ = false;
+  initial_battery_level_ = 0;
+  current_battery_level_ = 0;
+  start_discharge_time_ = base::Time();
+}
+
+void PowerUsageMonitor::OnBatteryStatusUpdate(
+    const blink::WebBatteryStatus& status) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  bool now_on_battery_power = (status.charging == 0);
+  bool was_on_battery_power = on_battery_power_;
+  double battery_level = status.level;
+
+  if (now_on_battery_power == was_on_battery_power) {
+    if (now_on_battery_power)
+      current_battery_level_ = battery_level;
+    return;
+  } else if (now_on_battery_power) {  // Wall power disconnected.
+    // If all browser windows are closed, don't report power metrics since
+    // Chrome's power draw is likely not significant.
+    if (live_renderer_ids_.size() > 0)
+      DischargeStarted(battery_level);

[Daniel Erat, 2014/10/23 19:05:34]

i don't understand this. if this if condition is false, wouldn't you need to
later call DischargeStarted() in response to
NOTIFICATION_RENDERER_PROCESS_CREATED? or are you depending on
OnBatteryStatusUpdate() getting called again at some point in the future after a
renderer has been created?

it'd be cleaner to call DischargeStarted() unconditionally whenever the battery
starts discharging and push any extra logic about not measuring the power draw
while there aren't any renderers into that method.

[jeremy, 2014/10/27 08:25:12]

Moved to DischargeStarted(), OnBatteryStatusUpdate() is called on a hearbeat so
yes, I'm counting on it getting called again.

+  } else {  // Wall power connected.
+    WallPowerConnected(battery_level);
+  }
+}
+
+void PowerUsageMonitor::BatteryLevelReported() {
+  if (first_histogram_report_timestamp_.is_null()) {
+    // Record timestamp of first histogram report.

[Daniel Erat, 2014/10/23 19:05:34]

delete unnecessary comment and omit curly brackets

[jeremy, 2014/10/27 08:25:12]

Done.

+    first_histogram_report_timestamp_ = system_interface_->Now();
+  }
+}
+
+void PowerUsageMonitor::SetSystemInterfaceForTest(
+    scoped_ptr<SystemInterface> interface) {
+  system_interface_ = interface.Pass();
+}
+
+void PowerUsageMonitor::SetNumLiveRenderersForTest(int num_renderers) {
+  live_renderer_ids_.clear();
+  for (int i = 0; i < num_renderers; ++i) {

[Daniel Erat, 2014/10/23 19:05:34]

nit: omit curly brackets

[jeremy, 2014/10/27 08:25:11]

Done.

+    live_renderer_ids_.insert(1);
+  }
+}
+
+void PowerUsageMonitor::OnSuspend() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PowerUsageMonitor::CancelPendingHistogramRecording,
+                 base::Unretained(this)));
+}
+
+void PowerUsageMonitor::Observe(int type,
+                                const NotificationSource& source,
+                                const NotificationDetails& details) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  RenderProcessHost* rph = Source<RenderProcessHost>(source).ptr();
+
+  size_t previous_num_live_renderers = live_renderer_ids_.size();
+
+  if (type == NOTIFICATION_RENDERER_PROCESS_CREATED) {
+    live_renderer_ids_.insert(rph->GetID());
+  } else if (type == NOTIFICATION_RENDERER_PROCESS_TERMINATED ||
+             type == NOTIFICATION_RENDERER_PROCESS_CLOSED) {
+    live_renderer_ids_.erase(rph->GetID());
+  }
+
+  size_t num_live_renderers = live_renderer_ids_.size();
+  if (num_live_renderers == 0 && previous_num_live_renderers != 0) {
+    // All render processes have died.
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&PowerUsageMonitor::CancelPendingHistogramRecording,
+                   base::Unretained(this)));
+  }
+}
+
+void PowerUsageMonitor::CancelPendingHistogramRecording() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  // Cancel any in-progress histogram reports and reporting of discharge UMA.
+  system_interface_->CancelPendingHistogramReports();
+  start_discharge_time_ = base::Time();
+}
+
+}  // namespace content