RFR: 8057536: Refactor G1 to allow context specific allocations

[Thomas Schatzl]

Hi Stefan,

On Thu, 2014-09-04 at 21:01 +0200, Stefan Johansson wrote:
> Hi Bengt,
> 
> On 2014-09-04 17:07, Bengt Rutisson wrote:
> >
> >
> > Hi Stefan,
> >
> > On 2014-09-04 14:45, 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/
> >
> > Overall I think this i a nice refactoring. Some comments below. Mostly 
> > style and code comments.
> >
> Thanks for the in depth review, here's the new webrev and a diff against 
> the first:
> http://cr.openjdk.java.net/~sjohanss/8035057/webrev.01/
> http://cr.openjdk.java.net/~sjohanss/8057536/webrev.00-01/
> 
> I also had to do a small addition in vm_operations.hpp, which was 
> forgotten in the previous webrev.

Overall it looks good; there are some minor superfluous newlines/spaces
I will list here, but they are not that important.

- declaration of G1DefaultParGCAllocator::retire_alloc_buffers() has an
extra space before the semicolon.

- extra newline in the G1DefaultParGCAllocator::G1DefaultParGCAllocator
implementation.

- extra newlines at the end of g1AllocRegion.cpp

;)

Some comments for future improvements; feel free to push without
considering them due to the urgency:

- Is it possible to document the new classes. "Base class for G1
allocators" does not explain anything to me. Same for "The default
allocator for G1" does not tell anything either.

- Maybe it is useful to name the region allocators (G1Allocator
subtypes) slightly more differently than the PLAB allocator
(G1ParGCAllocator). At least G1Allocator is really unspecific to me.
Maybe G1RegionAllocator? And rename G1ParGCAllocBuffer to
G1PLABAllocator.

- I think G1Allocator is very specific to the current purpose: i.e.
manages regions for exactly one mutator, survivor and old gc allocation
region.
Also G1ParGCAllocator is also rather tightly tuned to it.

- given that G1ParGCAllocator and G1Allocator are rather tightly
coupled, it might make sense to use that. I think it is possible to
actually remove the need to pass AllocationContext_t completely for the
current G1ParGCAllocator implementation.

- GCAllocPurpose and PLAB statistics should go in there (from
G1CollectedHeap) or at least move out of G1CollectedHeap as well imo.
This is really specific to the current generational setup and to how
G1ParGCAllocator work now.

Thanks,
  Thomas