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 On 2010/08/06 15:19:03, Søren Gjesse wrote:
Maybe change the term "total size" to something like "total reachable
size" or
"total retaining size".
Renamed "total size" -> "reachable size", "private (total non-shared) size" -> "retained size". http://codereview.chromium.org/3060008/diff/1/6#newcode1311 src/profile-generator.cc:1311: calculated_data_.Add(HeapEntryCalculatedData()); On 2010/08/06 15:19:03, Søren Gjesse wrote:
These are just cached for the whole lifetime of the heap snapshot,
right? Right. This data is lazily calculated and is stored outside HeapEntries to save space. 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. On 2010/08/06 15:19:03, Søren Gjesse wrote:
Where does the "we have interest in" come into play? Does it refer to WillAddEntry below?
Yes. I've enhanced the comment. 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, On 2010/08/06 15:19:03, Søren Gjesse wrote:
Any particular reason why you are not using the kXxxYxx naming for the
enum's? Sorry, bad habit. Renamed. http://codereview.chromium.org/3060008/diff/1/7#newcode456 src/profile-generator.h:456: struct { On 2010/08/06 15:19:03, Søren Gjesse wrote:
Do you need to put these in a struct?
No. Thanks for the hint! http://codereview.chromium.org/3060008/diff/1/7#newcode501 src/profile-generator.h:501: CLOSURE = v8::HeapGraphNode::CLOSURE On 2010/08/06 15:19:03, Søren Gjesse wrote:
Maybe add a "number of types" to the enum, and have a static assert
that you
only need 3 bits for the type.
No need for this: GCC has this check and emits a warning. http://codereview.chromium.org/3060008/diff/1/7#newcode514 src/profile-generator.h:514: HeapSnapshot* snapshot() { return snapshot_; } On 2010/08/06 15:19:03, Søren Gjesse wrote:
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.
A good idea, I will think about this optimization. http://codereview.chromium.org/3060008/diff/1/7#newcode557 src/profile-generator.h:557: static int EntriesSize(int entries_count, On 2010/08/06 15:19:03, Søren Gjesse wrote:
Maybe this should be a HeapSnapshot method.
My point is that HeapEntry's layout is encapsulated in it, and as size depends on layout, this method fits here better. http://codereview.chromium.org/3060008/diff/1/7#newcode570 src/profile-generator.h:570: HeapSnapshot* snapshot_; On 2010/08/06 15:19:03, Søren Gjesse wrote:
Maybe you can avoid this member (see comment above).
OK. http://codereview.chromium.org/3060008/diff/1/7#newcode571 src/profile-generator.h:571: struct { On 2010/08/06 15:19:03, Søren Gjesse wrote:
struct needed?
Removed. http://codereview.chromium.org/3060008/diff/1/7#newcode574 src/profile-generator.h:574: int calculated_data_index: 27; On 2010/08/06 15:19:03, Søren Gjesse wrote:
Please add a comment on where the calculated data is stored.
Done. http://codereview.chromium.org/3060008/diff/1/7#newcode830 src/profile-generator.h:830: On 2010/08/06 15:19:03, Søren Gjesse wrote:
A short comment on how this is used, and the use of aliasing will be
helpful. Done. http://codereview.chromium.org/3060008/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
