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

[Man Cao]

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/

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

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

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?

Thanks,
Man
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181212/a494cec3/attachment.htm>