Thanks a lot for comments, Søren.
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) { On 2010/01/12 08:07:14, Søren Gjesse wrote:
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.
Done. http://codereview.chromium.org/509035/diff/5006/5008#newcode1272 src/mark-compact.cc:1272: if (next_vacant_map == NULL) On 2010/01/12 08:07:14, Søren Gjesse wrote:
Maybe use this condition in the while loop instead of having a break.
Done. http://codereview.chromium.org/509035/diff/5006/5008#newcode1329 src/mark-compact.cc:1329: private: On 2010/01/12 08:07:14, Søren Gjesse wrote:
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?
Yes, precisely. How do you like new names? http://codereview.chromium.org/509035/diff/5006/5008#newcode1347 src/mark-compact.cc:1347: void UpdatePointer(Object** p) { On 2010/01/12 08:07:14, Søren Gjesse wrote:
UpdatePointer -> UpdateMapPointer?
Done. http://codereview.chromium.org/509035/diff/5006/5008#newcode1366 src/mark-compact.cc:1366: if (next == last) On 2010/01/12 08:07:14, Søren Gjesse wrote:
Maybe put the MapIterator in a state where it will always respond
false to
has_next now that last is hit.
Is there a way to achieve that? This functionality could be added to MapIterator, but I'd rather keep MapIterator as close to HeapObjectIterator as possible. HeapObjectIterator could be extended with new method though. Do you want to see it in this CL? http://codereview.chromium.org/509035/diff/5006/5008#newcode1412 src/mark-compact.cc:1412: ASSERT(Heap::map_space()->Contains(new_map)); On 2010/01/12 08:07:14, Søren Gjesse wrote:
Does this ASSERT check against the resulting size of map space after
compaction? Very good question. I don't think so as the space shouldn't have been shrunk yet. I've added the check, but it feels somewhat ugly to me (esp. this recalculation of # of live maps). Maybe we'd better add this check (if it's not present right now) into post GC verification code and remove it from here not to confuse readers? http://codereview.chromium.org/509035/diff/5006/5008#newcode1483 src/mark-compact.cc:1483: MapCompact map_compact(from_space); On 2010/01/12 08:07:14, Søren Gjesse wrote:
How about passing in Heap::map_space() explicitly as the to space
here? Sorry, don't quite understand you, could you elaborate? In case it's due to bad naming, just dropped the var :) 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() { On 2010/01/12 08:07:14, Søren Gjesse wrote:
ASSERT that the free list as actually empty at this point?
I am not sure it's empty, it should have been filled by sweeping. What am I missing? http://codereview.chromium.org/509035
-- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
