Clean up aligned allocation code in preparation for SIMD alignments.

Clean up aligned allocation code in preparation for SIMD alignments.

Moves alignment fill calculations into two static Heap methods.
Adds a Heap method to handle the complex case where filler is potentially needed before and after a heap object.
Makes DoubleAlignForDeserialization explicitly fill after an already
aligned object.

LOG=N
BUG=v8:4124
Committed: https://crrev.com/fcfb080eb9a637f0ae066bed4c45095e60df8a84
Cr-Commit-Position: refs/heads/master@{#28687}

Committed: https://crrev.com/43638cd4e8b8372ca5b002eef565ec0e75ac5972
Cr-Commit-Position: refs/heads/master@{#28702}

[bbudge, 2015-05-20 15:05:56]

Patchset #1 (id:1) has been deleted

[bbudge, 2015-05-20 15:14:30]

Patchset #1 (id:20001) has been deleted

[bbudge, 2015-05-20 15:14:42]

Patchset #1 (id:20001) has been deleted

[bbudge, 2015-05-20 15:39:38]

bbudge@chromium.org changed reviewers:
+ bmeurer@chromium.org, hpayer@chromium.org

[bbudge, 2015-05-20 15:39:39]

This seems to work, but it's complex code, so unit tests are needed. I will try
to add some, but am publishing now to get some early feedback on the approach.
Any feedback on how to approach unit testing here is welcome. I don't see any
Heap usage in existing unit tests.

https://codereview.chromium.org/1150593003/diff/60001/src/heap/spaces-inl.h
File src/heap/spaces-inl.h (left):

https://codereview.chromium.org/1150593003/diff/60001/src/heap/spaces-inl.h#o...
src/heap/spaces-inl.h:377: (kDoubleUnaligned && (OffsetFrom(obj) &
kDoubleAlignmentMask) != 0));
I eliminated this since it can never assert.

[Benedikt Meurer, 2015-05-21 04:47:45]

On 2015/05/20 15:39:39, bbudge wrote:
> This seems to work, but it's complex code, so unit tests are needed. I will
try
> to add some, but am publishing now to get some early feedback on the approach.
> Any feedback on how to approach unit testing here is welcome. I don't see any
> Heap usage in existing unit tests.

The general direction looks fine to me. Unit testing the Heap or space classes
is
probably not going to work for quite some time, because there's no separation
between the heap and the runtime. The approach that worked for other new stuff
like the idle handler is to write code in a testable way, independent of the
heap,
and only call into the code from heap.cc.

[Hannes Payer (out of office), 2015-05-21 10:38:18]

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

https://codereview.chromium.org/1150593003/diff/60001/src/heap/spaces-inl.h#n...
src/heap/spaces-inl.h:304: object = free_list_.Allocate(aligned_size_in_bytes);
I don't see how alignment works for free-list allocation. You always have to
over-allocate there because you cannot check upfront. And if the free-list
returns already an aligned pointer, you have to place the filler after the
object (to make the over-allocated memory iterable).

[bbudge, 2015-05-21 10:55:45]

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

https://codereview.chromium.org/1150593003/diff/60001/src/heap/spaces-inl.h#n...
src/heap/spaces-inl.h:304: object = free_list_.Allocate(aligned_size_in_bytes);
On 2015/05/21 10:38:17, Hannes Payer wrote:
> I don't see how alignment works for free-list allocation. You always have to
> over-allocate there because you cannot check upfront. And if the free-list
> returns already an aligned pointer, you have to place the filler after the
> object (to make the over-allocated memory iterable).

Thanks for pointing this out. I'll have to modify the filling to handle this
case. I would still like to avoid over-allocating when we know the alignment
constraints ahead of time.

[Hannes Payer (out of office), 2015-05-22 10:56:15]

This cl seems to be in a strange merge state with an older version of  Issue
1152513002: Generalize alignment in heap GC functions. Could you please resolve
that.

https://codereview.chromium.org/1150593003/diff/80001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/1150593003/diff/80001/src/globals.h#newcode441
src/globals.h:441: enum AllocationAlignment { kWordAligned, kDoubleAligned,
kDoubleValueAligned };
I called it kDoubleUnaligned because it is general. kDoubleValueAligned just
corresponds to the HeapNumber case, not necessarily to any other object type,
for which the value would have to be aligned differently.

[bbudge, 2015-05-22 11:25:49]

On 2015/05/22 10:56:15, Hannes Payer wrote:
> This cl seems to be in a strange merge state with an older version of  Issue
> 1152513002: Generalize alignment in heap GC functions. Could you please
resolve
> that.
> 
> https://codereview.chromium.org/1150593003/diff/80001/src/globals.h
> File src/globals.h (right):
> 
> https://codereview.chromium.org/1150593003/diff/80001/src/globals.h#newcode441
> src/globals.h:441: enum AllocationAlignment { kWordAligned, kDoubleAligned,
> kDoubleValueAligned };
> I called it kDoubleUnaligned because it is general. kDoubleValueAligned just
> corresponds to the HeapNumber case, not necessarily to any other object type,
> for which the value would have to be aligned differently.

Sorry, I should have said it's not quite ready for a real review. I'll change
back to your name then. I'll be adding a kSimd128Unaligned enum value in an
upcoming CL.

[bbudge, 2015-05-22 12:39:46]

OK, I think this is ready for review. PTAL

[Hannes Payer (out of office), 2015-05-26 09:22:30]

looking good. just a few nits.

https://codereview.chromium.org/1150593003/diff/200001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1150593003/diff/200001/src/heap/heap.h#newcod...
src/heap/heap.h:718: 
Please add comments about what these functions are doing.

https://codereview.chromium.org/1150593003/diff/200001/src/heap/spaces-inl.h
File src/heap/spaces-inl.h (left):

https://codereview.chromium.org/1150593003/diff/200001/src/heap/spaces-inl.h#...
src/heap/spaces-inl.h:377: (kDoubleUnaligned && (OffsetFrom(obj) &
kDoubleAlignmentMask) != 0));
why did you remove the dcheck?

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

