[tracing] Implement trace_event::AllocationContext

[tracing] Implement trace_event::AllocationContext

This implements the structure that will be stored together with allocation
details in a hash table that will be part of the heap profiler in
chrome://tracing.
See design doc linked from the bug for more details.

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

[Ruud van Asseldonk, 2015-09-25 09:24:44]

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

[picksi, 2015-09-25 09:44:35]

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

[picksi, 2015-09-25 09:44:35]

More drive-by observations!

https://codereview.chromium.org/1372523002/diff/1/base/trace_event/memory_pro...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1372523002/diff/1/base/trace_event/memory_pro...
base/trace_event/memory_profiler_allocation_context.cc:104: *dst = nullptr;
Q:Is the performance overhead for clearing all the remaining slots worthwhile?
If you cleared only the next slot would that suffice to stop iterations through
the slots?

https://codereview.chromium.org/1372523002/diff/1/base/trace_event/memory_pro...
base/trace_event/memory_profiler_allocation_context.cc:116: *dst = *src;
nit: This code is duplicated from 99-100, should it be broken out into a
function?

[Ruud van Asseldonk, 2015-09-25 11:46:19]

https://codereview.chromium.org/1372523002/diff/1/base/trace_event/memory_pro...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1372523002/diff/1/base/trace_event/memory_pro...
base/trace_event/memory_profiler_allocation_context.cc:104: *dst = nullptr;
On 2015/09/25 at 09:44:35, picksi wrote:
> Q:Is the performance overhead for clearing all the remaining slots worthwhile?
If you cleared only the next slot would that suffice to stop iterations through
the slots?

It would, but I am not sure how that would affect performance later on. To group
the allocations by backtrace, comparing the full backtrace padded with zeroes
can potentially be done very efficiently by doing a bitwise comparison of a
chunk of memory as large as the CPU can handle, and the loop is fixed size so it
can be unrolled. If there is garbage after the first null pointer this might
require a branch to early-out comparisons. If we group by using a hash table
then there should be no garbage at the end, for a tree with an operator< it is
probably good to early-out. I have no clue what would be better
performance-wise, but I am also not sure it is relevant here.

Do you want me to change it to a null-terminated list?

https://codereview.chromium.org/1372523002/diff/1/base/trace_event/memory_pro...
base/trace_event/memory_profiler_allocation_context.cc:116: *dst = *src;
On 2015/09/25 at 09:44:35, picksi wrote:
> nit: This code is duplicated from 99-100, should it be broken out into a
function?

Ideally there would be an |std::copy| that copies until either the destination
or source runs out, but there is none :( I wrote it manually now, but in my
opinion it just adds extra noise.

[picksi, 2015-09-25 12:31:26]

https://codereview.chromium.org/1372523002/diff/1/base/trace_event/memory_pro...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1372523002/diff/1/base/trace_event/memory_pro...
base/trace_event/memory_profiler_allocation_context.cc:104: *dst = nullptr;
No, leave it as it is! You make some good points :)

