Record allocation stack traces

Record allocation stack traces

This is initial implementation of allocation profiler.

Whenever new object allocation is reported to the HeapProfiler and allocation tracking is on we will capture current stack trace, add it to the collection of the allocation traces (a tree) and attribute the allocated size to the top JS function on the stack.

Format of serialized heap snapshot is extended to include information about recorded allocation stack traces.

BUG=chromium:277984
R=hpayer@chromium.org, loislo@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=17301

[loislo, 2013-10-16 15:14:07]

https://codereview.chromium.org/27227005/diff/1/src/allocation-tracker.cc
File src/allocation-tracker.cc (right):

https://codereview.chromium.org/27227005/diff/1/src/allocation-tracker.cc#new...
src/allocation-tracker.cc:192: printf("script->name is not a name\n");
please remove this

https://codereview.chromium.org/27227005/diff/1/src/allocation-tracker.cc#new...
src/allocation-tracker.cc:195: // Converting start offset into line and column
may cause heap allocations.
80 symbols

https://codereview.chromium.org/27227005/diff/1/src/allocation-tracker.h
File src/allocation-tracker.h (right):

https://codereview.chromium.org/27227005/diff/1/src/allocation-tracker.h#newc...
src/allocation-tracker.h:35: class HeapObjectsMap;
duplicate

https://codereview.chromium.org/27227005/diff/1/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/27227005/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:2764: EmbeddedVector<char, kBufferSize> buffer;
looks like you missed tracker check here.

https://codereview.chromium.org/27227005/diff/1/test/cctest/test-heap-profile...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/27227005/diff/1/test/cctest/test-heap-profile...
test/cctest/test-heap-profiler.cc:2037: "                script.push('    return
f_' + i + '_' + (j+1) + '(x+1);\\n');\n"
80 symbols

https://codereview.chromium.org/27227005/diff/1/test/cctest/test-heap-profile...
test/cctest/test-heap-profiler.cc:2039: "                script.push('    return
new f_' + i + '_' + (depth - 1) + '();\\n');\n"
80 symbols

[yurys, 2013-10-17 15:27:34]

https://codereview.chromium.org/27227005/diff/1/src/allocation-tracker.cc
File src/allocation-tracker.cc (right):

https://codereview.chromium.org/27227005/diff/1/src/allocation-tracker.cc#new...
src/allocation-tracker.cc:192: printf("script->name is not a name\n");
On 2013/10/16 15:14:08, loislo wrote:
> please remove this

Done.

https://codereview.chromium.org/27227005/diff/1/src/allocation-tracker.cc#new...
src/allocation-tracker.cc:195: // Converting start offset into line and column
may cause heap allocations.
On 2013/10/16 15:14:08, loislo wrote:
> 80 symbols

Done.

https://codereview.chromium.org/27227005/diff/1/src/allocation-tracker.h
File src/allocation-tracker.h (right):

https://codereview.chromium.org/27227005/diff/1/src/allocation-tracker.h#newc...
src/allocation-tracker.h:35: class HeapObjectsMap;
On 2013/10/16 15:14:08, loislo wrote:
> duplicate

Done.

https://codereview.chromium.org/27227005/diff/1/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/27227005/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:2764: EmbeddedVector<char, kBufferSize> buffer;
On 2013/10/16 15:14:08, loislo wrote:
> looks like you missed tracker check here.

The check is performed in its caller SerializeTraceTree. Also this method
doesn't use AllocationTracker, it operates on AllocationTraceNode.

https://codereview.chromium.org/27227005/diff/1/test/cctest/test-heap-profile...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/27227005/diff/1/test/cctest/test-heap-profile...
test/cctest/test-heap-profiler.cc:2037: "                script.push('    return
f_' + i + '_' + (j+1) + '(x+1);\\n');\n"
On 2013/10/16 15:14:08, loislo wrote:
> 80 symbols

Done.

https://codereview.chromium.org/27227005/diff/1/test/cctest/test-heap-profile...
test/cctest/test-heap-profiler.cc:2039: "                script.push('    return
new f_' + i + '_' + (depth - 1) + '();\\n');\n"
On 2013/10/16 15:14:08, loislo wrote:
> 80 symbols

Done.

[loislo, 2013-10-18 08:30:38]

lgtm

https://codereview.chromium.org/27227005/diff/23001/src/allocation-tracker.cc
File src/allocation-tracker.cc (right):

