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

Stefan Johansson stefan.johansson at oracle.com
Thu Nov 26 13:40:06 UTC 2015


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
>>
>



More information about the hotspot-gc-dev mailing list