RFR (S): 8073466: Remove buffer retaining functionality and clean up in ParGCAllocBuffer

[Thomas Schatzl]

Hi Kim,

On Mon, 2015-02-23 at 00:40 -0500, Kim Barrett wrote:
> On Feb 20, 2015, at 9:40 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> > 
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8073466
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8073466/webrev/
> > testing:
> > jprt, perf benchmarks with G1 and CMS
> 
> Looks good.
> 
> Thanks for the thorough description; that made the review pretty easy.
> 
> One minor comment, and a couple of pre-existing issues to perhaps do
> something about.
> 
> ------------------------------------------------------------------------------  
> src/share/vm/gc_implementation/shared/parGCAllocBuffer.hpp
>  148   void reset() {
>  149     _allocated = 0;
>  150     _wasted    = 0;
>  151     _unused    = 0;
>  152   }
> 
> This new private helper function seems to only be used in the one
> place at the end of adjust_desired_plab_sz, where equivalent code used
> to be inline.  I'm not sure making a helper function is actually
> beneficial here.  The code that actually uses those variables is
> earlier in the function.  Moving the reinitialization far away (and
> losing the comment about clearing the accumulators) doesn't seem
> helpful to me.
> 

I have around 7-8 patches building on that. The second, for JDK-8073013,
makes that method virtual since I am going to make special PLAB
statistics for G1.

We can fix that again if that other CR still does not make the
extraction of this method reasonable. 

> ------------------------------------------------------------------------------   
> src/share/vm/gc_implementation/shared/parGCAllocBuffer.hpp 
>   57   // Minimum PLAB size.
>   58   static const size_t min_size();
>   59   // Maximum PLAB size.
>   60   static const size_t max_size();
> 
> Pre-existing:
> 
> The const qualifier for the return types has no effect.

Removed.

> ------------------------------------------------------------------------------  
> src/share/vm/gc_implementation/shared/parGCAllocBuffer.cpp
>   44   assert (min_size() > AlignmentReserve, "Inconsistency!");
>   45   // ArrayOopDesc::header_size depends on command line initialization.
>   46   AlignmentReserve = oopDesc::header_size() > MinObjAlignment ? align_object_size(arrayOopDesc::header_size(T_INT)) : 0;
> 
> Pre-existing:
> 
> The assert on line 44 seems oddly placed, given the one and only
> assignment of AlignmentReserve immediately follows the assertion, and
> that min_size() depends on AlignmentReserve.

Fixed. I moved it after alignment reserve calculation. Min_size adds
something to AlignmentReserve, which is always true, but one could see
it as a guard against bad changes. I could also remove it if requested.

New webrevs at
http://cr.openjdk.java.net/~tschatzl/8073466/webrev.1/
http://cr.openjdk.java.net/~tschatzl/8073466/webrev.0_to_1/

Thanks,
  Thomas