RFR: 8143251: HeapRetentionTest.java Test is failing on jdk9/dev

[David Lindholm]

Stefan,

Looks good to me.

/David

On 2015-11-26 14:40, Stefan Johansson wrote:
> Hi again,
>
> I had overlooked a recent change to G1 that made this patch trigger an 
> assert. The fix for this is to add a call to 
> G1CollectorPolicy::abort_time_to_mixed_tracking when triggering the 
> user requested initial mark.
>
> Webrev: http://cr.openjdk.java.net/~sjohanss/8143251/hotspot.02/
> Inc webrev: http://cr.openjdk.java.net/~sjohanss/8143251/hotspot.01-02/
>
> Thanks,
> Stefan
>
> On 2015-11-25 16:55, Stefan Johansson wrote:
>> Thanks Thomas and David for the reviews.
>>
>> Stefan
>>
>> On 2015-11-24 17:03, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On Tue, 2015-11-24 at 10:15 +0100, Stefan Johansson wrote:
>>>> Thanks for looking at this Thomas,
>>>>
>>>> On 2015-11-23 13:27, Thomas Schatzl wrote:
>>>>> Hi,
>>>>>
>>>>>     some added comments about the problem:
>>>>>
>>>>> On Fri, 2015-11-20 at 18:07 +0100, Stefan Johansson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this fix for:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8143251
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~sjohanss/8143251/hotspot.00/
>>>>>>
>>>>>> Summary:
>>>>>> After some refactoring and fixing in the G1 state machine it was
>>>>>> possible to enter a state where we will ignore user requested 
>>>>>> concurrent
>>>>>> GCs.
>>>>> ^^ that is not true. G1 does not ignore these requests. It's just 
>>>>> that
>>>>> they will be delayed, that is delayed until the "next" GC. There are
>>>>> situations like test cases where this next GC does not occur because
>>>>> after sending that request, the application would stop allocating, so
>>>>> that request would "never" be executed.
>>>>>
>>>>>> This changes adds special treatment for the GC causes that are
>>>>>> currently user requested to allow them to always trigger a new
>>>>>> concurrent marking cycle.
>>>>> ... and potentially stopping the current reclamation phase, i.e. 
>>>>> mixed
>>>>> gcs. This comes with a big risk with adaptive IHOP, as at that 
>>>>> time G1
>>>>> is already (by definition of that feature) at low extra memory.
>>>>>
>>>>>> Testing:
>>>>>> * JPRT
>>>>>> * RBT with the failing tests and additional GC tests.
>>>>>    - I would prefer that the comment in g1CollectedHeap.hpp at
>>>>>
>>>>>    248   // These are defined in user_requested_concurrent_full_gc()
>>>>> because
>>>>>    249   // these sometimes need to be treated differently:
>>>>>
>>>>> would be moved below the list, and "these" qualified directly. E.g.
>>>>> "Cases c)-f) are defined..."
>>>>>
>>>>> I am not sure if the comment helps a lot. It seems to confuse the 
>>>>> reader
>>>>> more by being so unspecific, but ymmv.
>>>> I agree, removed lines 248, 249.
>>>>>    - in g1CollectorPolicy.cpp:1657, maybe the message should add 
>>>>> the kind
>>>>> of user requested concurrent mark too? Not really necessary 
>>>>> because the
>>>>> message in force_initial_mark_outside_cycle() already contains it. 
>>>>> It's
>>>>> your call.
>>>> This was my first idea as well but the ergo_verbose-macros made me 
>>>> back
>>>> of =) Also, as you say, we have the information in nearby logging, but
>>>> if you want I can file an RFE to add this once UL is in.
>>>>
>>>>>    - I would prefer if the comment in 1651 would directly mention 
>>>>> why 1653
>>>>> and 1654 are required: that G1 an initial mark gc must be a 
>>>>> young-only
>>>>> gc.
>>>> Updated comment.
>>>>>    - maybe the declaration of
>>>>> G1CollectedHeap::user_requested_concurrent_full_gc() moved closer to
>>>>> should_do_concurrent_full_gc(). Not sure, and I am not insisting on
>>>>> that. It's just that the referenced should_do_... is rather far away.
>>>>>
>>>>> Also please add a "is_" prefix to that getter. The comment also 
>>>>> does not
>>>>> tell what this method does. Or remove the existing comment. It sounds
>>>>> like a justification why this needed to be split out. Simple
>>>>> cross-referencing makes that understandable I think.
>>>> I added the is_ and removed the comment, but left it at its current
>>>> location since the reference is now gone.
>>>>
>>>> New webrev:
>>>> http://cr.openjdk.java.net/~sjohanss/8143251/hotspot.01/
>>>>
>>>> Inc webrev:
>>>> http://cr.openjdk.java.net/~sjohanss/8143251/hotspot.00-01/
>>>    looks good.
>>>
>>> Thomas
>>>
>>
>