RFR: 8048556: Unnecessary GCLocker-initiated young GCs

[Kim Barrett]

> On Jul 26, 2019, at 11:02 AM, Fairoz Matte <fairoz.matte at oracle.com> wrote:
> 
> Hi Kim,
> 
> Below is the updated webrev specific to GenCollectedHeap and ParalleGC
> http://cr.openjdk.java.net/~fmatte/8048556/webrev.01/

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/genCollectedHeap.cpp
 373       if(!GCLocker::needs_gc()) {
 374         gc_count_before = total_collections();

This leaves gc_count_before uninitialized if GCLocker::needs_gc() is
true, leading to undefined behavior when it is referenced.  It
probably passes testing because the uninitialized value that gets used
_probably_ differs from the current total_collections() value, leading
the VMOpt to report failure, which is what we want in this case.  But
it's still UB.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/genCollectedHeap.cpp
 373       if(!GCLocker::needs_gc()) {
 374         gc_count_before = total_collections();

Assuming the above UB problem is fixed, this thread could just spin a
bunch of times, logging a bunch of excessive retry messages, until the
GCLocker's GC request finally runs.

G1 puts GCLocker::stall_until_clear() on its version of this path.  I
think something more like what G1 does should be done here too.

------------------------------------------------------------------------------
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp
 319         // The GCLocker may not be active but the GCLocker initiated
 320         // GC may not yet have been performed (GCLocker::needs_gc()
 321         // returns true). In this case we do not try this GC and
 322         // wait until the GCLocker initiated GC is performed, and
 323         // then retry the allocation.
 324       } else if(GCLocker::needs_gc()) {
 325           should_try_gc = false;
 326       } else {
 327           should_try_gc = true;
 328       }

I think the comment ought to be inside the relevant clause.

I also think a better name than should_try_gc should be used;
something relevant to GCLocker.

------------------------------------------------------------------------------
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp
 319         // The GCLocker may not be active but the GCLocker initiated
 320         // GC may not yet have been performed (GCLocker::needs_gc()
 321         // returns true). In this case we do not try this GC and
 322         // wait until the GCLocker initiated GC is performed, and
 323         // then retry the allocation.
 324       } else if(GCLocker::needs_gc()) {
 325           should_try_gc = false;
 326       } else {
 327           should_try_gc = true;
 328       }

[Similar to the above mentioned problem in the genCollectedHeap change.]

If GCLocker::needs_gc() then this thread could just spin a bunch of
times, logging a bunch of excessive retry messages, until the
GCLocker's GC request finally runs.

G1 puts GCLocker::stall_until_clear() on its version of this path.  I
think something more like what G1 does should be done here too.

------------------------------------------------------------------------------