RFR: 8071462: Remove G1ParGCAllocator::alloc_buffer_waste

[Michail Chernov]

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.
>
> 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.
>
> - 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().
>
> 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.
Will do it.
>
> - 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).
Will do it too.
> Thanks,
>    Thomas
>
>
>
>
Thanks for your comments!
Michail