[PM] Support workers in ProcessPriorityAggregator

Bug: 1077217
Change-Id: I57457b30f2f4592c91d92799b7c2be2bc9f11c9b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490784
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820406}

chrome/browser/performance_manager/decorators/process_priority_aggregator.cc

@@ -4,9 +4,9 @@
 
 #include "chrome/browser/performance_manager/decorators/process_priority_aggregator.h"
 
-#include "components/performance_manager/graph/frame_node_impl.h"
 #include "components/performance_manager/graph/node_attached_data_impl.h"
 #include "components/performance_manager/graph/process_node_impl.h"
+#include "components/performance_manager/public/execution_context/execution_context_registry.h"
 #include "components/performance_manager/public/graph/node_data_describer_registry.h"
 
 namespace performance_manager {
@@ -113,54 +113,50 @@ ProcessPriorityAggregator::Data* ProcessPriorityAggregator::Data::GetForTesting(
 }
 
 ProcessPriorityAggregator::ProcessPriorityAggregator() = default;
-ProcessPriorityAggregator::~ProcessPriorityAggregator() = default;
 
-void ProcessPriorityAggregator::OnFrameNodeAdded(const FrameNode* frame_node) {
-  auto* process_node = ProcessNodeImpl::FromNode(frame_node->GetProcessNode());
-  DataImpl* data = DataImpl::Get(process_node);
-  data->Increment(frame_node->GetPriorityAndReason().priority());
-  // This is a nop if the priority didn't actually change.
-  process_node->set_priority(data->GetPriority());
-}
+ProcessPriorityAggregator::~ProcessPriorityAggregator() = default;
 
-void ProcessPriorityAggregator::OnBeforeFrameNodeRemoved(
-    const FrameNode* frame_node) {
-  auto* process_node = ProcessNodeImpl::FromNode(frame_node->GetProcessNode());
-  DataImpl* data = DataImpl::Get(process_node);
-  data->Decrement(frame_node->GetPriorityAndReason().priority());
-  // This is a nop if the priority didn't actually change.
-  process_node->set_priority(data->GetPriority());
-}
-
-void ProcessPriorityAggregator::OnPriorityAndReasonChanged(
-    const FrameNode* frame_node,
-    const PriorityAndReason& previous_value) {
-  // If the priority itself didn't change then ignore this notification.
-  const PriorityAndReason& new_value = frame_node->GetPriorityAndReason();
-  if (new_value.priority() == previous_value.priority())
-    return;
-
-  // Update the distinct frame priority counts, and set the process priority
-  // accordingly.
-  auto* process_node = ProcessNodeImpl::FromNode(frame_node->GetProcessNode());
-  DataImpl* data = DataImpl::Get(process_node);
-  data->Decrement(previous_value.priority());
-  data->Increment(new_value.priority());
-  // This is a nop if the priority didn't actually change.
-  process_node->set_priority(data->GetPriority());
+void ProcessPriorityAggregator::OnBeforeGraphDestroyed(Graph* graph) {
+  auto* registry =
+      execution_context::ExecutionContextRegistry::GetFromGraph(graph);
+  if (registry && registry->HasObserver(this))
+    registry->RemoveObserver(this);
 }
 
 void ProcessPriorityAggregator::OnPassedToGraph(Graph* graph) {
-  graph->AddFrameNodeObserver(this);
-  graph->AddProcessNodeObserver(this);
+  graph->AddGraphObserver(this);
   graph->GetNodeDataDescriberRegistry()->RegisterDescriber(this,
                                                            kDescriberName);
+  graph->AddProcessNodeObserver(this);
+
+  auto* registry =
+      execution_context::ExecutionContextRegistry::GetFromGraph(graph);
+  // We expect the registry to exist before we are passed to the graph.
+  DCHECK(registry);
+  registry->AddObserver(this);
 }
 
 void ProcessPriorityAggregator::OnTakenFromGraph(Graph* graph) {
-  graph->GetNodeDataDescriberRegistry()->UnregisterDescriber(this);
+  // Call OnBeforeGraphDestroyed as well. This unregisters us from the
+  // ExecutionContextRegistry in case we're being removed from the graph prior
+  // to its destruction.
+  OnBeforeGraphDestroyed(graph);
+
   graph->RemoveProcessNodeObserver(this);
-  graph->RemoveFrameNodeObserver(this);
+  graph->GetNodeDataDescriberRegistry()->UnregisterDescriber(this);
+  graph->RemoveGraphObserver(this);
+}
+
+base::Value ProcessPriorityAggregator::DescribeProcessNodeData(
+    const ProcessNode* node) const {
+  DataImpl* data = DataImpl::Get(ProcessNodeImpl::FromNode(node));
+  if (data == nullptr)
+    return base::Value();
+
+  base::Value ret(base::Value::Type::DICTIONARY);
+  ret.SetIntKey("user_visible_count", data->user_visible_count_);
+  ret.SetIntKey("user_blocking_count", data->user_blocking_count_);
+  return ret;
 }
 
 void ProcessPriorityAggregator::OnProcessNodeAdded(
@@ -182,16 +178,40 @@ void ProcessPriorityAggregator::OnBeforeProcessNodeRemoved(
 #endif
 }
 
-base::Value ProcessPriorityAggregator::DescribeProcessNodeData(
-    const ProcessNode* node) const {
-  DataImpl* data = DataImpl::Get(ProcessNodeImpl::FromNode(node));
-  if (data == nullptr)
-    return base::Value();
+void ProcessPriorityAggregator::OnExecutionContextAdded(
+    const execution_context::ExecutionContext* ec) {
+  auto* process_node = ProcessNodeImpl::FromNode(ec->GetProcessNode());
+  DataImpl* data = DataImpl::Get(process_node);
+  data->Increment(ec->GetPriorityAndReason().priority());
+  // This is a nop if the priority didn't actually change.
+  process_node->set_priority(data->GetPriority());
+}
 
-  base::Value ret(base::Value::Type::DICTIONARY);
-  ret.SetIntKey("user_visible_count", data->user_visible_count_);
-  ret.SetIntKey("user_blocking_count", data->user_blocking_count_);
-  return ret;
+void ProcessPriorityAggregator::OnBeforeExecutionContextRemoved(
+    const execution_context::ExecutionContext* ec) {
+  auto* process_node = ProcessNodeImpl::FromNode(ec->GetProcessNode());
+  DataImpl* data = DataImpl::Get(process_node);
+  data->Decrement(ec->GetPriorityAndReason().priority());
+  // This is a nop if the priority didn't actually change.
+  process_node->set_priority(data->GetPriority());
+}
+
+void ProcessPriorityAggregator::OnPriorityAndReasonChanged(
+    const execution_context::ExecutionContext* ec,
+    const PriorityAndReason& previous_value) {
+  // If the priority itself didn't change then ignore this notification.
+  const PriorityAndReason& new_value = ec->GetPriorityAndReason();
+  if (new_value.priority() == previous_value.priority())
+    return;
+
+  // Update the distinct frame priority counts, and set the process priority
+  // accordingly.
+  auto* process_node = ProcessNodeImpl::FromNode(ec->GetProcessNode());
+  DataImpl* data = DataImpl::Get(process_node);
+  data->Decrement(previous_value.priority());
+  data->Increment(new_value.priority());
+  // This is a nop if the priority didn't actually change.
+  process_node->set_priority(data->GetPriority());
 }
 
 }  // namespace performance_manager

chrome/browser/performance_manager/decorators/process_priority_aggregator.h

@@ -5,7 +5,7 @@
 #ifndef CHROME_BROWSER_PERFORMANCE_MANAGER_DECORATORS_PROCESS_PRIORITY_AGGREGATOR_H_
 #define CHROME_BROWSER_PERFORMANCE_MANAGER_DECORATORS_PROCESS_PRIORITY_AGGREGATOR_H_
 
-#include "components/performance_manager/public/graph/frame_node.h"
+#include "components/performance_manager/public/execution_context/execution_context.h"
 #include "components/performance_manager/public/graph/graph.h"
 #include "components/performance_manager/public/graph/node_data_describer.h"
 #include "components/performance_manager/public/graph/page_node.h"
@@ -19,32 +19,41 @@ class ProcessNodeImpl;
 // priority as an aggregate of the priorities of all executions contexts (frames
 // and workers) it hosts. A process will inherit the priority of the highest
 // priority context that it hosts.
-class ProcessPriorityAggregator : public FrameNode::ObserverDefaultImpl,
+class ProcessPriorityAggregator
+    : public GraphObserver,
       public GraphOwnedDefaultImpl,
       public NodeDataDescriberDefaultImpl,
-                                  public ProcessNode::ObserverDefaultImpl {
+      public ProcessNode::ObserverDefaultImpl,
+      public execution_context::ExecutionContextObserverDefaultImpl {
  public:
   class Data;
 
   ProcessPriorityAggregator();
   ~ProcessPriorityAggregator() override;
 
-  // FrameNodeObserver implementation:
-  void OnFrameNodeAdded(const FrameNode* frame_node) override;
-  void OnBeforeFrameNodeRemoved(const FrameNode* frame_node) override;
-  void OnPriorityAndReasonChanged(
-      const FrameNode* frame_node,
-      const PriorityAndReason& previous_value) override;
+  // GraphObserver implementation:
+  void OnBeforeGraphDestroyed(Graph* graph) override;
 
   // GraphOwned implementation:
   void OnPassedToGraph(Graph* graph) override;
   void OnTakenFromGraph(Graph* graph) override;
 
+  // NodeDataDescriber implementation:
+  base::Value DescribeProcessNodeData(const ProcessNode* node) const override;
+
   // ProcessNodeObserver implementation:
   void OnProcessNodeAdded(const ProcessNode* process_node) override;
   void OnBeforeProcessNodeRemoved(const ProcessNode* process_node) override;
 
-  base::Value DescribeProcessNodeData(const ProcessNode* node) const override;
+  // ExecutionContextObserver implementation:
+  void OnExecutionContextAdded(
+      const execution_context::ExecutionContext* ec) override;
+  void OnBeforeExecutionContextRemoved(
+      const execution_context::ExecutionContext* ec) override;
+  void OnPriorityAndReasonChanged(
+      const execution_context::ExecutionContext* ec,
+      const execution_context_priority::PriorityAndReason& previous_value)
+      override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ProcessPriorityAggregator);

chrome/browser/performance_manager/decorators/process_priority_aggregator_unittest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/performance_manager/decorators/process_priority_aggregator.h"
 
 #include "base/memory/ptr_util.h"
+#include "components/performance_manager/execution_context/execution_context_registry_impl.h"
 #include "components/performance_manager/graph/frame_node_impl.h"
 #include "components/performance_manager/graph/process_node_impl.h"
 #include "components/performance_manager/test_support/graph_test_harness.h"
@@ -22,6 +23,8 @@ static const char* kReason = FrameNodeImpl::kDefaultPriorityReason;
 class ProcessPriorityAggregatorTest : public GraphTestHarness {
  public:
   void SetUp() override {
+    graph()->PassToGraph(
+        std::make_unique<execution_context::ExecutionContextRegistryImpl>());
     ppa_ = new ProcessPriorityAggregator();
     graph()->PassToGraph(base::WrapUnique(ppa_));
   }
@@ -40,19 +43,22 @@ class ProcessPriorityAggregatorTest : public GraphTestHarness {
 }  // namespace
 
 TEST_F(ProcessPriorityAggregatorTest, ProcessAggregation) {
-  MockMultiplePagesWithMultipleProcessesGraph mock_graph(graph());
+  MockMultiplePagesAndWorkersWithMultipleProcessesGraph mock_graph(graph());
 
   auto& proc1 = mock_graph.process;
   auto& proc2 = mock_graph.other_process;
   auto& frame1_1 = mock_graph.frame;
   auto& frame1_2 = mock_graph.other_frame;
   auto& frame2_1 = mock_graph.child_frame;
+  auto& worker1 = mock_graph.worker;
+  auto& worker2 = mock_graph.other_worker;
 
   EXPECT_EQ(base::TaskPriority::LOWEST, proc1->priority());
   EXPECT_EQ(base::TaskPriority::LOWEST, proc2->priority());
   ExpectPriorityCounts(proc1.get(), 0, 0);
   ExpectPriorityCounts(proc2.get(), 0, 0);
 
+  // Set the priority of a frame in process 1 to USER_VISIBLE.
   frame1_1->SetPriorityAndReason(
       PriorityAndReason(base::TaskPriority::USER_VISIBLE, kReason));
   EXPECT_EQ(base::TaskPriority::USER_VISIBLE, proc1->priority());
@@ -60,6 +66,7 @@ TEST_F(ProcessPriorityAggregatorTest, ProcessAggregation) {
   ExpectPriorityCounts(proc1.get(), 1, 0);
   ExpectPriorityCounts(proc2.get(), 0, 0);
 
+  // Set the priority of a frame in process 2 to USER_VISIBLE.
   frame2_1->SetPriorityAndReason(
       PriorityAndReason(base::TaskPriority::USER_VISIBLE, kReason));
   EXPECT_EQ(base::TaskPriority::USER_VISIBLE, proc1->priority());
@@ -67,6 +74,8 @@ TEST_F(ProcessPriorityAggregatorTest, ProcessAggregation) {
   ExpectPriorityCounts(proc1.get(), 1, 0);
   ExpectPriorityCounts(proc2.get(), 1, 0);
 
+  // Set the priority of another frame in process 1 to USER_BLOCKING. This
+  // overwrites the vote from the first frame.
   frame1_2->SetPriorityAndReason(
       PriorityAndReason(base::TaskPriority::USER_BLOCKING, kReason));
   EXPECT_EQ(base::TaskPriority::USER_BLOCKING, proc1->priority());
@@ -74,13 +83,35 @@ TEST_F(ProcessPriorityAggregatorTest, ProcessAggregation) {
   ExpectPriorityCounts(proc1.get(), 1, 1);
   ExpectPriorityCounts(proc2.get(), 1, 0);
 
+  // Set the priority of a worker in process 2 to USER_BLOCKING. This overwrites
+  // the vote from the sole frame in this process.
+  worker2->SetPriorityAndReason(
+      PriorityAndReason(base::TaskPriority::USER_BLOCKING, kReason));
+  EXPECT_EQ(base::TaskPriority::USER_BLOCKING, proc1->priority());
+  EXPECT_EQ(base::TaskPriority::USER_BLOCKING, proc2->priority());
+  ExpectPriorityCounts(proc1.get(), 1, 1);
+  ExpectPriorityCounts(proc2.get(), 1, 1);
+
+  // Reduces the priority of the second frame in process 1 to USER_VISIBLE. Now
+  // both frames in this process are at USER_VISIBLE.
   frame1_2->SetPriorityAndReason(
       PriorityAndReason(base::TaskPriority::USER_VISIBLE, kReason));
   EXPECT_EQ(base::TaskPriority::USER_VISIBLE, proc1->priority());
+  EXPECT_EQ(base::TaskPriority::USER_BLOCKING, proc2->priority());
+  ExpectPriorityCounts(proc1.get(), 2, 0);
+  ExpectPriorityCounts(proc2.get(), 1, 1);
+
+  // Reduces the priority of the worker in process 2 to LOWEST. The highest
+  // execution context priority of that process is now due to the sole frame.
+  worker2->SetPriorityAndReason(
+      PriorityAndReason(base::TaskPriority::LOWEST, kReason));
+  EXPECT_EQ(base::TaskPriority::USER_VISIBLE, proc1->priority());
   EXPECT_EQ(base::TaskPriority::USER_VISIBLE, proc2->priority());
   ExpectPriorityCounts(proc1.get(), 2, 0);
   ExpectPriorityCounts(proc2.get(), 1, 0);
 
+  // Reduces the priority of the sole frame in process 2 to LOWEST. All
+  // execution contexts in this process are now at LOWEST.
   frame2_1->SetPriorityAndReason(
       PriorityAndReason(base::TaskPriority::LOWEST, kReason));
   EXPECT_EQ(base::TaskPriority::USER_VISIBLE, proc1->priority());
@@ -88,6 +119,8 @@ TEST_F(ProcessPriorityAggregatorTest, ProcessAggregation) {
   ExpectPriorityCounts(proc1.get(), 2, 0);
   ExpectPriorityCounts(proc2.get(), 0, 0);
 
+  // Reduces the priority of the first frame in process 1 to LOWEST. The highest
+  // execution priority of that process is now due to the second frame.
   frame1_1->SetPriorityAndReason(
       PriorityAndReason(base::TaskPriority::LOWEST, kReason));
   EXPECT_EQ(base::TaskPriority::USER_VISIBLE, proc1->priority());
@@ -95,12 +128,32 @@ TEST_F(ProcessPriorityAggregatorTest, ProcessAggregation) {
   ExpectPriorityCounts(proc1.get(), 1, 0);
   ExpectPriorityCounts(proc2.get(), 0, 0);
 
+  // Reduces the priority of the second frame in process 1 to LOWEST. All
+  // execution contexts in this process are now at LOWEST.
   frame1_2->SetPriorityAndReason(
       PriorityAndReason(base::TaskPriority::LOWEST, kReason));
   EXPECT_EQ(base::TaskPriority::LOWEST, proc1->priority());
   EXPECT_EQ(base::TaskPriority::LOWEST, proc2->priority());
   ExpectPriorityCounts(proc1.get(), 0, 0);
   ExpectPriorityCounts(proc2.get(), 0, 0);
+
+  // Set the priority of the worker in process 1. It is the execution context
+  // with the highest priority and thus dictates the priority of this process.
+  worker1->SetPriorityAndReason(
+      PriorityAndReason(base::TaskPriority::USER_VISIBLE, kReason));
+  EXPECT_EQ(base::TaskPriority::USER_VISIBLE, proc1->priority());
+  EXPECT_EQ(base::TaskPriority::LOWEST, proc2->priority());
+  ExpectPriorityCounts(proc1.get(), 1, 0);
+  ExpectPriorityCounts(proc2.get(), 0, 0);
+
+  // Reduces the priority of the worker in process 1 to LOWEST. All execution
+  // contexts in this process are now at LOWEST.
+  worker1->SetPriorityAndReason(
+      PriorityAndReason(base::TaskPriority::LOWEST, kReason));
+  EXPECT_EQ(base::TaskPriority::LOWEST, proc1->priority());
+  EXPECT_EQ(base::TaskPriority::LOWEST, proc2->priority());
+  ExpectPriorityCounts(proc1.get(), 0, 0);
+  ExpectPriorityCounts(proc2.get(), 0, 0);
 }
 
 }  // namespace performance_manager

components/performance_manager/execution_context/execution_context_registry_impl.cc

@@ -80,7 +80,6 @@ class DummyExecutionContextForLookup : public ExecutionContext {
 ExecutionContextRegistry::ExecutionContextRegistry() = default;
 
 ExecutionContextRegistry::~ExecutionContextRegistry() = default;
-
 // static
 ExecutionContextRegistry* ExecutionContextRegistry::GetFromGraph(Graph* graph) {
   return GraphRegisteredImpl<ExecutionContextRegistryImpl>::GetFromGraph(graph);

components/performance_manager/execution_context/execution_context_registry_impl.h

@@ -91,7 +91,9 @@ class ExecutionContextRegistryImpl
                      ExecutionContextKeyEqual>
       execution_contexts_;
 
-  base::ObserverList<ExecutionContextObserver, /* check_empty = */ true>
+  base::ObserverList<ExecutionContextObserver,
+                     /* check_empty = */ true,
+                     /* allow_reentrancy */ false>
       observers_;
 
   SEQUENCE_CHECKER(sequence_checker_);

components/performance_manager/test_support/mock_graphs.cc

@@ -109,4 +109,25 @@ void MockSinglePageWithFrameAndWorkerInSingleProcessGraph::DeleteWorker() {
   worker.reset();
 }
 
+MockMultiplePagesAndWorkersWithMultipleProcessesGraph::
+    MockMultiplePagesAndWorkersWithMultipleProcessesGraph(TestGraphImpl* graph)
+    : MockMultiplePagesWithMultipleProcessesGraph(graph),
+      worker(TestNodeWrapper<WorkerNodeImpl>::Create(
+          graph,
+          WorkerNode::WorkerType::kDedicated,
+          process.get())),
+      other_worker(TestNodeWrapper<WorkerNodeImpl>::Create(
+          graph,
+          WorkerNode::WorkerType::kDedicated,
+          other_process.get())) {
+  worker->AddClientFrame(frame.get());
+  other_worker->AddClientFrame(child_frame.get());
+}
+
+MockMultiplePagesAndWorkersWithMultipleProcessesGraph::
+    ~MockMultiplePagesAndWorkersWithMultipleProcessesGraph() {
+  other_worker->RemoveClientFrame(child_frame.get());
+  worker->RemoveClientFrame(frame.get());
+}
+
 }  // namespace performance_manager

components/performance_manager/test_support/mock_graphs.h

@@ -140,6 +140,39 @@ struct MockSinglePageWithFrameAndWorkerInSingleProcessGraph
   void DeleteWorker();
 };
 
+// The following graph topology is created to emulate a scenario where multiple
+// pages making use of workers are hosted in multiple processes (e.g.
+// out-of-process iFrames and multiple pages in a process):
+//
+//    Pg    OPg
+//    |     |
+//    F     OF
+//   /\    /  \
+//  W  \  /   CF
+//   \ | /    | \
+//     Pr     | OW
+//            | /
+//            OPr
+//
+// Where:
+// Pg: page
+// OPg: other_page
+// F: frame(frame_tree_id:0)
+// OF: other_frame(frame_tree_id:1)
+// CF: child_frame(frame_tree_id:3)
+// W: worker
+// OW: other_worker
+// Pr: process(pid:1)
+// OPr: other_process(pid:2)
+struct MockMultiplePagesAndWorkersWithMultipleProcessesGraph
+    : public MockMultiplePagesWithMultipleProcessesGraph {
+  explicit MockMultiplePagesAndWorkersWithMultipleProcessesGraph(
+      TestGraphImpl* graph);
+  ~MockMultiplePagesAndWorkersWithMultipleProcessesGraph();
+  TestNodeWrapper<WorkerNodeImpl> worker;
+  TestNodeWrapper<WorkerNodeImpl> other_worker;
+};
+
 }  // namespace performance_manager
 
 #endif  // COMPONENTS_PERFORMANCE_MANAGER_TEST_SUPPORT_MOCK_GRAPHS_H_