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

[Thomas Schatzl]

Hi all,

  I would like to ask for one Reviewer review.

Current webrevs are at:

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

Thanks,
  Thomas

On Fri, 2015-02-20 at 15:40 +0100, Thomas Schatzl wrote:
> Hi all,
> 
>   can I have reviews for the following cleanup? It removes the buffer
> retain functionality of ParGCAllocBuffer.
> 
> It has (once I guess, but actually I am not sure because that must have
> been long time ago) been used to retain the space at the end of the PLAB
> for the next GC.
> 
> Nobody uses it. So that and all related code can be removed.
> 
> Here is a long description of changes:
> 
> ParGCAllocBuffer::flush_stats_and_retire() is always called with true
> for "end_of_gc" and false for the "retain" parameter.
> When moving that into that call to retire(), you will notice that
> retire() is always called with retain=false.
> 
> Looking at ParGCAllocBuffer::retire(), if retain is always false,
> _retained will never be true, which means that _retained_filler is never
> used. With _retained always false, all of the asserts in retire() always
> evaluate to true, so the end_of_gc parameter is useless.
> 
> (Actually the whole _retained_filler is never loaded from, making this
> entire functionality non-working in the first place).
> 
> Now looking at what retire() does, there is actually a bug in
> flush_stats_and_retire() because retire() adds to the waste (the area
> between _top and _hard_top), but flush() is called first.
> 
> Looking at the code, we also only flush stats when ResizePLAB is set,
> due to performance concerns. However this means that the output provided
> by PrintPLAB will not be updated if ResizePLAB is not set, which is not
> desirable at all (for comparison of fragmentation with ResizePLAB
> enabled). I did not see the three atomic adds as a performance problem,
> as this method is called once per PLAB per GC.
> 
> There is a small additional change in PLABStats::adjust_desired_plab_sz
> where I merged the two separate "if (PrintPLAB)" statements.
> 
> Other cleanup changes include:
>   - removed an obscure "XXX" comment
>   - removed a PRAGMA
>   - cleaned up includes (BlockOffsetTable - huh?)
>   - tried to improve comments
> 
> 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
> 
> Thanks,
>   Thomas 
> 
>