[tracing] Introduce support for userland memory allocators dumps.

[tracing] Introduce support for userland memory allocators dumps.

This introduces the MemoryAllocatorDump which defines the ground data
model that every allocator MemoryDumpProvider must comply with in order
to get a coherent snapshot of the various allocators inside the trace.
Furthermore, MemoryDumpProvider(s) can specify further attributes that
will be attached to each dump point using the MemoryAllocatorAttributes
map.

BUG=466141
Committed: https://crrev.com/13cf4d070f757ce190f49c2ed736518f35ee410b
Cr-Commit-Position: refs/heads/master@{#321368}

[Primiano Tucci (use gerrit), 2015-03-11 15:51:29]

Patchset #5 (id:80001) has been deleted

[Primiano Tucci (use gerrit), 2015-03-11 15:59:56]

primiano@chromium.org changed reviewers:
+ nduca@chromium.org

[nduca, 2015-03-12 04:04:10]

lgtm awesome

https://codereview.chromium.org/987043003/diff/100001/base/trace_event/memory...
File base/trace_event/memory_allocator_dump.h (right):

https://codereview.chromium.org/987043003/diff/100001/base/trace_event/memory...
base/trace_event/memory_allocator_dump.h:45: const DictionaryValue&
extra_attributes() const { return extra_attributes_; }
i wonder if there's a way to do this that would be less heavy? E.g.
set_extra_attribute(name, Convertible*)?

https://codereview.chromium.org/987043003/diff/100001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/987043003/diff/100001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:52: MemoryAllocatorDump*
AddAllocatorDump(const std::string& name);
Add->Create?

[Primiano Tucci (use gerrit), 2015-03-13 19:39:08]

https://codereview.chromium.org/987043003/diff/100001/base/trace_event/memory...
File base/trace_event/memory_allocator_dump.h (right):

https://codereview.chromium.org/987043003/diff/100001/base/trace_event/memory...
base/trace_event/memory_allocator_dump.h:45: const DictionaryValue&
extra_attributes() const { return extra_attributes_; }
On 2015/03/12 04:04:10, nduca wrote:
> i wonder if there's a way to do this that would be less heavy? E.g.
> set_extra_attribute(name, Convertible*)?

Ok, I simplified a lot the extra_attributes logic.
Convertible is IMHO not a really user friendly option (needs to instantiate a
wrapper).
I'd be much more in favor of adding explicit setter getters like this. It's a
bit more work, but they have the great advantage of hiding the memory ownership
problem (the caller doesn't have to allocate anything).
This means that if tomorrow we hit performance / profiling heisenberg problems
we keep the control of the memory management inside the base/ classes, rather
than pushing the burden to the caller. 
WDYT?

https://codereview.chromium.org/987043003/diff/100001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/987043003/diff/100001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:52: MemoryAllocatorDump*
AddAllocatorDump(const std::string& name);
On 2015/03/12 04:04:10, nduca wrote:
> Add->Create?

Done.

[picksi, 2015-03-16 09:18:05]

picksi@google.com changed reviewers:
+ picksi@google.com

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

Looks good! One question about dump_provider_currently_active...

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
File base/trace_event/memory_allocator_dump.cc (right):

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
base/trace_event/memory_allocator_dump.cc:55: mdp->allocator_attributes();
mdp can bull a nullptr if this is called when a dump isn't going on. Should you
DCHECK this?

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:133: dump_provider_currently_active_ =
dump_provider;
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:18]

Thanks for the comments

+skyostil might have some insightful  opinion on picksi's concern about
dump_provider_currently_active_?

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
File base/trace_event/memory_allocator_dump.cc (right):

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
base/trace_event/memory_allocator_dump.cc:55: mdp->allocator_attributes();
On 2015/03/16 09:18:06, picksi wrote:
> mdp can bull a nullptr if this is called when a dump isn't going on. Should
you
> DCHECK this?

Usually the rule in chromium is to NOT dcheck for null pointer dereference.
Rationale: if you deref a null ptr, you will get a segfault and the stack trace
will point at that line. DCHECK is not going to add any extra useful behavior,
and just add more code clutter.
It would be different, though, if you want to doublecheck some consistency
(e.g., "If I am in state A, I expect this pointer, which I am not using here, to
be null or non-null"), but this is not the case here.

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:133: dump_provider_currently_active_ =
dump_provider;
On 2015/03/16 09:18:06, picksi wrote:
> Am I correct in thinking that dump_provider_currently_active_ is used for
> plumbing data into the "AsValueInto" function?
Yes, but I couldn't come up with a better alternative.

> If so it feels a little dangerous.
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. 

> 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?

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.
 
 
> If this is the only solution, we might want to use this pointer instead of the
> 'copy' in dump_provider in this function.
Not sure I get this, which copy?

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

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:133: dump_provider_currently_active_ =
dump_provider;
"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-16 16:09:10]

primiano@chromium.org changed reviewers:
+ dsinclair@chromium.org

[Primiano Tucci (use gerrit), 2015-03-16 16:09:10]

+dsinclair. Take it as a warming "welcome back":)

[dsinclair, 2015-03-16 16:13:00]

lgtm

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
File base/trace_event/memory_allocator_attributes.h (right):

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
base/trace_event/memory_allocator_attributes.h:19: // refer to
src/tools/perf/unit-info.json for the semantic of the type.
nit: Refer, capitalize the first letter of the sentence.

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

Patchset #8 (id:150001) has been deleted

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

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
File base/trace_event/memory_allocator_attributes.h (right):

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
base/trace_event/memory_allocator_attributes.h:19: // refer to
src/tools/perf/unit-info.json for the semantic of the type.
On 2015/03/16 16:13:00, dsinclair wrote:
> nit: Refer, capitalize the first letter of the sentence.

Done.

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/987043003/diff/120002/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:133: dump_provider_currently_active_ =
dump_provider;
On 2015/03/16 10:25:04, picksi wrote:
> "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.

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.

> A future dev may refactor the code and fail to set
> dump_provider_currently_active_ at all or to change its value. 
Good point, I added a test to the MemoryDumpManager to ascertain that it will
not happen.

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;

Ah, I see, makes sense. Done

[Primiano Tucci (use gerrit), 2015-03-17 23:11:50]

primiano@chromium.org changed reviewers:
+ danakj@chromium.org

[Primiano Tucci (use gerrit), 2015-03-17 23:11:51]

+danakj for build files OWNERS stamp... until crrev.com/1014923002 gets its
green lights :-)

