RC: Add memory measurement to render process probe.

Bug: 755840
Change-Id: I96af63e817abe9bdefb0916da4d652e97c620a2c
Reviewed-on: https://chromium-review.googlesource.com/1037631
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555844}

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.cc

@@ -7,7 +7,6 @@
 #include <vector>
 
 #include "base/bind.h"
-#include "base/values.h"
 #include "build/build_config.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
@@ -27,9 +26,6 @@ namespace {
 constexpr base::TimeDelta kDefaultMeasurementInterval =
     base::TimeDelta::FromMinutes(10);
 
-base::LazyInstance<ResourceCoordinatorRenderProcessProbe>::DestructorAtExit
-    g_probe = LAZY_INSTANCE_INITIALIZER;
-
 }  // namespace
 
 ResourceCoordinatorRenderProcessProbe::RenderProcessInfo::RenderProcessInfo() =
@@ -49,7 +45,8 @@ ResourceCoordinatorRenderProcessProbe::
 // static
 ResourceCoordinatorRenderProcessProbe*
 ResourceCoordinatorRenderProcessProbe::GetInstance() {
-  return g_probe.Pointer();
+  static base::NoDestructor<ResourceCoordinatorRenderProcessProbe> probe;
+  return probe.get();
 }
 
 // static
@@ -132,16 +129,24 @@ void ResourceCoordinatorRenderProcessProbe::
 
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&ResourceCoordinatorRenderProcessProbe::
-                         CollectAndDispatchRenderProcessMetricsOnIOThread,
-                     base::Unretained(this)));
+      base::BindOnce(
+          &ResourceCoordinatorRenderProcessProbe::
+              CollectRenderProcessMetricsAndStartMemoryDumpOnIOThread,
+          base::Unretained(this)));
 }
 
 void ResourceCoordinatorRenderProcessProbe::
-    CollectAndDispatchRenderProcessMetricsOnIOThread() {
+    CollectRenderProcessMetricsAndStartMemoryDumpOnIOThread() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(is_gathering_);
 
+  // Dispatch the memory collection request.
+  memory_instrumentation::MemoryInstrumentation::GetInstance()
+      ->RequestGlobalDump(
+          base::BindRepeating(&ResourceCoordinatorRenderProcessProbe::
+                                  ProcessGlobalMemoryDumpAndDispatchOnIOThread,
+                              base::Unretained(this)));
+
   RenderProcessInfoMap::iterator iter = render_process_info_map_.begin();
   while (iter != render_process_info_map_.end()) {
     auto& render_process_info = iter->second;
@@ -157,12 +162,21 @@ void ResourceCoordinatorRenderProcessProbe::
       continue;
     }
   }
+}
 
+void ResourceCoordinatorRenderProcessProbe::
+    ProcessGlobalMemoryDumpAndDispatchOnIOThread(
+        bool global_success,
+        std::unique_ptr<memory_instrumentation::GlobalMemoryDump> dump) {
   // Create the measurement batch.
   mojom::ProcessResourceMeasurementBatchPtr batch =
       mojom::ProcessResourceMeasurementBatch::New();
 
-  for (auto& render_process_info_map_entry : render_process_info_map_) {
+  // 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;
     // TODO(oysteine): Move the multiplier used to avoid precision loss
     // into a shared location, when this property gets used.
@@ -171,13 +185,35 @@ void ResourceCoordinatorRenderProcessProbe::
 
     measurement->pid = render_process_info.process.Pid();
     measurement->cpu_usage = render_process_info.cpu_usage;
-    // TODO(siggi): Add the private footprint.
 
     batch->measurements.push_back(std::move(measurement));
   }
 
-  bool should_restart = DispatchMetrics(std::move(batch));
+  if (dump) {
+    // Then amend the ones we have memory metrics for with their private
+    // footprint. The global dump may contain non-renderer processes, it may
+    // contain renderer processes we didn't capture at the start of the cycle,
+    // and it may not contain all the renderer processes we know about.
+    // This may happen due to the inherent race between the request and
+    // starting/stopping renderers, or because of other failures
+    // This may therefore provide incomplete information.
+    for (const auto& dump_entry : dump->process_dumps()) {
+      base::ProcessId pid = dump_entry.pid();
+
+      for (const auto& measurement : batch->measurements) {
+        if (measurement->pid == pid) {
+          measurement->private_footprint_kb =
+              dump_entry.os_dump().private_footprint_kb;
+          break;
+        }
+      }
+    }
+  } else {
+    // We should only get a nullptr in case of failure.
+    DCHECK(!global_success);
+  }
 
+  bool should_restart = DispatchMetrics(std::move(batch));
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
       base::BindOnce(

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.h

@@ -9,11 +9,12 @@
 #include <memory>
 #include <utility>
 
-#include "base/lazy_instance.h"
 #include "base/macros.h"
+#include "base/no_destructor.h"
 #include "base/process/process.h"
 #include "base/process/process_metrics.h"
 #include "base/timer/timer.h"
+#include "services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h"
 #include "services/resource_coordinator/public/cpp/system_resource_coordinator.h"
 
 namespace resource_coordinator {
@@ -41,6 +42,7 @@ class ResourceCoordinatorRenderProcessProbe {
   void StartSingleGather();
 
  protected:
+  // Internal state protected for testing.
   struct RenderProcessInfo {
     RenderProcessInfo();
     ~RenderProcessInfo();
@@ -51,9 +53,7 @@ class ResourceCoordinatorRenderProcessProbe {
   };
   using RenderProcessInfoMap = std::map<int, RenderProcessInfo>;
 
-  // Internal state protected for testing.
-  friend struct base::LazyInstanceTraitsBase<
-      ResourceCoordinatorRenderProcessProbe>;
+  friend class base::NoDestructor<ResourceCoordinatorRenderProcessProbe>;
 
   ResourceCoordinatorRenderProcessProbe();
   virtual ~ResourceCoordinatorRenderProcessProbe();
@@ -61,10 +61,13 @@ 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 and dispatch the render process metrics to the system
-  // coordination unit.
-  void CollectAndDispatchRenderProcessMetricsOnIOThread();
-  // (3) Initiate the next render process metrics collection cycle if the
+  // (2) Collect the render process CPU metrics and initiate a memory dump.
+  void CollectRenderProcessMetricsAndStartMemoryDumpOnIOThread();
+  // (3) Process the results of the memory dump and dispatch the results.
+  void ProcessGlobalMemoryDumpAndDispatchOnIOThread(
+      bool success,
+      std::unique_ptr<memory_instrumentation::GlobalMemoryDump> dump);
+  // (4) 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.

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe_browsertest.cc

@@ -135,8 +135,10 @@ IN_PROC_BROWSER_TEST_F(ResourceCoordinatorRenderProcessProbeBrowserTest,
   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)
+  for (const auto& measurement : probe.last_measurement_batch()->measurements) {
     EXPECT_EQ(0.0, measurement->cpu_usage);
+    EXPECT_NE(0u, measurement->private_footprint_kb);
+  }
 
   // Open a second tab and complete a navigation.
   ui_test_utils::NavigateToURLWithDisposition(
@@ -164,8 +166,10 @@ 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.
-  for (const auto& measurement : probe.last_measurement_batch()->measurements)
+  for (const auto& measurement : probe.last_measurement_batch()->measurements) {
     EXPECT_LE(0.0, measurement->cpu_usage);
+    EXPECT_NE(0u, measurement->private_footprint_kb);
+  }
 
   EXPECT_EQ(info_map_size, info_map.size());
   for (const auto& entry : probe.render_process_info_map()) {