https://codereview.chromium.org/1150593003/diff/200001/src/heap/spaces-inl.h#...
src/heap/spaces-inl.h:306: allocation_size = size_in_bytes + filler_size;
+= filler_size

https://codereview.chromium.org/1150593003/diff/200001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/1150593003/diff/200001/test/cctest/test-heap....
test/cctest/test-heap.cc:1785: 
It would be nice to have test for all the aligned allocation functions as well.
Unfortunately, we do not have that. Would you mind adding some tests?

[bbudge, 2015-05-26 11:09:18]

https://codereview.chromium.org/1150593003/diff/200001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1150593003/diff/200001/src/heap/heap.h#newcod...
src/heap/heap.h:718: 
On 2015/05/26 09:22:29, Hannes Payer wrote:
> Please add comments about what these functions are doing.

Done.

https://codereview.chromium.org/1150593003/diff/200001/src/heap/spaces-inl.h
File src/heap/spaces-inl.h (left):

https://codereview.chromium.org/1150593003/diff/200001/src/heap/spaces-inl.h#...
src/heap/spaces-inl.h:377: (kDoubleUnaligned && (OffsetFrom(obj) &
kDoubleAlignmentMask) != 0));
On 2015/05/26 09:22:29, Hannes Payer wrote:
> why did you remove the dcheck?

I removed it to keep usage of the alignment enum values in Heap. Also I think
it's incorrect. Shouldn't the first term be

alignment == kDoubleAligned

instead of just kDoubleAligned?

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

https://codereview.chromium.org/1150593003/diff/200001/src/heap/spaces-inl.h#...
src/heap/spaces-inl.h:306: allocation_size = size_in_bytes + filler_size;
On 2015/05/26 09:22:29, Hannes Payer wrote:
> += filler_size

Done.

[Hannes Payer (out of office), 2015-05-27 14:12:45]

Cool! I left a few comments about the tests.

