[Tracing] Make heap profiler type info a string

[Tracing] Make heap profiler type info a string

Instead of keeping a 16-bit "type ID" to do type profiling, the heap
profiler will just keep a |const char*| with the type name. The type
names are deduplicated at dump time, so in the trace log heap dumps
still reference types by ID, and there is one dictionary at the end that
maps type IDs to their names.

Internally, a null pointer type name is used to indicate "unknown type".
It is dumped in the trace as type name "[unknown]", so it requires no
special treatment from the UI. (Contrary to previous intent.) The
mapping dictionary follows the same convention as the current
"stackFrames" dictionary: it is one metadata event per process, with
name "typeNames". The args of this event contain a dictionary that maps
type IDs to their name.

This is part of the heap profiler in chrome://tracing.

BUG=524631
Committed: https://crrev.com/4adda6fd325178b3d3704c8fe1a8a842df7873bd
Cr-Commit-Position: refs/heads/master@{#361660}

[Ruud van Asseldonk, 2015-11-20 15:36:35]

Description was changed from

==========
[Tracing] Make heap profiler type info a string

Instead of keeping a 16-bit "type ID" to do type profiling, the heap
profiler will just keep a |const char*| with the type name. The type
names are deduplicated at dump time, so in the trace log heap dumps
still reference types by ID, and there is one dictionary at the end that
maps type IDs to their names.

Internally, a null pointer type name is used to indicate "unknown type".
It is dumped in the trace as type name "[unknown]", so it requires no
special treatment from the UI. (Contrary to previous intent.) The
mapping dictionary follows the same convention as the current
"stackFrames" dictionary: it is one metadata event per process, with
name "typeNames". The args of this event contain a dictionary that maps
type IDs to their name.

BUG=524631
==========

to

==========
[Tracing] Make heap profiler type info a string

Instead of keeping a 16-bit "type ID" to do type profiling, the heap
profiler will just keep a |const char*| with the type name. The type
names are deduplicated at dump time, so in the trace log heap dumps
still reference types by ID, and there is one dictionary at the end that
maps type IDs to their names.

Internally, a null pointer type name is used to indicate "unknown type".
It is dumped in the trace as type name "[unknown]", so it requires no
special treatment from the UI. (Contrary to previous intent.) The
mapping dictionary follows the same convention as the current
"stackFrames" dictionary: it is one metadata event per process, with
name "typeNames". The args of this event contain a dictionary that maps
type IDs to their name.

This is part of the heap profiler in chrome://tracing.

BUG=524631
==========

[Ruud van Asseldonk, 2015-11-20 15:37:58]

ruuda@google.com changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-11-23 15:46:48]

Can you move the Demangler part to another CL? I need some extra thinking there
(and probably to loop in somebody). I think as -is this might hit an assert on
Android and break attempts to use the heap profiler (due to
cxa_demangle_stub.cc)

If you remove the Demangler part (just dump all the full string for the moment)
this CL LGTM with few nits.

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_allocation_context.h (right):

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_allocation_context.h:60: // Type name of the type
stored in the allocated memory, as returned by
I'd remove the "as returned by typeid(T)" as it is a bit confusing (w.r.t
production builds that will have manual annotation)

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_allocation_context.h:65: const char* type_name;
in a follow up CL we might just hash this as well and make it work nicely. Not
100% sure in production builds identical strings are deduped by the linker

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_heap_dump_writer.cc (right):

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_heap_dump_writer.cc:120: // Format the type ID as
a string.
Don't you want to do something similar here for the "unknonwn type" -> "" ?

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_heap_dump_writer_unittest.cc (right):

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:116: int type_int =
tn_deduplicator->Insert(kInt);
s/type_/type_id_/ here and below

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_type_name_deduplicator.cc (right):

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_type_name_deduplicator.cc:112:
SStringPrintf(&stringify_buffer, ",\"%d\":\"%s\"", it->second, type_name);
use EscapeJSONString ?
I can already see the day somebody will use the manual annotation to do
something like:
SET_TYPE(this, "Array[\"string\"]")
Also make sure you cover this in the test

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_type_name_deduplicator.h (right):

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_type_name_deduplicator.h:37: int next_id_;
Can you use just type_ids_.count() as counter?

[Ruud van Asseldonk, 2015-11-23 18:23:26]

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_allocation_context.h (right):

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_allocation_context.h:60: // Type name of the type
stored in the allocated memory, as returned by
On 2015/11/23 15:46:48, Primiano Tucci wrote:
> I'd remove the "as returned by typeid(T)" as it is a bit confusing (w.r.t
> production builds that will have manual annotation)

