Heap profiler: reduce heap snapshots size.

Heap profiler: reduce heap snapshots size.

The size of a snapshot is now 65-80% of the JS heap size (tested on
GMail and Wave), previously it was >200%.

BUG=783
Committed: http://code.google.com/p/v8/source/detail?r=5211

[mnaganov (inactive), 2010-07-23 09:32:06]

Hi Soeren,

I apologize for a big change, but you should have a plenty of time to review it
-- I'll be on vacation till Aug, 9.

[Søren Thygesen Gjesse, 2010-08-06 15:19:03]

LGTM

http://codereview.chromium.org/3060008/diff/1/6
File src/profile-generator.cc (right):

http://codereview.chromium.org/3060008/diff/1/6#newcode1053
src/profile-generator.cc:1053: // To calculate non-shared total size, first we
paint all reachable
Maybe change the term "total size" to something like "total reachable size" or
"total retaining size".

http://codereview.chromium.org/3060008/diff/1/6#newcode1311
src/profile-generator.cc:1311: calculated_data_.Add(HeapEntryCalculatedData());
These are just cached for the whole lifetime of the heap snapshot, right?

http://codereview.chromium.org/3060008/diff/1/6#newcode1768
src/profile-generator.cc:1768: // If GPC references an object that we have
interest in, add the object.
Where does the "we have interest in" come into play? Does it refer to
WillAddEntry below?

http://codereview.chromium.org/3060008/diff/1/7
File src/profile-generator.h (right):

http://codereview.chromium.org/3060008/diff/1/7#newcode431
src/profile-generator.h:431: CONTEXT_VARIABLE =
v8::HeapGraphEdge::CONTEXT_VARIABLE,
Any particular reason why you are not using the kXxxYxx naming for the enum's?

http://codereview.chromium.org/3060008/diff/1/7#newcode456
src/profile-generator.h:456: struct {
Do you need to put these in a struct?

http://codereview.chromium.org/3060008/diff/1/7#newcode501
src/profile-generator.h:501: CLOSURE = v8::HeapGraphNode::CLOSURE
Maybe add a "number of types" to the enum, and have a static assert that you
only need 3 bits for the type.

http://codereview.chromium.org/3060008/diff/1/7#newcode514
src/profile-generator.h:514: HeapSnapshot* snapshot() { return snapshot_; }
Maybe you can avoid this member if these objects are allocated in large
contiguous arrays, then an address ranges could determine the heap snapshot.
Just a suggestion for future changes.

http://codereview.chromium.org/3060008/diff/1/7#newcode557
src/profile-generator.h:557: static int EntriesSize(int entries_count,
Maybe this should be a HeapSnapshot method.

http://codereview.chromium.org/3060008/diff/1/7#newcode570
src/profile-generator.h:570: HeapSnapshot* snapshot_;
Maybe you can avoid this member (see comment above).

http://codereview.chromium.org/3060008/diff/1/7#newcode571
src/profile-generator.h:571: struct {
struct needed?

http://codereview.chromium.org/3060008/diff/1/7#newcode574
src/profile-generator.h:574: int calculated_data_index: 27;
Please add a comment on where the calculated data is stored.

http://codereview.chromium.org/3060008/diff/1/7#newcode830
src/profile-generator.h:830: 
A short comment on how this is used, and the use of aliasing will be helpful.

[mnaganov (inactive), 2010-08-09 09:54:34]

http://codereview.chromium.org/3060008/diff/1/6
File src/profile-generator.cc (right):

http://codereview.chromium.org/3060008/diff/1/6#newcode1053
src/profile-generator.cc:1053: // To calculate non-shared total size, first we
paint all reachable
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> Maybe change the term "total size" to something like "total reachable size" or
> "total retaining size".

Renamed "total size" -> "reachable size", "private (total non-shared) size" ->
"retained size".

http://codereview.chromium.org/3060008/diff/1/6#newcode1311
src/profile-generator.cc:1311: calculated_data_.Add(HeapEntryCalculatedData());
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> These are just cached for the whole lifetime of the heap snapshot, right?

Right. This data is lazily calculated and is stored outside HeapEntries to save
space.

http://codereview.chromium.org/3060008/diff/1/6#newcode1768
src/profile-generator.cc:1768: // If GPC references an object that we have
interest in, add the object.
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> Where does the "we have interest in" come into play? Does it refer to
> WillAddEntry below?

Yes. I've enhanced the comment.

http://codereview.chromium.org/3060008/diff/1/7
File src/profile-generator.h (right):

http://codereview.chromium.org/3060008/diff/1/7#newcode431
src/profile-generator.h:431: CONTEXT_VARIABLE =
v8::HeapGraphEdge::CONTEXT_VARIABLE,
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> Any particular reason why you are not using the kXxxYxx naming for the enum's?

Sorry, bad habit. Renamed.

http://codereview.chromium.org/3060008/diff/1/7#newcode456
src/profile-generator.h:456: struct {
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> Do you need to put these in a struct?

No. Thanks for the hint!

http://codereview.chromium.org/3060008/diff/1/7#newcode501
src/profile-generator.h:501: CLOSURE = v8::HeapGraphNode::CLOSURE
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> Maybe add a "number of types" to the enum, and have a static assert that you
> only need 3 bits for the type.

No need for this: GCC has this check and emits a warning.

http://codereview.chromium.org/3060008/diff/1/7#newcode514
src/profile-generator.h:514: HeapSnapshot* snapshot() { return snapshot_; }
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> Maybe you can avoid this member if these objects are allocated in large
> contiguous arrays, then an address ranges could determine the heap snapshot.
> Just a suggestion for future changes.

A good idea, I will think about this optimization.

http://codereview.chromium.org/3060008/diff/1/7#newcode557
src/profile-generator.h:557: static int EntriesSize(int entries_count,
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> Maybe this should be a HeapSnapshot method.

My point is that HeapEntry's layout is encapsulated in it, and as size depends
on layout, this method fits here better.

http://codereview.chromium.org/3060008/diff/1/7#newcode570
src/profile-generator.h:570: HeapSnapshot* snapshot_;
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> Maybe you can avoid this member (see comment above).

OK.

http://codereview.chromium.org/3060008/diff/1/7#newcode571
src/profile-generator.h:571: struct {
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> struct needed?

Removed.

http://codereview.chromium.org/3060008/diff/1/7#newcode574
src/profile-generator.h:574: int calculated_data_index: 27;
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> Please add a comment on where the calculated data is stored.

Done.

http://codereview.chromium.org/3060008/diff/1/7#newcode830
src/profile-generator.h:830: 
On 2010/08/06 15:19:03, Søren Gjesse wrote:
> A short comment on how this is used, and the use of aliasing will be helpful.

Done.