https://codereview.chromium.org/1150593003/diff/260001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/1150593003/diff/260001/test/cctest/test-heap....
test/cctest/test-heap.cc:1841: // cause exactly one filler object to be created.
Let's also check if the filler is there.

https://codereview.chromium.org/1150593003/diff/260001/test/cctest/test-heap....
test/cctest/test-heap.cc:1849: CHECK(IsAddressAligned(obj1->address(),
kDoubleAlignment, kPointerSize));
This address should not be double aligned.

https://codereview.chromium.org/1150593003/diff/260001/test/cctest/test-heap....
test/cctest/test-heap.cc:1876: HeapObject* obj1 = AllocateUnalignedAndFill(
Here, I would rather test the free-list allocation and check if the object is a
the right alignment and the fillers are e.g. before and after the object.

[bbudge, 2015-05-28 09:28:33]

PTAL

https://codereview.chromium.org/1150593003/diff/260001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/1150593003/diff/260001/test/cctest/test-heap....
test/cctest/test-heap.cc:1841: // cause exactly one filler object to be created.
On 2015/05/27 14:12:45, Hannes Payer wrote:
> Let's also check if the filler is there.

Done.

https://codereview.chromium.org/1150593003/diff/260001/test/cctest/test-heap....
test/cctest/test-heap.cc:1849: CHECK(IsAddressAligned(obj1->address(),
kDoubleAlignment, kPointerSize));
On 2015/05/27 14:12:45, Hannes Payer wrote:
> This address should not be double aligned.

address + offset is aligned

(confusing, I'm not a fan of this utility function)

https://codereview.chromium.org/1150593003/diff/260001/test/cctest/test-heap....
test/cctest/test-heap.cc:1876: HeapObject* obj1 = AllocateUnalignedAndFill(
On 2015/05/27 14:12:45, Hannes Payer wrote:
> Here, I would rather test the free-list allocation and check if the object is
a
> the right alignment and the fillers are e.g. before and after the object.

Done.

[Hannes Payer (out of office), 2015-05-28 13:57:07]

Cool thanks. LGTM

[bbudge, 2015-05-28 17:17:48]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2015-05-28 17:18:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150593003/300001

[commit-bot: I haz the power, 2015-05-28 17:19:47]

Committed patchset #14 (id:300001)

[commit-bot: I haz the power, 2015-05-28 17:20:07]

Patchset 14 (id:??) landed as
https://crrev.com/fcfb080eb9a637f0ae066bed4c45095e60df8a84
Cr-Commit-Position: refs/heads/master@{#28687}

[bbudge, 2015-05-28 17:57:07]

A revert of this CL (patchset #14 id:300001) has been created in
https://codereview.chromium.org/1159123002/ by bbudge@chromium.org.

The reason for reverting is: Breaks mjsunit, webkit, mozilla, benchmarks..

[bbudge, 2015-05-29 10:22:45]

There were two problems with the previous CL.

1) DoubleAlignForDeserialization treated the size parameter as the object size,
rather than the object size + filler.

2) The tests were wrong when top was initially misaligned. Creating filler and a
kPointerSize object left top misaligned again, causing two filler objects.

PTAL

[Hannes Payer (out of office), 2015-05-29 13:15:38]

On 2015/05/29 10:22:45, bbudge wrote:
> There were two problems with the previous CL.
> 
> 1) DoubleAlignForDeserialization treated the size parameter as the object
size,
> rather than the object size + filler.
> 
> 2) The tests were wrong when top was initially misaligned. Creating filler and
a
> kPointerSize object left top misaligned again, causing two filler objects.
> 
> PTAL

Thanks lgtm

[bbudge, 2015-05-29 13:16:12]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2015-05-29 13:16:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150593003/320001

[commit-bot: I haz the power, 2015-05-29 13:18:19]

Committed patchset #15 (id:320001)

[commit-bot: I haz the power, 2015-05-29 13:18:37]

Patchset 15 (id:??) landed as
https://crrev.com/43638cd4e8b8372ca5b002eef565ec0e75ac5972
Cr-Commit-Position: refs/heads/master@{#28702}