RC: Index render process map by RPH ID instead of handle.

This avoids creating a new entry in the map for each process on each
cycle on Windows, which was happening as of my last change.
Add a state variable to keep track of when a measurement cycle is
underway in preparation for allowing measurement requests at arbitrary
times. Replace the delegate class with a simple virtual function, and
use protected access control in combination with a Testing* subclass
to expose class' internal state for testing. This allows removing all
test-specific code from the class declaration.

Bug: 755840
Change-Id: I7658a355525fffe92327bb85151ba124cc81d371
Reviewed-on: https://chromium-review.googlesource.com/1015299
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551794}

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.cc

@@ -36,43 +36,8 @@ RenderProcessInfo::RenderProcessInfo() = default;
 
 RenderProcessInfo::~RenderProcessInfo() = default;
 
-RenderProcessMetricsHandler::RenderProcessMetricsHandler() = default;
-
-RenderProcessMetricsHandler::~RenderProcessMetricsHandler() = default;
-
-class ResourceCoordinatorRenderProcessMetricsHandler
-    : public RenderProcessMetricsHandler {
- public:
-  ResourceCoordinatorRenderProcessMetricsHandler() = default;
-  ~ResourceCoordinatorRenderProcessMetricsHandler() override = default;
-
-  // Send collected metrics back to the |resource_coordinator| service
-  // and initiates another render process metrics gather cycle.
-  bool HandleMetrics(
-      const RenderProcessInfoMap& render_process_info_map) override {
-    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.
-
-      // 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) {
-        host->GetProcessResourceCoordinator()->SetCPUUsage(
-            render_process_info.cpu_usage);
-      }
-    }
-
-    return true;
-  }
-};
-
 ResourceCoordinatorRenderProcessProbe::ResourceCoordinatorRenderProcessProbe()
