Heap profiler: count the number of back references for objects.

Heap profiler: count the number of back references for objects.

Also, perform some refactoring to reuse common code between constructor and retainer profiles.

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

[Søren Thygesen Gjesse, 2009-09-18 11:18:28]

LGTM

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

http://codereview.chromium.org/209028/diff/1/2#newcode48
Line 48: static void InsertIntoTree(
We normally use this form for argument list that does not fit on one line.

static void InsertIntoTree(JSObjectsClusterTree* tree,
                           HeapObject* obj,
                           bool fine_grain);

Probable apply to other places below.

http://codereview.chromium.org/209028/diff/1/2#newcode74
Line 74: JSObjectsCluster Clusterizer::Clusterize(
void Clusterizer::Clusterize(..., ClusterrizeResult& result) {
  ...
  result.SetCluster(...);
  ...
  result.SetSize(...):
  ...
}

http://codereview.chromium.org/209028/diff/1/2#newcode180
Line 180: void Call(
Move arguments one line up.

http://codereview.chromium.org/209028/diff/1/2#newcode185
Line 185: static void Print(
Move arguments one line up. Probably more below where this apply.

http://codereview.chromium.org/209028/diff/1/3
File src/heap-profiler.h (right):

http://codereview.chromium.org/209028/diff/1/3#newcode130
Line 130: virtual void Call(
Move arguments one line up.

http://codereview.chromium.org/209028/diff/1/3#newcode236
Line 236: virtual void PrintRetainers(
Move arguments one line up and split.

http://codereview.chromium.org/209028/diff/1/3#newcode259
Line 259: void Call(
Move arguments one line up.

http://codereview.chromium.org/209028/diff/1/4
File src/log-utils.cc (right):

http://codereview.chromium.org/209028/diff/1/4#newcode258
Line 258: len = Log::kMessageBufferSize - pos_;
Wouldn't the right thing here be to assert that len is >= 0 and return if len ==
0?

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

http://codereview.chromium.org/209028/diff/1/8#newcode37
Line 37: const i::NumberAndSizeInfo& number_and_size) {
Please move the parameters one line up:

void Call(const JSObjectsCluster& cluster,
          const i::NumberAndSizeInfo& number_and_size) {
...

http://codereview.chromium.org/209028/diff/3001/3004
File src/log-utils.cc (right):

http://codereview.chromium.org/209028/diff/3001/3004#newcode315
Line 315: len = Log::kMessageBufferSize - pos_;
Wouldn't the right thing to do here be to assert len >= 0 and return if len ==
0?

http://codereview.chromium.org/209028/diff/3001/3006
File src/log.cc (right):

http://codereview.chromium.org/209028/diff/3001/3006#newcode904
Line 904: static const int event_text_len = strlen(event_text);
The actual output len will be 2 less as the %S is overtaken by the constructor
name. Maybe comment that being conservative with the length is OK.

[mnaganov (inactive), 2009-09-18 12:03:38]

Thanks to your comments, I've removed 21 lines from this patch.

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

http://codereview.chromium.org/209028/diff/1/2#newcode48
Line 48: static void InsertIntoTree(
On 2009/09/18 11:18:28, Søren Gjesse wrote:
> We normally use this form for argument list that does not fit on one line.
> 
> static void InsertIntoTree(JSObjectsClusterTree* tree,
>                            HeapObject* obj,
>                            bool fine_grain);
> 
> Probable apply to other places below.

This form you mention has a drawback that changing method name requires you to
realign arguments (and you become the person whom to blame according to SVN.)
But I regard consistency, so I've changed them to conform to project style.

http://codereview.chromium.org/209028/diff/1/2#newcode74
Line 74: JSObjectsCluster Clusterizer::Clusterize(
On 2009/09/18 11:18:28, Søren Gjesse wrote:
> void Clusterizer::Clusterize(..., ClusterrizeResult& result) {
>   ...
>   result.SetCluster(...);
>   ...
>   result.SetSize(...):
>   ...
> }

Thanks to your comment, I discovered that I can remove size calculation from
this function, as it only was here due to historical reasons.

http://codereview.chromium.org/209028/diff/1/2#newcode180
Line 180: void Call(
On 2009/09/18 11:18:28, Søren Gjesse wrote:
> Move arguments one line up.

Done.

http://codereview.chromium.org/209028/diff/1/2#newcode185
Line 185: static void Print(
On 2009/09/18 11:18:28, Søren Gjesse wrote:
> Move arguments one line up. Probably more below where this apply.

Done.

http://codereview.chromium.org/209028/diff/1/3
File src/heap-profiler.h (right):

http://codereview.chromium.org/209028/diff/1/3#newcode130
Line 130: virtual void Call(
On 2009/09/18 11:18:28, Søren Gjesse wrote:
> Move arguments one line up.

Done.

http://codereview.chromium.org/209028/diff/1/3#newcode236
Line 236: virtual void PrintRetainers(
On 2009/09/18 11:18:28, Søren Gjesse wrote:
> Move arguments one line up and split.

Done.

http://codereview.chromium.org/209028/diff/1/3#newcode259
Line 259: void Call(
On 2009/09/18 11:18:28, Søren Gjesse wrote:
> Move arguments one line up.

Done.

http://codereview.chromium.org/209028/diff/1/4
File src/log-utils.cc (right):

http://codereview.chromium.org/209028/diff/1/4#newcode258
Line 258: len = Log::kMessageBufferSize - pos_;
On 2009/09/18 11:18:28, Søren Gjesse wrote:
> Wouldn't the right thing here be to assert that len is >= 0 and return if len
==
> 0?

Done.

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

http://codereview.chromium.org/209028/diff/1/8#newcode37
Line 37: const i::NumberAndSizeInfo& number_and_size) {
On 2009/09/18 11:18:28, Søren Gjesse wrote:
> Please move the parameters one line up:
> 
> void Call(const JSObjectsCluster& cluster,
>           const i::NumberAndSizeInfo& number_and_size) {
> ...

Done.

http://codereview.chromium.org/209028/diff/3001/3004
File src/log-utils.cc (right):

http://codereview.chromium.org/209028/diff/3001/3004#newcode315
Line 315: len = Log::kMessageBufferSize - pos_;
On 2009/09/18 11:18:28, Søren Gjesse wrote:
> Wouldn't the right thing to do here be to assert len >= 0 and return if len ==
> 0?

Done.

http://codereview.chromium.org/209028/diff/3001/3006
File src/log.cc (right):

http://codereview.chromium.org/209028/diff/3001/3006#newcode904
Line 904: static const int event_text_len = strlen(event_text);
On 2009/09/18 11:18:28, Søren Gjesse wrote:
> The actual output len will be 2 less as the %S is overtaken by the constructor
> name. Maybe comment that being conservative with the length is OK.

Done.