> On 15 Sep 2017, at 14:32, Jaroslav Tulach <jaroslav.tul...@oracle.com> wrote:
>
> 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
That is correct - you have to obtain a HotspotGraalRuntime from a
HotSpotGraalCompiler. There can be other HotspotGraalRuntime instances (e.g.,
Truffle has it's own HotSpotGraalCompiler/HotSpotGraalRuntime).
-Doug
