[PM] Use the SystemNode observer to notify of new process metrics data

Bug: 998546
Change-Id: Ibad528d47add2430955af8142aa1372fee5d0648
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1774792Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692048}

chrome/browser/performance_manager/decorators/process_metrics_decorator.cc

@@ -7,45 +7,15 @@
 #include "chrome/browser/performance_manager/graph/graph_impl.h"
 #include "chrome/browser/performance_manager/graph/node_attached_data_impl.h"
 #include "chrome/browser/performance_manager/graph/process_node_impl.h"
+#include "chrome/browser/performance_manager/graph/system_node_impl.h"
 #include "services/resource_coordinator/public/cpp/memory_instrumentation/global_memory_dump.h"
 
 namespace performance_manager {
 
-// Provides ProcessMetricsDecorator machinery access to some internals of a
-// ProcessNodeImpl.
-class ProcessMetricsDecoratorAccess {
- public:
-  static std::unique_ptr<NodeAttachedData>* GetUniquePtrStorage(
-      ProcessNodeImpl* process_node) {
-    return &process_node->process_metrics_data_;
-  }
-};
 
 namespace {
-
 // The process metrics refresh interval.
 constexpr base::TimeDelta kRefreshTimerPeriod = base::TimeDelta::FromMinutes(2);
-
-// Private implementation of the node attached data. This keeps the complexity
-// out of the header file.
-class ProcessMetricsDataImpl
-    : public ProcessMetricsDecorator::Data,
-      public NodeAttachedDataImpl<ProcessMetricsDataImpl> {
- public:
-  struct Traits : public NodeAttachedDataOwnedByNodeType<ProcessNodeImpl> {};
-
-  explicit ProcessMetricsDataImpl(const ProcessNodeImpl* process_node) {}
-  ~ProcessMetricsDataImpl() override = default;
-
-  static std::unique_ptr<NodeAttachedData>* GetUniquePtrStorage(
-      ProcessNodeImpl* process_node) {
-    return ProcessMetricsDecoratorAccess::GetUniquePtrStorage(process_node);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ProcessMetricsDataImpl);
-};
-
 }  // namespace
 
 ProcessMetricsDecorator::ProcessMetricsDecorator() = default;
@@ -93,25 +63,27 @@ void ProcessMetricsDecorator::DidGetMemoryUsage(
     return;
 
   auto* graph_impl = GraphImpl::FromGraph(graph_);
+
+  // Refresh the process nodes with the data contained in |process_dumps|.
+  // Processes for which we don't receive any data will retain the previously
+  // set value.
+  // TODO(sebmarchand): Check if we should set the data to 0 instead, or add a
+  // timestamp to the data.
   for (const auto& process_dump_iter : process_dumps->process_dumps()) {
     // Check if there's a process node associated with this PID.
     auto* node = graph_impl->GetProcessNodeByPid(process_dump_iter.pid());
     if (!node)
       continue;
 
-    auto* data = ProcessMetricsDataImpl::GetOrCreate(node);
-    data->private_footprint_kb_ =
-        process_dump_iter.os_dump().private_footprint_kb;
-    data->resident_set_kb_ = process_dump_iter.os_dump().resident_set_kb;
+    node->set_private_footprint_kb(
+        process_dump_iter.os_dump().private_footprint_kb);
+    node->set_resident_set_kb(process_dump_iter.os_dump().resident_set_kb);
   }
 
+  GraphImpl::FromGraph(graph_)
+      ->FindOrCreateSystemNodeImpl()
+      ->OnProcessMemoryMetricsAvailable();
   refresh_timer_.Reset();
 }
 
-// static
-ProcessMetricsDecorator::Data* ProcessMetricsDecorator::Data::GetForTesting(
-    ProcessNode* process_node) {
-  return ProcessMetricsDataImpl::Get(ProcessNodeImpl::FromNode(process_node));
-}
-
 }  // namespace performance_manager

chrome/browser/performance_manager/decorators/process_metrics_decorator_unittest.cc

@@ -60,6 +60,17 @@ void LenientTestProcessMetricsDecorator::RequestProcessesMemoryMetrics(
                 memory_instrumentation::mojom::GlobalMemoryDump::New()));
 }
 
