RFR (M): 8212206: Refactor AdaptiveSizePolicy to separate out code related to GC overhead

[Thomas Schatzl]

On Wed, 2018-12-12 at 18:30 +0800, Man Cao wrote:
> Hi,
> 
> Addressed several comments. New webrev:
> https://cr.openjdk.java.net/~manc/8212206/webrev.01/
> Diff from webrev.00:
> https://cr.openjdk.java.net/~manc/8212206/webrev.diff.00-01/

Looks good, but see the comments below too.

> 
> > > Assuming that all collectors want to implement this, and actually
> > > need to I am leaning towards doing so. However the ZGC/Shenandoah
> > > people might object to this.
> 
> I haven't moved the GCOverheadChecker instance to CollectedHeap yet.
> Should I wait for ZGC/Shenandoah people to give some green light?
> 

I cc'ed zgc-dev and shenandoah-dev. 

> > Creating a CSR and getting it approved is not a big deal; it may 
> > even be useful as it clearly communicates the change to the users.
> > Additionally I think due to that translation table I mentioned, the
> > old name can still be used I think.
> 
> As for the hsperfdata counter sun.gc.policy.gcTimeLimitExceeded, I
> found two issues:
> (a) The translation table in aliasmap seems to mainly target JDK-
> internal usage of the counters.
> Only the PerfDataBufferImpl.findByName() method makes use of the
> aliasmap. There are use cases that doesn't work with the aliasmap.
> E.g.:
> $ jstat -J-Djstat.showUnsupported=true -name java.ci.totalTime <pid> 
>  // This works
> $ jstat -J-Djstat.showUnsupported=true -name hotspot.ci.total.time
> <pid> // This doesn't work
> This is because the "jstat -name" would invoke the
> PerfDataBufferImpl.findByPattern() method,
> which does not take the aliasmap into account.
> 
> In addition, there are independent implementations that read
> /tmp/hsperfdata_<user>/<pid>
> file directly, e.g.:
> 
https://github.com/twitter/commons/tree/master/src/python/twitter/common/java/perfdata
> And Google internally has a Java implementation that does the job
> (but uses Guava library).
> These tools do not support aliasmap.

Okay, I admit I do not know much about these counters...

> 
> As for this counter, fortunately I found it hardly used anywhere in
> OpenJDK or across Google's depot.
> And its current implementation is not that useful, as described
> below.
> 
> (b) This counter contains a boolean value that is set at every GC.
> This makes its usefulness limited, as it is very hard to catch the
> moment when it is set to 1. When a full GC sets it to 1 and throws an
> OOM exception due to GC overhead exceeded, the JVM could subsequently
> trigger another full GC and reset the counter to 0, then terminates
> due to the OOM exception.
> If -XX:PerfDataSaveFile=foo.hsperf is set, foo.hsperf would contain 0
> for this counter in this case,
> which is quite unexpected.
> 
> I'd propose to change this counter to a cumulative counter, i.e, the
> total number of GCs that trigger GC-overhead-limit-exceeded event,
> and rename this counter as the same time.
> I think it is cleaner to do this change in a separate RFE and CSR.
> What do you think?

I agree to separate this change out.

Thomas