- Do not bump allocate in old-space pages. Always use

- Do not bump allocate in old-space pages. Always use
  the freelist in preparation for chunked allocation.
Committed: https://code.google.com/p/dart/source/detail?r=13792

[cshapiro, 2012-10-17 02:53:22]

lgtm with comments

https://chromiumcodereview.appspot.com/11186013/diff/1/vm/pages.cc
File vm/pages.cc (right):

https://chromiumcodereview.appspot.com/11186013/diff/1/vm/pages.cc#newcode280
vm/pages.cc:280: uword obj_addr = page->first_object_start();
Why did this code have to change?  This was done in a few places.  I looked at
this code side by side with HeapPage::VisitObjects and it seems equivalent.

Anyhow, this expansion is repeated 2 times below.  For the sake of debugging and
test coverage, having one code path instead of 2 similar ones is strongly
preferable.

https://chromiumcodereview.appspot.com/11186013/diff/1/vm/pages.cc#newcode317
vm/pages.cc:317: uword obj_addr = page->first_object_start();
ditto

https://chromiumcodereview.appspot.com/11186013/diff/1/vm/pages.cc#newcode338
vm/pages.cc:338: uword obj_addr = page->first_object_start();
ditto

https://chromiumcodereview.appspot.com/11186013/diff/1/vm/pages.h
File vm/pages.h (right):

https://chromiumcodereview.appspot.com/11186013/diff/1/vm/pages.h#newcode62
vm/pages.h:62: uword alignment_;  // Needed to allocate old objects with
kOldObjectAlignment.
This is a bit confusing.  Is this member referenced anywhere or is it just
padding?  It looks new, but I do not see anything that references it elsewhere
in this change.

If this is just padding, a compile time assert should check the alignment. 
Renaming this to padding_ would be more descriptive as well.

An alternative is to round up in first_object_start and do away with this member
all together.  That would seem to be more robust and would make a nice location
for a static or dynamic assert.

[Ivan Posva, 2012-10-18 17:34:20]

https://chromiumcodereview.appspot.com/11186013/diff/1/vm/pages.cc
File vm/pages.cc (right):

https://chromiumcodereview.appspot.com/11186013/diff/1/vm/pages.cc#newcode280
vm/pages.cc:280: uword obj_addr = page->first_object_start();
On 2012/10/17 02:53:22, cshapiro wrote:
> Why did this code have to change?  This was done in a few places.  I looked at
> this code side by side with HeapPage::VisitObjects and it seems equivalent.
> 
> Anyhow, this expansion is repeated 2 times below.  For the sake of debugging
and
> test coverage, having one code path instead of 2 similar ones is strongly
> preferable.

I did not like the expansion either. Now that I have introduced object_end I am
undoing these changes.

https://chromiumcodereview.appspot.com/11186013/diff/1/vm/pages.h
File vm/pages.h (right):

https://chromiumcodereview.appspot.com/11186013/diff/1/vm/pages.h#newcode62
vm/pages.h:62: uword alignment_;  // Needed to allocate old objects with
kOldObjectAlignment.
On 2012/10/17 02:53:22, cshapiro wrote:
> This is a bit confusing.  Is this member referenced anywhere or is it just
> padding?  It looks new, but I do not see anything that references it elsewhere
> in this change.

It is padding, which I tried to convey with the name alignment_.

> If this is just padding, a compile time assert should check the alignment. 
> Renaming this to padding_ would be more descriptive as well.

I will reuse this slot for the end of the last object which corresponds to
memory_->end() for regularly sized pages and end of the object for large pages.

> An alternative is to round up in first_object_start and do away with this
member
> all together.  That would seem to be more robust and would make a nice
location
> for a static or dynamic assert.

I have added a dynamic assert in allocation to ensure that all newly allocated
objects in PageSpace do have the required alignment.

[siva, 2012-10-18 17:52:17]

lgtm