[10] RFR(M) 8182701: Modify JVMCI to allow Graal Compiler to expose platform MBean

[Jaroslav Tulach]

Thanks for your comments, Mandy.

On pondělí 21. srpna 2017 12:42:09 CEST mandy chung wrote:
> cc'ing serviceability-dev which is the right mailing list for platform
> management discussion.
> 
> JVMCI is currently named as `jdk.internal.vm.ci` (a JDK internal
> module).   I suppose this new module is intended to be kept as an
> internal module?

The module doesn't export any API - e.g. it is internal. The current name is 
jdk.vm.ci.management - shall I change that to something like 
jdk.internal.vm.ci.management? Or some other (shorter) suggestion?

>    30  * @since 9
> 
> should be @since 10.

I see. I'll fix that.

> JVMCIMBeans.java
> 
>    55         @Override
>    56         public Set<Class<?>> mbeanInterfaces() {
>    57             return Collections.singleton(mbean.getClass());
>    58         }
> 
> This mbeanInterfaces method should return the MBean interface class but
> not the class of the mbean implementation.  This allows the platform
> mbean to be looked up from ManagementFactory.getPlatformMXBean.  If this
> is a dynamic mbean, then this method simply returns an empty list (see
> DiagnosticCommandMBean).    

I am almost 100% sure that the bean(s) exposed from Truffle is going to be 
DynamicMBean - we compose the attributes dynamically, that wouldn't work with 
reflection.

I'll change the code to return an empty list if the bean is DynamicMBean.

> To support standard mbean,
> HotSpotJVMCICompilerFactory::mbeans method would need to include the
> mbean interface type in addition to the name and the mbean
> implementation object, i.e. may need to define a specific type for it.

As we don't have any use for this yet I suggest to keep things simple. I 
believe following contract could work:

public Set<Class<?>> mbeanInterfaces() {
   if (mbean instanceof DynamicMBean) {
     return Collections.emptySet();
   } else {
     Class<?>[] interfaces = mbean.getClass().getInterfaces();
     return new HashSet<>(Arrays.asList(interfaces));
   }
}

if this is acceptable, I would change documentation of the mbean() method to 
mention how the set of mbeanInterfaces() is computed. OK?

Thanks for the review.
-jt

> On 8/18/17 11:49 AM, Vladimir Kozlov wrote:
> > Updated changes in all repos:
> > http://cr.openjdk.java.net/~kvn/8182701/webrev.01/
> > 
> > On 8/18/17 7:12 AM, Jaroslav Tulach wrote:
> > 
> > Thanks for pushing me forward, Vladimir. Yes, the changes are still
> > needed if
> > we want the Graal compiler to expose its MBean in a lazy way. I am
> > offering
> > new webrev for review. It contains following changes:
> > 
> > Per Mandy's suggestion I created new module jdk.vm.ci.management to
> > bridge between
> > JVMCI and jdk.management. Adding new module was a bit tricky, but with
> > great help of Jan
> > Lahoda I even managed to register it as a boot module.
> > 
> > I renamed the JVMCIMXBean class and dropped X per Vladimir's advice. I
> > fixed
> > the non-standard location of JVMCIMXBean class.
> > 
> > I changed the interface to use Map, so the compiler is able to expose
> > more
> > than a single bean.
> > 
> > That's it. I am looking forward to your review comments.
> > -jt
> > 
> > Here are original changes for reference:
> > 
> > http://cr.openjdk.java.net/~kvn/8182701/webrev.jdk/
> > http://cr.openjdk.java.net/~kvn/8182701/webrev.hs/
> > 
> > On 8/17/17 11:54 AM, Vladimir Kozlov wrote:
> >> Hi Jaroslav,
> >> 
> >> What we should do with 8182701? Do you still need JVMCI changes?
> >> 
> >> Note, your changes to Graal [GR-5435] were integrated recently into
> >> JDK (jdk10/hs):
> >> https://bugs.openjdk.java.net/browse/JDK-8186158
> >> 
> >> Thanks,
> >> Vladimir
> >> 
> >> On 8/14/17 10:06 AM, Jaroslav Tulach wrote:
> >>> On čtvrtek 3. srpna 2017 17:03:39 CEST Jaroslav Tulach wrote:
> >>>> On čtvrtek 27. července 2017 15:01:17 CEST Alan Bateman wrote:
> >>>>> On 27/07/2017 10:07, Jaroslav Tulach wrote:
> >>>>>> Yes, it seems like a desirable goal to let Graal compiler work
> >>>>>> with just
> >>>>>> java.base. Is there a description how to build JDK9/10 with just
> >>>>>> java.base
> >>>>>> that I could follow and test against?
> >>>>> 
> >>>>> You can use jlink to create a run-time image that only contains
> >>>>> java.base (jlink --module-path $JAVA_HOME/jmods --add-modules
> >>>>> java.base
> >>>>> --output smalljre).
> >>>> 
> >>>> Status update: I've just tried to run Graal compiler against JDK9
> >>>> with only
> >>>> java.base and jdk.internal.vm.ci modules, and there are some
> >>>> problems. I
> >>>> need to resolve them first before I provide updated version of my
> >>>> patch.
> >>> 
> >>> FYI: As of
> >>> https://github.com/graalvm/graal/commit/ca9071941a1be7f1a3725529ecc231ff
> >>> 621d5ed0
> >>> 
> >>> the Graal compiler can run with java.base, jdk.unsupported and
> >>> jdk.vm.ci only
> >>> modules. But it wasn't easy, especially the
> >>> http://wiki.apidesign.org/wiki/PropertyChangeListener required a bit
> >>> of work.
> >>> 
> >>> -jt