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=Variable_Names#Variable_Names). 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. http://codereview.chromium.org/3020002/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
