Hi David, Thanks for the review.
On Tue, 2020-07-28 at 23:49 +1000, David Holmes wrote: > Hi Severin, > > On 28/07/2020 11:23 pm, Severin Gehwolf wrote: > > Hi David, > > > > On Tue, 2020-07-28 at 21:17 +1000, David Holmes wrote: > > > Hi Severin, > > > > > > On 28/07/2020 6:27 pm, Severin Gehwolf wrote: > > > > Hi, > > > > > > > > Please review this patch which makes the Java container metrics adhere > > > > to -XX:+/-UseContainerSupport so they can be disabled if heuristics > > > > turn out to be wrong. The approach taken is to use JNI and call into > > > > the JVM in order to determine the setting of UseContainerSupport before > > > > Metrics are being instantiated. > > > > > > > > The intention is for this patch to be backported to older JDKs so as to > > > > provide a means to disable container metrics should things go wrong > > > > with backports of the likes of JDK-8226575. > > > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8250627 > > > > webrev: > > > > https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8250627/01/webrev/ > > > > > > Seems quite simple and clean. > > > > > > One query though, I'm not clear on who the expected caller of > > > Metrics.getInstance() is? (Coming from the perspective of "might we want > > > to cache the fact container support is not enabled?".) > > > > I know of two uses so far: > > > > 1) Launcher (-XshowSettings:system): > > http://hg.openjdk.java.net/jdk/jdk/file/89fe9e02a522/src/java.base/share/classes/sun/launcher/LauncherHelper.java#l318 > > > > 2) OperatingSystemMXBean: > > http://hg.openjdk.java.net/jdk/jdk/file/89fe9e02a522/src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java#l48 > > > > Both uses seem OK as is. Is it worth caching something here? > > No that looks fine. I didn't find them because of the reflective > invocation in Metrics.systemMetrics(). > > > > Also note that we no longer update JVM_INTERFACE_VERSION (last update > > > was JDK 13) - it is meaningless now the JDK and hotspot are in sync > > > version wise. > > > > OK. Does that mean I should revert the version increment, then? > > I would leave it unchanged, yes. Here you go: https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8250627/02/webrev/ OK? Thanks, Severin