Done.

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_allocation_context.h:65: const char* type_name;
On 2015/11/23 15:46:47, Primiano Tucci wrote:
> in a follow up CL we might just hash this as well and make it work nicely. Not
> 100% sure in production builds identical strings are deduped by the linker

We could if it turns out to be an issue, but my experience with
PartitionAlloc/Blink so far is that this is not a problem in practice.

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_heap_dump_writer.cc (right):

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_heap_dump_writer.cc:120: // Format the type ID as
a string.
On 2015/11/23 15:46:48, Primiano Tucci wrote:
> Don't you want to do something similar here for the "unknonwn type" -> "" ?

At first I did, but now I think that it is better to not treat "unknown type"
specially. We can give it type id "0", and write "0": "[unknown]" in the
typeNames dictionary. There is no need to special-case it in the trace, but it
is still possible if that is desired, because the ID is guaranteed to be "0".

We could make the ID "" instead of "0", but I think that would unnecessarily
complicate things.

For backtrace the situation was a bit different, because "empty backtrace"
cannot be represented in the stackFrames dictionary. "unknown type" _can_ be
represented.

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_heap_dump_writer_unittest.cc (right):

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:116: int type_int =
tn_deduplicator->Insert(kInt);
On 2015/11/23 15:46:48, Primiano Tucci wrote:
> s/type_/type_id_/ here and below

Done.

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_type_name_deduplicator.cc (right):

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_type_name_deduplicator.cc:112:
SStringPrintf(&stringify_buffer, ",\"%d\":\"%s\"", it->second, type_name);
On 2015/11/23 15:46:48, Primiano Tucci wrote:
> use EscapeJSONString ?
> I can already see the day somebody will use the manual annotation to do
> something like:
> SET_TYPE(this, "Array[\"string\"]")
> Also make sure you cover this in the test

Done.

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_type_name_deduplicator.h (right):

https://codereview.chromium.org/1467453003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_type_name_deduplicator.h:37: int next_id_;
On 2015/11/23 15:46:48, Primiano Tucci wrote:
> Can you use just type_ids_.count() as counter?

Done.

[Ruud van Asseldonk, 2015-11-23 18:25:16]

ruuda@google.com changed reviewers:
+ pfeldman@chromium.org

[Ruud van Asseldonk, 2015-11-23 18:25:16]

+pfeldman, can you please take a look at this one-line change in content/child?

[Primiano Tucci (use gerrit), 2015-11-24 10:58:13]

primiano@chromium.org changed reviewers:
+ jochen@chromium.org
- pfeldman@chromium.org

[Primiano Tucci (use gerrit), 2015-11-24 10:58:14]

pfeldman -> jochen for one line change in
	content/child/web_memory_dump_provider_adapter.cc

Nothing personal with Pavel, just Jochen is in a better timezone :)

[pfeldman, 2015-11-24 23:34:44]

I cans still lgtm it.

[Primiano Tucci (use gerrit), 2015-11-24 23:38:02]

On 2015/11/24 23:34:44, pfeldman wrote:
> I cans still lgtm it.

Nevermind the better timezone thing. Thanks :)

[jochen (gone - plz use gerrit), 2015-11-25 08:40:37]

jochen@chromium.org changed reviewers:
+ pfeldman@chromium.org

[jochen (gone - plz use gerrit), 2015-11-25 08:40:38]

lgtm

[Ruud van Asseldonk, 2015-11-25 08:56:59]

The CQ bit was checked by ruuda@google.com

[Ruud van Asseldonk, 2015-11-25 08:57:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1467453003/#ps40001
(title: "Remove demangler for now")

[commit-bot: I haz the power, 2015-11-25 08:57:06]

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

[commit-bot: I haz the power, 2015-11-25 08:59:18]

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

[commit-bot: I haz the power, 2015-11-25 08:59: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...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ruud van Asseldonk, 2015-11-25 10:23:21]

The CQ bit was checked by ruuda@google.com

[Ruud van Asseldonk, 2015-11-25 10:23:21]

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

[commit-bot: I haz the power, 2015-11-25 10:23:54]

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

[commit-bot: I haz the power, 2015-11-25 12:20:53]

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

[commit-bot: I haz the power, 2015-11-25 12:20:54]

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

[Ruud van Asseldonk, 2015-11-25 12:28:37]

The CQ bit was checked by ruuda@google.com

[commit-bot: I haz the power, 2015-11-25 12:29:17]

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

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-25 15:20:42]

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