Looks good to me. Bob.
> On Jul 28, 2020, at 10:01 AM, Severin Gehwolf <sgehw...@redhat.com> wrote: > > 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 >
