RFR: 8143251: HeapRetentionTest.java Test is failing on jdk9/dev
stefan.johansson at oracle.com
Wed Nov 25 15:55:11 UTC 2015
Thanks Thomas and David for the reviews.
On 2015-11-24 17:03, Thomas Schatzl wrote:
> 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:
>>> some added comments about the problem:
>>> On Fri, 2015-11-20 at 18:07 +0100, Stefan Johansson wrote:
>>>> Please review this fix for:
>>>> After some refactoring and fixing in the G1 state machine it was
>>>> possible to enter a state where we will ignore user requested concurrent
>>> ^^ 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.
>>>> * 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()
>>> 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
>> 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:
>> Inc webrev:
> looks good.
More information about the hotspot-gc-dev