Request for review (s) - 8031323: Optionally align objects copied to survivor spaces

[Jon Masamitsu]

New webrev with problems addressed except as noted below.

http://cr.openjdk.java.net/~jmasa/8031323/webrev.01/

Thanks.


On 03/18/2014 06:30 AM, Bengt Rutisson wrote:
>
> Hi Jon,
>
> On 2014-03-17 05:40, Jon Masamitsu wrote:
>> 8031323: Optionally align objects copied to survivor spaces
>>
>> Add the option to specify an alignment for allocations into the
>> survivor spaces.  Survivor space allocations will overflow if the
>> survivor spaces have not been increased.   The expected use
>> is to align survivor space allocations to cache line sizes.
>>
>> http://cr.openjdk.java.net/~jmasa/8031323/webrev.00/
>
> For G1 it looks to me like we are aligning objects in the old regions 
> as well. I thought the intent was to only align in the survivor 
> regions. Am I missing something?

Yes, that was an error.   Fixed in the new webrev.

>
> Note that it is not enough to check the alloc purpose for G1 since 
> G1CollectedHeap::par_allocate_during_gc() only takes the purpose as a 
> hint and may still return buffers from other regions than expected for 
> a purpose. But I guess that in this case it is not too important to 
> get it exactly right, so it may be enough to check the alloc purpose 
> value.

I did use the purpose and there are cases where alignments in the 
survivor spaces
can be missed.   I think the number missed is a very small percentage of 
the total
so I'm going to let those slip by.  I thought that preferable to the 
more extensive
coding changes it would take to catch them.
>
>
> The code in PSYoungPromotionLAB::allcoate(), 
> ParGCAllocBuffer::allocate_aligned() and 
> ContiguousSpace::allocate_aligned() is very similar. Is there a way to 
> consolidate these methods?
>
I looked at this but did not come up with a suitable way to fix it. I've 
opened
CR to track the issue.

https://bugs.openjdk.java.net/browse/JDK-8042321


>
> The code in CollectedHeap::align_allocation_or_fail() looks quite 
> different from other places in the code base where we do padding. What 
> do you think about doing something like this instead?
>
> inline HeapWord* CollectedHeap::align_allocation_or_fail(HeapWord* 
> addr, HeapWord* end, unsigned short alignment_in_bytes) {
>   if (alignment_in_bytes <= ObjectAlignmentInBytes) {
>     return addr;
>   }
>
>   assert(is_ptr_aligned(addr, HeapWordSize), "");
>   assert(is_size_aligned(alignment_in_bytes, HeapWordSize), "");
>
>   HeapWord* new_addr = (HeapWord*) align_pointer_up(addr, 
> alignment_in_bytes);
>   size_t padding = pointer_delta(new_addr, addr);
> https://bugs.openjdk.java.net/browse/JDK-8042321
>   if (padding == 0) {
>     return addr;
>   }
>
>   if (padding < CollectedHeap::min_fill_size()) {
>     padding += alignment_in_bytes / HeapWordSize;
>     assert(padding >= CollectedHeap::min_fill_size(), "");
>     new_addr = addr + padding;
>   }
>
>   if(new_addr > addr && new_addr < end) {
>     CollectedHeap::fill_with_object(addr, padding);
>     return new_addr;
>   } else {
>     return NULL;
>   }
> }

Fixed that as suggested.

Jon
>
> Thanks,
> Bengt
>
>>
>> Jon
>