RFR: 8071462: Remove G1ParGCAllocator::alloc_buffer_waste
michail.chernov at oracle.com
Tue Apr 14 18:39:37 UTC 2015
On 13.04.2015 22:21, Thomas Schatzl wrote:
> Hi Michail,
> On Mon, 2015-04-13 at 16:40 +0300, Michail Chernov wrote:
>> Still have no response, please take a look!
>> On 08.04.2015 15:58, Michail Chernov wrote:
>>> Please could someone review this update?
>>> Changes were updated - fixed code formatting.
>>> On 07.04.2015 17:26, Michail Chernov wrote:
>>>> Can I have couple of reviews for next change?
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8071462
>>>> 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
G1ResManParGCAllocator (which is in closed part) can store more than two
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
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
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
> This would automatically update the undo_waste statistics for CMS/ParNew
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
Will do it too.
Thanks for your comments!
More information about the hotspot-gc-dev