RFR: 8077836: Make sure G1ParGCAllocBuffer are marked as retired

[Stefan Johansson]

Hi Kim,

On 2015-04-20 22:08, Kim Barrett wrote:
> On Apr 20, 2015, at 11:07 AM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
>> Thanks for reviewing Thomas,
>>
>> On 2015-04-20 17:00, Thomas Schatzl wrote:
>>> Hi Stefan,
>>>
>>> On Wed, 2015-04-15 at 15:08 +0200, Stefan Johansson wrote:
>>>> Hi,
>>>>
>>>> Please review this fix for:
>>>> https://bugs.openjdk.java.net/browse/JDK-8077836
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sjohanss/8077836/hotspot.00/
>>>>
>>>> Summary:
>>>> At the end of a G1 gc a call to G1ParGCAllocator::retire_alloc_buffers
>>>> is made, in this code we should make sure all alloc buffers are retired.
>>>> This is currently handle by calling flush_and_retire_stats for each
>>>> active buffer. Because this method is only implemented for
>>>> ParGCAllocBuffer and not for G1ParGCAllocBuffer we currently miss
>>>> updating the _retired field in G1ParGCAllocBuffer.
>>>>
>>>> The _retired field is checked in a guarantee in the G1ParGCAllocBuffer
>>>> destructor, but this destructor is not usually called because we are
>>>> missing a virtual destructor for G1ParGCAllocator.
>>>>
>>>> This patch adds this destructor and implements
>>>> G1ParGCAllocBuffer::flush_and_retire_stats by calling the version in
>>>> ParGCAllocBuffer and setting the _retired field to true.
>>>>
>>>> Testing:
>>>> Manual verification that G1 always fails with the new destructor and
>>>> that the fix avoids this.
>>> Looks good. One thing that might be interesting to add for consistency
>>> is to add the early-out like in retire() if the PLAB has already been
>>> retired.
>>> It does not seem to hurt to leave it out, but for consistency it might
>>> be desirable.
>> I thought of this too, but left it out because the change would then change the behavior if someone called flush_and_retire_stats twice and I wanted to avoid that in this change.
>>> Also the change needs to be rebased to the current repository.
>> Here's a rebased change:
>> http://cr.openjdk.java.net/~sjohanss/8077836/hotspot.01/
> Can you hold off on this for https://bugs.openjdk.java.net/browse/JDK-8078193 and the corrected push?
>
I just realized this while reading your mail about the backout. I will 
wait for this to be sorted out before pushing this change.

Thanks,
Stefan