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

[Jaroslav Tulach]

Hello Doug & co.,
I think your comments are about the old webrev. I hope most of them have been 
(by a chance) addressed in the newest webrev 01:

http://cr.openjdk.java.net/~kvn/8182701/webrev.01/

On pátek 18. srpna 2017 21:11:28 CEST Doug Simon wrote:
> I don't think JVMCIMXBeans should be in the hotspot-agnostic
> jdk.vm.ci.services.internal package since it has a direct reference to
> HotSpotJVMCIRuntime. I would suggest moving it to jdk.vm.ci.hotspot.

Mandy already suggested to put the JVMCIMBeans class into its own module with 
dependencies on jdk.management and jdk.vm.internal.ci modules. I think it is 
the cleanest solution from the point of modular architecture. The suggested 
module can be found in the webrev.01 under the name jdk.vm.ci.management.

> In HotSpotJVMCRuntime.java:
> 
>  558     public String mbeanName() {
>  568     public Object mbean() {
> 
> Any reason these methods don't follow the conventions of other getter
> methods in this class (i.e. getMBeanName() and getMBean())?

These two methods have been replaced by 

    /**
     * Provides compiler specific platform MBeans. These MBeans will be 
automatically
     * exposed once the management system gets initialized.
     *
     * @return map from MBean names to their instances
     */
    public Map<String, Object> mbeans() {
        return Collections.emptyMap();
    }

in the webrev.01. You are right, the name of the method could be getMBeans(). 
Let me know if I should rename that and also ...

> 
>   95     /** Name of the {@link MBeanInfo MBean} representing the internals
> 
> This should be:
> 
> /**
>  * Gets the name of the ...

...please give me a better version of Javadoc, in case the current 
"Provides..." isn't sufficient.

> Same comment for mbean() method. Also {@code null} is used in JVMCI instead
> of <code>null</code>.

The webrev.01 version never returns null. Instead Collections.emptyMap() is 
returned by default. Thus this comment no longer applies.

Thanks for your comments.
-jt

> > On 18 Aug 2017, at 20:49, Vladimir Kozlov <vladimir.kozlov at oracle.com>
> > 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