Heap profiler: allow returning aggregated snapshots via the new API.

Heap profiler: allow returning aggregated snapshots via the new API.

This is intended for smoother migration to the new API in Chromium.
Also, aggregated heap snapshots can be used for cheaply obtaining
heap statistics, e.g. in tests.

Committed: http://code.google.com/p/v8/source/detail?r=5297

[Søren Thygesen Gjesse, 2010-08-17 13:08:59]

LGTM

http://codereview.chromium.org/3124024/diff/1/2
File include/v8-profiler.h (right):

http://codereview.chromium.org/3124024/diff/1/2#newcode351
include/v8-profiler.h:351: bool aggregated = false);
How about using an enum instead of a bool here:

enum HeapSnapshotType {
  kHeapSnapshotTypeFull,
  kHeapSnapshotTypeAggregated
}

That would allow more changes to be added and provide more information when
looking at the call site.

http://codereview.chromium.org/3124024/diff/1/4
File src/heap-profiler.cc (right):

http://codereview.chromium.org/3124024/diff/1/4#newcode370
src/heap-profiler.cc:370: AggregatedHeapSnapshot agg_snapshot;
Can't agg_snapshot just be a member of AggregatedHeapSnapshotGenerator (it is
just passed in he constructor)?

http://codereview.chromium.org/3124024/diff/1/9
File test/cctest/test-heap-profiler.cc (right):

http://codereview.chromium.org/3124024/diff/1/9#newcode935
test/cctest/test-heap-profiler.cc:935: CHECK_NE(0, maps->GetId());
GetId() returns the number of objects in an aggregated snapshot? That seems a
bit odd. Is the intend that the heap is the same if the same number of objects
of a specific type is present -> same id?

[mnaganov (inactive), 2010-08-17 14:55:34]

Thanks for your comments!

Sorry, I forgot to send a notification email about this issue.

http://codereview.chromium.org/3124024/diff/1/2
File include/v8-profiler.h (right):

http://codereview.chromium.org/3124024/diff/1/2#newcode351
include/v8-profiler.h:351: bool aggregated = false);
On 2010/08/17 13:08:59, Søren Gjesse wrote:
> How about using an enum instead of a bool here:
> 
> enum HeapSnapshotType {
>   kHeapSnapshotTypeFull,
>   kHeapSnapshotTypeAggregated
> }
> 
> That would allow more changes to be added and provide more information when
> looking at the call site.

A good idea. I've introduced 'enum Type' in HeapSnapshot.

http://codereview.chromium.org/3124024/diff/1/4
File src/heap-profiler.cc (right):

http://codereview.chromium.org/3124024/diff/1/4#newcode370
src/heap-profiler.cc:370: AggregatedHeapSnapshot agg_snapshot;
On 2010/08/17 13:08:59, Søren Gjesse wrote:
> Can't agg_snapshot just be a member of AggregatedHeapSnapshotGenerator (it is
> just passed in he constructor)?

This is for alignment with HeapSnapshotGenerator. In my ideology, a generator
doesn't own the profile, it just fills it out.

http://codereview.chromium.org/3124024/diff/1/9
File test/cctest/test-heap-profiler.cc (right):

http://codereview.chromium.org/3124024/diff/1/9#newcode935
test/cctest/test-heap-profiler.cc:935: CHECK_NE(0, maps->GetId());
On 2010/08/17 13:08:59, Søren Gjesse wrote:
> GetId() returns the number of objects in an aggregated snapshot? That seems a
> bit odd. Is the intend that the heap is the same if the same number of objects
> of a specific type is present -> same id?

For an aggregated snapshots, instance ids just doesn't make sense, because they
contain no instances, so I reused this field for storing instances count. But
you are right, this is confusing, so I've split this into two specific methods
which check snapshot type.