-    : metrics_handler_(
-          std::make_unique<ResourceCoordinatorRenderProcessMetricsHandler>()),
-      interval_(kDefaultMeasurementInterval) {
+    : interval_(kDefaultMeasurementInterval) {
   UpdateWithFieldTrialParams();
 }
 
@@ -108,6 +73,7 @@ void ResourceCoordinatorRenderProcessProbe::StartGatherCycle() {
 void ResourceCoordinatorRenderProcessProbe::
     RegisterAliveRenderProcessesOnUIThread() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(!is_gathering_);
 
   ++current_gather_cycle_;
 
@@ -120,14 +86,15 @@ void ResourceCoordinatorRenderProcessProbe::
       continue;
     }
 
-    // Duplicate the process to retain ownership of it through the thread
-    // bouncing.
-    base::Process process(host->GetProcess().Duplicate());
-    auto& render_process_info = render_process_info_map_[process.Handle()];
+    auto& render_process_info = render_process_info_map_[host->GetID()];
     render_process_info.last_gather_cycle_active = current_gather_cycle_;
-    render_process_info.process = std::move(process);
-
     if (render_process_info.metrics.get() == nullptr) {
+      DCHECK(!render_process_info.process.IsValid());
+
+      // Duplicate the process to retain ownership of it through the thread
+      // bouncing.
+      render_process_info.process = host->GetProcess().Duplicate();
+
 #if defined(OS_MACOSX)
       render_process_info.metrics = base::ProcessMetrics::CreateProcessMetrics(
           render_process_info.process.Handle(),
@@ -140,6 +107,8 @@ void ResourceCoordinatorRenderProcessProbe::
     }
   }
 
+  is_gathering_ = true;
+
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::BindOnce(&ResourceCoordinatorRenderProcessProbe::
@@ -150,6 +119,7 @@ void ResourceCoordinatorRenderProcessProbe::
 void ResourceCoordinatorRenderProcessProbe::
     CollectRenderProcessMetricsOnIOThread() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  DCHECK(is_gathering_);
 
   RenderProcessInfoMap::iterator iter = render_process_info_map_.begin();
   while (iter != render_process_info_map_.end()) {
@@ -177,22 +147,33 @@ void ResourceCoordinatorRenderProcessProbe::
 void ResourceCoordinatorRenderProcessProbe::
     HandleRenderProcessMetricsOnUIThread() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  if (metrics_handler_->HandleMetrics(render_process_info_map_)) {
+  DCHECK(is_gathering_);
+  is_gathering_ = false;
+
+  if (HandleMetrics(render_process_info_map_)) {
     timer_.Start(FROM_HERE, interval_, this,
                  &ResourceCoordinatorRenderProcessProbe::
                      RegisterAliveRenderProcessesOnUIThread);
   }
 }
 
-bool ResourceCoordinatorRenderProcessProbe::
-    AllRenderProcessMeasurementsAreCurrentForTesting() const {
-  for (auto& render_process_info_map_entry : render_process_info_map_) {
+bool ResourceCoordinatorRenderProcessProbe::HandleMetrics(
+    const RenderProcessInfoMap& render_process_info_map) {
+  for (auto& render_process_info_map_entry : render_process_info_map) {
     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) {
-      return false;
+    // TODO(oysteine): Move the multiplier used to avoid precision loss
+    // into a shared location, when this property gets used.
+
+    // 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) {
+      host->GetProcessResourceCoordinator()->SetCPUUsage(
+          render_process_info.cpu_usage);
     }
   }
+
   return true;
 }
 

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.h

@@ -30,24 +30,13 @@ struct RenderProcessInfo {
   std::unique_ptr<base::ProcessMetrics> metrics;
 };
 
-using RenderProcessInfoMap = std::map<base::ProcessHandle, RenderProcessInfo>;
-
-// A delegate class for handling metrics collected after each
-// |ResourceCoordinatorRenderProcessProbe| gather cycle.
-class RenderProcessMetricsHandler {
- public:
-  RenderProcessMetricsHandler();
-  virtual ~RenderProcessMetricsHandler();
-  // Handle collected metrics. Returns |true| if the
-  // |ResourceCoordinatorRenderProcessProbe| should initiate another
-  // metrics collection gather cycle.
-  virtual bool HandleMetrics(
-      const RenderProcessInfoMap& render_process_info_map) = 0;
-};
+using RenderProcessInfoMap = std::map<int, RenderProcessInfo>;
 
 // |ResourceCoordinatorRenderProcessProbe| collects measurements about render
 // processes and propagates them to the |resource_coordinator| service.
 // Currently this is only supported for Chrome Metrics experiments.
+// The measurements are initiated and results are dispatched from the UI
+// thread, while acquiring the measurements is done on the IO thread.
 class ResourceCoordinatorRenderProcessProbe {
  public:
   // Returns the current |ResourceCoordinatorRenderProcessProbe| instance
@@ -56,9 +45,18 @@ class ResourceCoordinatorRenderProcessProbe {
 
   static bool IsEnabled();
 
-  // Render process metrics collection cycle:
-  // (0) Initialize and begin render process metrics collection.
+  // Starts the automatic, timed process metrics collection cycle.
+  // Can only be invoked from the UI thread.
   void StartGatherCycle();
+
+ protected:
+  // Internal state protected for testing.
+  friend struct base::LazyInstanceTraitsBase<
+      ResourceCoordinatorRenderProcessProbe>;
+
+  ResourceCoordinatorRenderProcessProbe();
+  virtual ~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();
@@ -70,19 +68,15 @@ class ResourceCoordinatorRenderProcessProbe {
   // consists of a delayed call to perform (1) via a timer.
   void HandleRenderProcessMetricsOnUIThread();
 
- private:
-  FRIEND_TEST_ALL_PREFIXES(ResourceCoordinatorRenderProcessProbeBrowserTest,
-                           TrackAndMeasureActiveRenderProcesses);
-  friend struct base::LazyInstanceTraitsBase<
-      ResourceCoordinatorRenderProcessProbe>;
-
-  ResourceCoordinatorRenderProcessProbe();
-  ~ResourceCoordinatorRenderProcessProbe();
-
-  // Delegate for handling metrics collected after each gather cycle.
-  std::unique_ptr<RenderProcessMetricsHandler> metrics_handler_;
+  // Handle collected metrics. Returns |true| 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.
+  virtual bool HandleMetrics(
+      const RenderProcessInfoMap& render_process_info_map);
 
-  // A map of currently running ProcessHandles to Process.
+  // A map of currently running render process host IDs to Process.
   RenderProcessInfoMap render_process_info_map_;
 
   // Time duration between measurements.
@@ -95,25 +89,12 @@ class ResourceCoordinatorRenderProcessProbe {
   // Number of measurements collected so far.
   size_t current_gather_cycle_ = 0u;
 
+  // True while a gathering cycle is underways on a background thread.
+  bool is_gathering_ = false;
+
   // Allows FieldTrial parameters to override defaults.
   void UpdateWithFieldTrialParams();
 
-  // Settings/getters for testing.
-  void set_render_process_metrics_handler_for_testing(
-      std::unique_ptr<RenderProcessMetricsHandler> metrics_handler) {
-    metrics_handler_ = std::move(metrics_handler);
-  }
-  const RenderProcessInfoMap& render_process_info_map_for_testing() const {
-    return render_process_info_map_;
-  }
-  size_t current_gather_cycle_for_testing() const {
-    return current_gather_cycle_;
-  }
-
-  // Returns |true| if all of the elements in |render_process_info_map_|
-  // are up-to-date with the |current_gather_cycle_|.
-  bool AllRenderProcessMeasurementsAreCurrentForTesting() const;
-
   DISALLOW_COPY_AND_ASSIGN(ResourceCoordinatorRenderProcessProbe);
 };
 

chrome/browser/resource_coordinator/resource_coordinator_render_process_probe_browsertest.cc

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <map>
 #include <string>
 
 #include "base/feature_list.h"
@@ -22,42 +23,60 @@
 
 namespace resource_coordinator {
 
-class MockResourceCoordinatorRenderProcessMetricsHandler
-    : public RenderProcessMetricsHandler {
+class TestingResourceCoordinatorRenderProcessProbe
+    : public ResourceCoordinatorRenderProcessProbe {
  public:
-  MockResourceCoordinatorRenderProcessMetricsHandler() = default;
-  ~MockResourceCoordinatorRenderProcessMetricsHandler() override = default;
+  TestingResourceCoordinatorRenderProcessProbe() = default;
+  ~TestingResourceCoordinatorRenderProcessProbe() override = default;
 
   bool HandleMetrics(
       const RenderProcessInfoMap& render_process_info_map) override {
-    base::RunLoop::QuitCurrentWhenIdleDeprecated();
+    current_run_loop_->QuitWhenIdle();
     return false;
   }
 
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockResourceCoordinatorRenderProcessMetricsHandler);
-};
+  // Returns |true| if all of the elements in |*render_process_info_map_|
+  // are up-to-date with |current_gather_cycle_|.
+  bool AllMeasurementsAreAtCurrentCycle() const {
+    for (auto& render_process_info_map_entry : render_process_info_map_) {
+      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) {
+        return false;
+      }
+    }
+    return true;
+  }
 
-class ResourceCoordinatorRenderProcessProbeBrowserTest
-    : public InProcessBrowserTest {
- public:
-  ResourceCoordinatorRenderProcessProbeBrowserTest() = default;
-  ~ResourceCoordinatorRenderProcessProbeBrowserTest() override = default;
+  size_t current_gather_cycle() const { return current_gather_cycle_; }
+  const RenderProcessInfoMap& render_process_info_map() const {
+    return render_process_info_map_;
+  }
 
   void StartGatherCycleAndWait() {
     base::RunLoop run_loop;
-    probe_->StartGatherCycle();
+    current_run_loop_ = &run_loop;
+
+    StartGatherCycle();
     run_loop.Run();
-  }
 
-  void set_probe(
-      resource_coordinator::ResourceCoordinatorRenderProcessProbe* probe) {
-    probe_ = probe;
+    current_run_loop_ = nullptr;
   }
 
  private:
-  resource_coordinator::ResourceCoordinatorRenderProcessProbe* probe_;
+  base::RunLoop* current_run_loop_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(TestingResourceCoordinatorRenderProcessProbe);
+};
 
+class ResourceCoordinatorRenderProcessProbeBrowserTest
+    : public InProcessBrowserTest {
+ public:
+  ResourceCoordinatorRenderProcessProbeBrowserTest() = default;
+  ~ResourceCoordinatorRenderProcessProbeBrowserTest() override = default;
+
+ private:
   DISALLOW_COPY_AND_ASSIGN(ResourceCoordinatorRenderProcessProbeBrowserTest);
 };
 
@@ -69,21 +88,18 @@ IN_PROC_BROWSER_TEST_F(ResourceCoordinatorRenderProcessProbeBrowserTest,
                                  features::kGRCRenderProcessCPUProfiling},
                                 {});
 
-  resource_coordinator::ResourceCoordinatorRenderProcessProbe probe;
-  probe.set_render_process_metrics_handler_for_testing(
-      std::make_unique<MockResourceCoordinatorRenderProcessMetricsHandler>());
-  set_probe(&probe);
+  TestingResourceCoordinatorRenderProcessProbe probe;
 
   ASSERT_TRUE(embedded_test_server()->Start());
-  EXPECT_EQ(0u, probe.current_gather_cycle_for_testing());
+  EXPECT_EQ(0u, probe.current_gather_cycle());
 
   // A tab is already open when the test begins.
-  StartGatherCycleAndWait();
-  EXPECT_EQ(1u, probe.current_gather_cycle_for_testing());
-  size_t initial_size = probe.render_process_info_map_for_testing().size();
+  probe.StartGatherCycleAndWait();
+  EXPECT_EQ(1u, probe.current_gather_cycle());
+  size_t initial_size = probe.render_process_info_map().size();
   EXPECT_LE(1u, initial_size);
   EXPECT_GE(2u, initial_size);  // If a spare RenderProcessHost is present.
-  EXPECT_TRUE(probe.AllRenderProcessMeasurementsAreCurrentForTesting());
+  EXPECT_TRUE(probe.AllMeasurementsAreAtCurrentCycle());
 
   // Open a second tab and complete a navigation.
   ui_test_utils::NavigateToURLWithDisposition(
@@ -91,11 +107,31 @@ IN_PROC_BROWSER_TEST_F(ResourceCoordinatorRenderProcessProbeBrowserTest,
       WindowOpenDisposition::NEW_FOREGROUND_TAB,
       ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB |
           ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
-  StartGatherCycleAndWait();
-  EXPECT_EQ(2u, probe.current_gather_cycle_for_testing());
-  EXPECT_EQ(initial_size + 1u,
-            probe.render_process_info_map_for_testing().size());
-  EXPECT_TRUE(probe.AllRenderProcessMeasurementsAreCurrentForTesting());
+  probe.StartGatherCycleAndWait();
+  EXPECT_EQ(2u, probe.current_gather_cycle());
+  EXPECT_EQ(initial_size + 1u, probe.render_process_info_map().size());
+  EXPECT_TRUE(probe.AllMeasurementsAreAtCurrentCycle());
+
+  // Verify that the elements in the map are reused across multiple
+  // 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);
+  }
+
+  size_t info_map_size = info_map.size();
+  probe.StartGatherCycleAndWait();
+
+  EXPECT_EQ(info_map_size, info_map.size());
+  for (const auto& entry : probe.render_process_info_map()) {
+    const int key = entry.first;
+    const RenderProcessInfo& info = entry.second;
+
+    EXPECT_EQ(&info, info_map[key]);
+  }
 
   // Kill one of the two open tabs.
   EXPECT_TRUE(browser()
@@ -104,10 +140,10 @@ IN_PROC_BROWSER_TEST_F(ResourceCoordinatorRenderProcessProbeBrowserTest,
                   ->GetMainFrame()
                   ->GetProcess()
                   ->FastShutdownIfPossible());
-  StartGatherCycleAndWait();
-  EXPECT_EQ(3u, probe.current_gather_cycle_for_testing());
-  EXPECT_EQ(initial_size, probe.render_process_info_map_for_testing().size());
-  EXPECT_TRUE(probe.AllRenderProcessMeasurementsAreCurrentForTesting());
+  probe.StartGatherCycleAndWait();
+  EXPECT_EQ(4u, probe.current_gather_cycle());
+  EXPECT_EQ(initial_size, probe.render_process_info_map().size());
+  EXPECT_TRUE(probe.AllMeasurementsAreAtCurrentCycle());
 }
 
 }  // namespace resource_coordinator