RFR: 8071462: Remove G1ParGCAllocator::alloc_buffer_waste

[Thomas Schatzl]

Hi Michail,

On Tue, 2015-04-14 at 21:39 +0300, Michail Chernov wrote:
> Hi Thomas,
> 
> On 13.04.2015 22:21, Thomas Schatzl wrote:
> > 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?
> G1ParGCAllocator can contain different allocation buffers (which can be 
> accessed by G1ParGCAllocator::alloc_buffer which is pure virtual). So, 
> _alloc_buffer_waste and _undo_waste are used to store sums of all values 
> from all buffers.

My argument is that it is not required to (temporarily) store these
values in G1ParGCAllocator::_alloc_buffer_waste and G1ParGCAllocator at
all. They are purely used for some very detailed logging, so performance
of that particular call is not really important, and the amount of extra
work not particular large.

So the calls could be made virtual and the implementations iterate over
the buffers themselves.

> > 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).
> G1ResManParGCAllocator (which is in closed part) can store more than two 
> G1ParGCAllocBuffer inside. 
> G1ParScanThreadState::print_termination_stats() doesn't know anything 
> about G1ParGCAllocator heirs. Therefore there are used 
> G1ParGCAllocator::alloc_buffer_waste() and undo_waste().
> > Then there is also no need to introduce
> > G1ParScanThreadState::retire_alloc_buffers().
> We cannot to use all statistics which is not flushed without 
> G1ParGCAllocator::retire_alloc_buffers(). Until this change 
> _g1_par_allocator->retire_alloc_buffers() is invoked only from 
> G1ParScanThreadState::~G1ParScanThreadState() , so statistics for all 
> ParGCAllocBuffer is not added to G1ParGCAllocator::_alloc_buffer_waste 
> and _undo_waste. Now retire_alloc_buffers() can be invoked directly 
> before printing statistics.

Thinking about this some more, I more and more dislike this approach:
this means that in some cases, the PLABs will be retired multiple times
when this flag is enabled (and only then). In case of G1 this has no
impact, because of its special G1PLAB/G1ParGCAllocBuffer implementation,
but it seems something unexpected.

Then there is 8040162: Avoid reallocating PLABs between GC phases in G1:
in this change the PLAB buffers (actually the current implementation
keeps G1ParScanThreadState instances) are kept during the entire young
gc without any in-between retirement of the PLABs (which is the point to
save memory)

With the current proposed change the efficiency of this idea would be
decreased if and only if that flag is set. Logging should not impact
execution that much imo.

It would be great if this change would already prepare for JDK-8040162
instead of adding more work (unless you see another way).

> > - 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.
> I thought that it is not good idea to mix logic of buffer and some 
> cleaning in ParGCAllocBuffer but I found that 
> ParGCAllocBuffer::retire_internal() invokes 
> CollectedHeap::fill_with_object, so I will refactor undo_allocation().

Thanks.

Thanks,
  Thomas