[tracing] Simplify the MemoryAllocatorDump API

[tracing] Simplify the MemoryAllocatorDump API

Up until now the API surface of the MemoryAllocatorDump has been
strongly typed, enforcing three basic counters (physical_size,
allocated_objects_count and allocated_objects_size) + arbitrary
extra attributes.
This CL simplifies the MAD interface as follows:
- Remove the two argument (allocator_name, heap_name) in favor of
  a simpler and single argument absolute_name (e.g.,
  CreateAllocatorDump("v8/isolate1/heap2") instead of
  CreateAllocatorDump("v8", "isolate1/heap2"))
- Remove the storngly typed properties in favor of a generic
  Add(attribute_name, type, units, value) method.
- Add helper methods MAD.AddScalar() and AddString().
- Rewrite the existing dumpers to use the new simpler API.
- Fix the tests.

BUG=466141
Committed: https://crrev.com/0842511c459cb46834d49766dede5c7fc4cb0187
Cr-Commit-Position: refs/heads/master@{#327310}

[Primiano Tucci (use gerrit), 2015-04-27 22:41:02]

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

[Primiano Tucci (use gerrit), 2015-04-27 22:41:03]

PTAL, this is just removing files

This is part of a series of 3 CLs to cleanup the MDM/MAD interface in
preparation for the upcoming Blink plumbing.

1) crrev.com/1107093004 (Simplify MemoryDumpProvider interface)
2) crrev.com/1104413002 (Get rid of MemoryAllocatorAttributesTypeInfo)  
3*) crrev.com/1105143003 [THIS CL] (Simplify the MemoryAllocatorDump API)

[nduca, 2015-04-28 00:25:58]

lgtm

[Primiano Tucci (use gerrit), 2015-04-28 08:08:10]

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

[Primiano Tucci (use gerrit), 2015-04-28 08:08:11]

+petrcermak, can you plz do a final review pass?

[petrcermak, 2015-04-28 09:37:10]

LGTM with some food for thought :-)

