Two-page newspace.

Two-page newspace.

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

[Erik Corry, 2011-06-07 08:49:55]

LGTM

It looks like you rely on the fact that pages are sorted in increasing address
order.  If you don't you will when you get the water mark to work right.  But I
can't see you assert it anywhere.

http://codereview.chromium.org/7124006/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7124006/diff/1/src/heap.cc#newcode4316
src/heap.cc:4316: void Heap::ZapFromSpace() {
I wonder why this isn't called NukeFromOrbit?

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

http://codereview.chromium.org/7124006/diff/1/src/spaces.cc#newcode1020
src/spaces.cc:1020: return false;
Why did AddFreshPage get called if the current page is empty?

http://codereview.chromium.org/7124006/diff/1/src/spaces.cc#newcode1023
src/spaces.cc:1023: // Failed to use a new page in to-space.
Failed to get?

http://codereview.chromium.org/7124006/diff/1/src/spaces.cc#newcode1026
src/spaces.cc:1026: // Clear remainder of current pag.
pag -> page

http://codereview.chromium.org/7124006/diff/1/src/spaces.cc#newcode1079
src/spaces.cc:1079: limit = (page == end_page) ? top() : page->body_limit();
This ternary operator together with the if above confuse me...

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

http://codereview.chromium.org/7124006/diff/1/src/spaces.h#newcode1637
src/spaces.h:1637: // The offset of an address from the beginning of the space.
We can't delete this function?

http://codereview.chromium.org/7124006/diff/1/src/spaces.h#newcode1964
src/spaces.h:1964: Address ToSpaceLow() { return to_space_.space_low(); }
This does not really make sense, at least for the purpose that the comment
claims it has.

http://codereview.chromium.org/7124006/diff/1/src/spaces.h#newcode1999
src/spaces.h:1999: 
Comment on what the return value means.

[Lasse Reichstein, 2011-06-07 09:36:06]

http://codereview.chromium.org/7124006/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7124006/diff/1/src/heap.cc#newcode4316
src/heap.cc:4316: void Heap::ZapFromSpace() {
We weren't sure.

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

http://codereview.chromium.org/7124006/diff/1/src/spaces.cc#newcode1020
src/spaces.cc:1020: return false;
Possibly because the size asked for was too large to be in one page.
That should probably be handled by the new-space-allocation-limit instead. I'll
add a TODO.

http://codereview.chromium.org/7124006/diff/1/src/spaces.cc#newcode1023
src/spaces.cc:1023: // Failed to use a new page in to-space.
On 2011/06/07 08:49:55, Erik Corry wrote:
> Failed to get?

Done.

http://codereview.chromium.org/7124006/diff/1/src/spaces.cc#newcode1026
src/spaces.cc:1026: // Clear remainder of current pag.
On 2011/06/07 08:49:55, Erik Corry wrote:
> pag -> page

Done.

http://codereview.chromium.org/7124006/diff/1/src/spaces.cc#newcode1079
src/spaces.cc:1079: limit = (page == end_page) ? top() : page->body_limit();
Rewritten.

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

http://codereview.chromium.org/7124006/diff/1/src/spaces.h#newcode1637
src/spaces.h:1637: // The offset of an address from the beginning of the space.
Whaddayaknow, we can!
And there was much rejoycing.

http://codereview.chromium.org/7124006/diff/1/src/spaces.h#newcode1964
src/spaces.h:1964: Address ToSpaceLow() { return to_space_.space_low(); }
Did it ever? The to_space_.high() value is the end of the committed space, which
is the same thing we get here.

http://codereview.chromium.org/7124006/diff/1/src/spaces.h#newcode1999
src/spaces.h:1999: 
Done.

[Lasse Reichstein, 2011-06-07 10:16:51]

And yes, I still rely on pages being ordered, but when I get watermark working
correctly, it shouldn't matter any more.