Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10012013:
Get rid of HeapEntry to ID map in heap profile serializer (Closed)
DescriptionWe can avoid putting all nodes into a hash map from HeapEntry to ID and sorting that map as the nodes are already stored in right order in HeapSnapshot::entries_ list.
Committed: https://code.google.com/p/v8/source/detail?r=11245
Patch Set 1 #
Total comments: 12
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 13 (0 generated)
https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:1293: return &sorted_entries_; It is not clear for me why we still need this sorted version of entries list in the snapshot. As far as I can see we are using it only for GetNodeById functionality. But It would be simpler to use the other entries map from HeapObjectsMap class. It is sorted by design and it always represent the latest known state of the heap. Probably the only thing that we have to do is add and maintain address property in EntryInfo structure.
https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:1293: return &sorted_entries_; On 2012/04/05 19:58:09, loislo wrote: > It is not clear for me why we still need this sorted version of entries list in > the snapshot. As far as I can see we are using it only for GetNodeById > functionality. But It would be simpler to use the other entries map from > HeapObjectsMap class. It is sorted by design and it always represent the latest > known state of the heap. > Probably the only thing that we have to do is add and maintain address property > in EntryInfo structure. In the past, the sorted was also needed for something else. You are right that now finding the object by id is the only remaining case. But we can't use HeapObjectsMap: a) it's a hash map, so there is no order, b) it operates on heap object addresses, not on heap snapshot entries' addresses. https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:1540: root_entry = NULL; What's the point in resetting this variable? https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:3647: nodes[0]->set_entry_index(prev_value); If you are now storing entry indexes in snapshot nodes, there is no need to calculate them on every serialization. Or. If it's OK to calculate them on every serialization -- why not to use a local hash map to avoid increasing memory usage per node?
https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:1293: return &sorted_entries_; Please correct me if I'm wrong but I see that HeapObjectsMap has two members. The map from address to index in entries_. And entries_ list. This list is sorted by Id and we can use it for GetNodeById. As I already wrote the only thing we have to do is add an address member into EntryInfo struct. https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:1293: return &sorted_entries_; On 2012/04/05 22:54:55, Mikhail Naganov (Chromium) wrote: > On 2012/04/05 19:58:09, loislo wrote: > > It is not clear for me why we still need this sorted version of entries list > in > > the snapshot. As far as I can see we are using it only for GetNodeById > > functionality. But It would be simpler to use the other entries map from > > HeapObjectsMap class. It is sorted by design and it always represent the > latest > > known state of the heap. > > Probably the only thing that we have to do is add and maintain address > property > > in EntryInfo structure. > > In the past, the sorted was also needed for something else. You are right that > now finding the object by id is the only remaining case. But we can't use > HeapObjectsMap: a) it's a hash map, so there is no order, b) it operates on heap > object addresses, not on heap snapshot entries' addresses. please correct me if I missed something but HeapObjectsMap has two members. The list of EntryInfo and the map from address to index in this list. My idea was to append address field into EntryInfo. After that we will be able to find the address for Id. It will cost us additional space and log2 time for GetNodeById as it is now. https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:3647: nodes[0]->set_entry_index(prev_value); On 2012/04/05 22:54:55, Mikhail Naganov (Chromium) wrote: > If you are now storing entry indexes in snapshot nodes, there is no need to > calculate them on every serialization. > > Or. If it's OK to calculate them on every serialization -- why not to use a > local hash map to avoid increasing memory usage per node? these indexes are different for different snapshots. Actually yurys uses this field as a temporary storage. It is a trade off between memory and time. At this moment we are fighting against time :)
https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:1293: return &sorted_entries_; after adding address field into EntryInfo we will have another gain. it is a simplification of RemoveDeadEntries method. At this moment it creates a fresh list for living entries, a list for dead entries and has second cycle for removing dead entries from the map. With this field we will be able to do everything in one pass without additional memory.
https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:3647: nodes[0]->set_entry_index(prev_value); looks like I'm wrong here. It is not a temporary storage.
https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:1293: return &sorted_entries_; On 2012/04/06 06:23:08, loislo wrote: > Please correct me if I'm wrong but I see that HeapObjectsMap has two members. > The map from address to index in entries_. And entries_ list. This list is > sorted by Id and we can use it for GetNodeById. As I already wrote the only > thing we have to do is add an address member into EntryInfo struct. Sounds like a good idea but I would prefer this to go in a separate patch. https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:1540: root_entry = NULL; On 2012/04/05 22:54:55, Mikhail Naganov (Chromium) wrote: > What's the point in resetting this variable? It is just for clarity so root_entry is not used after it's been removed from the hash map. https://chromiumcodereview.appspot.com/10012013/diff/1/src/profile-generator.... src/profile-generator.cc:3647: nodes[0]->set_entry_index(prev_value); On 2012/04/05 22:54:55, Mikhail Naganov (Chromium) wrote: > If you are now storing entry indexes in snapshot nodes, there is no need to > calculate them on every serialization. > You are right. We may calculate them only once. Fixed. > Or. If it's OK to calculate them on every serialization -- why not to use a > local hash map to avoid increasing memory usage per node? At the moment we are trying reduce the time required to take snapshot. Also adding a field to a node is not a big deal given that we have an order of magnitude more edges than nodes.
lgtm
https://chromiumcodereview.appspot.com/10012013/diff/1003/src/profile-generat... File src/profile-generator.cc (right): https://chromiumcodereview.appspot.com/10012013/diff/1003/src/profile-generat... src/profile-generator.cc:3649: for (int i = 1; i < nodes.length(); ++i) { too complicated. why not just: index = 1; for (i = 0; i < nodes.length(); ++i) { nodes[i]->set_entry_index(index); index += ... } https://chromiumcodereview.appspot.com/10012013/diff/1003/src/profile-generat... File src/profile-generator.h (right): https://chromiumcodereview.appspot.com/10012013/diff/1003/src/profile-generat... src/profile-generator.h:611: int entry_index_; why don't reuse ordered_index_ and eliminate the union?
https://chromiumcodereview.appspot.com/10012013/diff/1003/src/profile-generat... File src/profile-generator.cc (right): https://chromiumcodereview.appspot.com/10012013/diff/1003/src/profile-generat... src/profile-generator.cc:3649: for (int i = 1; i < nodes.length(); ++i) { On 2012/04/06 11:55:08, alexeif wrote: > too complicated. why not just: > index = 1; > for (i = 0; i < nodes.length(); ++i) { > nodes[i]->set_entry_index(index); > index += ... > } Done. https://chromiumcodereview.appspot.com/10012013/diff/1003/src/profile-generat... File src/profile-generator.h (right): https://chromiumcodereview.appspot.com/10012013/diff/1003/src/profile-generat... src/profile-generator.h:611: int entry_index_; On 2012/04/06 11:55:08, alexeif wrote: > why don't reuse ordered_index_ and eliminate the union? Now ordered_index_ is reused for retained_size_, you suggest us reuse it for entry_index_. I don't see much sense in that at the moment.
lgtm
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
