LGTM Sorry for the delay I must admit this got lost over Christmas.
The code is very readable and especially the ASSERT's provide good documentation
to what the code does. http://codereview.chromium.org/509035/diff/5006/5008 File src/mark-compact.cc (right): http://codereview.chromium.org/509035/diff/5006/5008#newcode1270 src/mark-compact.cc:1270: while (true) { Please make a comment about the fact that we know the exact number of maps after compaction and therefore using the next vacant map as the loop termination condition is ok. http://codereview.chromium.org/509035/diff/5006/5008#newcode1272 src/mark-compact.cc:1272: if (next_vacant_map == NULL) Maybe use this condition in the while loop instead of having a break. http://codereview.chromium.org/509035/diff/5006/5008#newcode1329 src/mark-compact.cc:1329: private: Maybe these names needs a bit of explanation, as both from and to space is the map space. As I see it the to space starts at the bottom of the map space and the from space starts where map space is known to stop when compacting is complete. Is that correct? http://codereview.chromium.org/509035/diff/5006/5008#newcode1347 src/mark-compact.cc:1347: void UpdatePointer(Object** p) { UpdatePointer -> UpdateMapPointer? http://codereview.chromium.org/509035/diff/5006/5008#newcode1366 src/mark-compact.cc:1366: if (next == last) Maybe put the MapIterator in a state where it will always respond false to has_next now that last is hit. http://codereview.chromium.org/509035/diff/5006/5008#newcode1412 src/mark-compact.cc:1412: ASSERT(Heap::map_space()->Contains(new_map)); Does this ASSERT check against the resulting size of map space after compaction? http://codereview.chromium.org/509035/diff/5006/5008#newcode1483 src/mark-compact.cc:1483: MapCompact map_compact(from_space); How about passing in Heap::map_space() explicitly as the to space here? http://codereview.chromium.org/509035/diff/5006/5007 File src/spaces.h (right): http://codereview.chromium.org/509035/diff/5006/5007#newcode1711 src/spaces.h:1711: void ResetFreeList() { ASSERT that the free list as actually empty at this point? http://codereview.chromium.org/509035
-- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
