[memory-infra] Add method to override importance of ownership edges

base/trace_event/process_memory_dump.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/trace_event/process_memory_dump.h"
 
 #include <errno.h>
 
 #include <vector>
 
@@ skipping 268 matching lines @@
 void ProcessMemoryDump::TakeAllDumpsFrom(ProcessMemoryDump* other) {
   DCHECK(!other->has_process_totals() && !other->has_process_mmaps());
 
   // Moves the ownership of all MemoryAllocatorDump(s) contained in |other|
   // into this ProcessMemoryDump, checking for duplicates.
   for (auto& it : other->allocator_dumps_)
     AddAllocatorDumpInternal(std::move(it.second));
   other->allocator_dumps_.clear();
 
   // Move all the edges.
-  allocator_dumps_edges_.insert(allocator_dumps_edges_.end(),
-                                other->allocator_dumps_edges_.begin(),
+  allocator_dumps_edges_.insert(other->allocator_dumps_edges_.begin(),
                                 other->allocator_dumps_edges_.end());
   other->allocator_dumps_edges_.clear();
 
   for (auto& it : other->heap_dumps_) {
     DCHECK_EQ(0ul, heap_dumps_.count(it.first));
     heap_dumps_.insert(std::make_pair(it.first, std::move(it.second)));
   }
   other->heap_dumps_.clear();
 }
 
@@ skipping 18 matching lines @@
   }
 
   if (heap_dumps_.size() > 0) {
     value->BeginDictionary("heaps");
     for (const auto& name_and_dump : heap_dumps_)
       value->SetValueWithCopiedName(name_and_dump.first, *name_and_dump.second);
     value->EndDictionary();  // "heaps"
   }
 
   value->BeginArray("allocators_graph");
-  for (const MemoryAllocatorDumpEdge& edge : allocator_dumps_edges_) {
+  for (const auto& edge : allocator_dumps_edges_) {
     value->BeginDictionary();
-    value->SetString("source", edge.source.ToString());
-    value->SetString("target", edge.target.ToString());
-    value->SetInteger("importance", edge.importance);
-    value->SetString("type", edge.type);
+    value->SetString("source", edge.first.ToString());

[Primiano Tucci (use gerrit), 2017/06/05 15:23:39]

since the edge has still the |source| I think this code would stay more readable
if you:

- name the argument of the loop an explict iterator (for const auto& it : ...)
- add a const MemoryAllocatorDumpEdge& edge = it.second; here
- keep doing edge.(like it was before)

[ssid, 2017/06/05 17:03:29]

Fixed.

+    value->SetString("target", edge.second.target.ToString());
+    value->SetInteger("importance", edge.second.importance);
+    value->SetString("type", edge.second.type);
     value->EndDictionary();
   }
   value->EndArray();
 }
 
 void ProcessMemoryDump::AddOwnershipEdge(const MemoryAllocatorDumpGuid& source,
                                          const MemoryAllocatorDumpGuid& target,
                                          int importance) {
-  allocator_dumps_edges_.push_back(
-      {source, target, importance, kEdgeTypeOwnership});
+  DCHECK(allocator_dumps_edges_.find(source) == allocator_dumps_edges_.end() ||

[Primiano Tucci (use gerrit), 2017/06/05 15:23:39]

allocator_dump_edges.count(source) == 0 is more concise (and performance I
think, as it doesn't have to return  an iterator)

[ssid, 2017/06/05 17:03:29]

Done.

+         allocator_dumps_edges_[source].overridable)
+      << "Cannot add multiple ownership edges between same allocator dumps";
+  allocator_dumps_edges_[source] = {target, importance, kEdgeTypeOwnership,
+                                    false /* overridable */};
 }
 
 void ProcessMemoryDump::AddOwnershipEdge(
     const MemoryAllocatorDumpGuid& source,
     const MemoryAllocatorDumpGuid& target) {
   AddOwnershipEdge(source, target, 0 /* importance */);
 }
 
+void ProcessMemoryDump::AddOverridableOwnershipEdge(
+    const MemoryAllocatorDumpGuid& source,
+    const MemoryAllocatorDumpGuid& target,
+    int importance) {
+  if (allocator_dumps_edges_.find(source) == allocator_dumps_edges_.end()) {
+    allocator_dumps_edges_[source] = {target, importance, kEdgeTypeOwnership,
+                                      true /* overridable */};
+  } else {
+    DCHECK(!allocator_dumps_edges_[source].overridable);

[Primiano Tucci (use gerrit), 2017/06/05 15:23:39]

isn't this branch forgetting to actually override the entry? 
I mean, how is this supposed to work to override an edge that has been already
created?

[ssid, 2017/06/05 17:03:29]

Actually I went for the model where we specify the edges that can be overridden
rather than a API that does "OverrideOwnershipEdge". I just didn't want to make
all edges overridable and we will be confused when reading the code. Added
comment here.

+  }
+}
+
 void ProcessMemoryDump::AddSuballocation(const MemoryAllocatorDumpGuid& source,
                                          const std::string& target_node_name) {
   // Do not create new dumps for suballocations in background mode.
   if (dump_args_.level_of_detail == MemoryDumpLevelOfDetail::BACKGROUND)
     return;
 
   std::string child_mad_name = target_node_name + "/__" + source.ToString();
   MemoryAllocatorDump* target_child_mad = CreateAllocatorDump(child_mad_name);
   AddOwnershipEdge(source, target_child_mad->guid());
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::GetBlackHoleMad() {
   DCHECK(is_black_hole_non_fatal_for_testing_);
   if (!black_hole_mad_)
     black_hole_mad_.reset(new MemoryAllocatorDump("discarded", this));
   return black_hole_mad_.get();
 }
 
 }  // namespace trace_event
 }  // namespace base