Heap profiler: implement diffing of snapshots.

Heap profiler: implement diffing of snapshots.

To trace objects between snapshots, an external map of object tags is
maintained. After the first heap snapshot has been taken, the map is
updated by reporting object moves from the GC. If no snapshots were
taken, there is no overhead (except for flag checking).

I considered graph comparison algorithms that doesn't require using
object tags, but they are all of a high computational complexity, and
will still fail to detect object moves properly, even for trivial
cases, so using tags looks like unavoidable.

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

[Mads Ager (chromium), 2010-07-15 10:55:30]

LGTM

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

http://codereview.chromium.org/3020002/diff/1/2#newcode302
include/v8-profiler.h:302: const HeapGraphNode* GetAdditionsHead() const;
Using the names GetAdditionsRootNode and GetDeletionsRootNode would match the
comment better.

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

http://codereview.chromium.org/3020002/diff/1/9#newcode917
src/profile-generator.cc:917: namespace {
I think we should remove this local namespace for consistency with the rest of
the code base.

http://codereview.chromium.org/3020002/diff/1/9#newcode932
src/profile-generator.cc:932: namespace {
Ditto.

http://codereview.chromium.org/3020002/diff/1/9#newcode987
src/profile-generator.cc:987: // with the first color for caclulating the total
size.
caclulating -> calculating

http://codereview.chromium.org/3020002/diff/1/9#newcode1518
src/profile-generator.cc:1518: return (*entry1_ptr)->id() == (*entry2_ptr)->id()
? 0
I would prefer:

if ((*entry1_ptr)->id() == (*entry2_ptr)->id()) return 0;
return (*entry1_ptr)->id() < (*entry2_ptr)->id() ? -1 : 1;

for readability.

http://codereview.chromium.org/3020002/diff/1/9#newcode1573
src/profile-generator.cc:1573: if (entry != NULL) {
Do you ever want to call MoveObject on an object you don't know is there? I'm
wondering if this should be an assert instead?

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

http://codereview.chromium.org/3020002/diff/1/10#newcode532
src/profile-generator.h:532: void ApplyAndPaintAllReachable(Visitor* visitor);
Could you add an empty line after this function? I think that improves
readability because it spans two lines because it is a template method.

http://codereview.chromium.org/3020002/diff/1/10#newcode719
src/profile-generator.h:719: uint64_t id;
id -> id_

http://codereview.chromium.org/3020002/diff/1/10#newcode720
src/profile-generator.h:720: bool accessed;
accessed -> accessed_

http://codereview.chromium.org/3020002/diff/1/10#newcode727
src/profile-generator.h:727: INLINE(static bool AddressesMatch(void* key1, void*
key2)) {
Is there a measurable effect of using the INLINE macro instead of just putting
in 'inline' here?

http://codereview.chromium.org/3020002/diff/1/10#newcode821
src/profile-generator.h:821: // HeapObject address -> id
Could you use a complete sentence and end with period? 'Mapping from HeapObject
addresses to ids.'

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

http://codereview.chromium.org/3020002/diff/1/11#newcode644
test/cctest/test-heap-profiler.cc:644: v8::Handle<v8::Context> env =
v8::Context::New();
This comment probably applies to this entire file. Could you use a stack
allocated LocalContext object as is done in test-api.cc?

The problem is that these tests allocate and enter contexts without leaving and
deallocating them. At the end there should be a env->Exit() and a
env->Dispose(). Having a scoped object that does this on destruction is what we
do in the API tests.

[mnaganov (inactive), 2010-07-15 13:19:48]

Thanks for your time!

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

http://codereview.chromium.org/3020002/diff/1/2#newcode302
include/v8-profiler.h:302: const HeapGraphNode* GetAdditionsHead() const;
On 2010/07/15 10:55:31, Mads Ager wrote:
> Using the names GetAdditionsRootNode and GetDeletionsRootNode would match the
> comment better.


A good point. For some reason I thought that CPU's profiles root node is called
'Head', but I've just checked and it's not ;) So I'll rename HeapSnapshot's
also, since it is not yet used in Chromium, so nobody will be broken.

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

http://codereview.chromium.org/3020002/diff/1/9#newcode917
src/profile-generator.cc:917: namespace {
On 2010/07/15 10:55:31, Mads Ager wrote:
> I think we should remove this local namespace for consistency with the rest of
> the code base.

OK, removed here and everywhere in this source file. Although, by the style
guide, we should be using them for internal classes.

http://codereview.chromium.org/3020002/diff/1/9#newcode932
src/profile-generator.cc:932: namespace {
On 2010/07/15 10:55:31, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/3020002/diff/1/9#newcode987
src/profile-generator.cc:987: // with the first color for caclulating the total
size.
On 2010/07/15 10:55:31, Mads Ager wrote:
> caclulating -> calculating

Done.

http://codereview.chromium.org/3020002/diff/1/9#newcode1518
src/profile-generator.cc:1518: return (*entry1_ptr)->id() == (*entry2_ptr)->id()
? 0
On 2010/07/15 10:55:31, Mads Ager wrote:
> I would prefer:
> 
> if ((*entry1_ptr)->id() == (*entry2_ptr)->id()) return 0;
> return (*entry1_ptr)->id() < (*entry2_ptr)->id() ? -1 : 1;
> 
> for readability.

Done.

http://codereview.chromium.org/3020002/diff/1/9#newcode1573
src/profile-generator.cc:1573: if (entry != NULL) {
On 2010/07/15 10:55:31, Mads Ager wrote:
> Do you ever want to call MoveObject on an object you don't know is there? I'm
> wondering if this should be an assert instead?

I'm not considering all heap objects when creating snapshots, just because not
all of them make sense to JS developer. For such objects, there will be no entry
in the map. So this code is fine.

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

http://codereview.chromium.org/3020002/diff/1/10#newcode532
src/profile-generator.h:532: void ApplyAndPaintAllReachable(Visitor* visitor);
On 2010/07/15 10:55:31, Mads Ager wrote:
> Could you add an empty line after this function? I think that improves
> readability because it spans two lines because it is a template method.

Done.

http://codereview.chromium.org/3020002/diff/1/10#newcode719
src/profile-generator.h:719: uint64_t id;
On 2010/07/15 10:55:31, Mads Ager wrote:
> id -> id_

The style guide says that struct members should not have an underscore suffix
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...).
And using 'struct' is legitimate here, as it's a pure data object.

http://codereview.chromium.org/3020002/diff/1/10#newcode720
src/profile-generator.h:720: bool accessed;
On 2010/07/15 10:55:31, Mads Ager wrote:
> accessed -> accessed_

Ditto.

http://codereview.chromium.org/3020002/diff/1/10#newcode727
src/profile-generator.h:727: INLINE(static bool AddressesMatch(void* key1, void*
key2)) {
On 2010/07/15 10:55:31, Mads Ager wrote:
> Is there a measurable effect of using the INLINE macro instead of just putting
> in 'inline' here?

I think, no. I'm not aware of our guidelines about using INLINE vs 'inline'. The
all-caps macro adds more visual clutter. So, OK, it's better not to use it here.
Removed.

http://codereview.chromium.org/3020002/diff/1/10#newcode821
src/profile-generator.h:821: // HeapObject address -> id
On 2010/07/15 10:55:31, Mads Ager wrote:
> Could you use a complete sentence and end with period? 'Mapping from
HeapObject
> addresses to ids.'

Fixed here and in other places in this file.

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

http://codereview.chromium.org/3020002/diff/1/11#newcode644
test/cctest/test-heap-profiler.cc:644: v8::Handle<v8::Context> env =
v8::Context::New();
On 2010/07/15 10:55:31, Mads Ager wrote:
> This comment probably applies to this entire file. Could you use a stack
> allocated LocalContext object as is done in test-api.cc?
> 
> The problem is that these tests allocate and enter contexts without leaving
and
> deallocating them. At the end there should be a env->Exit() and a
> env->Dispose(). Having a scoped object that does this on destruction is what
we
> do in the API tests.

Good catch! Fixed in all tests here.

[Mads Ager (chromium), 2010-07-15 13:25:42]

LGTM