[Primiano Tucci (use gerrit), 2015-10-05 12:35:38]

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.cc:80: // Returns a pointer
past the end of the fixed-size array |array| of |T| of
Hmm all this template magic makes this file very hard to read.
As discussed offline please remove this and just to two separate for-each below.
I wouldn't even have a common Copy method, as the things being copied are
different (backtrace vs fields) that the common method would IMHO make that less
clear

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.cc:99: AllocationContext
AllocationContextTracker::GetContext() {
Maybe you could rename this to GetContextSnapshot() or something like that to
make it clear that this is a copy

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.cc:100: auto tracker =
GetThreadLocalTracker();
AllocationContextTracker* tracker = GetThreadLocalTracker();

fits in one line and is more readable

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context.h (right):

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:66: //  0  0.8 #
The histogram is nice but it's probably too much for this header. I'd move this
to the doc and just leave the cumulative values (lines 89-91) + a link to the
doc

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:99: StackFrame frames[8];
Looking at the data above, shouldn't this be 10-12 ?

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:102: // The allocation
context is what is stored in the bookkeeping structure with
I'd probably word it as "The allocation context is the metadata kept for each
allocation".
Remember that an allocator might choose to not use your upcoming bookkeeping
structure but retain the AllocationContext in its internals

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:105: // struct is designed
such that a memzero'd instance is a valid context with
As discussed remove the memzero comment

[Ruud van Asseldonk, 2015-10-05 14:20:45]

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.cc:80: // Returns a pointer
past the end of the fixed-size array |array| of |T| of
On 2015/10/05 at 12:35:38, Primiano Tucci wrote:
> Hmm all this template magic makes this file very hard to read.
> As discussed offline please remove this and just to two separate for-each
below.
> I wouldn't even have a common Copy method, as the things being copied are
different (backtrace vs fields) that the common method would IMHO make that less
clear

Agreed, done, though see also comment #4. I did keep |End| to do STL-style
(begin, end) iteration as opposed to C-style (index, size) iteration.

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.cc:99: AllocationContext
AllocationContextTracker::GetContext() {
On 2015/10/05 at 12:35:38, Primiano Tucci wrote:
> Maybe you could rename this to GetContextSnapshot() or something like that to
make it clear that this is a copy

Done.

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.cc:100: auto tracker =
GetThreadLocalTracker();
On 2015/10/05 at 12:35:38, Primiano Tucci wrote:
> AllocationContextTracker* tracker = GetThreadLocalTracker();
> 
> fits in one line and is more readable

Done.

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context.h (right):

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:66: //  0  0.8 #
On 2015/10/05 at 12:35:38, Primiano Tucci wrote:
> The histogram is nice but it's probably too much for this header. I'd move
this to the doc and just leave the cumulative values (lines 89-91) + a link to
the doc

Done.

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:99: StackFrame frames[8];
On 2015/10/05 at 12:35:38, Primiano Tucci wrote:
> Looking at the data above, shouldn't this be 10-12 ?

The thing with statistics is that you can often use them to support any claim
... When storing the top frames I think you can get away with storing less
frames because the top of the stack can uniquely identify the full stack. (You
store the full stack in 75% of the cases, but you can probably identify the
stack in > 94% of the cases.) Now that we are going to store the bottom we need
more frames to get useful information. I changed it to 12.

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:102: // The allocation
context is what is stored in the bookkeeping structure with
On 2015/10/05 at 12:35:38, Primiano Tucci wrote:
> I'd probably word it as "The allocation context is the metadata kept for each
allocation".
> Remember that an allocator might choose to not use your upcoming bookkeeping
structure but retain the AllocationContext in its internals

Done.

https://codereview.chromium.org/1372523002/diff/60001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:105: // struct is designed
such that a memzero'd instance is a valid context with
On 2015/10/05 at 12:35:38, Primiano Tucci wrote:
> As discussed remove the memzero comment

Done.

[Primiano Tucci (use gerrit), 2015-10-05 15:49:27]

LGTM but could probably be simplified further (See notes inline) (LGTM is valid
either way)

https://codereview.chromium.org/1372523002/diff/100001/base/trace_event/memor...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1372523002/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.cc:101: auto dst_end =
End(ctx.backtrace.frames);
I might be a bit old-style but I think that you can get rid of the template
magic and replace lines 89-109 with a simple

const size_t num_frames = std::min(tracker->pseudo_stack_.size(),
                                   arraysize(ctx.backtrace.frames);
for (size_t i=0; i < num_frames; ++i)
  ctx.backtrace.frames[i] = tracker->pseudo_stack_[i];

std::fill(ctx.backtrace.frames[num_frames], arraysize(ctx.backtrace.frames),
nullptr)

https://codereview.chromium.org/1372523002/diff/100001/base/trace_event/memor...
File base/trace_event/memory_profiler_allocation_context.h (right):

https://codereview.chromium.org/1372523002/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.h:28: // Incrementing the
iterator iterates up the stack, from bottom to top.
It'd probably less ambiguous if you said here bottom (least recent call) to top
(most recent call, i.e. the one closer to the allocator)

[Ruud van Asseldonk, 2015-10-06 09:57:19]

The CQ bit was checked by ruuda@google.com

[Ruud van Asseldonk, 2015-10-06 09:57:20]

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/1372523002/#ps120001
(title: "Rebase + clarify comment")

[commit-bot: I haz the power, 2015-10-06 09:57:32]

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

[commit-bot: I haz the power, 2015-10-06 11:04:30]

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

[commit-bot: I haz the power, 2015-10-06 11:04:30]

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

[Ruud van Asseldonk, 2015-10-06 11:10:38]

The CQ bit was checked by ruuda@google.com

[commit-bot: I haz the power, 2015-10-06 11:10:44]

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

[commit-bot: I haz the power, 2015-10-06 13:51:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-06 13:52:28]

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