RFR: 8071462: Remove G1ParGCAllocator::alloc_buffer_waste

[Michail Chernov]

Hi Thomas,

I tried to make all what you said.
Webrev: http://cr.openjdk.java.net/~dpochepk/~mchernov/8071462/webrev.02/

ParGCAllocBuffer::add_undo_waste and 
ParGCAllocBuffer::undo_last_allocation were made as protected members.

ParGCAllocBuffer::undo_allocation now does the same as 
ParScanThreadState::undo_alloc_in_to_space and 
G1ParGCAllocator::undo_allocation

G1ParGCAllocator has virtual G1PLABWasteStat wasted() = 0; method which 
must be implemented in all heirs.

Introduced new class G1PLABWasteStat (g1Allocator.hpp) that retrieves 
wasted and undo wasted values from allocation buffers. If new class is 
not acceptable there, I can add 2 virtual methods to G1ParGCAllocator 
for retrieving wasted and undo wasted values.

Thanks,
Michail

On 15.04.2015 6:53, Thomas Schatzl wrote:
> 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
>
>
>
>