[tracing] Switch to new heap dump format.

base/trace_event/memory_dump_manager.cc

Index: base/trace_event/memory_dump_manager.cc
diff --git a/base/trace_event/memory_dump_manager.cc b/base/trace_event/memory_dump_manager.cc
index 18b255ab8b1db1303f3ee4567f309f819622e11b..4ca61be64387d14641de282682b15eebaa20e997 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -94,33 +94,6 @@ void OnGlobalDumpDone(MemoryDumpCallback wrapped_callback,
   }
 }
 
-// Proxy class which wraps a ConvertableToTraceFormat owned by the
-// |session_state| into a proxy object that can be added to the trace event log.
-// This is to solve the problem that the MemoryDumpSessionState is refcounted
-// but the tracing subsystem wants a std::unique_ptr<ConvertableToTraceFormat>.
-template <typename T>
-struct SessionStateConvertableProxy : public ConvertableToTraceFormat {
-  using GetterFunctPtr = T* (MemoryDumpSessionState::*)() const;
-
-  SessionStateConvertableProxy(
-      scoped_refptr<MemoryDumpSessionState> session_state,
-      GetterFunctPtr getter_function)
-      : session_state(session_state), getter_function(getter_function) {}
-
-  void AppendAsTraceFormat(std::string* out) const override {
-    return (session_state.get()->*getter_function)()->AppendAsTraceFormat(out);
-  }
-
-  void EstimateTraceMemoryOverhead(
-      TraceEventMemoryOverhead* overhead) override {
-    return (session_state.get()->*getter_function)()
-        ->EstimateTraceMemoryOverhead(overhead);
-  }
-
-  scoped_refptr<MemoryDumpSessionState> session_state;
-  GetterFunctPtr const getter_function;
-};
-
 }  // namespace
 
 // static
@@ -734,6 +707,55 @@ void MemoryDumpManager::FinalizeDumpAndAddToTrace(
         TRACE_EVENT_FLAG_HAS_ID);
   }
 
