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

[Thomas Schatzl]

Hi Jon,

  thanks for the review :)

On Fri, 2015-02-27 at 16:26 -0800, Jon Masamitsu wrote:
> Thomas,
> 
> http://cr.openjdk.java.net/~tschatzl/8073466/webrev.1/src/share/vm/gc_implementation/shared/parGCAllocBuffer.cpp.frames.html
> 
> Could you add a comment here that the stats are being "flushed".
> Something like
> 
> // Flush the statistics.
> >   67   stats->add_allocated(_allocated);
> >    68   // Retire() counts the unused space as wasted. So we need to remove it again
> >    69   // before updating the statistics.
> >    70   stats->add_wasted(_wasted - unused);

Done.

> In a context where the unused space at the end of a buffer might be used
> later (as when there was the "retaining functionality"),  I can see that
> you might want to differentiate between _wasted and unused.    Since there
> is no longer a "retain", it seems all to be just waste.   So
> I don't see the use of passing "_wasted - unused" as opposed to just
> "_wasted".

There is still use of "_unused" in the calculation of the desired PLAB
size. I tried to avoid changing the current PLAB sizing in this change,
and later for G1 I plan to override the desired_plab_size() method.

[This is needed in any case because of improved statistics for G1, where
we want to distinguish between waste within the PLABs and waste caused
by refill due to the current PLAB not fitting into the current region.]

The existing PLAB sizing calculates the amount of "target_refills" using
the _unused member. I admit I do not really understand that calculation,
and the results seem bogus in particular in presence of multiple threads
each having its own PLAB and considering the integer truncation in the
formula.

I can create a CR to remove _unused and fix the CMS PLAB sizing.

> If you would like to keep the differentiation between "_wasted - used"
> and "_wasted", consider adding back the "end_of_gc" flag to
> retire() and moving
> 
> >   127     _wasted += pointer_delta(_end, _top);  // unused  space
>
> out of invalidate() and into retire() in this fashion
> 
> if (!end_of_gc) {
>    _wasted += pointer_delta(_end, _top);  // unused  space
> }

Done. I did not use an additional parameter to retire, as we already
have two retiring methods (retire and flush_stats_and_retire) to be
called at different times during GC anyway.

Also, please ignore that G1 at the moment calls flush_stats_and_retire()
every time it allocates a G1ParScanThreadState. This is basically
JDK-8040162. I will ask for a review of that soon.

> Throwaway suggestion (your call, since not in your change)
> 
> Fix the indentation on the fields
> 
> >    39   char head[32];
> >    40   size_t _word_sz;          // In HeapWord units
> 
> >    48   char tail[32];

Fixed.

http://cr.openjdk.java.net/~tschatzl/8073466/webrev.1_to_2/
(incremental)
http://cr.openjdk.java.net/~tschatzl/8073466/webrev.2/ (full)

(The change also moves invalidate() to the protected section of
ParGCAllocBuffer).

Thanks,
  Thomas