memory-infra: fix post order iterator

The iterator should work on the scope of the entire graph as it relies
on shared visited set between processes to do its job.

Bug: 768373
Change-Id: I2be92608c8d709464a32f8f35a10cd5686fc7af9
Reviewed-on: https://chromium-review.googlesource.com/788870Reviewed-by: Primiano Tucci <primiano@chromium.org>
Commit-Queue: Lalit Maganti <lalitm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521684}

services/resource_coordinator/memory_instrumentation/graph.cc

@@ -48,6 +48,16 @@ Node* GlobalDumpGraph::CreateNode(Process* process_graph, Node* parent) {
   return &*all_nodes_.begin();
 }
 
+PostOrderIterator GlobalDumpGraph::VisitInDepthFirstPostOrder() {
+  std::vector<Node*> roots;
+  for (auto it = process_dump_graphs_.rbegin();
+       it != process_dump_graphs_.rend(); it++) {
+    roots.push_back(it->second->root());
+  }
+  roots.push_back(shared_memory_graph_->root());
+  return PostOrderIterator(std::move(roots));
+}
+
 Process::Process(base::ProcessId pid, GlobalDumpGraph* global_graph)
     : pid_(pid),
       global_graph_(global_graph),
@@ -102,10 +112,6 @@ Node* Process::FindNode(base::StringPiece path) {
   return current;
 }
 
-PostOrderIterator Process::VisitInDepthFirstPostOrder() {
-  return PostOrderIterator(root_);
-}
-
 Node::Node(Process* dump_graph, Node* parent)
     : dump_graph_(dump_graph), parent_(parent), owns_edge_(nullptr) {}
 Node::~Node() {}
@@ -171,9 +177,8 @@ Node::Entry::Entry(std::string value)
 Edge::Edge(Node* source, Node* target, int priority)
     : source_(source), target_(target), priority_(priority) {}
 
-PostOrderIterator::PostOrderIterator(Node* root) {
-  to_visit_.push_back(root);
-}
+PostOrderIterator::PostOrderIterator(std::vector<Node*> roots)
+    : to_visit_(std::move(roots)) {}
 PostOrderIterator::PostOrderIterator(PostOrderIterator&& other) = default;
 PostOrderIterator::~PostOrderIterator() = default;
 

services/resource_coordinator/memory_instrumentation/graph.h

