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

[Jon Masamitsu]

Bengt,

Thanks for looking at this.  I don't have answers yet but am
working on them.   And yes, this is only supposed to be
for copying into the survivor space for G1 so at least that
part needs to be fixed.

One comment below.


On 3/18/14 6: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?
>
> 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.
>
>
> The code in PSYoungPromotionLAB::allcoate(), 
> ParGCAllocBuffer::allocate_aligned() and 
> ContiguousSpace::allocate_aligned() is very similar. Is there a way to 
> consolidate these methods?
The ContiguousSpace::allocate_aligned() sets the _top field in 
ContiguousSpace.
The UseParallelGC collector which uses  PSYoungPromotionLAB  uses the
MutableSpace / ImmutableSpace hierarchy instead of the Space hierachy so
I didn't look hard at making the code more common.  It can be done but I 
think
requires at least 1 layer of classes to encapsulate MutableSpace / 
ImmutableSpace
and Space to abstract away the differences between the hierarchies. Were you
thinking along those lines?  Or a different approach?

Jon

>
>
> 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);
>
>   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;
>   }
> }
>
> Thanks,
> Bengt
>
>>
>> Jon
>