RC: Use batch interface to dispatch CPU measurement.

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

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.cc

@@ -124,7 +124,6 @@ void ResourceCoordinatorRenderProcessProbe::
       render_process_info.metrics = base::ProcessMetrics::CreateProcessMetrics(
           render_process_info.process.Handle());
 #endif
-      render_process_info.render_process_host_id = host->GetID();
     }
   }
 
@@ -133,12 +132,12 @@ void ResourceCoordinatorRenderProcessProbe::
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::BindOnce(&ResourceCoordinatorRenderProcessProbe::
-                         CollectRenderProcessMetricsOnIOThread,
+                         CollectAndDispatchRenderProcessMetricsOnIOThread,
                      base::Unretained(this)));
 }
 
 void ResourceCoordinatorRenderProcessProbe::
-    CollectRenderProcessMetricsOnIOThread() {
+    CollectAndDispatchRenderProcessMetricsOnIOThread() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(is_gathering_);
 
@@ -158,20 +157,22 @@ void ResourceCoordinatorRenderProcessProbe::
     }
   }
 
+  bool should_restart = DispatchMetrics();
+
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
-      base::BindOnce(&ResourceCoordinatorRenderProcessProbe::
-                         HandleRenderProcessMetricsOnUIThread,
-                     base::Unretained(this)));
+      base::BindOnce(
+          &ResourceCoordinatorRenderProcessProbe::FinishCollectionOnUIThread,
+          base::Unretained(this), should_restart));
 }
 