Thanks,
Petr

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
File base/trace_event/memory_allocator_dump.cc (right):

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.cc:19: // Returns the c-string pointer
from a dictionary vaue without performing extra
typo: "vaue" -> "value"

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.cc:49: // The |absolute_name| can contain
slash separator, but not leading or
Since we're checking for initial and trailing slashes, maybe we should also
verify that there are no subsequent slashes (e.g. "a//b"). Alternatively, we
could consider them a single slash in Trace Viewer.

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
File base/trace_event/memory_allocator_dump.h (right):

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.h:24: MemoryAllocatorDump(const
std::string& absolute_name,
Are we calling it "absolute name"? We named the same thing "full name" in Trace
Viewer (which is probably easier to modify).

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.h:39: static const char kTypeScalar[];   
// For scalar attriute types.
nit: I find the comments ("// For XYZ") slightly odd. Why not just say "//
Scalar attribute type."?

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.h:42: static const char kUnitsObjects[]; 
// For scalar attributes (# objects).
nit: I don't think there should be a space between "#" and "objects".

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.h:52: bool Get(const std::string& name,
Do we need a Get method (apart from testing)?

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.h:58: void AddScalar(const std::string&
name, const char* units, uint64 value);
nit: Even though the signature fits on one line, I would personally break it
apart to look like the other methods.

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump.h:57: //       path-like string (e.g.,
v8/isolate1/heap1, v8/isolate1/heap2).
Since you DCHECK it, I would add a note saying "Initial and trailing slashes are
not allowed"

[Primiano Tucci (use gerrit), 2015-04-28 09:52:50]

Patchset #2 (id:20001) has been deleted

[Primiano Tucci (use gerrit), 2015-04-28 09:54:18]

Thanks.

+reveman, +sebmarchand FYI

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
File base/trace_event/memory_allocator_dump.cc (right):

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.cc:19: // Returns the c-string pointer
from a dictionary vaue without performing extra
On 2015/04/28 09:37:09, petrcermak wrote:
> typo: "vaue" -> "value"

Done.

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.cc:49: // The |absolute_name| can contain
slash separator, but not leading or
On 2015/04/28 09:37:09, petrcermak wrote:
> Since we're checking for initial and trailing slashes, maybe we should also
> verify that there are no subsequent slashes (e.g. "a//b"). Alternatively, we
> could consider them a single slash in Trace Viewer.

Yeah but then would become a bit tougher in the chromium code.
My feeling here is that leading and trailing are the most common mistakes (e.g.
somebody trying to do /v8, or v8/heap/) and are the one I want to catch in
debug. My hope is that stuff like "v8//heap" should sound odd enough to authors
to perceive that is wrong even if there isn't a check in place :)

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
File base/trace_event/memory_allocator_dump.h (right):

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.h:24: MemoryAllocatorDump(const
std::string& absolute_name,
On 2015/04/28 09:37:09, petrcermak wrote:
> Are we calling it "absolute name"? We named the same thing "full name" in
Trace
> Viewer (which is probably easier to modify).

Yeah we had some long discussion with Nat about this. absolute is as in
"absolute path" here.

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.h:39: static const char kTypeScalar[];   
// For scalar attriute types.
On 2015/04/28 09:37:09, petrcermak wrote:
> nit: I find the comments ("// For XYZ") slightly odd. Why not just say "//
> Scalar attribute type."?

Reworded

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.h:42: static const char kUnitsObjects[]; 
// For scalar attributes (# objects).
On 2015/04/28 09:37:10, petrcermak wrote:
> nit: I don't think there should be a space between "#" and "objects".

Done.

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.h:52: bool Get(const std::string& name,
On 2015/04/28 09:37:09, petrcermak wrote:
> Do we need a Get method (apart from testing)?

I need it mainly for testing.
We never know if at some point allocators will need to sneak into what they (or
a parent) dumped.

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/memory_all...
base/trace_event/memory_allocator_dump.h:58: void AddScalar(const std::string&
name, const char* units, uint64 value);
On 2015/04/28 09:37:09, petrcermak wrote:
> nit: Even though the signature fits on one line, I would personally break it
> apart to look like the other methods.

Hmm my policy here is "git cl format is always right". If you don't like it file
a git cl format bug
I can fix it, but the next time I'll run git cl format that will be switched
back to this.

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/1105143003/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump.h:57: //       path-like string (e.g.,
v8/isolate1/heap1, v8/isolate1/heap2).
On 2015/04/28 09:37:10, petrcermak wrote:
> Since you DCHECK it, I would add a note saying "Initial and trailing slashes
are
> not allowed"

Done.

[Primiano Tucci (use gerrit), 2015-04-28 13:05:09]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-04-28 13:05:09]

The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org,
petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/1105143003/#ps40001
(title: "petrcermak's nits")

[commit-bot: I haz the power, 2015-04-28 13:05:18]

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

[commit-bot: I haz the power, 2015-04-28 13:07:36]

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

[commit-bot: I haz the power, 2015-04-28 13:07:37]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Primiano Tucci (use gerrit), 2015-04-28 13:56:43]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-04-28 13:56:43]

The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org,
petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/1105143003/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-28 13:56:52]

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

[commit-bot: I haz the power, 2015-04-28 14:40:02]

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

[commit-bot: I haz the power, 2015-04-28 14:40:03]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Primiano Tucci (use gerrit), 2015-04-28 14:47:57]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-04-28 14:47:58]

The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org,
petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/1105143003/#ps80001
(title: "Fix Win build")

[commit-bot: I haz the power, 2015-04-28 14:48:11]

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

[commit-bot: I haz the power, 2015-04-28 17:10:47]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-04-28 17:12:14]

Patchset 4 (id:??) landed as
https://crrev.com/0842511c459cb46834d49766dede5c7fc4cb0187
Cr-Commit-Position: refs/heads/master@{#327310}