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

[Thomas Schatzl]

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.

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

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

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

Thanks,
  Thomas