-void ResourceCoordinatorRenderProcessProbe::
-    HandleRenderProcessMetricsOnUIThread() {
+void ResourceCoordinatorRenderProcessProbe::FinishCollectionOnUIThread(
+    bool restart_cycle) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(is_gathering_);
   is_gathering_ = false;
 
-  if (DispatchMetrics() && is_gather_cycle_started_) {
+  if (restart_cycle && is_gather_cycle_started_) {
     timer_.Start(FROM_HERE, interval_, this,
                  &ResourceCoordinatorRenderProcessProbe::
                      RegisterAliveRenderProcessesOnUIThread);
@@ -190,7 +191,6 @@ void ResourceCoordinatorRenderProcessProbe::UpdateWithFieldTrialParams() {
 
 SystemResourceCoordinator*
 ResourceCoordinatorRenderProcessProbe::EnsureSystemResourceCoordinator() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!system_resource_coordinator_) {
     content::ServiceManagerConnection* connection =
         content::ServiceManagerConnection::GetForProcess();
@@ -204,29 +204,30 @@ ResourceCoordinatorRenderProcessProbe::EnsureSystemResourceCoordinator() {
 }
 
 bool ResourceCoordinatorRenderProcessProbe::DispatchMetrics() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   SystemResourceCoordinator* system_resource_coordinator =
       EnsureSystemResourceCoordinator();
 
   if (system_resource_coordinator) {
-    bool dispatched_measurement = false;
+    mojom::ProcessResourceMeasurementBatchPtr batch =
+        mojom::ProcessResourceMeasurementBatch::New();
+
     for (auto& render_process_info_map_entry : render_process_info_map_) {
       auto& render_process_info = render_process_info_map_entry.second;
       // TODO(oysteine): Move the multiplier used to avoid precision loss
       // into a shared location, when this property gets used.
+      mojom::ProcessResourceMeasurementPtr measurement =
+          mojom::ProcessResourceMeasurement::New();
+
+      measurement->pid = render_process_info.process.Pid();
+      measurement->cpu_usage = render_process_info.cpu_usage;
+      // TODO(siggi): Add the private footprint.
 
-      // Note that the RPH may have been deleted while the CPU metrics were
-      // acquired on a blocking thread.
-      content::RenderProcessHost* host = content::RenderProcessHost::FromID(
-          render_process_info.render_process_host_id);
-      if (host) {
-        dispatched_measurement = true;
-        host->GetProcessResourceCoordinator()->SetCPUUsage(
-            render_process_info.cpu_usage);
-      }
+      batch->measurements.push_back(std::move(measurement));
     }
 
-    if (dispatched_measurement)
-      system_resource_coordinator->OnProcessCPUUsageReady();
+    if (!batch->measurements.empty())
+      system_resource_coordinator->DistributeMeasurementBatch(std::move(batch));
   }
 
   return true;

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.h

@@ -24,14 +24,11 @@ struct RenderProcessInfo {
   ~RenderProcessInfo();
   base::Process process;
   double cpu_usage = -1.0;
-  // This structure bounces from the UI thread to blocking threads and back.
-  // It's therefore not safe to store RenderProcessHost pointers, so the ID is
-  // used instead.
-  int render_process_host_id = 0;
   size_t last_gather_cycle_active;
   std::unique_ptr<base::ProcessMetrics> metrics;
 };
 
+// This map is keyed by the RenderProcessHost's ID from the GetID function.
 using RenderProcessInfoMap = std::map<int, RenderProcessInfo>;
 
 // |ResourceCoordinatorRenderProcessProbe| collects measurements about render
@@ -67,27 +64,27 @@ class ResourceCoordinatorRenderProcessProbe {
   // (1) Identify all of the render processes that are active to measure.
   // Child render processes can only be discovered in the browser's UI thread.
   void RegisterAliveRenderProcessesOnUIThread();
-  // (2) Collect render process metrics.
-  void CollectRenderProcessMetricsOnIOThread();
-  // (3) Send the collected render process metrics to the appropriate
-  // coordination units in the |resource_coordinator| service. Then
-  // initiates the next render process metrics collection cycle, which
-  // consists of a delayed call to perform (1) via a timer.
-  void HandleRenderProcessMetricsOnUIThread();
+  // (2) Collect and dispatch the render process metrics to the system
+  // coordination unit.
+  void CollectAndDispatchRenderProcessMetricsOnIOThread();
+  // (3) Initiate the next render process metrics collection cycle if the
+  // cycle has been started and |restart_cycle| is true, which consists of a
+  // delayed call to perform (1) via a timer.
+  // Virtual for testing.
+  virtual void FinishCollectionOnUIThread(bool restart_cycle);
 
   // Allows FieldTrial parameters to override defaults.
   void UpdateWithFieldTrialParams();
 
   SystemResourceCoordinator* EnsureSystemResourceCoordinator();
 
-  // Dispatch the collected metrics. Returns |true| if another metrics
-  // collection gather cycle should be initiated. Virtual for testing.
-  // Default implementation sends collected metrics back to the resource
-  // coordinator service and initiates another render process metrics gather
-  // cycle.
+  // Dispatch the collected metrics, return true if the cycle should restart.
+  // Virtual for testing.
   virtual bool DispatchMetrics();
 
-  // A map of currently running render process host IDs to Process.
+  // A map of currently running render process host IDs to process.
+  // This map is accessed alternatively from the UI thread and the IO thread,
+  // but only one of the two at a time.
   RenderProcessInfoMap render_process_info_map_;
 
   // Time duration between measurements.

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe_browsertest.cc

@@ -31,10 +31,16 @@ class TestingResourceCoordinatorRenderProcessProbe
   ~TestingResourceCoordinatorRenderProcessProbe() override = default;
 
   bool DispatchMetrics() override {
-    current_run_loop_->QuitWhenIdle();
     return false;
   }
 
+  void FinishCollectionOnUIThread(bool restart_cycle) override {
+    ResourceCoordinatorRenderProcessProbe::FinishCollectionOnUIThread(
+        restart_cycle);
+
+    current_run_loop_->QuitWhenIdle();
+  }
+
   // Returns |true| if all of the elements in |*render_process_info_map_|
   // are up-to-date with |current_gather_cycle_|.
   bool AllMeasurementsAreAtCurrentCycle() const {
@@ -128,9 +134,7 @@ IN_PROC_BROWSER_TEST_F(ResourceCoordinatorRenderProcessProbeBrowserTest,
   // measurement cycles.
   std::map<int, const RenderProcessInfo*> info_map;
   for (const auto& entry : probe.render_process_info_map()) {
-    const int key = entry.first;
     const RenderProcessInfo& info = entry.second;
-    EXPECT_EQ(key, info.render_process_host_id);
     EXPECT_TRUE(info_map.insert(std::make_pair(entry.first, &info)).second);
   }
 

services/resource_coordinator/public/cpp/system_resource_coordinator.cc

@@ -15,10 +15,11 @@ SystemResourceCoordinator::SystemResourceCoordinator(
 
 SystemResourceCoordinator::~SystemResourceCoordinator() = default;
 
-void SystemResourceCoordinator::OnProcessCPUUsageReady() {
+void SystemResourceCoordinator::DistributeMeasurementBatch(
+    mojom::ProcessResourceMeasurementBatchPtr batch) {
   if (!service_)
     return;
-  service_->OnProcessCPUUsageReady();
+  service_->DistributeMeasurementBatch(std::move(batch));
 }
 
 void SystemResourceCoordinator::ConnectToService(

services/resource_coordinator/public/cpp/system_resource_coordinator.h

@@ -18,7 +18,8 @@ class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT SystemResourceCoordinator
   SystemResourceCoordinator(service_manager::Connector* connector);
   ~SystemResourceCoordinator() override;
 
-  void OnProcessCPUUsageReady();
+  void DistributeMeasurementBatch(
+      mojom::ProcessResourceMeasurementBatchPtr batch);
 
  private:
   void ConnectToService(mojom::CoordinationUnitProviderPtr& provider,