RFR: 8048556: Unnecessary GCLocker-initiated young GCs

[Kim Barrett]

> On Jul 24, 2019, at 6:01 AM, Fairoz Matte <fairoz.matte at oracle.com> wrote:
> 
> Hi,
> 
> Please review the change to avoid Unnecessary GCLocker Initiated GC's.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8048556  
> Webrev: http://cr.openjdk.java.net/~fmatte/8048556/webrev.00/ 
> 
> Background: This fix is proposed by "Tony Printezis" in the comments section. 
> GC locker-initiated young GC happens immediately after another young GC. 
> 
> The fix is to send the right gc counts and fixing the vm gc op not to signal "full"
> Reading right GC count from "GC_locker::jni_unlock(JavaThread* thread)" method and 
> provide implementations for GCH, PS, and G1 to get that count in Collect() method.
> I hope I have done justification to Tony's approach.
> 
> I have modified a regression test (again from Tony's work) that verifies when "GCLocker Initiated GC" 
> occurs, It runs gc only if young generation occupancy is > 75%.
> 
> Thanks,
> Fairoz

I had a number of other comments on or issues with this change, but
I'm not going to bring them up just now because I think they end up
not mattering due to what follows here.

I think G1 doesn't need this change.  From comments in the CR it seems
the problem hasn't been reproduced with G1.  I think I can explain that.

When G1 has an allocation failure that might require a GC, and it
can't satisfy the allocation in some other way (such as by allocating
more young regions), then if GCLocker::needs_gc() is true it will
stall and wait for the gc-locker gc request to be processed, and then
retry the allocation.

ParallelGC only stalls and retries if GCLocker::is_active_and_needs_gc().
This opens the window for the race condition Tony pointed out in the
first comment in the CR, because is_active may no longer true, even
though there may be a gc-locker gc request on the way.

GenCollectedHeap (used by SerialGC and CMS) is similar to ParallelGC.

This suggests an entirely different approach should be taken, directly
addressing ParallelGC and GenCollectedHeap and making them similarly
wait for a pending gc-locker gc request if there is one.  That would
eliminate the whole "capture and indirectly pass along the proper
counters" mechanism discussed in the CR and proposed by the current
webrev, which I have a number of issues with.

Note that this has nothing to do with the second (GCH-specific?) cause
for the issue, discussed in Tony's 3rd comment in the CR.  That part
of the change seems okay.