Refactor MemoryAllocator to allow big normal pages

Refactor MemoryAllocator to allow big normal pages (currently 1mb).

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

[Vyacheslav Egorov (Chromium), 2010-12-21 18:27:37]

Erik,

This is mostly request for comments. Please start from changes in
spaces.h/spaces.cc.

There are still some commented tests, no support for win/mac platforms, etc.

I will continue to work on these issues.

But I would appreciate your comments on the refactoring.

Thanks.

[Erik Corry, 2010-12-22 13:48:27]

spaces.h and spaces.cc lgtm

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

http://codereview.chromium.org/5987005/diff/1/src/spaces.cc#newcode326
src/spaces.cc:326: void* MemoryAllocator::ReserveAlignedMemory(const size_t
requested,
Shouldn't this return an Address?

http://codereview.chromium.org/5987005/diff/1/src/spaces.cc#newcode862
src/spaces.cc:862: ASSERT(IsAddressAligned(chunk_base_,
2*maximum_semispace_capacity, 0));
spaces around '*'

http://codereview.chromium.org/5987005/diff/1/src/spaces.cc#newcode876
src/spaces.cc:876: address_mask_ = ~(2*maximum_semispace_capacity - 1);
and here and a few other places

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

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode116
src/spaces.h:116: // MemoryChunk represents memory region owned by a specific
space.
represents memory -> represents a memory

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode130
src/spaces.h:130: MemoryChunk* next_chunk() const { return next_; }
For getters the function is normally called the same as the variable, so the
variable should be called next_chunk_ or the getter should be called next()

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode154
src/spaces.h:154: return flags_ & (1 << flag);
Implicit conversions from int to bool are not allowed, so you need to add "!= 0"
here.

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode232
src/spaces.h:232: // Returns the next page of this page.
The next page in the chain of pages?

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode234
src/spaces.h:234: return static_cast<Page*>(next_chunk());
Should we assert that the next chunk is a page?

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode238
src/spaces.h:238: set_next_chunk(page);
And assert here too?

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode270
src/spaces.h:270: // only after PagedSpace::PrepareForMarkCompact was called.
Should the comment go away?

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode658
src/spaces.h:658: static size_t capacity_;
I don't like unsigned types much, so I tend to prefer intptr_t to size-t

[Vyacheslav Egorov (Chromium), 2010-12-22 19:55:07]

Erik,

I addressed your comments.

If there be no new ones I will land this CL tomorrow as we agreed.

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

http://codereview.chromium.org/5987005/diff/1/src/spaces.cc#newcode326
src/spaces.cc:326: void* MemoryAllocator::ReserveAlignedMemory(const size_t
requested,
On 2010/12/22 13:48:27, Erik Corry wrote:
> Shouldn't this return an Address?

Done. I eradicated void* usages.

http://codereview.chromium.org/5987005/diff/1/src/spaces.cc#newcode862
src/spaces.cc:862: ASSERT(IsAddressAligned(chunk_base_,
2*maximum_semispace_capacity, 0));
On 2010/12/22 13:48:27, Erik Corry wrote:
> spaces around '*'

Done.

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

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode116
src/spaces.h:116: // MemoryChunk represents memory region owned by a specific
space.
On 2010/12/22 13:48:27, Erik Corry wrote:
> represents memory -> represents a memory

Done.

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode130
src/spaces.h:130: MemoryChunk* next_chunk() const { return next_; }
On 2010/12/22 13:48:27, Erik Corry wrote:
> For getters the function is normally called the same as the variable, so the
> variable should be called next_chunk_ or the getter should be called next()

Done.

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode154
src/spaces.h:154: return flags_ & (1 << flag);
On 2010/12/22 13:48:27, Erik Corry wrote:
> Implicit conversions from int to bool are not allowed, so you need to add "!=
0"
> here.

Done.

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode234
src/spaces.h:234: return static_cast<Page*>(next_chunk());
On 2010/12/22 13:48:27, Erik Corry wrote:
> Should we assert that the next chunk is a page?

I added an assert to check that pages have the same owner.

There is currently no page type flag stored in the page itself.

Maybe we should consider eradicating LargePage type completely.

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode270
src/spaces.h:270: // only after PagedSpace::PrepareForMarkCompact was called.
On 2010/12/22 13:48:27, Erik Corry wrote:
> Should the comment go away?

Done.

http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode658
src/spaces.h:658: static size_t capacity_;
On 2010/12/22 13:48:27, Erik Corry wrote:
> I don't like unsigned types much, so I tend to prefer intptr_t to size-t

I like signed types :-)

But the reason I changed these declaration is different: 
interface to MemoryAllocator (and platform interface) was initially based on
size_t so casts inside were a bit weird.