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

Jon Masamitsu jon.masamitsu at oracle.com
Wed May 14 14:15:28 UTC 2014


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