RC: Capture cumulative process CPU measurements in render process probe.

The old-style CPUUsage attributes are supported by transforming the
batched cumulatives in the System CU.

Bug: 755840
Change-Id: I3e5d3d66223070a90ad35a835a210db0bb257809
Reviewed-on: https://chromium-review.googlesource.com/1060127
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559209}

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.cc

@@ -140,12 +140,14 @@ void ResourceCoordinatorRenderProcessProbe::
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(is_gathering_);
 
+  base::TimeTicks collection_start_time = base::TimeTicks::Now();
+
   // Dispatch the memory collection request.
   memory_instrumentation::MemoryInstrumentation::GetInstance()
       ->RequestGlobalDump(
           base::BindRepeating(&ResourceCoordinatorRenderProcessProbe::
                                   ProcessGlobalMemoryDumpAndDispatchOnIOThread,
-                              base::Unretained(this)));
+                              base::Unretained(this), collection_start_time));
 
   RenderProcessInfoMap::iterator iter = render_process_info_map_.begin();
   while (iter != render_process_info_map_.end()) {
@@ -155,7 +157,7 @@ void ResourceCoordinatorRenderProcessProbe::
     // not current then it is assumed dead and should not be measured anymore.
     if (render_process_info.last_gather_cycle_active == current_gather_cycle_) {
       render_process_info.cpu_usage =
-          render_process_info.metrics->GetPlatformIndependentCPUUsage();
+          render_process_info.metrics->GetCumulativeCPUUsage();
       ++iter;
     } else {
       render_process_info_map_.erase(iter++);
@@ -166,15 +168,13 @@ void ResourceCoordinatorRenderProcessProbe::
 
 void ResourceCoordinatorRenderProcessProbe::
     ProcessGlobalMemoryDumpAndDispatchOnIOThread(
+        base::TimeTicks collection_start_time,
         bool global_success,
         std::unique_ptr<memory_instrumentation::GlobalMemoryDump> dump) {
   // Create the measurement batch.
   mojom::ProcessResourceMeasurementBatchPtr batch =
       mojom::ProcessResourceMeasurementBatch::New();
 
-  // TODO(siggi): Add start/end times. This will need some support from
-  //     the memory_instrumentation code.
-
   // Start by adding the render process hosts we know about to the batch.
   for (const auto& render_process_info_map_entry : render_process_info_map_) {
     auto& render_process_info = render_process_info_map_entry.second;
@@ -213,6 +213,13 @@ void ResourceCoordinatorRenderProcessProbe::
     DCHECK(!global_success);
   }
 
+  // TODO(siggi): Because memory dump requests may be combined with earlier,
+  //     in-progress requests, this is an upper bound for the start time.
+  //     It would be more accurate to get the start time from the memory dump
+  //     and use the minimum of the two here.
+  batch->batch_started_time = collection_start_time;
+  batch->batch_ended_time = base::TimeTicks::Now();
+
   bool should_restart = DispatchMetrics(std::move(batch));
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.h

@@ -47,7 +47,7 @@ class ResourceCoordinatorRenderProcessProbe {
     RenderProcessInfo();
     ~RenderProcessInfo();
     base::Process process;
-    double cpu_usage = -1.0;
+    base::TimeDelta cpu_usage;
     size_t last_gather_cycle_active = -1;
     std::unique_ptr<base::ProcessMetrics> metrics;
   };
@@ -65,6 +65,7 @@ class ResourceCoordinatorRenderProcessProbe {
   void CollectRenderProcessMetricsAndStartMemoryDumpOnIOThread();
   // (3) Process the results of the memory dump and dispatch the results.
   void ProcessGlobalMemoryDumpAndDispatchOnIOThread(
+      base::TimeTicks collection_start_time,
       bool success,
       std::unique_ptr<memory_instrumentation::GlobalMemoryDump> dump);
   // (4) Initiate the next render process metrics collection cycle if the

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe_browsertest.cc

@@ -55,7 +55,7 @@ class TestingResourceCoordinatorRenderProcessProbe
       auto& render_process_info = render_process_info_map_entry.second;
       if (render_process_info.last_gather_cycle_active !=
               current_gather_cycle_ ||
-          render_process_info.cpu_usage < 0.0) {
+          render_process_info.cpu_usage.is_zero()) {
         return false;
       }
     }
@@ -143,10 +143,17 @@ IN_PROC_BROWSER_TEST_F(ResourceCoordinatorRenderProcessProbeBrowserTest,
   EXPECT_GE(2u, initial_size);  // If a spare RenderProcessHost is present.
   EXPECT_TRUE(probe.AllMeasurementsAreAtCurrentCycle());
   EXPECT_EQ(initial_size, probe.last_measurement_batch()->measurements.size());
-  // A quirk of the process_metrics implementation is that the first CPU
-  // measurement returns zero.
-  for (const auto& measurement : probe.last_measurement_batch()->measurements)
-    EXPECT_EQ(0.0, measurement->cpu_usage);
+
+  // The CPU measurement is cumulative since the start of process, but it could
+  // be zero due to the measurement granularity of the OS.
+  std::map<uint32_t, base::TimeDelta> cpu_usage_map;
+  for (const auto& measurement : probe.last_measurement_batch()->measurements) {
+    EXPECT_LE(base::TimeDelta(), measurement->cpu_usage);
+    EXPECT_TRUE(
+        cpu_usage_map
+            .insert(std::make_pair(measurement->pid, measurement->cpu_usage))
+            .second);
+  }
 
   // There is an inherent race in memory measurement that may cause failures
   // which will result in zero private footprint returns. To work around this,
@@ -178,11 +185,13 @@ IN_PROC_BROWSER_TEST_F(ResourceCoordinatorRenderProcessProbeBrowserTest,
 
   size_t info_map_size = info_map.size();
   probe.StartGatherCycleAndWait();
-  // The second and subsequent CPU measurements should return some data,
-  // although the measurement granularity on some OSen is such that zero returns
-  // are almost certain.
-  for (const auto& measurement : probe.last_measurement_batch()->measurements)
-    EXPECT_LE(0.0, measurement->cpu_usage);
+  // Verify that CPU usage is monotonically increasing, though the measurement
+  // granulatity is such on some OSes that a zero difference is almost certain.
+  for (const auto& measurement : probe.last_measurement_batch()->measurements) {
+    if (cpu_usage_map.find(measurement->pid) != cpu_usage_map.end()) {
+      EXPECT_LT(cpu_usage_map[measurement->pid], measurement->cpu_usage);
+    }
+  }
 
   AtLeastOneMemoryMeasurementIsNonZero(probe.last_measurement_batch());
 

services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h

@@ -6,6 +6,7 @@
 #define SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_PROCESS_COORDINATION_UNIT_IMPL_H_
 
 #include "base/macros.h"
+#include "base/time/time.h"
 #include "services/resource_coordinator/coordination_unit/coordination_unit_base.h"
 
 namespace resource_coordinator {
@@ -41,6 +42,10 @@ class ProcessCoordinationUnitImpl
     private_footprint_kb_ = private_footprint_kb;
   }
   uint64_t private_footprint_kb() const { return private_footprint_kb_; }
+  void set_cumulative_cpu_usage(base::TimeDelta cumulative_cpu_usage) {
+    cumulative_cpu_usage_ = cumulative_cpu_usage;
+  }
+  base::TimeDelta cumulative_cpu_usage() const { return cumulative_cpu_usage_; }
 
   std::set<FrameCoordinationUnitImpl*> GetFrameCoordinationUnits() const;
   std::set<PageCoordinationUnitImpl*> GetAssociatedPageCoordinationUnits()
@@ -56,6 +61,7 @@ class ProcessCoordinationUnitImpl
   bool AddFrame(FrameCoordinationUnitImpl* frame_cu);
   bool RemoveFrame(FrameCoordinationUnitImpl* frame_cu);
 
+  base::TimeDelta cumulative_cpu_usage_;
   uint64_t private_footprint_kb_ = 0u;
 
   std::set<FrameCoordinationUnitImpl*> frame_coordination_units_;

services/resource_coordinator/coordination_unit/system_coordination_unit_impl.cc

@@ -28,6 +28,19 @@ void SystemCoordinationUnitImpl::DistributeMeasurementBatch(
   std::vector<ProcessCoordinationUnitImpl*> processes =
       ProcessCoordinationUnitImpl::GetAllProcessCoordinationUnits();
 
+  base::TimeDelta time_since_last_measurement;
+  if (!last_measurement_batch_time_.is_null()) {
+    // Use the end of the measurement batch as a proxy for when every
+    // measurement was acquired. For the purpose of estimating CPU usage
+    // over the duration from last measurement, it'll be near enough. The error
+    // will average out, and there's an inherent race in knowing when a
+    // measurement was actually acquired in any case.
+    time_since_last_measurement =
+        measurement_batch->batch_ended_time - last_measurement_batch_time_;
+    DCHECK_LE(base::TimeDelta(), time_since_last_measurement);
+  }
+  last_measurement_batch_time_ = measurement_batch->batch_ended_time;
+
   for (const auto& measurement : measurement_batch->measurements) {
     for (auto it = processes.begin(); it != processes.end(); ++it) {
       ProcessCoordinationUnitImpl* process = *it;
@@ -37,7 +50,21 @@ void SystemCoordinationUnitImpl::DistributeMeasurementBatch(
       //     PID.
       if (process->GetProperty(mojom::PropertyType::kPID, &process_pid) &&
           static_cast<base::ProcessId>(process_pid) == measurement->pid) {
-        process->SetCPUUsage(measurement->cpu_usage);
+        if (process->cumulative_cpu_usage().is_zero() ||
+            time_since_last_measurement.is_zero()) {
+          // Imitate the behavior of GetPlatformIndependentCPUUsage, which
+          // yields zero for the initial measurement of each process.
+          process->SetCPUUsage(0.0);
+        } else {
+          base::TimeDelta cumulative_cpu_delta =
+              measurement->cpu_usage - process->cumulative_cpu_usage();
+
+          double cpu_usage =
+              static_cast<double>(cumulative_cpu_delta.InMicroseconds() * 100) /
+              time_since_last_measurement.InMicroseconds();
+          process->SetCPUUsage(cpu_usage);
+        }
+        process->set_cumulative_cpu_usage(measurement->cpu_usage);
         process->set_private_footprint_kb(measurement->private_footprint_kb);
 
         // Remove found processes.

services/resource_coordinator/coordination_unit/system_coordination_unit_impl.h

@@ -6,6 +6,7 @@
 #define SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_SYSTEM_COORDINATION_UNIT_IMPL_H_
 
 #include "base/macros.h"
+#include "base/time/time.h"
 #include "services/resource_coordinator/coordination_unit/coordination_unit_base.h"
 
 namespace resource_coordinator {
@@ -28,6 +29,8 @@ class SystemCoordinationUnitImpl
       mojom::ProcessResourceMeasurementBatchPtr measurement_batch) override;
 
  private:
+  base::TimeTicks last_measurement_batch_time_;
+
   // CoordinationUnitInterface implementation:
   void OnEventReceived(mojom::Event event) override;
   void OnPropertyChanged(mojom::PropertyType property_type,

services/resource_coordinator/coordination_unit/system_coordination_unit_impl_unittest.cc

@@ -67,6 +67,29 @@ class SystemCoordinationUnitImplTest : public CoordinationUnitTestHarness {
   base::SimpleTestTickClock clock_;
 };
 
+mojom::ProcessResourceMeasurementBatchPtr CreateMeasurementBatch(
+    base::TimeTicks start_end_time,
+    size_t num_processes,
+    base::TimeDelta additional_cpu_time) {
+  mojom::ProcessResourceMeasurementBatchPtr batch =
+      mojom::ProcessResourceMeasurementBatch::New();
+  batch->batch_started_time = start_end_time;
+  batch->batch_ended_time = start_end_time;
+
+  for (size_t i = 1; i <= num_processes; ++i) {
+    mojom::ProcessResourceMeasurementPtr measurement =
+        mojom::ProcessResourceMeasurement::New();
+    measurement->pid = i;
+    measurement->cpu_usage =
+        base::TimeDelta::FromMicroseconds(i) + additional_cpu_time;
+    measurement->private_footprint_kb = static_cast<uint32_t>(i);
+
+    batch->measurements.push_back(std::move(measurement));
+  }
+
+  return batch;
+}
+
 }  // namespace
 
 TEST_F(SystemCoordinationUnitImplTest, OnProcessCPUUsageReady) {
@@ -88,51 +111,46 @@ TEST_F(SystemCoordinationUnitImplTest, DistributeMeasurementBatch) {
   EXPECT_EQ(0u, observer.system_event_seen_count());
 
   // Build and dispatch a measurement batch.
-  mojom::ProcessResourceMeasurementBatchPtr batch =
-      mojom::ProcessResourceMeasurementBatch::New();
-
-  for (size_t i = 1; i <= 3; ++i) {
-    mojom::ProcessResourceMeasurementPtr measurement =
-        mojom::ProcessResourceMeasurement::New();
-    measurement->pid = i;
-    measurement->cpu_usage = static_cast<double>(i);
-    measurement->private_footprint_kb = static_cast<uint32_t>(i);
-
-    batch->measurements.push_back(std::move(measurement));
-  }
+  base::TimeTicks start_time = base::TimeTicks::Now();
   EXPECT_EQ(0U, observer.process_property_change_seen_count());
-  cu_graph.system->DistributeMeasurementBatch(std::move(batch));
+  cu_graph.system->DistributeMeasurementBatch(
+      CreateMeasurementBatch(start_time, 3, base::TimeDelta()));
   // EXPECT_EQ(2U, observer.process_property_change_seen_count());
   EXPECT_EQ(1u, observer.system_event_seen_count());
 
-  // Validate that the measurements were distributed to the process CUs.
+  // The first measurement batch results in a zero CPU usage for the processes.
   int64_t cpu_usage;
   EXPECT_TRUE(cu_graph.process->GetProperty(mojom::PropertyType::kCPUUsage,
                                             &cpu_usage));
-  EXPECT_EQ(1000, cpu_usage);
+  EXPECT_EQ(0, cpu_usage);
   EXPECT_EQ(1u, cu_graph.process->private_footprint_kb());
 
   EXPECT_TRUE(cu_graph.other_process->GetProperty(
       mojom::PropertyType::kCPUUsage, &cpu_usage));
-  EXPECT_EQ(2000, cpu_usage);
+  EXPECT_EQ(0, cpu_usage);
   EXPECT_EQ(2u, cu_graph.other_process->private_footprint_kb());
 
+  // Dispatch another batch, and verify the CPUUsage is appropriately updated.
+  cu_graph.system->DistributeMeasurementBatch(
+      CreateMeasurementBatch(start_time + base::TimeDelta::FromMicroseconds(1),
+                             3, base::TimeDelta::FromMicroseconds(1)));
+  EXPECT_TRUE(cu_graph.process->GetProperty(mojom::PropertyType::kCPUUsage,
+                                            &cpu_usage));
+  EXPECT_EQ(100000, cpu_usage);
+  EXPECT_TRUE(cu_graph.other_process->GetProperty(
+      mojom::PropertyType::kCPUUsage, &cpu_usage));
+  EXPECT_EQ(100000, cpu_usage);
+
   // Now test that a measurement batch that leaves out a process clears the
   // properties of that process.
-  batch = mojom::ProcessResourceMeasurementBatch::New();
-  mojom::ProcessResourceMeasurementPtr measurement =
-      mojom::ProcessResourceMeasurement::New();
-  measurement->pid = 1;
-  measurement->cpu_usage = 30.0;
-  measurement->private_footprint_kb = 30u;
-  batch->measurements.push_back(std::move(measurement));
-
-  cu_graph.system->DistributeMeasurementBatch(std::move(batch));
+  cu_graph.system->DistributeMeasurementBatch(
+      CreateMeasurementBatch(start_time + base::TimeDelta::FromMicroseconds(2),
+                             1, base::TimeDelta::FromMicroseconds(31)));
 
   EXPECT_TRUE(cu_graph.process->GetProperty(mojom::PropertyType::kCPUUsage,
                                             &cpu_usage));
-  EXPECT_EQ(30000, cpu_usage);
-  EXPECT_EQ(30u, cu_graph.process->private_footprint_kb());
+  EXPECT_EQ(3000000, cpu_usage);
+  EXPECT_EQ(1u, cu_graph.process->private_footprint_kb());
 
   EXPECT_TRUE(cu_graph.other_process->GetProperty(
       mojom::PropertyType::kCPUUsage, &cpu_usage));

services/resource_coordinator/public/mojom/coordination_unit.mojom

@@ -28,11 +28,8 @@ struct ProcessResourceMeasurement {
   // Identifies the process associated with this measurement.
   mojo_base.mojom.ProcessId pid;
 
-  // The CPU usage accrued to this process since it was last measured, this is
-  // in units of CPU cores over the measurement interval.
-  // See base/process/process_metrics.h for the details, but a 1.0 means a
-  // single core was consumed fully since last measurement.
-  double cpu_usage = 0.0;
+  // The cumulative CPU usage accrued to this process from its start.
+  mojo_base.mojom.TimeDelta cpu_usage;
 
   // The private memory footprint of the process.
   uint32 private_footprint_kb = 0;
@@ -42,8 +39,8 @@ struct ProcessResourceMeasurementBatch {
   // These times bracket the capture of the entire dump, e.g. each distinct
   // measurement is captured somewhere between |batch_started_time| and
   // |batch_ended_time|.
-  mojo_base.mojom.Time batch_started_time;
-  mojo_base.mojom.Time batch_ended_time;
+  mojo_base.mojom.TimeTicks batch_started_time;
+  mojo_base.mojom.TimeTicks batch_ended_time;
 
   array<ProcessResourceMeasurement> measurements;
 };