RFR: 8071462: Remove G1ParGCAllocator::alloc_buffer_waste

[Thomas Schatzl]

Hi Michail,

On Mon, 2015-04-13 at 16:40 +0300, Michail Chernov wrote:
> Hi,
> 
> Still have no response, please take a look!
> 
> Michail
> On 08.04.2015 15:58, Michail Chernov wrote:
> > Hi,
> >
> > Please could someone review this update?
> >
> > Changes were updated - fixed code formatting.
> > http://cr.openjdk.java.net/~akulyakh/8071462/webrev.01/
> >
> > Thanks,
> > Michail
> >
> > On 07.04.2015 17:26, Michail Chernov wrote:
> >> Hi,
> >>
> >> Can I have couple of reviews for next change?
> >>
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8071462
> >> Webrev: 
> >> http://cr.openjdk.java.net/~eistepan/~mchernov/8071462/webrev.00/
> >>
> >> G1ParGCAllocator has methods add_to_alloc_buffer_waste(size_t waste) 
> >> and add_to_undo_waste(size_t waste) which are used wrong.
> >> The main idea of fix is to track waste and undo to waste in PLAB. We 
> >> cannot use PLABStats to store such values because PLABStats is shared 
> >> between all PLAB and cannot store separate values of waste in itself. 
> >> We can get waste and undo to waste values from ParGCAllocBuffer 
> >> before ParGCAllocBuffer::flush_and_reture() which is called from 
> >> G1ParScanThreadState destructor (destructor calls 
> >> G1DefaultParGCAllocator::retire_alloc_buffers which calls 
> >> ParGCAllocBuffer::flush_and_reture() ). Now destructor calls separate 
> >> method which is invoked before G1ParScanThreadState destruction, so 
> >> we can get waste and undo to waste to get termination stats.
> >>
> >> Also removed unused methods in G1ParScanThreadState: 
> >> add_to_alloc_buffer_waste and  add_to_undo_waste.
> >>
> >> Fix was checked locally with -XX:+UseG1GC -XX:+PrintTerminationStats 
> >> and was checked by JPRT.

- I am not completely clear why the change keeps
G1ParGCAllocator::_alloc_buffer_waste and G1ParGCAllocator::_undo_waste.
They are identical to ParGCAllocBuffer::_undo_waste and
ParGCAllocBuffer::_wasted anyway?

Why not let the getters access these values directly? At that point
their contents seem still be valid (i.e. not "flushed" to the global
PLABStats).

Then there is also no need to introduce
G1ParScanThreadState::retire_alloc_buffers().

- In G1ParGCAllocator::undo_allocation, the calls to alloc_buffer(dest,
context) could be factored out as a minor cleanup.

- In G1ParGCAllocator::undo_allocation(), in the else-part, maybe the
two lines

      CollectedHeap::fill_with_object(obj, word_sz);
      alloc_buffer(dest, context)->add_undo_waste(word_sz);

could be factored into a method of ParGCAllocBuffer like
undo_allocation() so that internal counter handling is completely hidden
by ParGCAllocBuffer.

Actually I think the entire G1ParGCAllocator::undo_allocation() code
could be moved to ParGCAllocBuffer. If you look at the other use of
current ParGCAllocBuffer::undo_allocation(), in
ParScanThreadState::undo_alloc_in_to_space(), the code is *exactly* the
same.
This would automatically update the undo_waste statistics for CMS/ParNew
too.

- there should imo be additional undo_waste() totals in PLABStats too,
even if it is not used for calculation of PLAB size (but printed e.g. in
-XX:+PrintPLAB).

Thanks,
  Thomas