[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(s): 8250627: Use -XX:+/-UseContainerSupport for enabling/disabling Java container metrics
From:       David Holmes <david.holmes () oracle ! com>
Date:       2020-07-28 22:48:31
Message-ID: d6fd7bdc-7479-16e1-abd8-15e25f0ae597 () oracle ! com
[Download RAW message or body]

Update looks good.

Thanks,
David

On 29/07/2020 12:01 am, Severin Gehwolf 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
> 


[prev in list] [next in list] [prev in thread] [next in thread]