On 9/12/17 10:44 AM, Jaroslav Tulach wrote:
Dear reviewers,
after several reconsiderations I have webrev #4 ready for your review. Can you
please take a look at
http://cr.openjdk.java.net/~jtulach/8182701/webrev.04/
and let me know if it is in a reasonable shape? Thanks a lot.
-jt
Yes defining a new provider module for the platform mbean registration
is a reasonable solution. Generally the patch looks right. I have a
question on the build and a comment on the new mbean method.
./make/common/Modules.gmk
Nit: can you move jdk.internal.vm.compiler.management to keep the
list in alphabetical order
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.
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.
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.
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. 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.
Mandy
