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

[mandy chung]

On 8/22/17 6:19 AM, Jaroslav Tulach wrote:
> 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?
>

Will get back to you on any suggestion.
>> 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.

Since this is mainly for Truffle, keeping it simple is good.
> 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));
>     }
> }

This isn't exactly right when there is one DynamicMBean and one 
StandardMBean.   What about making this version only supporting 
DynamicMBean - i.e. the provider constructor throws IAE if the mbean 
instance is not DynamicMBean and mbeanInterfaces method simply returns 
an empty set.   This could be enhanced in the future if there is a need 
to support standard mbean.
> if this is acceptable, I would change documentation of the mbean() method to
> mention how the set of mbeanInterfaces() is computed. OK?
A comment would help.
Mandy
> Thanks for the review.
> -jt
>