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

Jon Masamitsu jon.masamitsu at oracle.com
Wed Jul 30 04:27:01 UTC 2014


Bengt and Thomas,

I updated with the latest hs-gc and had to do some merging by
hand.  The part of the change that merged cleanly is here.

http://cr.openjdk.java.net/~jmasa/8031323/webrev_merge.03/

The changes I merged by hand are

http://cr.openjdk.java.net/~jmasa/8031323/webrev_delta.03/

The complete change is

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

I moved the file parGCAllocBuffer.inline.hpp to

src/share/vm/gc_implementation/shared/parGCAllocBuffer.inline.hpp

from  src/share/vm/memory

I compared the webrev.02 with these and believe these are
consistent with the webrev.02.  I re-tested and there were no
surprises.

Jon

On 5/14/2014 8:38 AM, Bengt Rutisson wrote:
>
> 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