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
