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

Bengt Rutisson bengt.rutisson at oracle.com
Wed May 14 15:38:34 UTC 2014


Hi Jon,

Thanks for fixing all of this!

Looks good. Reviewed.

Bengt

On 5/14/14 4:15 PM, Jon Masamitsu wrote:
> Bengt,
>
> Thanks for looking at this.  Responses in-line.
>
> On 5/5/2014 4:52 AM, Bengt Rutisson wrote:
>>
>> 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.
>>
> Now compiles with and without USE_PRECOMPILED_HEADER=0.  I left the
> inline methods as inline.
>
>>
>>
>> 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) {
>>
>>
>
> Fixed.
>
>>
>> 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.
>>
>
> Fixed.
>
>>
>> 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;
>> }
>>
> Fixed.  Both have an else clause.
>
>> 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.
>>
>
> Fixed.
>
>>
>> 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.
>>
>>
> Deleted comment.
>
>>
>> Another very minor thing. The extra line feed added around line 1200 
>> in parNewGeneration.cpp does not seem related to this change:
>
> I think the extra blank line was in the original.  I added it back.
>
>>
>> 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?
>
> I added diagnostics to count the number of allocations that were aligned
> to 64 bytes.  Then ran with different values of SurvivorAlignmentInBytes.
> In particular 8 bytes and 64 bytes.   I printed the fraction of 
> survivor allocations
> and promoted allocations that were aligned and not aligned after each 
> collection.
> I looked at the output such as (for 8byte alignment)
>
> Promoted addresses aligned 462 missed 3178 p_fraction  0.126923
> Survivor addresses aligned 933 missed 6050 ss_fraction  0.133610
>
> and (for 64 byte alignment)
>
> Promoted addresses aligned 471 missed 3168 p_fraction  0.129431
> Survivor addresses aligned 7107 missed 0 ss_fraction  1.000000
>
> to see if it was as I expected it to be.
>
> The webrev with the changes are at
>
> http://cr.openjdk.java.net/~jmasa/8031323/webrev_delta.02/
>
> The webrev shows the changes but also contains (if you look at other
> than just the changes), the diagnostic printing code I described above.
> You can ignore that since it is not in the complete webrev.
>
> The complete webrev is at
>
> http://cr.openjdk.java.net/~jmasa/8031323/webrev.02/
>
> Thanks again.
>
> Jon
>
>
>
>>
>> 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