Heap Profiler: Add trace category group names as type names for allocations

Heap Profiler: Add trace category group names as type names for allocations

The category for allocations are decided using the file that posted
current task. But, this does not work always since sometimes the code
is invoked by event listeners and tasks are not posted. To work around
this issue, the category name from trace events is marked as the
category for allocations as fallback.

BUG=594803
Committed: https://crrev.com/ee345086542ba00bae935fcfb13780ac5bdb63f1
Cr-Commit-Position: refs/heads/master@{#416873}

[ssid, 2016-08-23 22:50:25]

Patchset #1 (id:1) has been deleted

[ssid, 2016-08-23 22:56:00]

Patchset #1 (id:20001) has been deleted

[ssid, 2016-08-23 22:58:04]

Patchset #1 (id:40001) has been deleted

[ssid, 2016-08-23 23:03:59]

Patchset #1 (id:60001) has been deleted

[ssid, 2016-08-23 23:06:13]

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

[ssid, 2016-08-23 23:06:13]

PTAL, thanks.

[Primiano Tucci (use gerrit), 2016-08-25 16:31:15]

not a huge fan of having to do a strchr on every trace_event.
question: at this point why not using always the category?
Feels a bit confusing that some allocations will get the task name, and some
others the tracing category name.

https://codereview.chromium.org/2272843002/diff/100001/base/trace_event/heap_...
File base/trace_event/heap_profiler_allocation_context_tracker.cc (right):

https://codereview.chromium.org/2272843002/diff/100001/base/trace_event/heap_...
base/trace_event/heap_profiler_allocation_context_tracker.cc:112: const char*
category,
s/category/trace_event_category/ for consistency with trace_event_name below

https://codereview.chromium.org/2272843002/diff/100001/base/trace_event/heap_...
base/trace_event/heap_profiler_allocation_context_tracker.cc:122: if
(strchr(category, ',') == nullptr &&
Can we do this check in the UI? This is going to slow down every trace event
filtered.
Also, isn't this going to make the trace_categories_ stack out of sync?

[ssid, 2016-08-25 19:05:39]

On 2016/08/25 16:31:15, Primiano Tucci wrote:
> not a huge fan of having to do a strchr on every trace_event.
> question: at this point why not using always the category?
> Feels a bit confusing that some allocations will get the task name, and some
> others the tracing category name.

Just using the trace categories does not seem very accurate. There are lots of
cases which just shows toplevel. Then there are categories like benchmark and
browser, which do not make much sense like the folder name does. But this is
mainly useful in cases of "IPC" messages and "net" since that accounts for most
of the unknown in the categories. We would still see some not useful categories,
but majority of them are useful.
In cases of common categories it is not confusing to see folder names and trace
categories together because the trace categories are mostly namespaces like ipc,
net, blink, sync, etc. This does not look very different from folder names like:
net/sdpy, ipc/.., or webkit/dom. So, I think it is okay to have them together.

The other way could be to have the category as default and when we see toplevel,
then we know that we are in a task and use the task context. But, then there are
other cases like "browser" and categories with ",".

https://codereview.chromium.org/2272843002/diff/100001/base/trace_event/heap_...
File base/trace_event/heap_profiler_allocation_context_tracker.cc (right):

https://codereview.chromium.org/2272843002/diff/100001/base/trace_event/heap_...
base/trace_event/heap_profiler_allocation_context_tracker.cc:112: const char*
category,
On 2016/08/25 16:31:15, Primiano Tucci wrote:
> s/category/trace_event_category/ for consistency with trace_event_name below

I just wanted the declaration in header file to fit in one line :P now it is 2.

https://codereview.chromium.org/2272843002/diff/100001/base/trace_event/heap_...
base/trace_event/heap_profiler_allocation_context_tracker.cc:122: if
(strchr(category, ',') == nullptr &&
On 2016/08/25 16:31:15, Primiano Tucci wrote:
> Can we do this check in the UI? This is going to slow down every trace event
> filtered.
> Also, isn't this going to make the trace_categories_ stack out of sync?

Yess, that is the reason I have a separate stack for categories :P instead of
making the "pseudo_stack_" a vector of structs.
Ideally the check can actually be done in deduplicator itself instead of UI. The
reason I avoided doing this is because the allocations if they get a category
that has ",", then it is difficult to choose which one to take. If I ignore the
categories that have "," then we would still be left with some [unknown]. This
is better because the category is taken from the previous event in stack.

The other option would be to just take the first category if there are 2. Only
issue is we have both kinds of trace events like ones with "browser,startup" and
"startup,browser". So, strchr seems like a better option.

I have one other complex idea of having an array of boolean and the TraceLog
sends us the category_index aklong with category name. Using this the check for
"," can be done only once and every time we can read the array.

[ssid, 2016-08-30 19:17:23]

Patchset #4 (id:140001) has been deleted

[ssid, 2016-08-30 19:23:54]

Patchset #4 (id:160001) has been deleted

[ssid, 2016-08-30 19:58:35]

So, I removed the strchr calls at trace events and added to deduplicator. ptal,
thanks.

[Primiano Tucci (use gerrit), 2016-09-05 18:48:26]

Sorry for the delay. LGTM % 1 comment, plz look at that.

https://codereview.chromium.org/2272843002/diff/180001/base/trace_event/heap_...
File base/trace_event/heap_profiler_type_name_deduplicator.cc (right):

https://codereview.chromium.org/2272843002/diff/180001/base/trace_event/heap_...
base/trace_event/heap_profiler_type_name_deduplicator.cc:36:
SplitStringPiece(result, ",", KEEP_WHITESPACE, SPLIT_WANT_NONEMPTY);
This (splitstring) is really overkill.
Here you want just:
result = result. substr(0, result_of_find_dot);
or .remove_suffix(length - result_of_find_dot)
or something light like that.

[ssid, 2016-09-06 22:28:58]

The CQ bit was checked by ssid@chromium.org to run a CQ dry run

[ssid, 2016-09-06 22:29:17]

fixed, thanks.

https://codereview.chromium.org/2272843002/diff/180001/base/trace_event/heap_...
File base/trace_event/heap_profiler_type_name_deduplicator.cc (right):

https://codereview.chromium.org/2272843002/diff/180001/base/trace_event/heap_...
base/trace_event/heap_profiler_type_name_deduplicator.cc:36:
SplitStringPiece(result, ",", KEEP_WHITESPACE, SPLIT_WANT_NONEMPTY);
On 2016/09/05 18:48:26, Primiano Tucci wrote:
> This (splitstring) is really overkill.
> Here you want just:
> result = result. substr(0, result_of_find_dot);
> or .remove_suffix(length - result_of_find_dot)
> or something light like that.

Done.

[commit-bot: I haz the power, 2016-09-06 22:29:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 23:12:25]

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

[commit-bot: I haz the power, 2016-09-06 23:12:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ssid, 2016-09-06 23:20:09]

Patchset #5 (id:200001) has been deleted

[ssid, 2016-09-06 23:20:21]

The CQ bit was checked by ssid@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 23:20:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 00:14:31]

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

[commit-bot: I haz the power, 2016-09-07 00:14:32]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ssid, 2016-09-07 04:37:28]

Patchset #5 (id:220001) has been deleted

[ssid, 2016-09-07 04:37:35]

The CQ bit was checked by ssid@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 04:37:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 07:14:55]

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