[danakj, 2015-03-17 23:25:34]

LGTM

[Primiano Tucci (use gerrit), 2015-03-19 12:22:44]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-03-19 12:22:44]

The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org,
dsinclair@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/987043003/#ps190001
(title: "Rebase + use hex strings")

[commit-bot: I haz the power, 2015-03-19 12:23:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987043003/190001

[commit-bot: I haz the power, 2015-03-19 12:28:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-19 12:28:24]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Primiano Tucci (use gerrit), 2015-03-19 12:40:50]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-03-19 12:40:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, nduca@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/987043003/#ps210001
(title: "Whitespaces seem to be a thing for ios_dbg_simulator_ninja")

[commit-bot: I haz the power, 2015-03-19 12:41:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987043003/210001

[commit-bot: I haz the power, 2015-03-19 12:46:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-19 12:46:34]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Primiano Tucci (use gerrit), 2015-03-19 13:21:44]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-03-19 13:21:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, nduca@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/987043003/#ps230001
(title: "Fix death test")

[Primiano Tucci (use gerrit), 2015-03-19 13:21:49]

Patchset #10 (id:210001) has been deleted

[commit-bot: I haz the power, 2015-03-19 13:21:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987043003/230001

[commit-bot: I haz the power, 2015-03-19 13:26:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-19 13:26:19]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Primiano Tucci (use gerrit), 2015-03-19 15:13:04]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-03-19 15:13:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, nduca@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/987043003/#ps250001
(title: "Fix DEATH test not supported on android")

[commit-bot: I haz the power, 2015-03-19 15:15:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987043003/250001

[commit-bot: I haz the power, 2015-03-19 16:24:44]

Committed patchset #11 (id:250001)

[commit-bot: I haz the power, 2015-03-19 16:25:18]

Patchset 11 (id:??) landed as
https://crrev.com/13cf4d070f757ce190f49c2ed736518f35ee410b
Cr-Commit-Position: refs/heads/master@{#321368}