The main comment is about the names. The rest are just observations.
https://chromiumcodereview.appspot.com/10854043/diff/1/src/heap.cc File src/heap.cc (right): https://chromiumcodereview.appspot.com/10854043/diff/1/src/heap.cc#newcode464 src/heap.cc:464: isolate_->counters()->total_heap_size()->AddSample(CommittedMemory() / KB); It doesn't make much of a difference, but we could sample these last four numbers even if CommittedMemory() == 0. https://chromiumcodereview.appspot.com/10854043/diff/1/src/v8-counters.cc File src/v8-counters.cc (right): https://chromiumcodereview.appspot.com/10854043/diff/1/src/v8-counters.cc#newcode49 src/v8-counters.cc:49: Histogram name = { #caption, 1000, 500000, 50, NULL, false }; \ This seems to set the maximum value for memory histograms to ~500MB, the total heap size can easily exceed that. On ia32 we limit the old generation to 700MB, on x64 to 1400MB. I'm not that familiar with how the histograms work and if that poses a problem, just wanted to point it out. https://chromiumcodereview.appspot.com/10854043/diff/1/src/v8-counters.h File src/v8-counters.h (right): https://chromiumcodereview.appspot.com/10854043/diff/1/src/v8-counters.h#newcode79 src/v8-counters.h:79: HM(total_cell_space_size, V8.MemoryCellSpaceSize) We should start using consistent names for the heap-related counters and histograms, otherwise this will get out of hand. Especially when it comes to "live" vs. "used" and "total" vs. "committed". How about the following names for the new histograms? WDYT? - heap_fraction_map_space - heap_fraction_cell_space - heap_sample_[total_]committed (the "total_" could be omitted) - heap_sample_[total_]used (likewise) - heap_sample_map_space_committed - heap_sample_cell_space_committed I have no strong opinion about the externally visible names, so we can leave them as they are. https://chromiumcodereview.appspot.com/10854043/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
