remove recursion from NewSpace::AllocateRaw*

remove recursion from NewSpace::AllocateRaw*

The recursion between AllocateRaw* and SlowAllocateRaw makes incremental
stepping very complicated. This patch removes the recursion. Follow-on
patches will improve accounting of allocations done by incremental mark.

See: https://codereview.chromium.org/1252053003/#msg5

BUG=
R=hpayer@chromium.org

Committed: https://crrev.com/7f2dc889d40ca70d2e38035cfb195763278b002a
Cr-Commit-Position: refs/heads/master@{#29989}

[Hannes Payer (out of office), 2015-07-30 10:38:25]

Thanks for doing this cleanup! It is going in the right direction. I have a
couple of comments...

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h
File src/heap/spaces-inl.h (right):

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:348: int filler_size =
Heap::GetFillToAlign(allocation_info_.top(), alignment);
Keep top in a temporary variable.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:354: if (EnsureAllocation(size_in_bytes, alignment) ==
false) {
Instead of "== false" use "!".

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:357: 
if top changed, read out top again and recalculate aligned_size_in_bytes.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:359: aligned_size_in_bytes) {
This check should not be needed.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:365: HeapObject* obj =
HeapObject::FromAddress(allocation_info_.top());
Use the temporary top variable here.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:379: AllocationResult NewSpace::AllocateRawUnaligned(int
size_in_bytes) {
Comments from above apply also here.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces.cc#newcode1474
src/heap/spaces.cc:1474: if (allocation_info_.limit() < high) {
This method should in principle take care of two cases:
1) there is room in this page: top + size_in_bytes < high
2) there is no room in this page.

Case one right now does not guarantee that there is room. We just go there
whenever limit is smaller than high, but a huge allocation would just go over
high.

We should restructure the code to check if there is room: top + size_in_bytes <
high. If there is room perform case 1) If there is no room add a fresh page (if
possible) and do the special accounting. Then perform case 1).

This method should return true if it can guarantee that the bump pointer
allocation will succeed.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces.cc#newcode1495
src/heap/spaces.cc:1495: } else {
Remove the else case and just return false.

[ofrobots, 2015-07-30 20:26:17]

Done.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h
File src/heap/spaces-inl.h (right):

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:348: int filler_size =
Heap::GetFillToAlign(allocation_info_.top(), alignment);
On 2015/07/30 10:38:25, Hannes Payer wrote:
> Keep top in a temporary variable.

Done.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:354: if (EnsureAllocation(size_in_bytes, alignment) ==
false) {
On 2015/07/30 10:38:25, Hannes Payer wrote:
> Instead of "== false" use "!".

Done.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:357: 
On 2015/07/30 10:38:25, Hannes Payer wrote:
> if top changed, read out top again and recalculate aligned_size_in_bytes.

Done.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:359: aligned_size_in_bytes) {
On 2015/07/30 10:38:25, Hannes Payer wrote:
> This check should not be needed.

Done.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:365: HeapObject* obj =
HeapObject::FromAddress(allocation_info_.top());
On 2015/07/30 10:38:25, Hannes Payer wrote:
> Use the temporary top variable here.

Done.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces-inl.h#newco...
src/heap/spaces-inl.h:379: AllocationResult NewSpace::AllocateRawUnaligned(int
size_in_bytes) {
On 2015/07/30 10:38:25, Hannes Payer wrote:
> Comments from above apply also here.

Done.

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1265443003/diff/1/src/heap/spaces.cc#newcode1474
src/heap/spaces.cc:1474: if (allocation_info_.limit() < high) {
On 2015/07/30 10:38:25, Hannes Payer wrote:
> This method should in principle take care of two cases:
> 1) there is room in this page: top + size_in_bytes < high
> 2) there is no room in this page.
> 
> Case one right now does not guarantee that there is room. We just go there
> whenever limit is smaller than high, but a huge allocation would just go over
> high.
> 
> We should restructure the code to check if there is room: top + size_in_bytes
<
> high. If there is room perform case 1) If there is no room add a fresh page
(if
> possible) and do the special accounting. Then perform case 1).
> 
> This method should return true if it can guarantee that the bump pointer
> allocation will succeed.

Done.

[Hannes Payer (out of office), 2015-07-31 07:50:52]

I like it a lot, it is way simpler now! Looking almost good. A few comments
left.

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces-inl.h
File src/heap/spaces-inl.h (right):

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces-inl.h#n...
src/heap/spaces-inl.h:358: top = allocation_info_.top();
We got a new top here, we have to recalculate 
filler_size = Heap::GetFillToAlign(top, alignment);
and
aligned_size_in_bytes = size_in_bytes + filler_size;

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:1474: if (old_top + size_in_bytes >= high) {
We should compare against aligned_size_in_bytes here. We can pass it in, or
recalculate in the beginning.

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:1484: top_on_previous_step_ = to_space_.page_low();
to_space_.page_low() should be the same as allocation_info_.top() in this case.
Just set top_on_previous_step_ = old_top after setting old_top.

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:1486: old_top = allocation_info_.top();
Since we got a new top here, we have to recalculate aligned_size_in_bytes.

[ofrobots, 2015-07-31 13:47:39]

Thanks for the review. I have addressed your comments. PTAL.

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces-inl.h
File src/heap/spaces-inl.h (right):

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces-inl.h#n...
src/heap/spaces-inl.h:358: top = allocation_info_.top();
On 2015/07/31 07:50:52, Hannes Payer wrote:
> We got a new top here, we have to recalculate 
> filler_size = Heap::GetFillToAlign(top, alignment);
> and
> aligned_size_in_bytes = size_in_bytes + filler_size;

Done.

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:1474: if (old_top + size_in_bytes >= high) {
On 2015/07/31 07:50:52, Hannes Payer wrote:
> We should compare against aligned_size_in_bytes here. We can pass it in, or
> recalculate in the beginning.

Done.

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:1484: top_on_previous_step_ = to_space_.page_low();
On 2015/07/31 07:50:52, Hannes Payer wrote:
> to_space_.page_low() should be the same as allocation_info_.top() in this
case.
> Just set top_on_previous_step_ = old_top after setting old_top.

Done.

https://codereview.chromium.org/1265443003/diff/20001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:1486: old_top = allocation_info_.top();
On 2015/07/31 07:50:52, Hannes Payer wrote:
> Since we got a new top here, we have to recalculate aligned_size_in_bytes.

Done.

[Hannes Payer (out of office), 2015-08-03 15:58:33]

Thanks! Way nicer now. LGTM

[ofrobots, 2015-08-03 20:18:07]

The CQ bit was checked by ofrobots@google.com

[commit-bot: I haz the power, 2015-08-03 20:18:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265443003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265443003/40001

[commit-bot: I haz the power, 2015-08-03 20:43:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-03 20:44:05]

Patchset 3 (id:??) landed as
https://crrev.com/7f2dc889d40ca70d2e38035cfb195763278b002a
Cr-Commit-Position: refs/heads/master@{#29989}