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

[David Lindholm]

Stefan,

The hotspot.01 webrev looks good to me. Reviewed.


Thanks,
David

On 2015-11-24 10:15, 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/
>
> Thanks,
> Stefan
>>
>> Thanks,
>>    Thomas
>>
>>
>