Request for review (s) - 8031323: Optionally align objects copied to survivor spaces
Bengt Rutisson
bengt.rutisson at oracle.com
Mon May 5 11:52:00 UTC 2014
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.
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) {
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.
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;
}
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.
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.
Another very minor thing. The extra line feed added around line 1200 in
parNewGeneration.cpp does not seem related to this change:
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?
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