[tracing] Enable actual generation of memory dump points.

base/trace_event/memory_dump_manager.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/memory_dump_manager.h"
 
 #include <algorithm>
 
 #include "base/compiler_specific.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event_argument.h"
 
 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
 // trace event synthetic delays.
 const char MemoryDumpManager::kTraceCategory[] =
     TRACE_DISABLED_BY_DEFAULT("memory-dumps");
 
 // static
 MemoryDumpManager* MemoryDumpManager::GetInstance() {
   if (g_instance_for_testing)
@@ skipping 46 matching lines @@
   if (it != dump_providers_enabled_.end())
     dump_providers_enabled_.erase(it);
 }
 
 void MemoryDumpManager::RequestDumpPoint(DumpPointType type) {
   // TODO(primiano): this will have more logic, IPC broadcast & co.
   // Bail out immediately if tracing is not enabled at all.
   if (!UNLIKELY(subtle::NoBarrier_Load(&memory_tracing_enabled_)))
     return;
 
-  CreateLocalDumpPoint();
+  CreateLocalDumpPoint(type);
 }
 
 void MemoryDumpManager::BroadcastDumpRequest() {
   NOTREACHED();  // TODO(primiano): implement IPC synchronization.
 }
 
 // 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());
 
   for (auto it = dump_providers_enabled_.begin();
        it != dump_providers_enabled_.end();) {
     MemoryDumpProvider* dump_provider = *it;
     if (!dump_provider->DumpInto(pmd.get())) {

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

Not part of this CL, but it would read easier if you removed the '!' and swapped
the if/else bodies around.

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

Makes sense. Done

       LOG(ERROR) << "The memory dumper " << dump_provider->GetFriendlyName()
                  << " failed, possibly due to sandboxing (crbug.com/461788), "
                     "disabling it for current process. Try restarting chrome "
                     "with the --no-sandbox switch.";
       it = dump_providers_enabled_.erase(it);
     } else {
       did_any_provider_dump = true;
       ++it;
     }
   }
 
   // Don't create a dump point if all the dumpers failed.
   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() {
   // TODO(primiano): at this point we query  TraceLog::GetCurrentCategoryFilter
   // to figure out (and cache) which dumpers should be enabled or not.
   // For the moment piggy back everything on the generic "memory" category.
   bool enabled;
   TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTraceCategory, &enabled);
 
   AutoLock lock(lock_);
   if (enabled) {
     dump_providers_enabled_.assign(dump_providers_registered_.begin(),
                                    dump_providers_registered_.end());
   } else {
     dump_providers_enabled_.clear();
   }
   subtle::NoBarrier_Store(&memory_tracing_enabled_, 1);
 }
 
 void MemoryDumpManager::OnTraceLogDisabled() {
   AutoLock lock(lock_);
   dump_providers_enabled_.clear();
   subtle::NoBarrier_Store(&memory_tracing_enabled_, 0);
 }
 
 }  // namespace trace_event
 }  // namespace base