<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Hi,<br></div><div><br></div><div>Addressed several comments. New webrev:</div><div><a href="https://cr.openjdk.java.net/~manc/8212206/webrev.01/">https://cr.openjdk.java.net/~manc/8212206/webrev.01/</a><br></div><div>Diff from webrev.00:<br></div><div><a href="https://cr.openjdk.java.net/~manc/8212206/webrev.diff.00-01/">https://cr.openjdk.java.net/~manc/8212206/webrev.diff.00-01/</a><br></div><div><br></div><div><span style="color:rgb(80,0,80)">> > Assuming that all collectors want to implement this, and actually</span><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> > need to I am leaning towards doing so. However the ZGC/Shenandoah</span><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> > people might object to this.</span><br></div><div><br></div><div>I haven't moved the GCOverheadChecker instance to CollectedHeap yet.<br></div>Should I wait for ZGC/Shenandoah people to give some green light?</div><div dir="ltr"><br></div><div dir="ltr"><span style="color:rgb(80,0,80)">></span><span style="color:rgb(80,0,80)"> </span>Creating a CSR and getting it approved is not a big deal; it may even<br><span style="color:rgb(80,0,80)">> </span>be useful as it clearly communicates the change to the users.<br><span style="color:rgb(80,0,80)">> </span>Additionally I think due to that translation table I mentioned, the old<br><span style="color:rgb(80,0,80)">> </span>name can still be used I think.</div><div dir="ltr"><br><div>As for the hsperfdata counter sun.gc.policy.gcTimeLimitExceeded, I found two</div><div>issues:</div><div>(a) The translation table in aliasmap seems to mainly target JDK-internal usage of the counters.</div><div>Only the PerfDataBufferImpl.findByName() method makes use of the aliasmap.</div><div>There are use cases that doesn't work with the aliasmap. E.g.:</div><div>$ jstat -J-Djstat.showUnsupported=true -name java.ci.totalTime <pid>   // This works</div><div>$ jstat -J-Djstat.showUnsupported=true -name hotspot.ci.total.time <pid> // This doesn't work<br></div><div>This is because the "jstat -name" would invoke the PerfDataBufferImpl.findByPattern() method,</div><div>which does not take the aliasmap into account.</div><div><br></div><div>In addition, there are independent implementations that read /tmp/hsperfdata_<user>/<pid></div><div>file directly, e.g.:</div><div><a href="https://github.com/twitter/commons/tree/master/src/python/twitter/common/java/perfdata">https://github.com/twitter/commons/tree/master/src/python/twitter/common/java/perfdata</a></div><div>And Google internally has a Java implementation that does the job (but uses Guava library).</div><div>These tools do not support aliasmap.</div><div><br></div><div>As for this counter, fortunately I found it hardly used anywhere in OpenJDK or across Google's depot.</div><div>And its current implementation is not that useful, as described below.</div><div><br></div><div>(b) This counter contains a boolean value that is set at every GC. This makes its usefulness limited,</div><div>as it is very hard to catch the moment when it is set to 1. When a full GC sets it to 1 and<br></div><div>throws an OOM exception due to GC overhead exceeded, the JVM could subsequently trigger</div><div>another full GC and reset the counter to 0, then terminates due to the OOM exception.</div><div>If -XX:PerfDataSaveFile=foo.hsperf is set, foo.hsperf would contain 0 for this counter in this case,</div><div>which is quite unexpected.</div><div><br></div><div>I'd propose to change this counter to a cumulative counter, i.e, the total number of GCs that trigger</div><div>GC-overhead-limit-exceeded event, and rename this counter as the same time.</div><div>I think it is cleaner to do this change in a separate RFE and CSR. What do you think?</div><div><br></div><div>Thanks,</div></div><div dir="ltr"><div><div dir="ltr" class="gmail-m_9105683288713075243gmail_signature"><div dir="ltr">Man</div></div></div></div></div></div></div></div></div></div></div></div></div>