@@ -42,11 +42,6 @@ class GlobalDumpGraph {
     // if no such node exists in the provided |graph|.
     GlobalDumpGraph::Node* FindNode(base::StringPiece path);
 
-    // Returns an iterator which yields nodes in the nodes in this graph in
-    // post-order. That is, children and owners of nodes are returned before the
-    // node itself.
-    GlobalDumpGraph::PostOrderIterator VisitInDepthFirstPostOrder();
-
     base::ProcessId pid() const { return pid_; }
     GlobalDumpGraph* global_graph() const { return global_graph_; }
     GlobalDumpGraph::Node* root() const { return root_; }
@@ -179,7 +174,7 @@ class GlobalDumpGraph {
   // An iterator-esque class which yields nodes in a depth-first post order.
   class PostOrderIterator {
    public:
-    PostOrderIterator(Node* root);
+    PostOrderIterator(std::vector<Node*> root_nodes);
     PostOrderIterator(PostOrderIterator&& other);
     ~PostOrderIterator();
 
@@ -208,6 +203,11 @@ class GlobalDumpGraph {
   // and edge priority.
   void AddNodeOwnershipEdge(Node* owner, Node* owned, int priority);
 
+  // Returns an iterator which yields nodes in the nodes in this graph in
+  // post-order. That is, children and owners of nodes are returned before the
+  // node itself.
+  PostOrderIterator VisitInDepthFirstPostOrder();
+
   const GuidNodeMap& nodes_by_guid() const { return nodes_by_guid_; }
   GlobalDumpGraph::Process* shared_memory_graph() const {
     return shared_memory_graph_.get();

services/resource_coordinator/memory_instrumentation/graph_processor.cc

@@ -136,32 +136,18 @@ void GraphProcessor::CalculateSizesForGraph(GlobalDumpGraph* global_graph) {
   // Eighth pass: calculate the size field for nodes by considering the sizes
   // of their children and owners.
   {
-    auto it = global_graph->shared_memory_graph()->VisitInDepthFirstPostOrder();
+    auto it = global_graph->VisitInDepthFirstPostOrder();
     while (Node* node = it.next()) {
       CalculateSizeForNode(node);
     }
-
-    for (auto& pid_to_process : global_graph->process_dump_graphs()) {
-      auto it = pid_to_process.second->VisitInDepthFirstPostOrder();
-      while (Node* node = it.next()) {
-        CalculateSizeForNode(node);
-      }
-    }
   }
 
   // Ninth pass: Calculate not-owned and not-owning sub-sizes of all nodes.
   {
-    auto it = global_graph->shared_memory_graph()->VisitInDepthFirstPostOrder();
+    auto it = global_graph->VisitInDepthFirstPostOrder();
     while (Node* node = it.next()) {
       CalculateDumpSubSizes(node);
     }
-
-    for (auto& pid_to_process : global_graph->process_dump_graphs()) {
-      auto it = pid_to_process.second->VisitInDepthFirstPostOrder();
-      while (Node* node = it.next()) {
-        CalculateDumpSubSizes(node);
-      }
-    }
   }
 }
 

services/resource_coordinator/memory_instrumentation/graph_unittest.cc

@@ -9,10 +9,16 @@
 
 namespace memory_instrumentation {
 
+namespace {
+
 using base::trace_event::MemoryAllocatorDumpGuid;
 using Node = memory_instrumentation::GlobalDumpGraph::Node;
 using Process = memory_instrumentation::GlobalDumpGraph::Process;
 
+const MemoryAllocatorDumpGuid kEmptyGuid;
+
+}  // namespace
+
 TEST(GlobalDumpGraphTest, CreateContainerForProcess) {
   GlobalDumpGraph global_dump_graph;
 
@@ -41,39 +47,34 @@ TEST(GlobalDumpGraphTest, AddNodeOwnershipEdge) {
 
 TEST(GlobalDumpGraphTest, VisitInDepthFirstPostOrder) {
   GlobalDumpGraph graph;
-  Process* process = graph.shared_memory_graph();
-  Node* root = process->root();
-
-  Node c1(process, root);
-  Node c2(process, root);
-  Node c2_c1(process, &c2);
-  Node c2_c2(process, &c2);
-  Node c3(process, root);
-  Node c3_c1(process, &c3);
-  Node c3_c2(process, &c3);
-
-  root->InsertChild("c1", &c1);
-  root->InsertChild("c2", &c2);
-  root->InsertChild("c3", &c3);
-  c2.InsertChild("c1", &c2_c1);
-  c2.InsertChild("c2", &c2_c2);
-  c3.InsertChild("c1", &c3_c1);
-  c3.InsertChild("c2", &c3_c2);
+  Process* process_1 = graph.CreateGraphForProcess(1);
+  Process* process_2 = graph.CreateGraphForProcess(2);
+
+  Node* c1 = process_1->CreateNode(kEmptyGuid, "c1", false);
+  Node* c2 = process_1->CreateNode(kEmptyGuid, "c2", false);
+  Node* c2_c1 = process_1->CreateNode(kEmptyGuid, "c2/c1", false);
+  Node* c2_c2 = process_1->CreateNode(kEmptyGuid, "c2/c2", false);
+
+  Node* c3 = process_2->CreateNode(kEmptyGuid, "c3", false);
+  Node* c3_c1 = process_2->CreateNode(kEmptyGuid, "c3/c1", false);
+  Node* c3_c2 = process_2->CreateNode(kEmptyGuid, "c3/c2", false);
 
   // |c3_c2| owns |c2_c2|.
-  graph.AddNodeOwnershipEdge(&c3_c2, &c2_c2, 1);
+  graph.AddNodeOwnershipEdge(c3_c2, c2_c2, 1);
 
   // This method should always call owners and then children before the node
   // itself.
-  auto iterator = process->VisitInDepthFirstPostOrder();
-  ASSERT_EQ(iterator.next(), &c1);
-  ASSERT_EQ(iterator.next(), &c2_c1);
-  ASSERT_EQ(iterator.next(), &c3_c2);
-  ASSERT_EQ(iterator.next(), &c2_c2);
-  ASSERT_EQ(iterator.next(), &c2);
-  ASSERT_EQ(iterator.next(), &c3_c1);
-  ASSERT_EQ(iterator.next(), &c3);
-  ASSERT_EQ(iterator.next(), root);
+  auto iterator = graph.VisitInDepthFirstPostOrder();
+  ASSERT_EQ(iterator.next(), graph.shared_memory_graph()->root());
+  ASSERT_EQ(iterator.next(), c1);
+  ASSERT_EQ(iterator.next(), c2_c1);
+  ASSERT_EQ(iterator.next(), c3_c2);
+  ASSERT_EQ(iterator.next(), c2_c2);
+  ASSERT_EQ(iterator.next(), c2);
+  ASSERT_EQ(iterator.next(), process_1->root());
+  ASSERT_EQ(iterator.next(), c3_c1);
+  ASSERT_EQ(iterator.next(), c3);
+  ASSERT_EQ(iterator.next(), process_2->root());
   ASSERT_EQ(iterator.next(), nullptr);
 }