The new JS Heap Profiler: the main part.

The new JS Heap Profiler: the main part.

There is no test in this patch, because the test uses V8 API,
which is coming in a separate change list.

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

[mnaganov (inactive), 2010-06-09 12:46:18]

Hi Soeren,

Sorry, this change is large, but I think it's better not to split it, because it
includes all the interacting parts, so you'll be able to grasp a full context.

[Søren Thygesen Gjesse, 2010-06-10 09:32:44]

LGTM with comments.

How much memory is taken up by a heap snapshot? I assume that your StringStorage
is to avoid keeping a copy of all stings in each snapshot. How is the ratoi
between the string data and the rest?

http://codereview.chromium.org/2722005/diff/5001/6002
File src/profile-generator.cc (right):

http://codereview.chromium.org/2722005/diff/5001/6002#newcode876
src/profile-generator.cc:876: int HeapEntry::TotalSize() {
Please use constant instead of -1.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode881
src/profile-generator.cc:881: int HeapEntry::NonSharedTotalSize() {
Ditto.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode919
src/profile-generator.cc:919: void Apply(HeapEntry* entry) {
Please use constants for the different kind of paint. It is not clear what the
difference between color 1 and 2 is.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode931
src/profile-generator.cc:931: int HeapEntry::CalculateNonSharedTotalSize() {
Please add some comments to indicate what the two loops does.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode965
src/profile-generator.cc:965: NonSharedSizeCalculator calculator;
Looks as if the NonSharedSizeCalculator only looks at paint == 1. What am I
missing here?

http://codereview.chromium.org/2722005/diff/5001/6002#newcode1002
src/profile-generator.cc:1002: };
Please add another empty line.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode1112
src/profile-generator.cc:1112: case JS_OBJECT: return "/js obj/";
Why not just "object" instead of "js obj"?

http://codereview.chromium.org/2722005/diff/5001/6002#newcode1233
src/profile-generator.cc:1233: next_gc_root_id_(1) {
next_gc_root_id_ is not used.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode1448
src/profile-generator.cc:1448: HeapSnapshot* snapshot = new HeapSnapshot(this,
name, uid);
It is by design, that HeapSnapshotGenerator is nou used immediately to fill the
snapshot?

http://codereview.chromium.org/2722005/diff/5001/6002#newcode1495
src/profile-generator.cc:1495: ExtractElementReferences(js_obj, entry);
How about the references to the prototype and constructor?

http://codereview.chromium.org/2722005/diff/5001/6003
File src/profile-generator.h (right):

http://codereview.chromium.org/2722005/diff/5001/6003#newcode609
src/profile-generator.h:609: 
Please add a few lines describing these classes and how they connect. I have
guessed that you create a SnapshotCollection, call NewSnapshot on that and use
HeapSnapshotGenerator afterwards, sounds right?

[tfarina, 2010-06-10 13:45:24]

http://codereview.chromium.org/2722005/diff/5001/6002
File src/profile-generator.cc (right):

http://codereview.chromium.org/2722005/diff/5001/6002#newcode832
src/profile-generator.cc:832: static void DeleteHeapGraphEdge(HeapGraphEdge**
p_edge) {
the prefix p_ stands for parameter? If so I'd drop it. Same below.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode917
src/profile-generator.cc:917: int non_shared_total_size() { return
non_shared_total_size_; }
if we add 'const' this will break? If not, could you add it?

[tfarina, 2010-06-10 13:50:34]

http://codereview.chromium.org/2722005/diff/5001/6003
File src/profile-generator.h (right):

http://codereview.chromium.org/2722005/diff/5001/6003#newcode576
src/profile-generator.h:576: };
if we don't allow copy and assign, DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/2722005/diff/5001/6003#newcode605
src/profile-generator.h:605: };
DISALLOW_COPY_AND_ASSIN? Same thing for the new classes below. Ignore me if we
are using copy and assign of them.

[mnaganov (inactive), 2010-06-11 16:34:16]

Thanks, guys! Some of your comments were very helpful.

Currently, heap snapshots for real web apps are bigger than JS heap, because
external strings contents get copied. But I plan to fix this soon.

http://codereview.chromium.org/2722005/diff/5001/6002
File src/profile-generator.cc (right):

http://codereview.chromium.org/2722005/diff/5001/6002#newcode832
src/profile-generator.cc:832: static void DeleteHeapGraphEdge(HeapGraphEdge**
p_edge) {
On 2010/06/10 13:45:26, tfarina wrote:
> the prefix p_ stands for parameter? If so I'd drop it. Same below.

No, it stands for "pointer" to indicate that this is a double pointer. I really
should use '_ptr' suffix for consistency with existing code. Fixed.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode876
src/profile-generator.cc:876: int HeapEntry::TotalSize() {
On 2010/06/10 09:32:44, Søren Gjesse wrote:
> Please use constant instead of -1.

Done.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode881
src/profile-generator.cc:881: int HeapEntry::NonSharedTotalSize() {
On 2010/06/10 09:32:44, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode917
src/profile-generator.cc:917: int non_shared_total_size() { return
non_shared_total_size_; }
On 2010/06/10 13:45:26, tfarina wrote:
> if we add 'const' this will break? If not, could you add it?

Done.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode919
src/profile-generator.cc:919: void Apply(HeapEntry* entry) {
On 2010/06/10 09:32:44, Søren Gjesse wrote:
> Please use constants for the different kind of paint. It is not clear what the
> difference between color 1 and 2 is.

Done. I clarified the code by introducing named constants and methods.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode931
src/profile-generator.cc:931: int HeapEntry::CalculateNonSharedTotalSize() {
On 2010/06/10 09:32:44, Søren Gjesse wrote:
> Please add some comments to indicate what the two loops does.

Done.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode965
src/profile-generator.cc:965: NonSharedSizeCalculator calculator;
On 2010/06/10 09:32:44, Søren Gjesse wrote:
> Looks as if the NonSharedSizeCalculator only looks at paint == 1. What am I
> missing here?

That's right. It only takes into account nodes which were not repainted into
second color as reachable from other nodes. I added a comment describing the
algorithm.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode1002
src/profile-generator.cc:1002: };
On 2010/06/10 09:32:44, Søren Gjesse wrote:
> Please add another empty line.

Done.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode1112
src/profile-generator.cc:1112: case JS_OBJECT: return "/js obj/";
On 2010/06/10 09:32:44, Søren Gjesse wrote:
> Why not just "object" instead of "js obj"?

Done.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode1233
src/profile-generator.cc:1233: next_gc_root_id_(1) {
On 2010/06/10 09:32:44, Søren Gjesse wrote:
> next_gc_root_id_ is not used.

Good catch. Removed.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode1448
src/profile-generator.cc:1448: HeapSnapshot* snapshot = new HeapSnapshot(this,
name, uid);
On 2010/06/10 09:32:44, Søren Gjesse wrote:
> It is by design, that HeapSnapshotGenerator is nou used immediately to fill
the
> snapshot?

I'm resembling the design of CPU profiler framework.

http://codereview.chromium.org/2722005/diff/5001/6002#newcode1495
src/profile-generator.cc:1495: ExtractElementReferences(js_obj, entry);
On 2010/06/10 09:32:44, Søren Gjesse wrote:
> How about the references to the prototype and constructor?

A good point! I added 'prototype', but not 'constructor', because the latter is
referenced from the former. Also, some prototypes has a 'constructor' property
which for some reason references other value than 'map()->constructor()' --
confusing.

http://codereview.chromium.org/2722005/diff/5001/6003
File src/profile-generator.h (right):

http://codereview.chromium.org/2722005/diff/5001/6003#newcode576
src/profile-generator.h:576: };
On 2010/06/10 13:50:34, tfarina wrote:
> if we don't allow copy and assign, DISALLOW_COPY_AND_ASSIGN?

Added here and in other classes.

http://codereview.chromium.org/2722005/diff/5001/6003#newcode605
src/profile-generator.h:605: };
On 2010/06/10 13:50:34, tfarina wrote:
> DISALLOW_COPY_AND_ASSIN? Same thing for the new classes below. Ignore me if we
> are using copy and assign of them.

Done.

http://codereview.chromium.org/2722005/diff/5001/6003#newcode609
src/profile-generator.h:609: 
On 2010/06/10 09:32:44, Søren Gjesse wrote:
> Please add a few lines describing these classes and how they connect. I have
> guessed that you create a SnapshotCollection, call NewSnapshot on that and use
> HeapSnapshotGenerator afterwards, sounds right?

Done.