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