[tracing] Introduce support for userland memory allocators dumps.

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 8b2d930a4bc5ab1e09b0f4295910eff07d474a53..47669d66c611e4c4cc889ee2569889cc95b1a658 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -59,7 +59,8 @@ void MemoryDumpManager::SetInstanceForTesting(MemoryDumpManager* instance) {
   g_instance_for_testing = instance;
 }
 
-MemoryDumpManager::MemoryDumpManager() : memory_tracing_enabled_(0) {
+MemoryDumpManager::MemoryDumpManager()
+    : dump_provider_currently_active_(nullptr), memory_tracing_enabled_(0) {
 }
 
 MemoryDumpManager::~MemoryDumpManager() {
@@ -129,6 +130,7 @@ void MemoryDumpManager::CreateLocalDumpPoint(DumpPointType dump_point_type,
     for (auto it = dump_providers_enabled_.begin();
          it != dump_providers_enabled_.end();) {
       MemoryDumpProvider* dump_provider = *it;
+      dump_provider_currently_active_ = dump_provider;

[picksi, 2015/03/16 09:18:06]

Am I correct in thinking that dump_provider_currently_active_ is used for
plumbing data into the "AsValueInto" function? If so it feels a little
dangerous. Is there any way we can guarantee its correctness; e.g. pass this
pointer as a parameter or bake it into pmd as part of the constructor?

If this is the only solution, we might want to use this pointer instead of the
'copy' in dump_provider in this function.

[Primiano Tucci (use gerrit), 2015/03/16 09:57:17]

Yes, but I couldn't come up with a better alternative.
Hmm why? If you are dumping some provider must be active.

But, yes you are correct, PMD needs to access the dump provider only during the
DumpInto function. 
Yeah I thought about that, but that would add extra, useless, memory weight to
PMD. We would have one instance of PMD per each dump point. That would mean
adding an extra pointer. Besides the memory itself for the pointer, now it
becomes a memory ownership problem.

It is not safe to assume that the DumpProvider will outlive the PMD, as
DumpProviders can be unregistered dynamically at runtime. But then it means that
the DumpProvider needs to be be a refcounted object, which would add more memory
and runtime overhead.
 
 
Not sure I get this, which copy?

[picksi, 2015/03/16 10:25:04]

"It feels a little dangerous" to me because AsValueInto() is relying on
dump_provider_currently_active_ being correct without any guarantees that this
is the case. A future dev may refactor the code and fail to set
dump_provider_currently_active_ at all or to change its value. If this is a one
off problem it may not be worth the extra plumbing/code bloat that it would take
to "fix" it, but if, later on, it becomes a common use-pattern (setting up a
context before calling AsValueInto) we might want to re-investigate this.


Re: Which Copy

Current code is:
MemoryDumpProvider* dump_provider = *it;
dump_provider_currently_active_ = dump_provider;

I'm wondering about not having dump_provider as a local, instead using
dump_provider_currently_active_ directly, e.g.:

dump_provider_currently_active_ = *it;
if (dump_provider_currently_active_->DumpInto(pmd.get())) {

Not sure if I actually think this is a good idea any more :)

[Primiano Tucci (use gerrit), 2015/03/17 10:30:59]

I see the point. But on the other side of the tradeoff, this would mean adding
extra arguments to both MDP::DumpInto and recursively into
MemoryAllocatorDump::AsValueInto, which would make the classes look terribly
ugly.
Also, remember that this is required only until crbug.com/466121 get fixed.
In the long term, the MemoryAllocatorDump will not need to know anything about
the current dumper.
Good point, I added a test to the MemoryDumpManager to ascertain that it will
not happen.

If this is a one
Ah, I see, makes sense. Done

       if (dump_provider->DumpInto(pmd.get())) {
         did_any_provider_dump = true;
         ++it;
@@ -139,6 +141,7 @@ void MemoryDumpManager::CreateLocalDumpPoint(DumpPointType dump_point_type,
                       "with the --no-sandbox switch.";
         it = dump_providers_enabled_.erase(it);
       }
+      dump_provider_currently_active_ = nullptr;
     }
   }