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

[Jon Masamitsu]

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?

To consolidate this code I've added CollectedHeap::allocate_inc_top()

http://javaweb.us.oracle.com/net/arches-sunw.us.oracle.com/export/home1/java/jdk9-gc-8031323/webrev_inc_top/webrev/src/share/vm/gc_interface/collectedHeap.inline.hpp.frames.html

Note "top" passed by reference.

  278 inline HeapWord* CollectedHeap::allocate_inc_top(HeapWord* &top,
  279                                                 HeapWord* end,
  280                                                 size_t word_size) {
  281   HeapWord* res = top;
  282   if (pointer_delta(end, top) >= word_size) {
  283     top = top + word_size;
  284     assert(is_ptr_aligned(res, HeapWordSize) &&
  285       is_ptr_aligned(top, HeapWordSize), "checking alignment");
  286     return res;
  287   } else {
  288     return NULL;
  289   }

and use it in places such as

http://javaweb.us.oracle.com/net/arches-sunw.us.oracle.com/export/home1/java/jdk9-gc-8031323/webrev_inc_top/webrev/src/share/vm/gc_implementation/parallelScavenge/psPromotionLAB.hpp.udiff.html

-    HeapWord* new_top = obj + size;
-    // The 'new_top>obj' check is needed to detect overflow of obj+size.
-    if (new_top > obj && new_top <= end()) {
-      set_top(new_top);
-      assert(is_object_aligned((intptr_t)obj) && is_object_aligned((intptr_t)new_top),
-             "checking alignment");
-      return obj;
-    }
      set_top(obj);
-    return NULL;
+    return CollectedHeap::allocate_inc_top(_top, end(), size);


Does this do what you want in an OK way?

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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140422/803bbbbf/attachment.htm>