Remove HeapSnapshotsCollection class

Remove HeapSnapshotsCollection class

The class added unecessary level of indirection to the heap profiler implementation. I merged some of its implementation into HeapProfiler and deleted the rest. This refactoring is also a prerequisite for keeping HeapObjectsMap alive even when all snapshots are deleted.

BUG=None
LOG=N
R=alph@chromium.org, mstarzinger@chromium.org

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

[Michael Starzinger, 2013-12-03 08:54:50]

LGTM.

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.cc
File src/heap-profiler.cc (right):

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.cc#newcode119
src/heap-profiler.cc:119: ASSERT(allocation_tracker_.is_empty());
nit: I think it makes sense to use the is_tracking_allocations() predicate in
this assert, as that is the counterpart of what is used in
HeapProfiler::StopHeapObjectsTracking below.

[alph, 2013-12-03 09:20:20]

lgtm

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.cc
File src/heap-profiler.cc (right):

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.cc#newcode198
src/heap-profiler.cc:198: heap()->CollectAllGarbage(Heap::kMakeHeapIterableMask,
Doesn't look like a Full GC.

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.h
File src/heap-profiler.h (right):

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.h#newcode58
src/heap-profiler.h:58: AllocationTracker* allocation_tracker() { return
*allocation_tracker_; }
Using operator* to extract a pointer makes brain damage. Could you please add a
'get' method to the SmartPointer class.
Although perhaps it deserves a separate patch.

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.h#newcode92
src/heap-profiler.h:92: List<HeapSnapshot*> snapshots_;
Is it possible to use List of SmartPointer's?

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

https://codereview.chromium.org/101393002/diff/1/src/heap-snapshot-generator....
src/heap-snapshot-generator.cc:2042: StringsStorage* names_;
What is the reason of storing these, since you can always get them through
snapshot_? Speed?

[yurys, 2013-12-03 09:41:30]

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.cc
File src/heap-profiler.cc (right):

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.cc#newcode119
src/heap-profiler.cc:119: ASSERT(allocation_tracker_.is_empty());
On 2013/12/03 08:54:51, Michael Starzinger wrote:
> nit: I think it makes sense to use the is_tracking_allocations() predicate in
> this assert, as that is the counterpart of what is used in
> HeapProfiler::StopHeapObjectsTracking below.

Done.

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.cc#newcode198
src/heap-profiler.cc:198: heap()->CollectAllGarbage(Heap::kMakeHeapIterableMask,
On 2013/12/03 09:20:20, alph wrote:
> Doesn't look like a Full GC.

This comment was moved, I think it is outdated. Removed to avoid confusion.

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.h
File src/heap-profiler.h (right):

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.h#newcode58
src/heap-profiler.h:58: AllocationTracker* allocation_tracker() { return
*allocation_tracker_; }
On 2013/12/03 09:20:20, alph wrote:
> Using operator* to extract a pointer makes brain damage. Could you please add
a
> 'get' method to the SmartPointer class.
> Although perhaps it deserves a separate patch.

I was going to do this and some other changes to the SmartPointer in a separate
patch.

https://codereview.chromium.org/101393002/diff/1/src/heap-profiler.h#newcode92
src/heap-profiler.h:92: List<HeapSnapshot*> snapshots_;
On 2013/12/03 09:20:20, alph wrote:
> Is it possible to use List of SmartPointer's?

No, it wouldn't work as ~List wouldn't call destructors for its elements (btw I
just moved this code from HeapSnapshotsCollection).

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

https://codereview.chromium.org/101393002/diff/1/src/heap-snapshot-generator....
src/heap-snapshot-generator.cc:2042: StringsStorage* names_;
On 2013/12/03 09:20:20, alph wrote:
> What is the reason of storing these, since you can always get them through
> snapshot_? Speed?

The usage looks more readable to my taste, also this approach might be a bit
faster. I don't have strong opinion here and could change it if you suggest so.

[yurys, 2013-12-03 09:48:41]

Committed patchset #2 manually as r18221 (presubmit successful).