RFR (S): 8073052: Rename and clean up the allocation manager hierarchy in g1Allocator.?pp

[Thomas Schatzl]

Hi Stefan, Kim,

  could you look at the updated CR again? Also, I need a Reviewer to
look at this.

Thanks,
  Thomas

On Mon, 2015-03-09 at 12:02 +0100, Thomas Schatzl wrote:
> Hi Stefan,
> 
>   thanks for the review.
> 
> There is a new webrev at
> http://cr.openjdk.java.net/~tschatzl/8073052/webrev.2 (full)
> http://cr.openjdk.java.net/~tschatzl/8073052/webrev.1_to_2 (diff)
> 
> Further comments inline:
> 
> On Fri, 2015-03-06 at 10:00 +0100, Stefan Johansson wrote:
> > Hi Thomas,
> > 
> > On 2015-03-05 15:58, Thomas Schatzl wrote:
> > > Hi all,
> > >
> > > On Wed, 2015-03-04 at 16:28 +0100, Thomas Schatzl wrote:
> > >> Hi all,
> > >>
> > >>    can I have reviews for the following change that does some renamings
> > >> in the allocation related class hierarchy, also adding a few lines of
> > >> documentation.
> > >>
> > >> This change is intentionally limited to renames to keep it simple, in a
> > >> subsequent patch there will be more cleanup changes, moving methods
> > >> around.
> > >> Also, if there is demand, I am open to rename the files to something
> > >> different (suggestions?) in a follow-up change (to not confuse the
> > >> webrev tool too much).
> > >>
> > >> CR:
> > >> https://bugs.openjdk.java.net/browse/JDK-8073052
> > >> Webrev:
> > >> http://cr.openjdk.java.net/~tschatzl/8073052/webrev.00/
> > >> Testing:
> > > I talked with Stefan Johansson a bit about the change, with the result
> > > that we keep the name of G1Allocator, since in the upcoming changes (see
> > > below for more) that class is supposed to handle all allocations within
> > > the current allocation regions.
> > >
> > > Webrev:
> > > http://cr.openjdk.java.net/~tschatzl/8073052/webrev.01
> > Generally change looks good, but I agree with Kim that PLABAllocator 
> > could still use the G1 prefix for consistency. Some other comments:
> 
> Done.
> 
> > ---
> > g1CollectedHeap.hpp:
> > +  // Allocates a new heap region instance.
> > +  virtual HeapRegion* new_heap_region(uint hrs_index, MemRegion mr);
> > 
> > No need to have this one virtual, no one extends G1CollectedHeap.
> 
> Done.
> 
> > ---
> > vmStructs_g1.hpp:
> > Since _summary_bytes_used have been moved in the native code this needs 
> > to be reflected in the Java agent as well. You need to move the 
> > functionality from G1Allocator.java to G1CollectedHeap.java.
> 
> Done.
> 
> Thanks,
>   Thomas
> 
>