tracing: memory-infra cleanup post-C++11 + test coverage

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>
 
 #include "base/process/process_metrics.h"
@@ skipping 106 matching lines @@
     LOG(ERROR) << "CountResidentBytes failed. The resident size is invalid";
   }
   return total_resident_size;
 }
 #endif  // defined(COUNT_RESIDENT_BYTES_SUPPORTED)
 
 ProcessMemoryDump::ProcessMemoryDump(
     const scoped_refptr<MemoryDumpSessionState>& session_state)
     : has_process_totals_(false),
       has_process_mmaps_(false),
-      session_state_(session_state) {
-}
+      session_state_(session_state) {}
 
-ProcessMemoryDump::~ProcessMemoryDump() {
-}
+ProcessMemoryDump::~ProcessMemoryDump() {}
 
 MemoryAllocatorDump* ProcessMemoryDump::CreateAllocatorDump(
     const std::string& absolute_name) {
-  MemoryAllocatorDump* mad = new MemoryAllocatorDump(absolute_name, this);
-  AddAllocatorDumpInternal(mad);  // Takes ownership of |mad|.
-  return mad;
+  return AddAllocatorDumpInternal(
+      make_scoped_ptr(new MemoryAllocatorDump(absolute_name, this)));
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::CreateAllocatorDump(
     const std::string& absolute_name,
     const MemoryAllocatorDumpGuid& guid) {
-  MemoryAllocatorDump* mad = new MemoryAllocatorDump(absolute_name, this, guid);
-  AddAllocatorDumpInternal(mad);  // Takes ownership of |mad|.
-  return mad;
+  return AddAllocatorDumpInternal(
+      make_scoped_ptr(new MemoryAllocatorDump(absolute_name, this, guid)));
 }
 
-void ProcessMemoryDump::AddAllocatorDumpInternal(MemoryAllocatorDump* mad) {
-  DCHECK_EQ(0ul, allocator_dumps_.count(mad->absolute_name()));
-  allocator_dumps_storage_.push_back(mad);
-  allocator_dumps_[mad->absolute_name()] = mad;
+MemoryAllocatorDump* ProcessMemoryDump::AddAllocatorDumpInternal(
+    scoped_ptr<MemoryAllocatorDump> mad) {
+  auto insertion_result = allocator_dumps_.insert(
+      std::make_pair(mad->absolute_name(), std::move(mad)));

[petrcermak, 2016/02/22 13:33:25]

Why don't you change the signature of this method to
'scoped_ptr<MemoryAllocatorDump> &&' instead? That should be safer because the
compiler would check that you always call it with rvalues.

[Primiano Tucci (use gerrit), 2016/02/22 15:56:07]

There doesn't seem to be any precedent for this. Also scoped_ptr is a move-only
type.

+  DCHECK(insertion_result.second) << "Duplicate name: " << mad->absolute_name();
+  return insertion_result.first->second.get();
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::GetAllocatorDump(
     const std::string& absolute_name) const {
   auto it = allocator_dumps_.find(absolute_name);
-  return it == allocator_dumps_.end() ? nullptr : it->second;
+  return it == allocator_dumps_.end() ? nullptr : it->second.get();
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::GetOrCreateAllocatorDump(
     const std::string& absolute_name) {
   MemoryAllocatorDump* mad = GetAllocatorDump(absolute_name);
   return mad ? mad : CreateAllocatorDump(absolute_name);
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::CreateSharedGlobalAllocatorDump(
     const MemoryAllocatorDumpGuid& guid) {
@@ skipping 34 matching lines @@
   if (has_process_totals_) {
     process_totals_.Clear();
     has_process_totals_ = false;
   }
 
   if (has_process_mmaps_) {
     process_mmaps_.Clear();
     has_process_mmaps_ = false;
   }
 
-  allocator_dumps_storage_.clear();
   allocator_dumps_.clear();
   allocator_dumps_edges_.clear();
   heap_dumps_.clear();
 }
 
 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.
-  for (MemoryAllocatorDump* mad : other->allocator_dumps_storage_) {
-    // Check that we don't merge duplicates.
-    DCHECK_EQ(0ul, allocator_dumps_.count(mad->absolute_name()));
-    allocator_dumps_storage_.push_back(mad);
-    allocator_dumps_[mad->absolute_name()] = mad;
-  }
-  other->allocator_dumps_storage_.weak_clear();
+  // into this ProcessMemoryDump, checking for duplicates.
+  for (auto& it : other->allocator_dumps_)
+    AddAllocatorDumpInternal(std::move(it.second));

[petrcermak, 2016/02/22 13:33:25]

Note that if you keep the std::move conversion on line 147, you'll be moving
this value twice. I don't think you need that.

[Primiano Tucci (use gerrit), 2016/02/22 15:56:07]

See reply above, no precedent.
Even if that should happen, amen, that's not a fast-path.
But my hope about all these move semantic was that the compiler was smart enough
to elide the unnecessary paths.
Don't have time to check right now, but very likely the entire
AddAllocatorDumpInternal gets inlined.

   other->allocator_dumps_.clear();
 
   // Move all the edges.
   allocator_dumps_edges_.insert(allocator_dumps_edges_.end(),
                                 other->allocator_dumps_edges_.begin(),
                                 other->allocator_dumps_edges_.end());
   other->allocator_dumps_edges_.clear();
 
   heap_dumps_.insert(other->heap_dumps_.begin(), other->heap_dumps_.end());
   other->heap_dumps_.clear();
 }
 
 void ProcessMemoryDump::AsValueInto(TracedValue* value) const {
   if (has_process_totals_) {
     value->BeginDictionary("process_totals");
     process_totals_.AsValueInto(value);
     value->EndDictionary();
   }
 
   if (has_process_mmaps_) {
     value->BeginDictionary("process_mmaps");
     process_mmaps_.AsValueInto(value);
     value->EndDictionary();
   }
 
-  if (allocator_dumps_storage_.size() > 0) {
+  if (allocator_dumps_.size() > 0) {
     value->BeginDictionary("allocators");
-    for (const MemoryAllocatorDump* allocator_dump : allocator_dumps_storage_)
-      allocator_dump->AsValueInto(value);
+    for (const auto& allocator_dump_it : allocator_dumps_)
+      allocator_dump_it.second->AsValueInto(value);
     value->EndDictionary();
   }
 
   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"
   }
 
@@ skipping 24 matching lines @@
 
 void ProcessMemoryDump::AddSuballocation(const MemoryAllocatorDumpGuid& source,
                                          const std::string& target_node_name) {
   std::string child_mad_name = target_node_name + "/__" + source.ToString();
   MemoryAllocatorDump* target_child_mad = CreateAllocatorDump(child_mad_name);
   AddOwnershipEdge(source, target_child_mad->guid());
 }
 
 }  // namespace trace_event
 }  // namespace base