LGTM
http://codereview.chromium.org/3060008/diff/1/6 File src/profile-generator.cc (right): http://codereview.chromium.org/3060008/diff/1/6#newcode1053 src/profile-generator.cc:1053: // To calculate non-shared total size, first we paint all reachable Maybe change the term "total size" to something like "total reachable size" or "total retaining size". http://codereview.chromium.org/3060008/diff/1/6#newcode1311 src/profile-generator.cc:1311: calculated_data_.Add(HeapEntryCalculatedData()); These are just cached for the whole lifetime of the heap snapshot, right? http://codereview.chromium.org/3060008/diff/1/6#newcode1768 src/profile-generator.cc:1768: // If GPC references an object that we have interest in, add the object. Where does the "we have interest in" come into play? Does it refer to WillAddEntry below? http://codereview.chromium.org/3060008/diff/1/7 File src/profile-generator.h (right): http://codereview.chromium.org/3060008/diff/1/7#newcode431 src/profile-generator.h:431: CONTEXT_VARIABLE = v8::HeapGraphEdge::CONTEXT_VARIABLE, Any particular reason why you are not using the kXxxYxx naming for the enum's? http://codereview.chromium.org/3060008/diff/1/7#newcode456 src/profile-generator.h:456: struct { Do you need to put these in a struct? http://codereview.chromium.org/3060008/diff/1/7#newcode501 src/profile-generator.h:501: CLOSURE = v8::HeapGraphNode::CLOSURE Maybe add a "number of types" to the enum, and have a static assert that you only need 3 bits for the type. http://codereview.chromium.org/3060008/diff/1/7#newcode514 src/profile-generator.h:514: HeapSnapshot* snapshot() { return snapshot_; } Maybe you can avoid this member if these objects are allocated in large contiguous arrays, then an address ranges could determine the heap snapshot. Just a suggestion for future changes. http://codereview.chromium.org/3060008/diff/1/7#newcode557 src/profile-generator.h:557: static int EntriesSize(int entries_count, Maybe this should be a HeapSnapshot method. http://codereview.chromium.org/3060008/diff/1/7#newcode570 src/profile-generator.h:570: HeapSnapshot* snapshot_; Maybe you can avoid this member (see comment above). http://codereview.chromium.org/3060008/diff/1/7#newcode571 src/profile-generator.h:571: struct { struct needed? http://codereview.chromium.org/3060008/diff/1/7#newcode574 src/profile-generator.h:574: int calculated_data_index: 27; Please add a comment on where the calculated data is stored. http://codereview.chromium.org/3060008/diff/1/7#newcode830 src/profile-generator.h:830: A short comment on how this is used, and the use of aliasing will be helpful. http://codereview.chromium.org/3060008/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