https://codereview.chromium.org/27227005/diff/23001/src/allocation-tracker.cc...
src/allocation-tracker.cc:54: AllocationTraceNode* node = children_[i];
what are you expectations about the average number of children.
Especially in case of the highe stacks (more than 64)?

https://codereview.chromium.org/27227005/diff/23001/src/allocation-tracker.h
File src/allocation-tracker.h (right):

https://codereview.chromium.org/27227005/diff/23001/src/allocation-tracker.h#...
src/allocation-tracker.h:101: AllocationTraceTree* allocation_traces() { return
&allocation_traces_; }
allocation_trace_tree?

[yurys, 2013-10-18 09:04:32]

https://codereview.chromium.org/27227005/diff/23001/src/allocation-tracker.cc
File src/allocation-tracker.cc (right):

https://codereview.chromium.org/27227005/diff/23001/src/allocation-tracker.cc...
src/allocation-tracker.cc:54: AllocationTraceNode* node = children_[i];
On 2013/10/18 08:30:38, loislo wrote:
> what are you expectations about the average number of children.
> Especially in case of the highe stacks (more than 64)?

I'd expect it to be <10, if we see some nodes with a great number of children
passing some threshold we can switch to a hashmap instead of the list. It think
plain list is fine for now.

https://codereview.chromium.org/27227005/diff/23001/src/allocation-tracker.h
File src/allocation-tracker.h (right):

https://codereview.chromium.org/27227005/diff/23001/src/allocation-tracker.h#...
src/allocation-tracker.h:101: AllocationTraceTree* allocation_traces() { return
&allocation_traces_; }
On 2013/10/18 08:30:38, loislo wrote:
> allocation_trace_tree?

Done.

[Hannes Payer (out of office), 2013-10-21 09:57:21]

LGTM, minor nits.

https://codereview.chromium.org/27227005/diff/63001/src/allocation-tracker.cc
File src/allocation-tracker.cc (right):

https://codereview.chromium.org/27227005/diff/63001/src/allocation-tracker.cc...
src/allocation-tracker.cc:80: indent, ' ');
May fit in one line or may be more compact?

https://codereview.chromium.org/27227005/diff/63001/src/allocation-tracker.h
File src/allocation-tracker.h (right):

https://codereview.chromium.org/27227005/diff/63001/src/allocation-tracker.h#...
src/allocation-tracker.h:127: static const int kMaxAllocationTraceLength = 64;
style guide: declare static const data members before regular data members

https://codereview.chromium.org/27227005/diff/63001/src/heap-snapshot-generat...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/27227005/diff/63001/src/heap-snapshot-generat...
src/heap-snapshot-generator.cc:2761: + 4 + 1 + 1;
Why don't you define this constant in the header and initialize it in the
constructor?

https://codereview.chromium.org/27227005/diff/63001/src/heap-snapshot-generat...
src/heap-snapshot-generator.cc:2806: + 6 + 1 + 1;
Same here.

[yurys, 2013-10-21 15:07:55]

https://codereview.chromium.org/27227005/diff/63001/src/allocation-tracker.cc
File src/allocation-tracker.cc (right):

https://codereview.chromium.org/27227005/diff/63001/src/allocation-tracker.cc...
src/allocation-tracker.cc:80: indent, ' ');
On 2013/10/21 09:57:22, Hannes Payer wrote:
> May fit in one line or may be more compact?

Done.

https://codereview.chromium.org/27227005/diff/63001/src/allocation-tracker.h
File src/allocation-tracker.h (right):

https://codereview.chromium.org/27227005/diff/63001/src/allocation-tracker.h#...
src/allocation-tracker.h:127: static const int kMaxAllocationTraceLength = 64;
On 2013/10/21 09:57:22, Hannes Payer wrote:
> style guide: declare static const data members before regular data members

Done.

https://codereview.chromium.org/27227005/diff/63001/src/heap-snapshot-generat...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/27227005/diff/63001/src/heap-snapshot-generat...
src/heap-snapshot-generator.cc:2761: + 4 + 1 + 1;
On 2013/10/21 09:57:22, Hannes Payer wrote:
> Why don't you define this constant in the header and initialize it in the
> constructor?

Not sure I follow you here, this constant is used as the template parameter here
(similar to the other methods in this file) and it is an implementation detail
of this particular method. What's the point in moving the constant into the
header?

[yurys, 2013-10-21 15:22:21]

Committed patchset #10 manually as r17301 (presubmit successful).