[PM] Always create the SystemNode singleton.

The SystemNode singleton is currently created on demand by the first
call to "Graph::FindOrCreateSystemNode". This is problematic because
some policies might register themselves as SystemNodeObserver without
this node ever being created. This CL causes the SystemNode to be
created by default when setting up the graph.

Change-Id: Ibdd9af2821df87e1c18197ad9f424ec69b5d5a8e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2951241
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#892575}

chrome/browser/performance_manager/decorators/frozen_frame_aggregator.cc

@@ -188,7 +188,7 @@ void FrozenFrameAggregator::RegisterObservers(Graph* graph) {
   // This observer presumes that it's been added before any nodes exist in the
   // graph.
   // TODO(chrisha): Add graph introspection functions to Graph.
-  DCHECK(GraphImpl::FromGraph(graph)->nodes().empty());
+  DCHECK(graph->HasOnlySystemNode());
   graph->AddFrameNodeObserver(this);
   graph->AddPageNodeObserver(this);
   graph->AddProcessNodeObserver(this);

chrome/browser/performance_manager/policies/high_pmf_discard_policy_unittest.cc

@@ -61,7 +61,7 @@ TEST_F(HighPMFDiscardPolicyTest, EndToEnd) {
   policy()->set_pmf_limit_for_testing(kPMFLimitKb);
 
   process_node()->set_private_footprint_kb(kPMFLimitKb - 1);
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
   // Make sure that no task get posted to the discarder.
   task_env().RunUntilIdle();
   ::testing::Mock::VerifyAndClearExpectations(discarder());
@@ -71,15 +71,15 @@ TEST_F(HighPMFDiscardPolicyTest, EndToEnd) {
   process_node()->set_private_footprint_kb(kPMFLimitKb);
   EXPECT_CALL(*discarder(), DiscardPageNodeImpl(page_node()))
       .WillOnce(::testing::Return(true));
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
   task_env().RunUntilIdle();
   ::testing::Mock::VerifyAndClearExpectations(discarder());
 
   // Lower the PMF of process_node, this is equivalent to discarding a page and
   // lowering the total PMF.
   process_node()->set_private_footprint_kb(kPMFLimitKb - 1);
   // Call OnProcessMemoryMetricsAvailable to record the post discard metrics.
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
   histogram_tester()->ExpectUniqueSample(
       "Discarding.HighPMFPolicy.DiscardAttemptsCount", 1, 1);
   histogram_tester()->ExpectUniqueSample(
@@ -101,7 +101,7 @@ TEST_F(HighPMFDiscardPolicyTest, Histograms) {
   // Pretend that the discard attempt has failed.
   EXPECT_CALL(*discarder(), DiscardPageNodeImpl(page_node()))
       .WillOnce(::testing::Return(false));
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
   ::testing::Mock::VerifyAndClearExpectations(discarder());
 
   // There's been one unsuccessful discard attempt so far. The total PMF is
@@ -110,7 +110,7 @@ TEST_F(HighPMFDiscardPolicyTest, Histograms) {
   EXPECT_CALL(*discarder(), DiscardPageNodeImpl(page_node()))
       .WillOnce(::testing::Return(true));
   PageDiscardingHelper::RemovesDiscardAttemptMarkerForTesting(page_node());
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
   histogram_tester()->ExpectTotalCount(
       "Discarding.HighPMFPolicy.DiscardAttemptsCount", 0);
   histogram_tester()->ExpectTotalCount(
@@ -128,7 +128,7 @@ TEST_F(HighPMFDiscardPolicyTest, Histograms) {
   EXPECT_CALL(*discarder(), DiscardPageNodeImpl(page_node()))
       .WillOnce(::testing::Return(true));
   PageDiscardingHelper::RemovesDiscardAttemptMarkerForTesting(page_node());
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
   histogram_tester()->ExpectTotalCount(
       "Discarding.HighPMFPolicy.DiscardAttemptsCount", 0);
   histogram_tester()->ExpectTotalCount(
@@ -141,7 +141,7 @@ TEST_F(HighPMFDiscardPolicyTest, Histograms) {
       "Discarding.HighPMFPolicy.DiscardSuccess", true, 1);
 
   process_node()->set_private_footprint_kb(kPMFLimitKb - 1);
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
 
   histogram_tester()->ExpectUniqueSample(
       "Discarding.HighPMFPolicy.DiscardAttemptsCount", 3, 1);
@@ -161,12 +161,12 @@ TEST_F(HighPMFDiscardPolicyTest, NegativeMemoryReclaimGoesInUnderflowBucket) {
 
   EXPECT_CALL(*discarder(), DiscardPageNodeImpl(page_node()))
       .WillOnce(::testing::Return(true));
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
   ::testing::Mock::VerifyAndClearExpectations(discarder());
   process_node()->set_private_footprint_kb(
       process_node()->private_footprint_kb() + 1);
 
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
 
   // The total PMF has increased, the memory reclaimed should be reported in the
   // underflow bucket.
@@ -183,7 +183,7 @@ TEST_F(HighPMFDiscardPolicyTest, MemoryPressureHistograms) {
 
   EXPECT_CALL(*discarder(), DiscardPageNodeImpl(page_node()))
       .WillOnce(::testing::Return(false));
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
   task_env().RunUntilIdle();
   ::testing::Mock::VerifyAndClearExpectations(discarder());
 
@@ -195,7 +195,7 @@ TEST_F(HighPMFDiscardPolicyTest, MemoryPressureHistograms) {
   // Test with MEMORY_PRESSURE_LEVEL_MODERATE.
   memory_pressure_monitor().SetAndNotifyMemoryPressure(
       base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE);
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
   task_env().RunUntilIdle();
 
   histogram_tester()->ExpectBucketCount(
@@ -206,7 +206,7 @@ TEST_F(HighPMFDiscardPolicyTest, MemoryPressureHistograms) {
   // Test with MEMORY_PRESSURE_LEVEL_CRITICAL.
   memory_pressure_monitor().SetAndNotifyMemoryPressure(
       base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL);
-  graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
+  graph()->GetSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
   task_env().RunUntilIdle();
 
   histogram_tester()->ExpectBucketCount(

chrome/browser/performance_manager/test_support/page_discarding_utils.cc

@@ -27,8 +27,7 @@ void LenientMockPageDiscarder::DiscardPageNode(
   std::move(post_discard_cb).Run(DiscardPageNodeImpl(page_node));
 }
 
-GraphTestHarnessWithMockDiscarder::GraphTestHarnessWithMockDiscarder()
-    : system_node_(TestNodeWrapper<SystemNodeImpl>::Create(graph())) {
+GraphTestHarnessWithMockDiscarder::GraphTestHarnessWithMockDiscarder() {
   // Some tests depends on the existence of the PageAggregator.
   graph()->PassToGraph(std::make_unique<PageAggregator>());
 }

chrome/browser/performance_manager/test_support/page_discarding_utils.h

@@ -55,7 +55,7 @@ class GraphTestHarnessWithMockDiscarder : public GraphTestHarness {
   PageNodeImpl* page_node() { return page_node_.get(); }
   ProcessNodeImpl* process_node() { return process_node_.get(); }
   FrameNodeImpl* frame_node() { return main_frame_node_.get(); }
-  SystemNodeImpl* system_node() { return system_node_.get(); }
+  SystemNodeImpl* system_node() { return graph()->GetSystemNodeImpl(); }
   void ResetFrameNode() { main_frame_node_.reset(); }
   testing::MockPageDiscarder* discarder() { return mock_discarder_; }
 
@@ -67,8 +67,6 @@ class GraphTestHarnessWithMockDiscarder : public GraphTestHarness {
       process_node_;
   performance_manager::TestNodeWrapper<performance_manager::FrameNodeImpl>
       main_frame_node_;
-  performance_manager::TestNodeWrapper<performance_manager::SystemNodeImpl>
-      system_node_;
 };
 
 // Make sure that |page_node| is discardable.

components/performance_manager/decorators/frame_visibility_decorator.cc

@@ -51,7 +51,7 @@ FrameVisibilityDecorator::FrameVisibilityDecorator() = default;
 FrameVisibilityDecorator::~FrameVisibilityDecorator() = default;
 
 void FrameVisibilityDecorator::OnPassedToGraph(Graph* graph) {
-  DCHECK(graph->IsEmpty());
+  DCHECK(graph->HasOnlySystemNode());
   graph->AddPageNodeObserver(this);
   graph->AddFrameNodeObserver(this);
 }

components/performance_manager/decorators/page_load_tracker_decorator.cc

@@ -169,7 +169,7 @@ void PageLoadTrackerDecorator::RegisterObservers(Graph* graph) {
   // This observer presumes that it's been added before any nodes exist in the
   // graph.
   // TODO(chrisha): Add graph introspection functions to Graph.
-  DCHECK(GraphImpl::FromGraph(graph)->nodes().empty());
+  DCHECK(graph->HasOnlySystemNode());
   graph->AddFrameNodeObserver(this);
   graph->AddProcessNodeObserver(this);
 }

components/performance_manager/decorators/process_metrics_decorator.cc

@@ -178,7 +178,7 @@ void ProcessMetricsDecorator::DidGetMemoryUsage(
   }
 
   GraphImpl::FromGraph(graph_)
-      ->FindOrCreateSystemNodeImpl()
+      ->GetSystemNodeImpl()
       ->OnProcessMemoryMetricsAvailable();
   refresh_timer_.Reset();
 }

components/performance_manager/execution_context/execution_context_registry_impl.cc

@@ -175,7 +175,7 @@ ExecutionContextRegistryImpl::GetExecutionContextForWorkerNodeImpl(
 
 void ExecutionContextRegistryImpl::OnPassedToGraph(Graph* graph) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(graph->IsEmpty());
+  DCHECK(graph->HasOnlySystemNode());
   graph->RegisterObject(this);
   graph->AddFrameNodeObserver(this);
   graph->AddWorkerNodeObserver(this);

components/performance_manager/graph/graph_impl.cc

@@ -182,6 +182,11 @@ GraphImpl::~GraphImpl() {
   DCHECK(nodes_.empty());
 }
 
+void GraphImpl::SetUp() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  CreateSystemNode();
+}
+
 void GraphImpl::TearDown() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
@@ -286,9 +291,10 @@ void GraphImpl::UnregisterObject(GraphRegistered* object) {
   registered_objects_.UnregisterObject(object);
 }
 
-const SystemNode* GraphImpl::FindOrCreateSystemNode() {
+const SystemNode* GraphImpl::GetSystemNode() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return FindOrCreateSystemNodeImpl();
+  DCHECK(system_node_.get());
+  return system_node_.get();
 }
 
 std::vector<const ProcessNode*> GraphImpl::GetAllProcessNodes() const {
@@ -307,9 +313,9 @@ std::vector<const WorkerNode*> GraphImpl::GetAllWorkerNodes() const {
   return GetAllNodesOfType<WorkerNodeImpl, const WorkerNode*>();
 }
 
-bool GraphImpl::IsEmpty() const {
+bool GraphImpl::HasOnlySystemNode() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return nodes_.empty();
+  return nodes_.size() == 1 && *nodes_.begin() == GetSystemNodeImpl();
 }
 
 ukm::UkmRecorder* GraphImpl::GetUkmRecorder() const {
@@ -350,18 +356,6 @@ GraphImpl* GraphImpl::FromGraph(const Graph* graph) {
   return reinterpret_cast<GraphImpl*>(const_cast<void*>(graph->GetImpl()));
 }
 
-SystemNodeImpl* GraphImpl::FindOrCreateSystemNodeImpl() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!system_node_) {
-    // Create the singleton system node instance. Ownership is taken by the
-    // graph.
-    system_node_ = std::make_unique<SystemNodeImpl>();
-    AddNewNode(system_node_.get());
-  }
-
-  return system_node_.get();
-}
-
 bool GraphImpl::NodeInGraph(const NodeBase* node) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   const auto& it = nodes_.find(const_cast<NodeBase*>(node));
@@ -550,11 +544,8 @@ void GraphImpl::DispatchNodeAddedNotifications(NodeBase* node) {
       for (auto* observer : process_node_observers_)
         observer->OnProcessNodeAdded(process_node);
     } break;
-    case NodeTypeEnum::kSystem: {
-      auto* system_node = SystemNodeImpl::FromNodeBase(node);
-      for (auto* observer : system_node_observers_)
-        observer->OnSystemNodeAdded(system_node);
-    } break;
+    case NodeTypeEnum::kSystem:
+      break;
     case NodeTypeEnum::kWorker: {
       auto* worker_node = WorkerNodeImpl::FromNodeBase(node);
       for (auto* observer : worker_node_observers_)
@@ -585,11 +576,8 @@ void GraphImpl::DispatchNodeRemovedNotifications(NodeBase* node) {
       for (auto* observer : process_node_observers_)
         observer->OnBeforeProcessNodeRemoved(process_node);
     } break;
-    case NodeTypeEnum::kSystem: {
-      auto* system_node = SystemNodeImpl::FromNodeBase(node);
-      for (auto* observer : system_node_observers_)
-        observer->OnBeforeSystemNodeRemoved(system_node);
-    } break;
+    case NodeTypeEnum::kSystem:
+      break;
     case NodeTypeEnum::kWorker: {
       auto* worker_node = WorkerNodeImpl::FromNodeBase(node);
       for (auto* observer : worker_node_observers_)
@@ -664,6 +652,14 @@ std::vector<ReturnNodeType> GraphImpl::GetAllNodesOfType() const {
   return ret;
 }
 
+void GraphImpl::CreateSystemNode() {
+  DCHECK(!system_node_);
+  // Create the singleton system node instance. Ownership is taken by the
+  // graph.
+  system_node_ = std::make_unique<SystemNodeImpl>();
+  AddNewNode(system_node_.get());
+}
+
 void GraphImpl::ReleaseSystemNode() {
   if (!system_node_.get())
     return;

components/performance_manager/graph/graph_impl.h

@@ -54,6 +54,9 @@ class GraphImpl : public Graph {
   GraphImpl(const GraphImpl&) = delete;
   GraphImpl& operator=(const GraphImpl&) = delete;
 
+  // Set up the graph.
+  void SetUp();
+
   // Tear down the graph to prepare for deletion.
   void TearDown();
 
@@ -74,12 +77,12 @@ class GraphImpl : public Graph {
   std::unique_ptr<GraphOwned> TakeFromGraph(GraphOwned* graph_owned) override;
   void RegisterObject(GraphRegistered* object) override;
   void UnregisterObject(GraphRegistered* object) override;
-  const SystemNode* FindOrCreateSystemNode() override;
+  const SystemNode* GetSystemNode() const override;
   std::vector<const ProcessNode*> GetAllProcessNodes() const override;
   std::vector<const FrameNode*> GetAllFrameNodes() const override;
   std::vector<const PageNode*> GetAllPageNodes() const override;
   std::vector<const WorkerNode*> GetAllWorkerNodes() const override;
-  bool IsEmpty() const override;
+  bool HasOnlySystemNode() const override;
   ukm::UkmRecorder* GetUkmRecorder() const override;
   NodeDataDescriberRegistry* GetNodeDataDescriberRegistry() const override;
   uintptr_t GetImplType() const override;
@@ -102,7 +105,10 @@ class GraphImpl : public Graph {
     return ukm_recorder_;
   }
 
-  SystemNodeImpl* FindOrCreateSystemNodeImpl();
+  SystemNodeImpl* GetSystemNodeImpl() const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    return system_node_.get();
+  }
   std::vector<ProcessNodeImpl*> GetAllProcessNodeImpls() const;
   std::vector<FrameNodeImpl*> GetAllFrameNodeImpls() const;
   std::vector<PageNodeImpl*> GetAllPageNodeImpls() const;
@@ -202,6 +208,7 @@ class GraphImpl : public Graph {
   template <typename NodeType, typename ReturnNodeType>
   std::vector<ReturnNodeType> GetAllNodesOfType() const;
 
+  void CreateSystemNode() VALID_CONTEXT_REQUIRED(sequence_checker_);
   void ReleaseSystemNode() VALID_CONTEXT_REQUIRED(sequence_checker_);
 
   std::unique_ptr<SystemNodeImpl> system_node_

components/performance_manager/graph/graph_impl_unittest.cc

@@ -26,13 +26,9 @@ TEST_F(GraphImplTest, SafeCasting) {
   EXPECT_EQ(graph(), GraphImpl::FromGraph(graph_base));
 }
 
-TEST_F(GraphImplTest, FindOrCreateSystemNode) {
-  EXPECT_TRUE(graph()->IsEmpty());
-  SystemNodeImpl* system_node = graph()->FindOrCreateSystemNodeImpl();
-  EXPECT_FALSE(graph()->IsEmpty());
-
-  // A second request should return the same instance.
-  EXPECT_EQ(system_node, graph()->FindOrCreateSystemNodeImpl());
+TEST_F(GraphImplTest, GetSystemNodeImpl) {
+  // The SystemNode singleton should be created by default.
+  EXPECT_NE(nullptr, graph()->GetSystemNodeImpl());
 }
 
 TEST_F(GraphImplTest, GetProcessNodeByPid) {
@@ -291,7 +287,7 @@ TEST_F(GraphImplTest, NodeDataDescribers) {
   AssertDictValueContainsListKey(descr, "d1", "d1", "ProcessNode");
   EXPECT_EQ(1u, descr.DictSize());
 
-  descr = registry->DescribeNodeData(graph()->FindOrCreateSystemNodeImpl());
+  descr = registry->DescribeNodeData(graph()->GetSystemNode());
   AssertDictValueContainsListKey(descr, "d1", "d1", "SystemNode");
   EXPECT_EQ(1u, descr.DictSize());
 

components/performance_manager/graph/policies/process_priority_policy.cc

@@ -104,7 +104,7 @@ void ProcessPriorityPolicy::ClearCallbackForTesting() {
 }
 
 void ProcessPriorityPolicy::OnPassedToGraph(Graph* graph) {
-  DCHECK(graph->IsEmpty());
+  DCHECK(graph->HasOnlySystemNode());
   graph->AddProcessNodeObserver(this);
 }