Thanks for the review! Please take a look.
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(); On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
I'm not sure this will compile on all platforms. Have you checked
Win64 for
example?
I'm not sure, as I don't have a Windows machine. I added a cast anyway. 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), On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
Please introduce a constant instead of the magic value.
Done. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.cc#newcode1161 src/profile-generator.cc:1161: CHECK(root_entry_ == 0); On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
Are you using CHECKs here and below intentionally? They will remain in
release
mode.
I added them intentionally in places that are non performance critical. Is it wrong? Is there a policy on ASSERT vs. CHECK usage? https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.cc#newcode1204 src/profile-generator.cc:1204: children().Rewind(edges().length()); On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
Perhaps, add a helper method to the 'List' class for this sequence of
actions? Done. 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) { On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
I suppose, it will be a bit faster to set sorted_entries_ capacity by
entries_
size before filling.
Done. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.cc#newcode1281 src/profile-generator.cc:1281: entries_.capacity() * sizeof(HeapEntry) + On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
Perhaps, add an utility method to the 'List' class for this? It will
also make
the code resilient to list item type changes.
I'm not sure this function is generic enough to be put into the list. I made it static in the profile-generator.cc if you don't object. 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); On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
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.
Done. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode466 src/profile-generator.h:466: void UpdateToEntry(HeapSnapshot* snapshot); On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
Perhaps, rename this method to "ReplaceToIndexWithEntry", the current
name isn't
clear enough.
Done. 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, On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
nit: During entries population |to_| is used for storing the index,
afterwards
it is replaced with a pointer to the entry.
Done. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode495 src/profile-generator.h:495: int to_; On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
Perhaps, call this field 'to_index_' for clarity?
Done. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode519 src/profile-generator.h:519: void Init(HeapSnapshot* snapshot, On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
Also can be converted into a constructor.
Done. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode538 src/profile-generator.h:538: int set_children_index(int index) { On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
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).
Done. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode544 src/profile-generator.h:544: int set_retainers_index(int index) { On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
ditto
Done. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode586 src/profile-generator.h:586: HeapSnapshot* snapshot_; On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
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.
Done. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode1138 src/profile-generator.h:1138: static const int kEdgeFieldsCount = 3; On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
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.
Done. https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.h#newcode1140 src/profile-generator.h:1140: static const int kNodeFieldsCount = 7; On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote:
ditto
Done. https://chromiumcodereview.appspot.com/10353010/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
