[tracing] Enable actual generation of memory dump points.

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 3430169630f6e172a0ed1417f30b3c50e43a9731..83584c53104591291ec86a79c58043616188be13 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -15,7 +15,27 @@ namespace base {
 namespace trace_event {
 
 namespace {
+
 MemoryDumpManager* g_instance_for_testing = nullptr;
+const int kTraceEventNumArgs = 1;
+const char* kTraceEventArgNames[] = {"dumps"};
+const unsigned char kTraceEventArgTypes[] = {TRACE_VALUE_TYPE_CONVERTABLE};
+uint64 g_next_guid = 0;
+
+const char* DumpPointTypeToString(const DumpPointType& type) {
+  switch (type) {
+    case DumpPointType::TASK_BEGIN:
+      return "TASK_BEGIN";
+    case DumpPointType::TASK_END:
+      return "TASK_END";
+    case DumpPointType::PERIODIC_INTERVAL:
+      return "PERIODIC_INTERVAL";
+    case DumpPointType::EXPLICITLY_TRIGGERED:
+      return "EXPLICITLY_TRIGGERED";
+  }
+  NOTREACHED();
+  return "UNKNOWN";
+}
 }
 
 // TODO(primiano): this should be smarter and should do something similar to
@@ -82,7 +102,7 @@ void MemoryDumpManager::RequestDumpPoint(DumpPointType type) {
   if (!UNLIKELY(subtle::NoBarrier_Load(&memory_tracing_enabled_)))
     return;
 
-  CreateLocalDumpPoint();
+  CreateLocalDumpPoint(type);
 }
 
 void MemoryDumpManager::BroadcastDumpRequest() {
@@ -90,7 +110,7 @@ void MemoryDumpManager::BroadcastDumpRequest() {
 }
 
 // Creates a dump point for the current process and appends it to the trace.
-void MemoryDumpManager::CreateLocalDumpPoint() {
+void MemoryDumpManager::CreateLocalDumpPoint(DumpPointType type) {
   AutoLock lock(lock_);
   bool did_any_provider_dump = false;
   scoped_ptr<ProcessMemoryDump> pmd(new ProcessMemoryDump());
@@ -114,9 +134,16 @@ void MemoryDumpManager::CreateLocalDumpPoint() {
   if (!did_any_provider_dump)
     return;
 

[picksi, 2015/02/26 17:58:04]

[Without thinking too deeply about this...] Does the remainder of this function
need to be inside the lock? (if g_next_guid was atomically incremented, I
suppose?) Are TRACE_EVENTS thread safe?

[Primiano Tucci (use gerrit), 2015/03/01 11:32:28]

Done. Turns out I needed to change the g_next_guid logic anyways.
My understanding is that TRACE_EVENTs use thread-local buffers so should be
thread safe.

-  scoped_refptr<TracedValue> value(new TracedValue());
-  pmd->AsValueInto(value.get());
-  // TODO(primiano): add the dump point to the trace at this point.
+  scoped_refptr<ConvertableToTraceFormat> event_value(new TracedValue());
+  pmd->AsValueInto(static_cast<TracedValue*>(event_value.get()));
+  const char* const event_name = DumpPointTypeToString(type);
+  const uint64 event_guid = ++g_next_guid;

[nduca, 2015/02/26 17:26:29]

i think you need TRACE_ID_DONT_MANGLE -- see trace_event.h macro explanation

[Primiano Tucci (use gerrit), 2015/03/01 11:32:28]

Right.
FTR: the rationale here is that we don't want tracing to mangle the id, because
we need to read it back to pass it to the other processes via IPC.
Removing the mangling logic and adding a TODO + bug (crbug.com/462931) to track
this (a longer description of the problem is avilable in the bug). Will do in a
separate CL.


My understanding is that TRACE_ID_DONT_MANGLE is not needed in this case (that
is for the higher level TRACE_EVENT_ macros not when using
TRACE_EVENT_API_ADD_TRACE_EVENT). I just need to remove the
TRACE_EVENT_FLAG_MANGLE_ID flag I added.

At the end all boils down to having or not the TRACE_EVENT_FLAG_MANGLE_ID flag
in the event).
In other words:
TRACE_ID_DONT_MANGLE -> TraceID::DontMangle -> Noop
TRACE_ID_MANGLe -> TraceID::ForceMangle  -> Sets the TRACE_EVENT_FLAG_MANGLE_ID
flag.

+
+  TRACE_EVENT_API_ADD_TRACE_EVENT(

[dsinclair, 2015/02/26 17:54:54]

I'm not sure if want to leak this out of trace_event.h or not, it's an
implementation detail.

Would it be better to add an macro?

TRACE_EVENT_MEMORY_DUMP(kTraceCategory, event_name, event_guid, pmd) 

Or something?

[Primiano Tucci (use gerrit), 2015/03/01 11:32:28]

Oh, I though that was meant to be part of the public API as it was in
trace_event.h (vs _impl), anyways memory_dump_manager is part of trace_event
(read: not sure if it should be considered a tracing "client" or an
implementation detail of tracing).
(Note, there is one precedent for TRACE_EVENT_API_ADD_TRACE_EVENT in
ui/gl/gl_implementation_win.cc)
I had this discussion with skyostil@ when writing this.
The line of reasoning of our discussion was:
This should be the only point in the memory tracing code which creates actual
trace events. In other words, I don't expect to see any other
TRACE_EVENT_API_ADD_TRACE_EVENT(..'v'..) anywhere else in the memory tracing
code. All the future code should ultimately reach into the memory dump manager.
In this optic, it felt more maintainable using an existing API rather than
creating a macro which is going to be used only once in the codebase.

In summary:
My personal preference would be macroizing this only the day that we'd need a
2nd call to this, and keep as it is until that point. WDYT?

+      TRACE_EVENT_PHASE_MEMORY_DUMP,
+      TraceLog::GetCategoryGroupEnabled(kTraceCategory), event_name, event_guid,
+      kTraceEventNumArgs, kTraceEventArgNames, kTraceEventArgTypes, NULL,

[picksi, 2015/02/26 17:58:04]

It would make this code clearer if the NULL was a named const saying what is
was.

[Primiano Tucci (use gerrit), 2015/03/01 11:32:28]

Makes sense making it explicit. IIRC in chromium we tend to avoid dummy consts
in favor of this pattern:
NULL /* arg_values */

Done

+      &event_value, TRACE_EVENT_FLAG_HAS_ID | TRACE_EVENT_FLAG_MANGLE_ID);
 }
 
 void MemoryDumpManager::OnTraceLogEnabled() {