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

Bengt Rutisson bengt.rutisson at oracle.com
Mon May 5 11:52:00 UTC 2014


Hi Jon,

On 2014-05-02 19:25, Jon Masamitsu wrote:
> New webrev with problems addressed except as noted below.
>
> http://cr.openjdk.java.net/~jmasa/8031323/webrev.01/


To make it compile I had to remove the inline keyword from the 
declaration of CollectedHeap::align_allocation_or_fail():

diff --git a/src/share/vm/gc_interface/collectedHeap.hpp 
b/src/share/vm/gc_interface/collectedHeap.hpp
--- a/src/share/vm/gc_interface/collectedHeap.hpp
+++ b/src/share/vm/gc_interface/collectedHeap.hpp
@@ -353,7 +353,7 @@

    // Return the address "addr" aligned by "alignment_in_bytes" if such
    // an address is below "end".  Return NULL otherwise.
-  inline static HeapWord* align_allocation_or_fail(HeapWord* addr,
+  static HeapWord* align_allocation_or_fail(HeapWord* addr,
                                                     HeapWord* end,
                                                     unsigned short 
alignment_in_bytes);


The includes for the usages of CollectedHeap::align_allocation_or_fail() 
need to be updated.

space.cpp should include collectedHeap.inline.hpp.

psPromotionLAB.hpp is using CollectedHeap::align_allocation_or_fail() 
and thus would need the include too, but we should not include inline 
files from hpp files, so you need to move 
PSYoungPromotionLAB::allocate() into psPromotionLAB.inline.hpp and have 
that include collectedHeap.inline.hpp.

Similarly parGCAllocBuffer.hpp would need the include too since 
ParGCAllocBuffer::allocate_aligned() uses 
CollectedHeap::align_allocation_or_fail(). But again we should not 
include inline files in hpp files so 
ParGCAllocBuffer::allocate_aligned() should probably be moved in to the 
cpp file. Or, if we are worried about performance regressions we need to 
introduce a parGCAllocBuffer.inline.hpp file and move it there.



In CollectedHeap::align_allocation_or_fail(), I think we can replace this:

   if(new_addr > addr && new_addr < end) {

with:

   assert(new_addr > addr, "some good err_msg");
   if(new_addr < end) {



In ContiguousSpace::allocate_aligned() I think we can make the assert 
for the alignment of obj a bit stronger. Now we do:

assert(is_aligned(obj) && is_aligned(new_top), "checking alignment");

But we have just gone through some trouble to make sure that obj is 
aligned to SurvivorAlignmentInBytes, so I think we should assert that 
rather than just the 8 byte alignment that Space::is_aligned() gives us.


PSYoungPromotionLAB::allocate() and ContiguousSpace::allocate_aligned() 
is part of the code duplication that you plan on fixing with 
JDK-8042321. I'm fine with fixing that in a separate change, but it 
would be nice if we can make these methods look more similar. Right now 
they use different return patterns.

PSYoungPromotionLAB::allocate() uses:

if (allocation worked) {
   return obj;
}
...
return NULL;

But ContiguousSpace::allocate_aligned() uses an else statement:

if (allocation worked) {
   return obj;
} else {
   ...
   return NULL;
}

Similarly to the comment about the assert in 
ContiguousSpace::allocate_aligned() I think the assert in 
PSYoungPromotionLAB::allocate() can be strengthened to assert that obj 
is SurvivorAlignmentInBytes aligned.


A minor thing about oop.pcgc.inline.hpp

-// Used by parallel old GC.
+// forward_to_atomic() is used by parallel old and G1 GCs.

You updated this comment to include G1, but the method is actually also 
used by ParNew (see 
ParNewGeneration::copy_to_survivor_space_avoiding_promotion_undo()) and 
I don't find any usages in ParallelOld. I would suggest to just get rid 
of the comment. It is not important to have a comment saying who uses it.



Another very minor thing. The extra line feed added around line 1200 in 
parNewGeneration.cpp does not seem related to this change:

http://cr.openjdk.java.net/~jmasa/8031323/webrev.01/src/share/vm/gc_implementation/parNew/parNewGeneration.cpp.cdiff.html


Testing. There are tests for this feature coming, right? What kind of 
testing have you done so far on the current patch?

Thanks,
Bengt


>
> 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
>>
>



More information about the hotspot-gc-dev mailing list