Request for review (s) - 8031323: Optionally align objects copied to survivor spaces
Bengt Rutisson
bengt.rutisson at oracle.com
Mon Aug 4 08:42:11 UTC 2014
Hi Jon,
Looks good to me.
Bengt
On 2014-07-30 06:27, Jon Masamitsu wrote:
> 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