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