Compact map space when doing mark-sweep if after collection size of map space...

Compact map space when doing mark-sweep if after collection size of map space would
drop below threshold.


Committed: http://code.google.com/p/v8/source/detail?r=3600

[antonm, 2009-12-22 20:47:36]

Kevin and Søren,

that's not quite a call for review (but I would really appreciate if you could
comment).

The problem is I am not aware of the way to find out number of maps which are
marked.  Currently I piggyback on additional stuff in Debug mode (which actually
calculates the number of live maps).  Are you aware of any other ways to find
out number of live maps cheaply?

If not, I can see two ways out:

1) move live map (all objects?) counting logic out of debug mode (performance
overhead?);
2) use another compaction approach you suggested before (compacting from the end
if the space).

Modulo that this a working CL (in debug mode).

[antonm, 2009-12-23 07:37:36]

That should have been Søren instead of Kevic C.

Sorry for noise.

[antonm, 2009-12-24 16:08:07]

Kevin, Søren,

could you have now a look?

I opted to do an additional pass over map space to get as a side-effect number
of live maps.  I could blend compacting and sweeping, but the code would be more
complicated.  I hope that map compactions should not happen often, so that could
work fine.

[antonm, 2010-01-11 15:24:08]

ping

[Søren Thygesen Gjesse, 2010-01-12 08:07:14]

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?

[antonm, 2010-01-12 16:53:36]

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?

[Søren Thygesen Gjesse, 2010-01-13 08:55:29]

LGTM

http://codereview.chromium.org/509035/diff/5006/5008
File src/mark-compact.cc (right):

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 16:53:36, antonm wrote:
> 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?
> 

You are right it it both ugly and slow. Your suggestion to add some map space
verification sounds much better,

http://codereview.chromium.org/509035/diff/5006/5008#newcode1483
src/mark-compact.cc:1483: MapCompact map_compact(from_space);
On 2010/01/12 16:53:36, antonm wrote:
> 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 :)

My suggenstion was to add another parameter indicating the from space, but you
can ignore that. Thinking about it again how about dropping the current
parameter? It's up to you.

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 16:53:36, antonm wrote:
> 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?

I was not thinking clearly and figured that the free list was kept updated while
evacuating maps, and therefore should be empty in the end. It is empty in the
end so this is fine.

[antonm, 2010-01-13 19:15:36]

Thanks a lot for review, Søren, committing.

http://codereview.chromium.org/509035/diff/5006/5008
File src/mark-compact.cc (right):

http://codereview.chromium.org/509035/diff/5006/5008#newcode1412
src/mark-compact.cc:1412: ASSERT(Heap::map_space()->Contains(new_map));
On 2010/01/13 08:55:30, Søren Gjesse wrote:
> On 2010/01/12 16:53:36, antonm wrote:
> > 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?
> > 
> 
> You are right it it both ugly and slow. Your suggestion to add some map space
> verification sounds much better,

Ok.  I double checked and it looks like all the cctest are ran with verify_heap
flag on, so we should get it for free.  Just removing.

http://codereview.chromium.org/509035/diff/5006/5008#newcode1483
src/mark-compact.cc:1483: MapCompact map_compact(from_space);
On 2010/01/13 08:55:30, Søren Gjesse wrote:
> On 2010/01/12 16:53:36, antonm wrote:
> > 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 :)
> 
> My suggenstion was to add another parameter indicating the from space, but you
> can ignore that. Thinking about it again how about dropping the current
> parameter? It's up to you.

Good idea, done.  I used to depend on it in the body before, but not now.  Note:
couldn't drop it completely as it depends on live map count.

[Søren Thygesen Gjesse, 2010-01-14 08:28:07]

On 2010/01/13 19:15:36, antonm wrote:
> Thanks a lot for review, Søren, committing.
> 
> http://codereview.chromium.org/509035/diff/5006/5008
> File src/mark-compact.cc (right):
> 
> http://codereview.chromium.org/509035/diff/5006/5008#newcode1412
> src/mark-compact.cc:1412: ASSERT(Heap::map_space()->Contains(new_map));
> On 2010/01/13 08:55:30, Søren Gjesse wrote:
> > On 2010/01/12 16:53:36, antonm wrote:
> > > 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?
> > > 
> > 
> > You are right it it both ugly and slow. Your suggestion to add some map
space
> > verification sounds much better,
> 
> Ok.  I double checked and it looks like all the cctest are ran with
verify_heap
> flag on, so we should get it for free.  Just removing.
> 
> http://codereview.chromium.org/509035/diff/5006/5008#newcode1483
> src/mark-compact.cc:1483: MapCompact map_compact(from_space);
> On 2010/01/13 08:55:30, Søren Gjesse wrote:
> > On 2010/01/12 16:53:36, antonm wrote:
> > > 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 :)
> > 
> > My suggenstion was to add another parameter indicating the from space, but
you
> > can ignore that. Thinking about it again how about dropping the current
> > parameter? It's up to you.
> 
> Good idea, done.  I used to depend on it in the body before, but not now. 
Note:
> couldn't drop it completely as it depends on live map count.

I talked with Kasper about adding a test for this. How taking the JavaScript
code from test/mjsunit/regress/regress-524.js and allocate even more to exceed
the map compaction limit and then freeing up data to make map compaction run. It
will probably require a cctest.