Thanks, guys! Some of your comments were very helpful.
Currently, heap snapshots for real web apps are bigger than JS heap, because external strings contents get copied. But I plan to fix this soon. http://codereview.chromium.org/2722005/diff/5001/6002 File src/profile-generator.cc (right): http://codereview.chromium.org/2722005/diff/5001/6002#newcode832 src/profile-generator.cc:832: static void DeleteHeapGraphEdge(HeapGraphEdge** p_edge) { On 2010/06/10 13:45:26, tfarina wrote:
the prefix p_ stands for parameter? If so I'd drop it. Same below.
No, it stands for "pointer" to indicate that this is a double pointer. I really should use '_ptr' suffix for consistency with existing code. Fixed. http://codereview.chromium.org/2722005/diff/5001/6002#newcode876 src/profile-generator.cc:876: int HeapEntry::TotalSize() { On 2010/06/10 09:32:44, Søren Gjesse wrote:
Please use constant instead of -1.
Done. http://codereview.chromium.org/2722005/diff/5001/6002#newcode881 src/profile-generator.cc:881: int HeapEntry::NonSharedTotalSize() { On 2010/06/10 09:32:44, Søren Gjesse wrote:
Ditto.
Done. http://codereview.chromium.org/2722005/diff/5001/6002#newcode917 src/profile-generator.cc:917: int non_shared_total_size() { return non_shared_total_size_; } On 2010/06/10 13:45:26, tfarina wrote:
if we add 'const' this will break? If not, could you add it?
Done. http://codereview.chromium.org/2722005/diff/5001/6002#newcode919 src/profile-generator.cc:919: void Apply(HeapEntry* entry) { On 2010/06/10 09:32:44, Søren Gjesse wrote:
Please use constants for the different kind of paint. It is not clear
what the
difference between color 1 and 2 is.
Done. I clarified the code by introducing named constants and methods. http://codereview.chromium.org/2722005/diff/5001/6002#newcode931 src/profile-generator.cc:931: int HeapEntry::CalculateNonSharedTotalSize() { On 2010/06/10 09:32:44, Søren Gjesse wrote:
Please add some comments to indicate what the two loops does.
Done. http://codereview.chromium.org/2722005/diff/5001/6002#newcode965 src/profile-generator.cc:965: NonSharedSizeCalculator calculator; On 2010/06/10 09:32:44, Søren Gjesse wrote:
Looks as if the NonSharedSizeCalculator only looks at paint == 1. What
am I
missing here?
That's right. It only takes into account nodes which were not repainted into second color as reachable from other nodes. I added a comment describing the algorithm. http://codereview.chromium.org/2722005/diff/5001/6002#newcode1002 src/profile-generator.cc:1002: }; On 2010/06/10 09:32:44, Søren Gjesse wrote:
Please add another empty line.
Done. http://codereview.chromium.org/2722005/diff/5001/6002#newcode1112 src/profile-generator.cc:1112: case JS_OBJECT: return "/js obj/"; On 2010/06/10 09:32:44, Søren Gjesse wrote:
Why not just "object" instead of "js obj"?
Done. http://codereview.chromium.org/2722005/diff/5001/6002#newcode1233 src/profile-generator.cc:1233: next_gc_root_id_(1) { On 2010/06/10 09:32:44, Søren Gjesse wrote:
next_gc_root_id_ is not used.
Good catch. Removed. http://codereview.chromium.org/2722005/diff/5001/6002#newcode1448 src/profile-generator.cc:1448: HeapSnapshot* snapshot = new HeapSnapshot(this, name, uid); On 2010/06/10 09:32:44, Søren Gjesse wrote:
It is by design, that HeapSnapshotGenerator is nou used immediately to
fill the
snapshot?
I'm resembling the design of CPU profiler framework. http://codereview.chromium.org/2722005/diff/5001/6002#newcode1495 src/profile-generator.cc:1495: ExtractElementReferences(js_obj, entry); On 2010/06/10 09:32:44, Søren Gjesse wrote:
How about the references to the prototype and constructor?
A good point! I added 'prototype', but not 'constructor', because the latter is referenced from the former. Also, some prototypes has a 'constructor' property which for some reason references other value than 'map()->constructor()' -- confusing. http://codereview.chromium.org/2722005/diff/5001/6003 File src/profile-generator.h (right): http://codereview.chromium.org/2722005/diff/5001/6003#newcode576 src/profile-generator.h:576: }; On 2010/06/10 13:50:34, tfarina wrote:
if we don't allow copy and assign, DISALLOW_COPY_AND_ASSIGN?
Added here and in other classes. http://codereview.chromium.org/2722005/diff/5001/6003#newcode605 src/profile-generator.h:605: }; On 2010/06/10 13:50:34, tfarina wrote:
DISALLOW_COPY_AND_ASSIN? Same thing for the new classes below. Ignore
me if we
are using copy and assign of them.
Done. http://codereview.chromium.org/2722005/diff/5001/6003#newcode609 src/profile-generator.h:609: On 2010/06/10 09:32:44, Søren Gjesse wrote:
Please add a few lines describing these classes and how they connect.
I have
guessed that you create a SnapshotCollection, call NewSnapshot on that
and use
HeapSnapshotGenerator afterwards, sounds right?
Done. http://codereview.chromium.org/2722005/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
