LGTM with comments. Are all the classes in the heap-profile.h used outside heap-profile.cc otherwise consider moving them to heap-profile.h.
I am looking forward to trying this out. http://codereview.chromium.org/200132/diff/1/3 File src/heap-profiler.cc (right): http://codereview.chromium.org/200132/diff/1/3#newcode78 Line 78: } else if ((*o)->IsFixedArray() && !insideArray_) { Please comment on this logic. I assume you are traversing the properties and elements of JSObjects here. http://codereview.chromium.org/200132/diff/1/3#newcode110 Line 110: ConstructorHeapProfile::TreeConfig::kNoValue; Initialize kNoValue to something. http://codereview.chromium.org/200132/diff/1/3#newcode114 Line 114: : zscope_(DELETE_ON_EXIT) { 4 space indent of :. http://codereview.chromium.org/200132/diff/1/3#newcode204 Line 204: if (this == &src) return *this; Add {} to if statement. http://codereview.chromium.org/200132/diff/1/3#newcode216 Line 216: if (cmp != 0) return cmp; Add {} to if statements. http://codereview.chromium.org/200132/diff/1/3#newcode221 Line 221: if (cmp != 0) return cmp; Add {} to if statement. http://codereview.chromium.org/200132/diff/1/3#newcode238 Line 238: if (!cluster.can_be_coarsed()) return; Add {} to if statement. http://codereview.chromium.org/200132/diff/1/3#newcode254 Line 254: if (currentSet_->Find(eq, &loc)) return; Add {} to if statement. http://codereview.chromium.org/200132/diff/1/3#newcode270 Line 270: if (last_eq_clusters == curr_eq_clusters) break; Add {} to if statement. http://codereview.chromium.org/200132/diff/1/3#newcode290 Line 290: if (!cluster.can_be_coarsed()) return JSObjectsCluster(); Add {} to if statement. http://codereview.chromium.org/200132/diff/1/3#newcode329 Line 329: const JSObjectsCluster ClustersCoarser::ClusterEqualityConfig::kNoKey; Initialize? (times 3) http://codereview.chromium.org/200132/diff/1/3#newcode352 Line 352: if (constructor == Heap::Object_symbol() || Please add {}'s to if statement. http://codereview.chromium.org/200132/diff/1/3#newcode415 Line 415: if (coarser_.HasAnEquivalent(cluster)) return; Add {} to if statement. http://codereview.chromium.org/200132/diff/1/3#newcode435 Line 435: ++retainers_printed_; // avoid printing ellipsis next time. Is there any way to add "(XXX more)" when done? http://codereview.chromium.org/200132/diff/1/3#newcode446 Line 446: if (coarse_cluster_tree_->Insert(eq, &loc)) { Please comment somewhere that the coarse_cluster_tree_ is used to avoid repeatedly reporting of the same cluster - and maybe give it a different name. Using a member variable for this type of temporary state is a bit confusing I think. http://codereview.chromium.org/200132/diff/1/4 File src/heap-profiler.h (right): http://codereview.chromium.org/200132/diff/1/4#newcode91 Line 91: : constructor_(constructor), instance_(NULL) {} 4 space indent of : here (and two times just below) http://codereview.chromium.org/200132/diff/1/4#newcode93 Line 93: : constructor_(reinterpret_cast<String*>(special)), instance_(NULL) {} Casting 1 and 2 to String* seems a bit hackish. How about just allocating two strings for this. They could perhaps be symbols with illegal JavaScript identifier names. http://codereview.chromium.org/200132/diff/1/4#newcode192 Line 192: static const int INITIAL_BACKREFS_LIST_CAPACITY = 2; Please use kXxxYyy format for constants (times 3). http://codereview.chromium.org/200132/diff/1/4#newcode228 Line 228: static const int MAX_RETAINERS_TO_PRINT = 50; Please use kMaxRetainersToPrint. http://codereview.chromium.org/200132/diff/1/12 File test/cctest/test-heap-profiler.cc (right): http://codereview.chromium.org/200132/diff/1/12#newcode86 Line 86: if (ref1 != NULL) o_tree->Insert(*ref1, &loc); Add {} to if statement. http://codereview.chromium.org/200132/diff/1/12#newcode286 Line 286: if (pos != NULL) ++pos; Add {} to if statement. http://codereview.chromium.org/200132 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
