[tracing] Add method to create "weak" global dumps

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 117 matching lines @@
 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) {
   // A shared allocator dump can be shared within a process and the guid could
   // have been created already.
-  MemoryAllocatorDump* allocator_dump = GetSharedGlobalAllocatorDump(guid);
-  return allocator_dump ? allocator_dump
-                        : CreateAllocatorDump(
-                              GetSharedGlobalAllocatorDumpName(guid), guid);
+  MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
+  // The weak flag is unset because this method should create non-weak dump.

[petrcermak, 2016/01/18 11:23:46]

nit: s/non-weak/a non-weak/

[ssid, 2016/01/18 16:28:36]

Done.

+  if (mad)

[petrcermak, 2016/01/18 11:23:46]

I think that the following would be more readable:

MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
if (mad) {
  // Unset the weak flag of the existing dump (if applicable).
  mad->unset_flags(MemoryAllocatorDump::Flag::WEAK);
  return mad;
}
return CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);

[ssid, 2016/01/18 16:28:36]

Done.

+    mad->unset_flags(MemoryAllocatorDump::Flag::WEAK);
+  else
+    mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);
+  return mad;
+}
+
+MemoryAllocatorDump* ProcessMemoryDump::CreateWeakSharedGlobalAllocatorDump(
+    const MemoryAllocatorDumpGuid& guid) {
+  MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
+  if (!mad) {
+    mad = new MemoryAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), this,

[petrcermak, 2016/01/18 11:23:46]

At this point, I wonder whether it wouldn't make more sense to drop the new
MemoryAllocatorDump constructor (with the flags argument) and do the following
here:

if (!mad) {
  mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);
  mad->set_flags(MemoryAllocatorDump::Flag::WEAK);
}
return mad;

Rationale: Make the source code of CreateSharedGlobalAllocatorDump and
CreateWeakSharedGlobalAllocatorDump more similar.


In addition, similarly to the first method, I think that the following would be
more readable:

MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
if (mad)
  return mad;
mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);
mad->set_flags(MemoryAllocatorDump::Flag::WEAK);
return mad;


One more thing, the CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid),
guid) could be factored out into a separate method (and shared between
CreateSharedGlobalAllocatorDump and CreateWeakSharedGlobalAllocatorDump), for
example:

MemoryAllocatorDump* ProcessMemoryDump::AddSharedGlobalAllocatorDumpInternal(
    const MemoryAllocatorDumpGuid& guid) {
  return CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);
}

[ssid, 2016/01/18 16:28:36]

Hm not sure if one line method is really needed. Used the second snippet you
suggested.

[petrcermak, 2016/01/18 17:18:12]

Acknowledged.

+                                  guid, MemoryAllocatorDump::Flag::WEAK);
+    AddAllocatorDumpInternal(mad);  // Takes ownership of |mad|.
+  }
+  return mad;
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::GetSharedGlobalAllocatorDump(
     const MemoryAllocatorDumpGuid& guid) const {
   return GetAllocatorDump(GetSharedGlobalAllocatorDumpName(guid));
 }
 
 void ProcessMemoryDump::AddHeapDump(const std::string& absolute_name,
                                     scoped_refptr<TracedValue> heap_dump) {
   DCHECK_EQ(0ul, heap_dumps_.count(absolute_name));
@@ skipping 95 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