https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8-profiler.h File include/v8-profiler.h (right):
https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8-profiler.h#newcode421 include/v8-profiler.h:421: * This method gives us an update of the heap state. Comments look a bit weird. How about putting StartHeapObjectsTracking first, and annotating it something like this: "Starts tracking of heap objects population statistics. After calling this method, all heap objects relocations done by the garbage collector are being registered." Then, for PushHeapObjectsStats we can write something like: "Adds a new time interval entry to the aggregated statistics array. The time interval entry contains information on the current heap objects population size. The method also updates aggregated statistics and reports updates for all previous time intervals via the OutputStream object. Updates on each time interval are provided as pairs of time interval index and updated heap objects count. StartHeapObjectsTracking must be called before the first call to this method." And for StopHeapObjectsTracking: "Stops tracking of heap objects population statistics, cleans up all collected data. StartHeapObjectsTracking must be called again prior to calling PushHeapObjectsStats next time." https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8-profiler.h#newcode428 include/v8-profiler.h:428: static void PushHeapObjectsStats(OutputStream* stream); nit: add a blank line before the comment https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8-profiler.h#newcode433 include/v8-profiler.h:433: static void StartHeapObjectsTracking(); nit: add a blank line before the comment https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8.h File include/v8.h (right): https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8.h#newcode3769 include/v8.h:3769: virtual WriteResult WriteUint32Chunk(uint32_t* data, int size) = 0; Is this really data size or items count? Looking at the implementation I'd say that this is count, not size. https://chromiumcodereview.appspot.com/10049002/diff/14001/src/profile-generator.cc File src/profile-generator.cc (right): https://chromiumcodereview.appspot.com/10049002/diff/14001/src/profile-generator.cc#newcode1453 src/profile-generator.cc:1453: ASSERT(entries_->length()); entries_->length() != 0 or, alternatively !entries_->is_empty() https://chromiumcodereview.appspot.com/10049002/diff/14001/src/profile-generator.cc#newcode1456 src/profile-generator.cc:1456: uint32_t count = 0; count -> entries_count ? https://chromiumcodereview.appspot.com/10049002/diff/14001/src/profile-generator.cc#newcode1470 src/profile-generator.cc:1470: stream->WriteUint32Chunk(&stats_buffer.first(), Have you considered checking the result and aborting the serialization? https://chromiumcodereview.appspot.com/10049002/diff/14001/src/profile-generator.cc#newcode1478 src/profile-generator.cc:1478: if (stats_buffer.length()) { stats_buffer.length() > 0 or, alternatively !stats_buffer.is_empty() https://chromiumcodereview.appspot.com/10049002/diff/14001/test/cctest/test-heap-profiler.cc File test/cctest/test-heap-profiler.cc (right): https://chromiumcodereview.appspot.com/10049002/diff/14001/test/cctest/test-heap-profiler.cc#newcode824 test/cctest/test-heap-profiler.cc:824: CHECK_EQ(2, stats_update.numbers_written()); Why no check for entries_count? https://chromiumcodereview.appspot.com/10049002/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
