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..33abb27add219d595c44f51aedb6ff8fa14dc4d3
--- /dev/null
+++ b/content/browser/power_usage_monitor_impl.cc
@@ -0,0 +1,288 @@
+// 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"

[timvolodine, 2014/10/28 23:44:03]

no need to include this, it's already included in .h

[jeremy, 2014/11/02 15:06:20]

Done.

+#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 content {
+
+namespace {
+
+class PowerUsageMonitorSystemInterface
+    : public PowerUsageMonitor::SystemInterface {
+ public:
+  PowerUsageMonitorSystemInterface(PowerUsageMonitor* owner)

[Daniel Erat, 2014/10/27 16:15:58]

add 'explicit'

[jeremy, 2014/11/02 15:06:21]

Done.

+      : power_usage_monitor_(owner),
+        weak_ptr_factory_(this) {}
+  virtual ~PowerUsageMonitorSystemInterface() {}
+
+  void ScheduleHistogramReport(base::TimeDelta delay) override {
+    BrowserThread::PostDelayedTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(
+            &PowerUsageMonitorSystemInterface::ReportBatteryLevelHistogram,
+            weak_ptr_factory_.GetWeakPtr(),
+            Now(),
+            delay),
+        delay);
+  }
+
+  void CancelPendingHistogramReports() override {
+    weak_ptr_factory_.InvalidateWeakPtrs();
+  }
+
+  base::Time Now() override { return base::Time::Now(); }
+
+ protected:
+  void ReportBatteryLevelHistogram(base::Time start_time,
+                                   base::TimeDelta discharge_time) override {
+    CHECK(BrowserThread::CurrentlyOn(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.
+
+    // Delayed tasks are subject to timer coalescing and can fire anywhere from
+    // delay -> delay * 1.5) . In most cases, the OS should fire the task
+    // at the next wakeup and not as late as it can.
+    // A threshold of 2 minutes is used, since that should be large enough to
+    // take the slop factor due to coalescing into account.
+    base::TimeDelta threshold = discharge_time +
+        base::TimeDelta::FromMinutes(2);
+    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,
+                                    1,
+                                    100,
+                                    102,

[timvolodine, 2014/10/28 23:44:03]

why is this 102? is this intentional?

[jeremy, 2014/11/02 15:06:21]

Done.

+                                    base::Histogram::kUmaTargetedHistogramFlag);
+    double discharge_amount = power_usage_monitor_->discharge_amount();
+    histogram->Add(discharge_amount * 100);
+
+    power_usage_monitor_->BatteryLevelReported();
+  }
+
+ private:
+  PowerUsageMonitor* power_usage_monitor_;  // Not owned.
+
+  // Used to cancel in progress delayed tasks.
+  base::WeakPtrFactory<PowerUsageMonitorSystemInterface> weak_ptr_factory_;
+};
+
+}  // namespace
+
+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,

[timvolodine, 2014/10/28 23:44:03]

nit: why not put this in the member initialization list above?

[jeremy, 2014/11/02 15:06:21]

Done.

+                         base::Unretained(this));
+}
+
+PowerUsageMonitor::~PowerUsageMonitor() {
+  if (started_)
+    base::PowerMonitor::Get()->RemoveObserver(this);
+}
+
+void PowerUsageMonitor::Start() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  started_ = true;
+
+  // Power Monitor is used to get suspend/resume notifications.
+  base::PowerMonitor::Get()->AddObserver(this);
+
+  registrar_.Add(this,
+                 NOTIFICATION_RENDERER_PROCESS_CREATED,
+                 NotificationService::AllBrowserContextsAndSources());
+

[timvolodine, 2014/10/28 23:44:03]

remove empty line?

[jeremy, 2014/11/02 15:06:21]

Done.

+  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));
+
+  // 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)

[Daniel Erat, 2014/10/27 16:15:58]

live_renderer_ids_.empty()

[jeremy, 2014/11/02 15:06:21]

Done.

+    return;

[Daniel Erat, 2014/10/27 16:15:58]

it still seems weird that you intentionally return early without setting
|on_battery_power_| to true here (and also below in the reported-recently case).
i expect a variable with that name to reflect the current state of the system.

[jeremy, 2014/11/02 15:06:20]

This is intentional, the object doesn't change state when no renderers are open.
 Since everything is wired up to occurs on state transitions and I don't want
those to happen when the object is disabled.

Unless you feel strongly about this I'd rather leave this as is.

+
+  // Cancel any in-progress ReportBatteryLevelHistogram() calls.
+  system_interface_->CancelPendingHistogramReports();
+
+  // Rate-limit power recording to once every 24 hours.
+  if (!first_histogram_report_time_.is_null()) {
+    base::TimeDelta time_since_last_report =
+        system_interface_->Now() - first_histogram_report_time_;
+    if (time_since_last_report.InHours() < 24)
+      return;
+
+    // More than 24 hours have passed, reset timestamp.
+    first_histogram_report_time_ = 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();

[Daniel Erat, 2014/10/27 16:15:58]

InMinutes() truncates, so i think you have a potential divide-by-zero here

[jeremy, 2014/11/02 15:06:20]

Good catch, fixed.

+
+    if (discharge_time.InMinutes() > 30)

[Daniel Erat, 2014/10/27 16:15:58]

add curly brackets since the statement part is two lines

[jeremy, 2014/11/02 15:06:20]

Done.

+      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));

[timvolodine, 2014/10/28 23:44:03]

I think after "mojofication" of the battery API
(https://codereview.chromium.org/685703002/) we are running callbacks on the
main thread..

[jeremy, 2014/11/02 15:06:20]

Thanks! I don't have my laptop with me today, will test and make necessary
modifications before next round.

+
+  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.
+    DischargeStarted(battery_level);
+  } else {  // Wall power connected.
+    WallPowerConnected(battery_level);
+  }
+}
+
+void PowerUsageMonitor::BatteryLevelReported() {

[Daniel Erat, 2014/10/27 16:15:58]

assert the thread here (looks like it should be IO)

[jeremy, 2014/11/02 15:06:21]

Done.

+  if (first_histogram_report_time_.is_null())
+    first_histogram_report_time_ = system_interface_->Now();
+}
+
+void PowerUsageMonitor::SetSystemInterfaceForTest(
+    scoped_ptr<SystemInterface> interface) {
+  system_interface_ = interface.Pass();
+}
+
+void PowerUsageMonitor::SetRenderersForTest(bool has_live_renderers) {
+  live_renderer_ids_.clear();
+  if (has_live_renderers)
+    live_renderer_ids_.insert(1);
+}
+
+void PowerUsageMonitor::OnSuspend() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PowerUsageMonitor::CancelPendingHistogramReporting,
+                 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());
+  }

[Daniel Erat, 2014/10/27 16:15:58]

add NOTREACHED() for unexpected notification types

[jeremy, 2014/11/02 15:06:21]

Done.

+
+  size_t num_live_renderers = live_renderer_ids_.size();
+  if (num_live_renderers == 0 && previous_num_live_renderers != 0) {

[Daniel Erat, 2014/10/27 16:15:58]

just inline live_renderer_ids_.empty() here

[jeremy, 2014/11/02 15:06:21]

Done.

+    // All render processes have died.
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&PowerUsageMonitor::CancelPendingHistogramReporting,
+                   base::Unretained(this)));
+  }
+}
+
+void PowerUsageMonitor::CancelPendingHistogramReporting() {
+  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