I like the simplification of the code and especially the speed boost!
https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator-inl.h File src/profile-generator-inl.h (right): https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator-inl.h#newcode109 src/profile-generator-inl.h:109: return this - &snapshot_->entries().first(); I'm not sure this will compile on all platforms. Have you checked Win64 for example? https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.cc#newcode1116 src/profile-generator.cc:1116: root_entry_(-1), Please introduce a constant instead of the magic value. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.cc#newcode1161 src/profile-generator.cc:1161: CHECK(root_entry_ == 0); Are you using CHECKs here and below intentionally? They will remain in release mode. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.cc#newcode1204 src/profile-generator.cc:1204: children().Rewind(edges().length()); Perhaps, add a helper method to the 'List' class for this sequence of actions? https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.cc#newcode1265 src/profile-generator.cc:1265: for (int i = 0; i < entries_.length(); ++i) { I suppose, it will be a bit faster to set sorted_entries_ capacity by entries_ size before filling. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.cc#newcode1281 src/profile-generator.cc:1281: entries_.capacity() * sizeof(HeapEntry) + Perhaps, add an utility method to the 'List' class for this? It will also make the code resilient to list item type changes. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h File src/profile-generator.h (right): https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode464 src/profile-generator.h:464: void Init(Type type, const char* name, int from, int to); You can make 'Init' to be a constructor now. The reason it was a separate function is because we were calling it on a pre-allocated memory area. You still need an empty constructor for List<HeapGraphEdge> though. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode466 src/profile-generator.h:466: void UpdateToEntry(HeapSnapshot* snapshot); Perhaps, rename this method to "ReplaceToIndexWithEntry", the current name isn't clear enough. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode493 src/profile-generator.h:493: // During entries population it uses |to_| field to store index, nit: During entries population |to_| is used for storing the index, afterwards it is replaced with a pointer to the entry. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode495 src/profile-generator.h:495: int to_; Perhaps, call this field 'to_index_' for clarity? https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode519 src/profile-generator.h:519: void Init(HeapSnapshot* snapshot, Also can be converted into a constructor. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode538 src/profile-generator.h:538: int set_children_index(int index) { This function isn't a trivial one-liner, so it needs to be named CamelCase and to be moved into the .cc file (or -inl.h file if you think it needs to be inlined). https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode544 src/profile-generator.h:544: int set_retainers_index(int index) { ditto https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode586 src/profile-generator.h:586: HeapSnapshot* snapshot_; Are you sure this reordering doesn't affect entry size on Win64? MSVC loves to add padding if wider values precede narrower ones. This was the reason why all pointers had been put at the end. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode1138 src/profile-generator.h:1138: static const int kEdgeFieldsCount = 3; I would suggest providing a storage for this constant in the .cc file. As it is used in expressions evaluated at runtime, some compilers might generate code assuming that is has storage, and the program will fail to link. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode1140 src/profile-generator.h:1140: static const int kNodeFieldsCount = 7; ditto https://chromiumcodereview.appspot.com/10353010/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
