Put empty pages discovered during sweeping to the end of the list of pages...

Put empty pages discovered during sweeping to the end of the list of pages
instead of adding them to the free list.

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

[Vyacheslav Egorov (Chromium), 2010-04-20 15:56:35]

Kevin do you have enough time in your freelist to take a look?

// It is reincarnation of CL#1659010 which I accidentally deleted.

[Mads Ager (chromium), 2010-04-22 13:19:28]

LGTM with these comments addressed.

http://codereview.chromium.org/1683001/diff/1/8
File src/mark-compact.cc (right):

http://codereview.chromium.org/1683001/diff/1/8#newcode1066
src/mark-compact.cc:1066: p->ObjectAreaStart(),
I believe the indentation of these arguments were correct before. When all
arguments are on a new line, they should have a four space indent compared to
the method name.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

http://codereview.chromium.org/1683001/diff/1/8#newcode1289
src/mark-compact.cc:1289: // of pages without live objects and free them
(instead of putting it to free
it to -> them on the?

http://codereview.chromium.org/1683001/diff/1/8#newcode1346
src/mark-compact.cc:1346: int size_in_bytes =
static_cast<int>(p->AllocationTop() - free_start);
Isn't there a corner case here where size_in_bytes is incorrect? If the page is
empty because nothing is allocated (p->ObjectAreaStart() == p->AllocationTop())
then free_start will be NULL at this point which will make size_in_bytes much
bigger than the actual 0 bytes?

http://codereview.chromium.org/1683001/diff/1/8#newcode1358
src/mark-compact.cc:1358: // We have to deallocate and put into freelist ending
area
I think this comment could be a bit clearer. How about something like: 

If there is a free ending area on one of the previous pages we deallocate that
area and put it on the free list?

http://codereview.chromium.org/1683001/diff/1/8#newcode1382
src/mark-compact.cc:1382: // to the beggining of first empty page.
beggining -> beginning.

http://codereview.chromium.org/1683001/diff/1/8#newcode1395
src/mark-compact.cc:1395: if (first_empty_page == NULL) {
Combine all of this into one ASSERT or use #ifdef DEBUG?

http://codereview.chromium.org/1683001/diff/1/6
File src/spaces.cc (right):

http://codereview.chromium.org/1683001/diff/1/6#newcode675
src/spaces.cc:675: if (prev->is_valid())
Please use braces around the body of a multi-line if statement.

http://codereview.chromium.org/1683001/diff/1/6#newcode683
src/spaces.cc:683: if (p->WasInUseBeforeMC())
Ditto.

http://codereview.chromium.org/1683001/diff/1/6#newcode691
src/spaces.cc:691: if (last_page->WasInUseBeforeMC())
Ditto.

http://codereview.chromium.org/1683001/diff/1/6#newcode755
src/spaces.cc:755: pages_list_is_chunk_ordered_ = true;
pages_list -> page_list

http://codereview.chromium.org/1683001/diff/1/5
File src/spaces.h (right):

http://codereview.chromium.org/1683001/diff/1/5#newcode283
src/spaces.h:283: intptr_t flags;
Is this just a bit field?  If so, why use an intptr_t here instead of an int?

http://codereview.chromium.org/1683001/diff/1/5#newcode433
src/spaces.h:433: // and p precedes q in C or p and q belong to the different
chunks and p is last
the different chunks -> different chunks.
p is last -> p is the last.

http://codereview.chromium.org/1683001/diff/1/5#newcode550
src/spaces.h:550: static void RelinkPagesListInChunkOrder(PagedSpace* space,
PagesList -> PageList

[Vyacheslav Egorov (Chromium), 2010-04-22 14:27:12]

Thanks for the review Mads! 

I fixed glitches and I am going to commit this change.

http://codereview.chromium.org/1683001/diff/1/8
File src/mark-compact.cc (right):

http://codereview.chromium.org/1683001/diff/1/8#newcode1066
src/mark-compact.cc:1066: p->ObjectAreaStart(),
On 2010/04/22 13:19:29, Mads Ager wrote:
> I believe the indentation of these arguments were correct before. When all
> arguments are on a new line, they should have a four space indent compared to
> the method name.
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

Done. It seems there is a bug/inconsistency in the emacs indenter. Apparently it
thinks that ProcessNonLive is function we are calling.

http://codereview.chromium.org/1683001/diff/1/8#newcode1289
src/mark-compact.cc:1289: // of pages without live objects and free them
(instead of putting it to free
On 2010/04/22 13:19:29, Mads Ager wrote:
> it to -> them on the?

Done.

http://codereview.chromium.org/1683001/diff/1/8#newcode1346
src/mark-compact.cc:1346: int size_in_bytes =
static_cast<int>(p->AllocationTop() - free_start);
On 2010/04/22 13:19:29, Mads Ager wrote:
> Isn't there a corner case here where size_in_bytes is incorrect? If the page
is
> empty because nothing is allocated (p->ObjectAreaStart() ==
p->AllocationTop())
> then free_start will be NULL at this point which will make size_in_bytes much
> bigger than the actual 0 bytes?

Indeed! Thanks for spotting this. This loop is awfully complicated as was
rewritten several times so I missed it.

http://codereview.chromium.org/1683001/diff/1/8#newcode1358
src/mark-compact.cc:1358: // We have to deallocate and put into freelist ending
area
On 2010/04/22 13:19:29, Mads Ager wrote:
> I think this comment could be a bit clearer. How about something like: 
> 
> If there is a free ending area on one of the previous pages we deallocate that
> area and put it on the free list?

Done.

http://codereview.chromium.org/1683001/diff/1/8#newcode1382
src/mark-compact.cc:1382: // to the beggining of first empty page.
On 2010/04/22 13:19:29, Mads Ager wrote:
> beggining -> beginning.

Done.

http://codereview.chromium.org/1683001/diff/1/8#newcode1395
src/mark-compact.cc:1395: if (first_empty_page == NULL) {
On 2010/04/22 13:19:29, Mads Ager wrote:
> Combine all of this into one ASSERT or use #ifdef DEBUG?

Done.

http://codereview.chromium.org/1683001/diff/1/6
File src/spaces.cc (right):

http://codereview.chromium.org/1683001/diff/1/6#newcode675
src/spaces.cc:675: if (prev->is_valid())
On 2010/04/22 13:19:29, Mads Ager wrote:
> Please use braces around the body of a multi-line if statement.

Done.

http://codereview.chromium.org/1683001/diff/1/6#newcode683
src/spaces.cc:683: if (p->WasInUseBeforeMC())
On 2010/04/22 13:19:29, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/1683001/diff/1/6#newcode691
src/spaces.cc:691: if (last_page->WasInUseBeforeMC())
On 2010/04/22 13:19:29, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/1683001/diff/1/6#newcode755
src/spaces.cc:755: pages_list_is_chunk_ordered_ = true;
On 2010/04/22 13:19:29, Mads Ager wrote:
> pages_list -> page_list

Done.

http://codereview.chromium.org/1683001/diff/1/5
File src/spaces.h (right):

http://codereview.chromium.org/1683001/diff/1/5#newcode283
src/spaces.h:283: intptr_t flags;
On 2010/04/22 13:19:29, Mads Ager wrote:
> Is this just a bit field?  If so, why use an intptr_t here instead of an int?

It's legacy. I used this field to link pages into auxiliary list.  
Removed.

http://codereview.chromium.org/1683001/diff/1/5#newcode433
src/spaces.h:433: // and p precedes q in C or p and q belong to the different
chunks and p is last
On 2010/04/22 13:19:29, Mads Ager wrote:
> the different chunks -> different chunks.
> p is last -> p is the last.

Done.

http://codereview.chromium.org/1683001/diff/1/5#newcode550
src/spaces.h:550: static void RelinkPagesListInChunkOrder(PagedSpace* space,
On 2010/04/22 13:19:29, Mads Ager wrote:
> PagesList -> PageList

Done.