+  const size_t kHeapDumpNumArgs = 1;
+  const char* kHeapDumpArgNames[] = {"data"};
+  const unsigned char kHeapDumpArgTypes[] = {TRACE_VALUE_TYPE_CONVERTABLE};
+  for (const auto& kv : pmd_async_state->process_dumps) {
+    ProcessId pid = kv.first;
+    ProcessMemoryDump* process_memory_dump = kv.second.get();
+    if (pid != kNullProcessId) {
+      // We expect heap dumps only from the current process.
+      DCHECK(process_memory_dump->heap_dumps().empty());
+      continue;
+    }
+
+    auto heap_dump_value = MakeUnique<TracedValue>();
+
+    // Add version specifying format of the data below.
+    heap_dump_value->SetInteger("version", 1);

[Primiano Tucci (use gerrit), 2017/02/17 17:07:05]

I think that historically we avoided any sort of versioning in tracing. The deal
is that when you change something is either:
- incremental changes that can still be parsed by the old UI
- you change the name of the key so that old version of the UI will ignore this
completely.

The problem with the version approach, is that we definitely run into cases
where the UI is older than the trace format. So the day that you will introduce
version "2" here, there will be people out there running a version of the UI
that doesn't know about the existance of version 2 and will still try to parse
this as version 1.
As such let's remove the versioning  and keep using the same pattern

[DmitrySkiba, 2017/02/23 07:17:19]

My position is exactly the opposite - versioning is better all around:

1. With versioning old UI can gracefully handle new traces by explaining why
certain features are missing, and how to fix that (upgrade to the latest
version). With key renaming those features will just be missing, without any
explanation.

2. Versioning means code can be sure certain keys will be there. With key
renaming code had to be written defensively, with fallback paths in mind.

Are there any drawbacks to having a version field?

[hjd, 2017/02/23 12:33:27]

I was motivated to ask Dmitry to add the version key to make keeping backwards
compatibility simpler. We already support two older versions of the format and
the code for that is a little gnarly [1-3]. I hoped that adding an explicit
version key would remove some of the guesswork (e.g. "key x is present but key y
is missing so I guess this is the middle format when we had type info but still
used sum totals?"). The design of the reader hopefully makes it trivial to be
backwards compatible forever.

Like Dmitry I think that when we make breaking changes it is better for the UI
to call this out rather than to silently ignore them. However I hadn't really
considered forward compatibility (incremental changes that can still be parsed
by the old UI). With a version field forward compatible changes would still be
possible (to make them explicit we could even have semvar style version numbers
"[breakingVersion].[compatibleVersion]") but it's true that having a version
field encourages people to make breaking changes where before they might have
made forwards compatible ones.

I'm sensitive to the fact that this would be the first versioned thing in the
tracing format - but I think if we're ever going to version any part of the
trace format it would be this - the thing we're versioning is basically a wire
format, it has no semantic content, it's only describing a transformation from a
struct-of-arrays to an array-of-structs. Significantly we can modify the
'logical' heap dump format without bumping this version number but we can also
optimise how we send that data (by bumping the version number) without changing
the logical format as understood by the heap dump specific part of the importer.

...that's my 2c anyway ;P

[1]:
https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/e...
[2]:
https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/e...
[3]:
https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/e...

+
+    // Add "malloc", "blink_gc", etc. nodes, each containing a dump.
+    for (const auto& name_and_dump : process_memory_dump->heap_dumps()) {
+      heap_dump_value->SetValueWithCopiedName(name_and_dump.first.c_str(),
+                                              *name_and_dump.second);
+    }
+
+    // Add "maps" node.
+    if (auto* session_state = pmd_async_state->session_state.get()) {
+      heap_dump_value->BeginDictionary("maps");
+      if (auto* deduplicator = session_state->type_name_deduplicator()) {
+        heap_dump_value->BeginArray("types");
+        deduplicator->AppendIncrementally(&*heap_dump_value);
+        heap_dump_value->EndArray();
+      }
+      if (auto* deduplicator = session_state->stack_frame_deduplicator()) {
+        heap_dump_value->BeginArray("nodes");
+        deduplicator->AppendIncrementally(&*heap_dump_value);
+        heap_dump_value->EndArray();
+      }
+      heap_dump_value->EndDictionary();
+    }
+
+    std::unique_ptr<ConvertableToTraceFormat> values[kHeapDumpNumArgs] = {
+        std::move(heap_dump_value)};
+    TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID(
+        TRACE_EVENT_PHASE_SAMPLE,
+        TraceLog::GetCategoryGroupEnabled(kTraceCategory), "heap_profile",

[Primiano Tucci (use gerrit), 2017/02/17 17:07:05]

this is the only thing where I'm not fully sure. Why do we want to emit another
dump type if, at the end, this is generated synchronously with the memory-infra
dump and there are dependencies between the two?
I don't think this will make that much of a difference in terms of code reusing
in the importer.

[DmitrySkiba, 2017/02/23 07:17:19]

I think we discussed this exact concern several times. Last times Hector said
that this would simplify things.

[hjd, 2017/02/23 12:33:27]

As I mentioned before if we eventually want to share stack trace/type info with
V8 the a separate event is slightly neater in the tracing code (there would be a
single path for adding new stack trace/type info as opposed to two). It is also
a bit logically cleaner (a new event type as opposed to a new pseudo-event thing
which is stuffed into args). It isn't worth fighting over though, at least on
the importer side it is pretty easy to either.

+        trace_event_internal::kGlobalScope, dump_guid, pid, kHeapDumpNumArgs,
+        kHeapDumpArgNames, kHeapDumpArgTypes, nullptr /* arg_values */, values,
+        TRACE_EVENT_FLAG_HAS_ID);
+  }
+
   bool tracing_still_enabled;
   TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTraceCategory, &tracing_still_enabled);
   if (!tracing_still_enabled) {
@@ -783,18 +805,6 @@ void MemoryDumpManager::OnTraceLogEnabled() {
 
     session_state->SetTypeNameDeduplicator(
         WrapUnique(new TypeNameDeduplicator));
-
-    TRACE_EVENT_API_ADD_METADATA_EVENT(
-        TraceLog::GetCategoryGroupEnabled("__metadata"), "stackFrames",
-        "stackFrames",
-        MakeUnique<SessionStateConvertableProxy<StackFrameDeduplicator>>(
-            session_state, &MemoryDumpSessionState::stack_frame_deduplicator));
-
-    TRACE_EVENT_API_ADD_METADATA_EVENT(
-        TraceLog::GetCategoryGroupEnabled("__metadata"), "typeNames",
-        "typeNames",
-        MakeUnique<SessionStateConvertableProxy<TypeNameDeduplicator>>(
-            session_state, &MemoryDumpSessionState::type_name_deduplicator));
   }
 
   {