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

[Jaroslav Tulach]

Thanks for the review Mandy, Daniel. Now, that the consolidated JDK10 
repository is available, I have updated my webrev to its structure. In 
addition to that I addressed your comments:

On úterý 12. září 2017 11:19:45 CEST mandy chung wrote:
> ./make/common/Modules.gmk
>      Nit: can you move jdk.internal.vm.compiler.management to keep the
> list in alphabetical order

Inserted at appropriate place.

>   199 # Filter out Graal specific modules if Graal build is disabled
>   200
>   201 ifeq ($(INCLUDE_GRAAL), false)
>   202   MODULES_FILTER += jdk.internal.vm.compiler
>   203 endif
> 
> When will INCLUDE_GRAAL be set to false?  I think
> jdk.internal.vm.compiler.management should also be filtered if
> jdk.internal.vm.compiler is disabled.

That is probably true. Fixed.

> 
> Is jdk.internal.vm.compiler and jdk.internal.vm.compiler.management
> built for all platforms in JDK 10? If not,
>     jdk/src/java.management/share/classes/module-info.java may fail to
> compile when jdk.internal.vm.compiler.management is not present.   We
> can consult with the build team when you find out what configuration
> that jdk.internal.vm.compiler is not built.

I haven't found configuration where jdk.internal.vm.compiler wouldn't be built. 
However I wasn't looking very extensively...

> hotspot/src/jdk.internal.vm.compiler/share/classes/module-info.java 29
> requires transitive jdk.internal.vm.ci;
> do you get any error without this requires transitive?
> jdk.internal.vm.compiler.management already requires
> jdk.internal.vm.ci.  I would think this requires transitive is not
> necessary.

Looks like this change isn't necessary. I am not sure what was the problem 
before, when I introduced it.

> Is HotSpotGraalCompiler::mbean method necessary?  In GraalMBeans.java
> 
>    53     public static Object findGraalRuntimeBean() {
>    54         JVMCIRuntime r = JVMCI.getRuntime();
>    55         JVMCICompiler c = r.getCompiler();
>    56         if (c instanceof HotSpotGraalCompiler) {
>    57             return ((HotSpotGraalCompiler) c).mbean();
>    58         }
>    59         return null;
>    60     }
> 
> It seems that you can call HotspotGraalRuntime::mbean directly. 

I don't think I can. There is no way to get to HotspotGraalRuntime except 
asking the HotSpotGraalCompiler. The HotspotGraalRuntime isn't JVMCIRuntime... 
At least I think so, there is slightly too much runtimes and providers in the 
codebase for my taste. However that isn't something I can change as part of 
JDK-8182701

> As we
> discussed offline, we agree that HotSpotRuntimeMBean should belong to
> this new module but it requires some refactoring which may take some
> amount of work.  Such clean up will be followed up in a separate JBS issue.

Right.

> GraalMBeans.java:
> 
>    77         @Override
>    78         public Set<String> mbeanInterfaceNames() {
>    79             return Collections.singleton(name);
>    80         }
> 
> This is not correct. The return set should be a set of
> MXBean interface names, as in Class.getName(), not a set
> of MXBean ObjectName strings.

I see. Thanks.

On středa 13. září 2017 8:23:22 CEST mandy chung wrote:
> On 9/13/17 2:28 AM, Daniel Fuchs wrote:
> Good catch, Daniel.  This should return empty set as mbeanInterfaces()
> returns.  mbeanInterfaceNames returns the class name of the mbean
> interfaces.

OK, returning empty set.

The webrev #5 is available at
http://cr.openjdk.java.net/~jtulach/8182701/webrev.05/

-jt