RFR: 8057536: Refactor G1 to allow context specific allocations

Mikael Gerdin mikael.gerdin at oracle.com
Fri Sep 5 07:15:38 UTC 2014


Hi Stefan,


On Thursday 04 September 2014 21.01.11 Stefan Johansson wrote:
> Hi Mikael,
> 
> On 2014-09-04 17:04, Mikael Gerdin wrote:
> > Hi Stefan,
> > 
> > On Thursday 04 September 2014 14.45.30 Stefan Johansson wrote:
> >> Hi,
> >> 
> >> Please review these changes for:
> >> https://bugs.openjdk.java.net/browse/JDK-8057536
> >> 
> >> Webrev:
> >> http://cr.openjdk.java.net/~sjohanss/8057536/webrev.00/
> > 
> > g1Allocator.hpp:
> > 
> > reuse_retaind_... => reuse_retained_...
> > 
> > G1ParGCAllocator::allocate does not have the SurvivorAlignmentInBytes
> > change when it was broken out from G1ParScanThreadState.
> 
> Fixed.
> 
> > g1Allocator.cpp:
> > 
> > reuse_retaind_... => reuse_retained_...
> > two spaces after return type of retire_alloc_buffers
> > align parameters to flush_stats_and_retire
> 
> Fixed.
> 
> > g1CollectedHeap.hpp:
> > 
> > align the parameters to "attempt_allocation_at_safepoint",
> > "survivor_attempt_allocation", "old_attempt_allocation"
> 
> Fixed.
> 
> > g1CollectedHeap.cpp:
> > 
> > You didn't move the methods of Mutator/SurvivorGC/OldGC alloc region to
> > g1Allocator.cpp
> 
> Realized that those were missed when moving stuff before, and Bengt
> pointed out that they probably should move to G1AllocRegion.hpp as well,
> so that has been done.
> 
> > align the parameters at the call to "attempt_allocation"
> > align the parameters to "attempt_allocation_at_safepoint"
> 
> Fixed.
> 
> > g1CollectedHeap.inline.hpp:
> > 
> > align the parameters at the call to "attempt_allocation"
> > 
> > align the parameters to "survivor_attempt_allocation",
> > "old_attempt_allocation"
> 
> Fixed.
> 
> > heapRegion.cpp:
> > 
> > HeapRegion::print_on is designed to produce short output. Can you shorten
> > the output? Either by disabling the printout if contexts aren't used or
> > disable it for context-0 regions. Or have a format like "C%4u".
> 
> Change to "AC%4u".
> 
> > For most of the style changes I'm ok with a follow-up change to address
> > them. The rest of the change looks ok to me.
> 
> As you see I've addressed most of them, here is an updated webrev as
> well as a diff between the two:
> http://cr.openjdk.java.net/~sjohanss/8057536/webrev.01/
> http://cr.openjdk.java.net/~sjohanss/8057536/webrev.00-01/

Looks good.
/Mikael

> 
> I also had to do a small addition in vm_operations.hpp, which was
> forgotten in the previous webrev.
> 
> Thanks for doing a through review,
> Stefan
> 
> > I'm not a 8u Reviewer.
> > 
> > /Mikael
> > 
> >> Summary:
> >> These changes are made to allow G1 to do context specific allocation. As
> >> part of this a G1Allocator class has be refactored out of
> >> G1CollectedHeap which can be extended to implement the specific context
> >> code. Currently only the G1DefaultAllocator is implemented, this
> >> allocator only makes use of one context to have the same behavior as
> >> prior to this enhancement.
> >> 
> >> Testing:
> >> * JPRT
> >> * Manual verification
> >> * Local ute runs of vm.quick.testlist
> >> 
> >> Thanks,
> >> Stefan
H



More information about the hotspot-gc-dev mailing list