+class LenientMockSystemNodeObserver
+    : public SystemNodeImpl::ObserverDefaultImpl {
+ public:
+  LenientMockSystemNodeObserver() {}
+  ~LenientMockSystemNodeObserver() override {}
+
+  MOCK_METHOD1(OnProcessMemoryMetricsAvailable, void(const SystemNode*));
+};
+using MockSystemNodeObserver =
+    ::testing::StrictMock<LenientMockSystemNodeObserver>;
+
 struct MemoryDumpProcInfo {
   base::ProcessId pid;
   uint32_t resident_set_kb;
@@ -125,6 +136,9 @@ class ProcessMetricsDecoratorTest : public GraphTestHarness {
 };
 
 TEST_F(ProcessMetricsDecoratorTest, RefreshTimer) {
+  MockSystemNodeObserver sys_node_observer;
+
+  graph()->AddSystemNodeObserver(&sys_node_observer);
   auto memory_dump = base::make_optional(std::move(
       GenerateMemoryDump({{mock_graph()->process->process_id(),
                            kFakeResidentSetKb, kFakePrivateFootprintKb},
@@ -135,27 +149,23 @@ TEST_F(ProcessMetricsDecoratorTest, RefreshTimer) {
       .WillOnce(testing::Return(testing::ByMove(std::move(memory_dump))));
 
   // There's no data available initially.
-  auto* data =
-      ProcessMetricsDecorator::Data::GetForTesting(mock_graph()->process.get());
-  EXPECT_FALSE(data);
-  data = ProcessMetricsDecorator::Data::GetForTesting(
-      mock_graph()->other_process.get());
-  EXPECT_FALSE(data);
+  EXPECT_EQ(0U, mock_graph()->process->resident_set_kb());
+  EXPECT_EQ(0U, mock_graph()->process->private_footprint_kb());
+
+  EXPECT_CALL(sys_node_observer, OnProcessMemoryMetricsAvailable(testing::_));
 
   // Advance the timer, this should trigger a refresh of the metrics.
   task_env().FastForwardBy(base::TimeDelta::FromMinutes(2));
 
-  data =
-      ProcessMetricsDecorator::Data::GetForTesting(mock_graph()->process.get());
-  EXPECT_TRUE(data);
-  EXPECT_EQ(kFakeResidentSetKb, data->resident_set_kb_);
-  EXPECT_EQ(kFakePrivateFootprintKb, data->private_footprint_kb_);
-
-  data = ProcessMetricsDecorator::Data::GetForTesting(
-      mock_graph()->other_process.get());
-  EXPECT_TRUE(data);
-  EXPECT_EQ(kFakeResidentSetKb, data->resident_set_kb_);
-  EXPECT_EQ(kFakePrivateFootprintKb, data->private_footprint_kb_);
+  EXPECT_EQ(kFakeResidentSetKb, mock_graph()->process->resident_set_kb());
+  EXPECT_EQ(kFakePrivateFootprintKb,
+            mock_graph()->process->private_footprint_kb());
+
+  EXPECT_EQ(kFakeResidentSetKb, mock_graph()->other_process->resident_set_kb());
+  EXPECT_EQ(kFakePrivateFootprintKb,
+            mock_graph()->other_process->private_footprint_kb());
+
+  graph()->RemoveSystemNodeObserver(&sys_node_observer);
 }
 
 TEST_F(ProcessMetricsDecoratorTest, PartialRefresh) {
@@ -170,15 +180,9 @@ TEST_F(ProcessMetricsDecoratorTest, PartialRefresh) {
 
   task_env().FastForwardBy(base::TimeDelta::FromMinutes(2));
 
-  auto* data =
-      ProcessMetricsDecorator::Data::GetForTesting(mock_graph()->process.get());
-  EXPECT_TRUE(data);
-  EXPECT_EQ(kFakeResidentSetKb, data->resident_set_kb_);
-  EXPECT_EQ(kFakePrivateFootprintKb, data->private_footprint_kb_);
-
-  data = ProcessMetricsDecorator::Data::GetForTesting(
-      mock_graph()->other_process.get());
-  EXPECT_FALSE(data);
+  EXPECT_EQ(kFakeResidentSetKb, mock_graph()->process->resident_set_kb());
+  EXPECT_EQ(kFakePrivateFootprintKb,
+            mock_graph()->process->private_footprint_kb());
 
   // Do another partial refresh but this time for the other process. The data
   // attached to |mock_graph()->process| shouldn't change.
@@ -191,17 +195,14 @@ TEST_F(ProcessMetricsDecoratorTest, PartialRefresh) {
 
   task_env().FastForwardBy(base::TimeDelta::FromMinutes(2));
 
-  data =
-      ProcessMetricsDecorator::Data::GetForTesting(mock_graph()->process.get());
-  EXPECT_TRUE(data);
-  EXPECT_EQ(kFakeResidentSetKb, data->resident_set_kb_);
-  EXPECT_EQ(kFakePrivateFootprintKb, data->private_footprint_kb_);
-
-  data = ProcessMetricsDecorator::Data::GetForTesting(
-      mock_graph()->other_process.get());
-  EXPECT_TRUE(data);
-  EXPECT_EQ(kFakeResidentSetKb * 2, data->resident_set_kb_);
-  EXPECT_EQ(kFakePrivateFootprintKb * 2, data->private_footprint_kb_);
+  EXPECT_EQ(kFakeResidentSetKb, mock_graph()->process->resident_set_kb());
+  EXPECT_EQ(kFakePrivateFootprintKb,
+            mock_graph()->process->private_footprint_kb());
+
+  EXPECT_EQ(kFakeResidentSetKb * 2,
+            mock_graph()->other_process->resident_set_kb());
+  EXPECT_EQ(kFakePrivateFootprintKb * 2,
+            mock_graph()->other_process->private_footprint_kb());
 }
 
 TEST_F(ProcessMetricsDecoratorTest, RefreshFailure) {
@@ -210,9 +211,8 @@ TEST_F(ProcessMetricsDecoratorTest, RefreshFailure) {
 
   task_env().FastForwardBy(base::TimeDelta::FromMinutes(2));
 
-  auto* data =
-      ProcessMetricsDecorator::Data::GetForTesting(mock_graph()->process.get());
-  EXPECT_FALSE(data);
+  EXPECT_EQ(0U, mock_graph()->process->resident_set_kb());
+  EXPECT_EQ(0U, mock_graph()->process->private_footprint_kb());
 }
 
 }  // namespace performance_manager

chrome/browser/performance_manager/graph/process_node_impl.cc

@@ -126,6 +126,7 @@ void ProcessNodeImpl::SetProcessImpl(base::Process process,
   // Also clear the measurement data (if any), as it references the previous
   // process.
   private_footprint_kb_ = 0;
+  resident_set_kb_ = 0;
   cumulative_cpu_usage_ = base::TimeDelta();
 
   process_id_ = new_pid;
@@ -200,6 +201,11 @@ uint64_t ProcessNodeImpl::GetPrivateFootprintKb() const {
   return private_footprint_kb();
 }
 
+uint64_t ProcessNodeImpl::GetResidentSetKb() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return resident_set_kb();
+}
+
 const RenderProcessHostProxy& ProcessNodeImpl::GetRenderProcessHostProxy()
     const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

chrome/browser/performance_manager/graph/process_node_impl.h

@@ -31,7 +31,7 @@ class WorkerNodeImpl;
 // 1. Created, no PID.
 // 2. Process started, have PID - in the case where the associated render
 //    process fails to start, this state may not occur.
-// 3. Process died or falied to start, have exit status.
+// 3. Process died or failed to start, have exit status.
 // 4. Back to 2.
 class ProcessNodeImpl
     : public PublicNodeImpl<ProcessNodeImpl, ProcessNode>,
@@ -68,6 +68,10 @@ class ProcessNodeImpl
   void set_cumulative_cpu_usage(base::TimeDelta cumulative_cpu_usage) {
     cumulative_cpu_usage_ = cumulative_cpu_usage;
   }
+  uint64_t resident_set_kb() const { return resident_set_kb_; }
+  void set_resident_set_kb(uint64_t resident_set_kb) {
+    resident_set_kb_ = resident_set_kb;
+  }
   base::TimeDelta cumulative_cpu_usage() const { return cumulative_cpu_usage_; }
 
   const base::flat_set<FrameNodeImpl*>& frame_nodes() const;
@@ -134,6 +138,7 @@ class ProcessNodeImpl
   double GetCpuUsage() const override;
   base::TimeDelta GetCumulativeCpuUsage() const override;
   uint64_t GetPrivateFootprintKb() const override;
+  uint64_t GetResidentSetKb() const override;
   const RenderProcessHostProxy& GetRenderProcessHostProxy() const override;
 
   void OnAllFramesInProcessFrozen();
@@ -144,6 +149,7 @@ class ProcessNodeImpl
 
   base::TimeDelta cumulative_cpu_usage_;
   uint64_t private_footprint_kb_ = 0u;
+  uint64_t resident_set_kb_ = 0;
 
   base::ProcessId process_id_ = base::kNullProcessId;
   ObservedProperty::NotifiesAlways<
@@ -173,8 +179,6 @@ class ProcessNodeImpl
   // Inline storage for FrozenFrameAggregator user data.
   InternalNodeAttachedDataStorage<sizeof(uintptr_t) + 8> frozen_frame_data_;
 
-  std::unique_ptr<NodeAttachedData> process_metrics_data_;
-
   DISALLOW_COPY_AND_ASSIGN(ProcessNodeImpl);
 };
 

chrome/browser/performance_manager/graph/process_node_impl_unittest.cc

@@ -67,10 +67,12 @@ TEST_F(ProcessNodeImplTest, ProcessLifeCycle) {
   EXPECT_FALSE(process_node->exit_status());
 
   EXPECT_EQ(0U, process_node->private_footprint_kb());
+  EXPECT_EQ(0U, process_node->resident_set_kb());
   EXPECT_EQ(base::TimeDelta(), process_node->cumulative_cpu_usage());
 
   constexpr base::TimeDelta kCpuUsage = base::TimeDelta::FromMicroseconds(1);
   process_node->set_private_footprint_kb(10u);
+  process_node->set_resident_set_kb(20u);
   process_node->set_cumulative_cpu_usage(kCpuUsage);
 
   // Kill it again.
@@ -81,6 +83,7 @@ TEST_F(ProcessNodeImplTest, ProcessLifeCycle) {
 
   EXPECT_EQ(launch_time, process_node->launch_time());
   EXPECT_EQ(10u, process_node->private_footprint_kb());
+  EXPECT_EQ(20u, process_node->resident_set_kb());
   EXPECT_EQ(kCpuUsage, process_node->cumulative_cpu_usage());
 
   // Resurrect again and verify the launch time and measurements
@@ -90,6 +93,7 @@ TEST_F(ProcessNodeImplTest, ProcessLifeCycle) {
 
   EXPECT_EQ(launch2_time, process_node->launch_time());
   EXPECT_EQ(0U, process_node->private_footprint_kb());
+  EXPECT_EQ(0U, process_node->resident_set_kb());
   EXPECT_EQ(base::TimeDelta(), process_node->cumulative_cpu_usage());
 }
 
@@ -254,6 +258,10 @@ TEST_F(ProcessNodeImplTest, PublicInterface) {
   process_node->set_private_footprint_kb(628);
   EXPECT_EQ(process_node->private_footprint_kb(),
             public_process_node->GetPrivateFootprintKb());
+
+  process_node->set_resident_set_kb(398);
+  EXPECT_EQ(process_node->resident_set_kb(),
+            public_process_node->GetResidentSetKb());
 }
 
 }  // namespace performance_manager

chrome/browser/performance_manager/graph/system_node_impl.cc

@@ -24,4 +24,10 @@ SystemNodeImpl::~SystemNodeImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
+void SystemNodeImpl::OnProcessMemoryMetricsAvailable() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  for (auto* observer : GetObservers())
+    observer->OnProcessMemoryMetricsAvailable(this);
+}
+
 }  // namespace performance_manager

chrome/browser/performance_manager/graph/system_node_impl.h

@@ -27,6 +27,10 @@ class SystemNodeImpl : public PublicNodeImpl<SystemNodeImpl, SystemNode>,
   explicit SystemNodeImpl(GraphImpl* graph);
   ~SystemNodeImpl() override;
 
+  // This should be called after refreshing the memory usage data of the process
+  // nodes.
+  void OnProcessMemoryMetricsAvailable();
+
  private:
   DISALLOW_COPY_AND_ASSIGN(SystemNodeImpl);
 };

chrome/browser/performance_manager/graph/system_node_impl_unittest.cc

@@ -78,6 +78,7 @@ class LenientMockObserver : public SystemNodeImpl::Observer {
 
   MOCK_METHOD1(OnSystemNodeAdded, void(const SystemNode*));
   MOCK_METHOD1(OnBeforeSystemNodeRemoved, void(const SystemNode*));
+  MOCK_METHOD1(OnProcessMemoryMetricsAvailable, void(const SystemNode*));
 
   void SetNotifiedSystemNode(const SystemNode* system_node) {
     notified_system_node_ = system_node;
@@ -110,7 +111,10 @@ TEST_F(SystemNodeImplTest, ObserverWorks) {
   const SystemNode* system_node = graph()->FindOrCreateSystemNode();
   EXPECT_EQ(system_node, obs.TakeNotifiedSystemNode());
 
-  // "OnProcessCPUUsageReady" is tested explicitly in the above unittests.
+  EXPECT_CALL(obs, OnProcessMemoryMetricsAvailable(_))
+      .WillOnce(Invoke(&obs, &MockObserver::SetNotifiedSystemNode));
+  SystemNodeImpl::FromNode(system_node)->OnProcessMemoryMetricsAvailable();
+  EXPECT_EQ(system_node, obs.TakeNotifiedSystemNode());
 
   // Release the system node and expect a call to "OnBeforeSystemNodeRemoved".
   EXPECT_CALL(obs, OnBeforeSystemNodeRemoved(_))

chrome/browser/performance_manager/public/graph/process_node.h

@@ -89,10 +89,15 @@ class ProcessNode : public Node {
   // lifetime, expressed as CPU seconds.
   virtual base::TimeDelta GetCumulativeCpuUsage() const = 0;
 
-  // Returns the most recently measured private memory footprint of the render
-  // process, in kilobytes.
+  // Returns the most recently measured private memory footprint of the process.
+  // This is roughly private, anonymous, non-discardable, resident or swapped
+  // memory in kilobytes. For more details, see https://goo.gl/3kPb9S.
   virtual uint64_t GetPrivateFootprintKb() const = 0;
 
+  // Returns the most recently measured resident set of the process, in
+  // kilobytes.
+  virtual uint64_t GetResidentSetKb() const = 0;
+
   // Returns a proxy to the RenderProcessHost associated with this node. The
   // proxy may only be dereferenced on the UI thread.
   virtual const RenderProcessHostProxy& GetRenderProcessHostProxy() const = 0;

chrome/browser/performance_manager/public/graph/system_node.h

@@ -41,6 +41,10 @@ class SystemNodeObserver {
   // Called before the |system_node| is removed from the graph.
   virtual void OnBeforeSystemNodeRemoved(const SystemNode* system_node) = 0;
 
+  // Called when a new set of process memory metrics is available.
+  virtual void OnProcessMemoryMetricsAvailable(
+      const SystemNode* system_node) = 0;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(SystemNodeObserver);
 };
@@ -56,6 +60,8 @@ class SystemNode::ObserverDefaultImpl : public SystemNodeObserver {
   // SystemNodeObserver implementation:
   void OnSystemNodeAdded(const SystemNode* system_node) override {}
   void OnBeforeSystemNodeRemoved(const SystemNode* system_node) override {}
+  void OnProcessMemoryMetricsAvailable(const SystemNode* system_node) override {
+  }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ObserverDefaultImpl);