[commit-bot: I haz the power, 2016-09-07 07:14:56]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-09-07 07:24:24]

The CQ bit was checked by ssid@chromium.org

[ssid, 2016-09-07 07:24:25]

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/2272843002/#ps240001
(title: "use substr and build fix.")

[commit-bot: I haz the power, 2016-09-07 07:24:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 07:29:08]

Committed patchset #5 (id:240001)

[commit-bot: I haz the power, 2016-09-07 07:32:15]

Description was changed from

==========
Heap Profiler: Add trace category group names as type names for allocations

The category for allocations are decided using the file that posted
current task. But, this does not work always since sometimes the code
is invoked by event listeners and tasks are not posted. To work around
this issue, the category name from trace events is marked as the
category for allocations as fallback.

BUG=594803
==========

to

==========
Heap Profiler: Add trace category group names as type names for allocations

The category for allocations are decided using the file that posted
current task. But, this does not work always since sometimes the code
is invoked by event listeners and tasks are not posted. To work around
this issue, the category name from trace events is marked as the
category for allocations as fallback.

BUG=594803

Committed: https://crrev.com/ee345086542ba00bae935fcfb13780ac5bdb63f1
Cr-Commit-Position: refs/heads/master@{#416873}
==========

[commit-bot: I haz the power, 2016-09-07 07:32:16]

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

[ssid, 2016-09-12 06:53:05]

Patchset #3 (id:120001) has been deleted