RC: Index process CUs by PID.

Also make some implementation-only constants private while I was
in there.

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

services/resource_coordinator/coordination_unit/coordination_unit_graph.cc

@@ -30,7 +30,21 @@ CoordinationUnitGraph::CoordinationUnitGraph()
     : system_coordination_unit_id_(CoordinationUnitType::kSystem,
                                    CoordinationUnitID::RANDOM_ID) {}
 
-CoordinationUnitGraph::~CoordinationUnitGraph() = default;
+CoordinationUnitGraph::~CoordinationUnitGraph() {
+  // Because the graph has ownership of the CUs, and because the process CUs
+  // unregister on destruction, there is reentrancy to this class on
+  // destruction. The order of operations here is optimized to minimize the work
+  // done on destruction, as well as to make sure the cleanup is independent of
+  // the declaration order of member variables.
+
+  // Kill all the observers first.
+  observers_.clear();
+  // Then clear up the CUs to ensure this happens before the PID map is
+  // destructed.
+  coordination_units_.clear();
+
+  DCHECK_EQ(0u, processes_by_pid_.size());
+}
 
 void CoordinationUnitGraph::OnStart(
     service_manager::BinderRegistryWithArgs<
@@ -104,6 +118,15 @@ CoordinationUnitBase* CoordinationUnitGraph::GetCoordinationUnitByID(
   return it->second.get();
 }
 
+ProcessCoordinationUnitImpl*
+CoordinationUnitGraph::GetProcessCoordinationUnitByPid(base::ProcessId pid) {
+  auto it = processes_by_pid_.find(pid);
+  if (it == processes_by_pid_.end())
+    return nullptr;
+
+  return ProcessCoordinationUnitImpl::FromCoordinationUnitBase(it->second);
+}
+
 std::vector<CoordinationUnitBase*>
 CoordinationUnitGraph::GetCoordinationUnitsOfType(CoordinationUnitType type) {
   std::vector<CoordinationUnitBase*> results;
@@ -143,4 +166,18 @@ void CoordinationUnitGraph::DestroyCoordinationUnit(CoordinationUnitBase* cu) {
   DCHECK_EQ(1u, erased);
 }
 
+void CoordinationUnitGraph::BeforeProcessPidChange(
+    ProcessCoordinationUnitImpl* process,
+    base::ProcessId new_pid) {
+  if (process->process_id() != base::kNullProcessId) {
+    size_t erased = processes_by_pid_.erase(process->process_id());
+    DCHECK_EQ(1u, erased);
+  }
+  if (new_pid != base::kNullProcessId) {
+    bool inserted =
+        processes_by_pid_.insert(std::make_pair(new_pid, process)).second;
+    DCHECK(inserted);
+  }
+}
+
 }  // namespace resource_coordinator

services/resource_coordinator/coordination_unit/coordination_unit_graph.h

@@ -12,6 +12,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/process/process_handle.h"
 #include "services/metrics/public/cpp/mojo_ukm_recorder.h"
 #include "services/metrics/public/cpp/ukm_recorder.h"
 #include "services/resource_coordinator/public/cpp/coordination_unit_id.h"
@@ -75,6 +76,9 @@ class CoordinationUnitGraph {
       CoordinationUnitType type);
 
   std::vector<ProcessCoordinationUnitImpl*> GetAllProcessCoordinationUnits();
+  // Retrieves the process CU with PID |pid|, if any.
+  ProcessCoordinationUnitImpl* GetProcessCoordinationUnitByPid(
+      base::ProcessId pid);
   CoordinationUnitBase* GetCoordinationUnitByID(const CoordinationUnitID cu_id);
 
   std::vector<std::unique_ptr<CoordinationUnitGraphObserver>>&
@@ -85,6 +89,8 @@ class CoordinationUnitGraph {
  private:
   using CUIDMap = std::unordered_map<CoordinationUnitID,
                                      std::unique_ptr<CoordinationUnitBase>>;
+  using ProcessByPidMap =
+      std::unordered_map<base::ProcessId, ProcessCoordinationUnitImpl*>;
 
   // Lifetime management functions for CoordinationUnitBase.
   friend class CoordinationUnitBase;
@@ -92,8 +98,14 @@ class CoordinationUnitGraph {
       std::unique_ptr<CoordinationUnitBase> new_cu);
   void DestroyCoordinationUnit(CoordinationUnitBase* cu);
 
+  // Process PID map for use by ProcessCoordinationUnitImpl.
+  friend class ProcessCoordinationUnitImpl;
+  void BeforeProcessPidChange(ProcessCoordinationUnitImpl* process,
+                              base::ProcessId new_pid);
+
   CoordinationUnitID system_coordination_unit_id_;
   CUIDMap coordination_units_;
+  ProcessByPidMap processes_by_pid_;
   std::vector<std::unique_ptr<CoordinationUnitGraphObserver>> observers_;
   ukm::UkmRecorder* ukm_recorder_ = nullptr;
   std::unique_ptr<CoordinationUnitProviderImpl> provider_;

services/resource_coordinator/coordination_unit/coordination_unit_graph_unittest.cc

@@ -3,11 +3,31 @@
 // found in the LICENSE file.
 
 #include "services/resource_coordinator/coordination_unit/coordination_unit_graph.h"
+#include "services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h"
 #include "services/resource_coordinator/coordination_unit/system_coordination_unit_impl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace resource_coordinator {
 
+TEST(CoordinationUnitGraphTest, DestructionWhileCUSOutstanding) {
+  std::unique_ptr<CoordinationUnitGraph> graph(new CoordinationUnitGraph());
+
+  for (size_t i = 0; i < 10; ++i) {
+    auto* process = graph->CreateProcessCoordinationUnit(
+        CoordinationUnitID(CoordinationUnitType::kProcess,
+                           CoordinationUnitID::RANDOM_ID),
+        nullptr);
+    EXPECT_NE(nullptr, process);
+
+    process->SetPID(i + 100);
+  }
+
+  EXPECT_NE(nullptr, graph->FindOrCreateSystemCoordinationUnit(nullptr));
+
+  // This should destroy all the CUs without incident.
+  graph.reset();
+}
+
 TEST(CoordinationUnitGraphTest, FindOrCreateSystemCoordinationUnit) {
   CoordinationUnitGraph graph;
 
@@ -24,4 +44,25 @@ TEST(CoordinationUnitGraphTest, FindOrCreateSystemCoordinationUnit) {
   EXPECT_NE(nullptr, system_cu);
 }
 
+TEST(CoordinationUnitGraphTest, GetProcessCoordinationUnitByPid) {
+  CoordinationUnitGraph graph;
+
+  ProcessCoordinationUnitImpl* process = graph.CreateProcessCoordinationUnit(
+      CoordinationUnitID(CoordinationUnitType::kProcess,
+                         CoordinationUnitID::RANDOM_ID),
+      nullptr);
+
+  EXPECT_EQ(base::kNullProcessId, process->process_id());
+
+  static constexpr base::ProcessId kPid = 10;
+
+  EXPECT_EQ(nullptr, graph.GetProcessCoordinationUnitByPid(kPid));
+  process->SetPID(kPid);
+  EXPECT_EQ(process, graph.GetProcessCoordinationUnitByPid(kPid));
+
+  process->Destruct();
+
+  EXPECT_EQ(nullptr, graph.GetProcessCoordinationUnitByPid(12));
+}
+
 }  // namespace resource_coordinator

services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc

@@ -17,6 +17,11 @@ ProcessCoordinationUnitImpl::ProcessCoordinationUnitImpl(
     : CoordinationUnitInterface(id, graph, std::move(service_ref)) {}
 
 ProcessCoordinationUnitImpl::~ProcessCoordinationUnitImpl() {
+  // Make as if we're transitioning to the null PID before we die to clear this
+  // instance from the PID map.
+  if (process_id_ != base::kNullProcessId)
+    graph()->BeforeProcessPidChange(this, base::kNullProcessId);
+
   for (auto* child_frame : frame_coordination_units_)
     child_frame->RemoveProcessCoordinationUnit(this);
 }
@@ -64,6 +69,11 @@ void ProcessCoordinationUnitImpl::SetMainThreadTaskLoadIsLow(
 }
 
 void ProcessCoordinationUnitImpl::SetPID(int64_t pid) {
+  // The PID can only be set once.
+  DCHECK_EQ(process_id_, base::kNullProcessId);
+
+  graph()->BeforeProcessPidChange(this, pid);
+  process_id_ = static_cast<base::ProcessId>(pid);
   SetProperty(mojom::PropertyType::kPID, pid);
 }
 

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/process/process_handle.h"
 #include "base/time/time.h"
 #include "services/resource_coordinator/coordination_unit/coordination_unit_base.h"
 
@@ -51,6 +52,8 @@ class ProcessCoordinationUnitImpl
   std::set<PageCoordinationUnitImpl*> GetAssociatedPageCoordinationUnits()
       const;
 
+  base::ProcessId process_id() const { return process_id_; }
+
  private:
   friend class FrameCoordinationUnitImpl;
 
@@ -65,6 +68,8 @@ class ProcessCoordinationUnitImpl
   base::TimeDelta cumulative_cpu_usage_;
   uint64_t private_footprint_kb_ = 0u;
 
+  base::ProcessId process_id_ = base::kNullProcessId;
+
   std::set<FrameCoordinationUnitImpl*> frame_coordination_units_;
 
   DISALLOW_COPY_AND_ASSIGN(ProcessCoordinationUnitImpl);

services/resource_coordinator/coordination_unit/system_coordination_unit_impl.cc

@@ -8,6 +8,9 @@
 #include "services/resource_coordinator/coordination_unit/page_coordination_unit_impl.h"
 #include "services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h"
 
+#include <algorithm>
+#include <iterator>
+
 #include "base/macros.h"
 #include "base/process/process_handle.h"
 
@@ -27,10 +30,6 @@ void SystemCoordinationUnitImpl::OnProcessCPUUsageReady() {
 
 void SystemCoordinationUnitImpl::DistributeMeasurementBatch(
     mojom::ProcessResourceMeasurementBatchPtr measurement_batch) {
-  // Grab all the processes.
-  std::vector<ProcessCoordinationUnitImpl*> processes =
-      graph_->GetAllProcessCoordinationUnits();
-
   base::TimeDelta time_since_last_measurement;
   if (!last_measurement_end_time_.is_null()) {
     // Use the end of the measurement batch as a proxy for when every
@@ -56,76 +55,86 @@ void SystemCoordinationUnitImpl::DistributeMeasurementBatch(
   // Keep track of the pages updated with CPU cost for the second pass,
   // where their memory usage is updated.
   std::set<PageCoordinationUnitImpl*> pages;
+  std::vector<ProcessCoordinationUnitImpl*> found_processes;
   for (const auto& measurement : measurement_batch->measurements) {
-    for (auto it = processes.begin(); it != processes.end(); ++it) {
-      ProcessCoordinationUnitImpl* process = *it;
-      int64_t process_pid;
-      // TODO(siggi): This seems pretty silly - we're going O(N^2) in processes
-      //     here, and going through a relatively expensive accessor for the
-      //     PID.
-      if (process->GetProperty(mojom::PropertyType::kPID, &process_pid) &&
-          static_cast<base::ProcessId>(process_pid) == measurement->pid) {
-        base::TimeDelta cumulative_cpu_delta =
-            measurement->cpu_usage - process->cumulative_cpu_usage();
-        DCHECK_LE(base::TimeDelta(), cumulative_cpu_delta);
-
-        // Distribute the CPU delta to the pages that own the frames in this
-        // process.
-        std::set<FrameCoordinationUnitImpl*> frames =
-            process->GetFrameCoordinationUnits();
-        if (!frames.empty()) {
-          // To make sure we don't systemically truncate the remainder of the
-          // delta, simply subtract the remainder and "hold it back" from the
-          // measurement. Since our measurement is cumulative, we'll see that
-          // CPU time again in the next measurement.
-          cumulative_cpu_delta -=
-              cumulative_cpu_delta %
-              base::TimeDelta::FromMicroseconds(frames.size());
-
-          for (FrameCoordinationUnitImpl* frame : frames) {
-            PageCoordinationUnitImpl* page = frame->GetPageCoordinationUnit();
-            if (page) {
-              page->set_usage_estimate_time(last_measurement_end_time_);
-              page->set_cumulative_cpu_usage_estimate(
-                  page->cumulative_cpu_usage_estimate() +
-                  cumulative_cpu_delta / frames.size());
-
-              pages.insert(page);
-            }
+    ProcessCoordinationUnitImpl* process =
+        graph()->GetProcessCoordinationUnitByPid(measurement->pid);
+    if (process) {
+      base::TimeDelta cumulative_cpu_delta =
+          measurement->cpu_usage - process->cumulative_cpu_usage();
+      DCHECK_LE(base::TimeDelta(), cumulative_cpu_delta);
+
+      // Distribute the CPU delta to the pages that own the frames in this
+      // process.
+      std::set<FrameCoordinationUnitImpl*> frames =
+          process->GetFrameCoordinationUnits();
+      if (!frames.empty()) {
+        // To make sure we don't systemically truncate the remainder of the
+        // delta, simply subtract the remainder and "hold it back" from the
+        // measurement. Since our measurement is cumulative, we'll see that
+        // CPU time again in the next measurement.
+        cumulative_cpu_delta -=
+            cumulative_cpu_delta %
+            base::TimeDelta::FromMicroseconds(frames.size());
+
+        for (FrameCoordinationUnitImpl* frame : frames) {
+          PageCoordinationUnitImpl* page = frame->GetPageCoordinationUnit();
+          if (page) {
+            page->set_usage_estimate_time(last_measurement_end_time_);
+            page->set_cumulative_cpu_usage_estimate(
+                page->cumulative_cpu_usage_estimate() +
+                cumulative_cpu_delta / frames.size());
+
+            pages.insert(page);
           }
-        } else {
-          // TODO(siggi): The process has zero frames, maybe this is a newly
-          //    started renderer and if so, this might be a good place to
-          //    estimate the process overhead. Alternatively perhaps the first
-          //    measurement for each process, or a lower bound thereof will
-          //    converge to a decent estimate.
-        }
-
-        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 {
-          double cpu_usage = 100.0 * cumulative_cpu_delta.InMicrosecondsF() /
-                             time_since_last_measurement.InMicrosecondsF();
-          process->SetCPUUsage(cpu_usage);
         }
-        process->set_cumulative_cpu_usage(process->cumulative_cpu_usage() +
-                                          cumulative_cpu_delta);
-        process->set_private_footprint_kb(measurement->private_footprint_kb);
+      } else {
+        // TODO(siggi): The process has zero frames, maybe this is a newly
+        //    started renderer and if so, this might be a good place to
+        //    estimate the process overhead. Alternatively perhaps the first
+        //    measurement for each process, or a lower bound thereof will
+        //    converge to a decent estimate.
+      }
 
-        // Remove found processes.
-        processes.erase(it);
-        break;
+      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 {
+        double cpu_usage = 100.0 * cumulative_cpu_delta.InMicrosecondsF() /
+                           time_since_last_measurement.InMicrosecondsF();
+        process->SetCPUUsage(cpu_usage);
       }
+      process->set_cumulative_cpu_usage(process->cumulative_cpu_usage() +
+                                        cumulative_cpu_delta);
+      process->set_private_footprint_kb(measurement->private_footprint_kb);
+
+      // Note the found processes.
+      found_processes.push_back(process);
     }
   }
 
-  // Clear processes we didn't get data for.
-  for (ProcessCoordinationUnitImpl* process : processes) {
-    process->SetCPUUsage(0.0);
-    process->set_private_footprint_kb(0);
+  // Grab all the processes to see if there were any we didn't get data for.
+  std::vector<ProcessCoordinationUnitImpl*> processes =
+      graph_->GetAllProcessCoordinationUnits();
+
+  if (found_processes.size() != processes.size()) {
+    // We didn't find them all, compute the difference and clear the data for
+    // the processes we didn't find.
+    std::sort(processes.begin(), processes.end());
+    std::sort(found_processes.begin(), found_processes.end());
+    std::vector<ProcessCoordinationUnitImpl*> not_found_processes;
+    std::set_difference(
+        processes.begin(), processes.end(), found_processes.begin(),
+        found_processes.end(),
+        std::inserter(not_found_processes, not_found_processes.begin()));
+
+    // Clear processes we didn't get data for.
+    for (ProcessCoordinationUnitImpl* process : not_found_processes) {
+      process->SetCPUUsage(0.0);
+      process->set_private_footprint_kb(0);
+    }
   }
 
   // Iterate through the pages involved to distribute the memory to them.

services/resource_coordinator/observers/ipc_volume_reporter_unittest.cc

@@ -62,7 +62,6 @@ TEST_F(IPCVolumeReporterTest, Basic) {
       base::TimeDelta::FromMilliseconds(1));
   cu_graph.process->SetLaunchTime(base::Time());
   cu_graph.process->SetMainThreadTaskLoadIsLow(true);
-  cu_graph.process->SetPID(1);
 
   reporter_->mock_timer()->Fire();
 

services/resource_coordinator/observers/page_signal_generator_impl.cc

@@ -52,12 +52,12 @@ constexpr base::TimeDelta PageSignalGeneratorImpl::kLoadedAndIdlingTimeout =
 constexpr base::TimeDelta PageSignalGeneratorImpl::kWaitingForIdleTimeout =
     base::TimeDelta::FromMinutes(1);
 
-// Ensure the timeouts make sense relative to each other.
-static_assert(PageSignalGeneratorImpl::kWaitingForIdleTimeout >
-                  PageSignalGeneratorImpl::kLoadedAndIdlingTimeout,
-              "timeouts must be well ordered");
-
-PageSignalGeneratorImpl::PageSignalGeneratorImpl() = default;
+PageSignalGeneratorImpl::PageSignalGeneratorImpl() {
+  // Ensure the timeouts make sense relative to each other.
+  static_assert(PageSignalGeneratorImpl::kWaitingForIdleTimeout >
+                    PageSignalGeneratorImpl::kLoadedAndIdlingTimeout,
+                "timeouts must be well ordered");
+}
 
 PageSignalGeneratorImpl::~PageSignalGeneratorImpl() = default;
 

services/resource_coordinator/observers/page_signal_generator_impl.h

@@ -28,18 +28,6 @@ namespace resource_coordinator {
 class PageSignalGeneratorImpl : public CoordinationUnitGraphObserver,
                                 public mojom::PageSignalGenerator {
  public:
-  // The amount of time a page has to be idle post-loading in order for it to be
-  // considered loaded and idle. This is used in UpdateLoadIdleState
-  // transitions.
-  static const base::TimeDelta kLoadedAndIdlingTimeout;
-
-  // The maximum amount of time post-DidStopLoading a page can be waiting for
-  // an idle state to occur before the page is simply considered loaded anyways.
-  // Since PageAlmostIdle is intended as an "initial loading complete" signal,
-  // it needs to eventually terminate. This is strictly greater than the
-  // kLoadedAndIdlingTimeout.
-  static const base::TimeDelta kWaitingForIdleTimeout;
-
   PageSignalGeneratorImpl();
   ~PageSignalGeneratorImpl() override;
 
@@ -74,6 +62,18 @@ class PageSignalGeneratorImpl : public CoordinationUnitGraphObserver,
       const service_manager::BindSourceInfo& source_info);
 
  private:
+  // The amount of time a page has to be idle post-loading in order for it to be
+  // considered loaded and idle. This is used in UpdateLoadIdleState
+  // transitions.
+  static const base::TimeDelta kLoadedAndIdlingTimeout;
+
+  // The maximum amount of time post-DidStopLoading a page can be waiting for
+  // an idle state to occur before the page is simply considered loaded anyways.
+  // Since PageAlmostIdle is intended as an "initial loading complete" signal,
+  // it needs to eventually terminate. This is strictly greater than the
+  // kLoadedAndIdlingTimeout.
+  static const base::TimeDelta kWaitingForIdleTimeout;
+
   friend class PageSignalGeneratorImplTest;
   FRIEND_TEST_ALL_PREFIXES(PageSignalGeneratorImplTest, IsLoading);
   FRIEND_TEST_ALL_PREFIXES(PageSignalGeneratorImplTest, IsIdling);