RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Mon Sep 28 09:25:11 UTC 2015 

On 2015-09-25 22:00, Christian Thalinger wrote:
>
>> On Sep 25, 2015, at 12:20 AM, Magnus Ihse Bursie 
>> <magnus.ihse.bursie at oracle.com 
>> <mailto:magnus.ihse.bursie at oracle.com>> wrote:
>>
>> On 2015-09-24 20:57, Christian Thalinger wrote:
>>>> On Sep 21, 2015, at 12:24 PM, Christian Thalinger 
>>>> <christian.thalinger at oracle.com 
>>>> <mailto:christian.thalinger at oracle.com>> wrote:
>>>>
>>>>
>>>>> On Sep 18, 2015, at 2:19 PM, Christian Thalinger 
>>>>> <christian.thalinger at oracle.com 
>>>>> <mailto:christian.thalinger at oracle.com>> wrote:
>>>>>
>>>>>
>>>>>> On Sep 18, 2015, at 7:00 AM, Christian Thalinger 
>>>>>> <christian.thalinger at oracle.com 
>>>>>> <mailto:christian.thalinger at oracle.com>> wrote:
>>>>>>
>>>>>>
>>>>>>> On Sep 17, 2015, at 11:00 PM, Volker Simonis 
>>>>>>> <volker.simonis at gmail.com <mailto:volker.simonis at gmail.com>> wrote:
>>>>>>>
>>>>>>> To which repository will you finally push these changes?
>>>>>>>
>>>>>>> The current webrevs are against jdk9/jdk9/hotspot but I doubt that
>>>>>>> they will be integrated there.
>>>>>> Good point.
>>>>>>
>>>>>>> Unfortunately the patches don't apply to jdk9/dev/hotspot anymore
>>>>>>> because of '8135067: Preparatory refactorings for compiler control'
>>>>>>> and  "8134626: Misc cleanups after generation array removal":
>>>>>>>
>>>>>>> 3 out of 18 hunks FAILED -- saving rejects to file
>>>>>>> src/share/vm/compiler/compileBroker.cpp.rej
>>>>>>> 1 out of 5 hunks FAILED -- saving rejects to file
>>>>>>> src/share/vm/compiler/compileBroker.hpp.rej
>>>>>>> 6 out of 40 hunks FAILED -- saving rejects to file
>>>>>>> src/share/vm/runtime/vmStructs.cpp.rej
>>>>>>>
>>>>>>> Will you update the patches and if yes against which repositories?
>>>>>> Yes.  Let me update the graal-jvmci-9 repository to hs-comp.
>>>>> Done.  Now I have to create new webrevs…
>>>> Here are new webrevs against hs-comp:
>>>>
>>>> http://cr.openjdk.java.net/~twisti/8136421/webrev/ 
>>>> <http://cr.openjdk.java.net/%7Etwisti/8136421/webrev/> 
>>>> <http://cr.openjdk.java.net/~twisti/8136421/webrev/ 
>>>> <http://cr.openjdk.java.net/%7Etwisti/8136421/webrev/>>
>>>> http://cr.openjdk.java.net/~twisti/8136421/hotspot/webrev/ 
>>>> <http://cr.openjdk.java.net/%7Etwisti/8136421/hotspot/webrev/> 
>>>> <http://cr.openjdk.java.net/~twisti/8136421/hotspot/webrev/ 
>>>> <http://cr.openjdk.java.net/%7Etwisti/8136421/hotspot/webrev/>>
>>> I have refreshed these webrevs.  They contain all the changes we 
>>> discussed here plus a bunch of new tests that SQE finished.
>>
>> Hi Christian,
>>
>> I didn't see Volkers proposed fix about undefined-bool-conversion:
>>
>>> If this comes only from adlc, you could fix for adlc only by adding
>>>
>>> WARNINGS_ARE_ERRORS += -Wno-undefined-bool-conversion
>>>
>>> to make/linux/makefiles/adlc.make instead of 
>>> make/linux/makefiles/gcc.make
>>
>> While this is really a fix for an unrelated issue (support for a 
>> newer version of clang), I do believe Volker's suggestion to only add 
>> it to adlc is much better, if you really need to take this in as part 
>> of the JVMCI integration.
>
> Sorry I dropped this.  After Gilles’ hint I remembered the code in 
> question was removed with:
>
> [JDK-8041620] Solaris Studio 12.4 C++ 5.13 change in behavior for 
> placing friend declarations within surrounding scope - Java Bug System 
> <https://bugs.openjdk.java.net/browse/JDK-8041620>
>
> and the remaining ones were fixed with:
>
> [JDK-8077364] "if( !this )" construct prevents build on Xcode 6.3 - 
> Java Bug System <https://bugs.openjdk.java.net/browse/JDK-8077364>
>
> I’ve removed the change:
>
> http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/6de55f563ee1
Good!

>
>>
>> Apart from this, the makefile changes (still) look good.
>
> Btw. we found a bug in creating the OptionDescriptors files; we get 
> duplicate entries:
>
> $ cat 
> build/macosx-x86_64-normal-server-release/jdk/modules/java.base/META-INF/services/jdk.internal.jvmci.options.OptionDescriptors 
>
> jdk.internal.jvmci.compiler.Compiler_OptionDescriptors
> jdk.internal.jvmci.hotspot.HotSpotConstantReflectionProvider_OptionDescriptors
> jdk.internal.jvmci.hotspot.HotSpotResolvedJavaFieldImpl_OptionDescriptors
> jdk.internal.jvmci.hotspot.HotSpotResolvedJavaMethodImpl_OptionDescriptors
> jdk.internal.jvmci.compiler.Compiler_OptionDescriptors
> jdk.internal.jvmci.hotspot.HotSpotConstantReflectionProvider_OptionDescriptors
> jdk.internal.jvmci.hotspot.HotSpotResolvedJavaFieldImpl_OptionDescriptors
> jdk.internal.jvmci.hotspot.HotSpotResolvedJavaMethodImpl_OptionDescriptors
> …
>
> Would this be the right fix?
>
> diff -r db1a815d2f6c make/gensrc/Gensrc-java.base.gmk
> --- a/make/gensrc/Gensrc-java.base.gmkThu Sep 24 15:35:49 2015 -1000
> +++ b/make/gensrc/Gensrc-java.base.gmkFri Sep 25 18:18:35 2015 +0200
> @@ -94,6 +94,7 @@
>     $(GENSRC_DIR)/_gensrc_proc_done
> $(MKDIR) -p $(@D)
> ($(CD) $(GENSRC_DIR)/META-INF/jvmci.options && \
> +    $(RM) -f $@; \
>     for i in $$(ls); do \
>       echo $${i}_OptionDescriptors >> $@; \
>     done)
>
That seems like a reasonable fix, yes.

/Magnus

> And I see the same behavior for HotSpotJVMCIBackendFactory:
>
> $ cat 
> build/macosx-x86_64-normal-server-release/jdk/modules/java.base/META-INF/services/jdk.internal.jvmci.hotspot.HotSpotJVMCIBackendFactory 
>
> jdk.internal.jvmci.hotspot.amd64.AMD64HotSpotJVMCIBackendFactory
> jdk.internal.jvmci.hotspot.sparc.SPARCHotSpotJVMCIBackendFactory
> jdk.internal.jvmci.hotspot.amd64.AMD64HotSpotJVMCIBackendFactory
> jdk.internal.jvmci.hotspot.sparc.SPARCHotSpotJVMCIBackendFactory
> …
>
> So I think a similar fix needs to be applied there.
>
>>
>> /Magnus
>>
>>>
>>>>>>> If I want to work on the ppc64 implementation, which repository 
>>>>>>> should I use?
>>>>>> graal-jvmci-9.  Are you working on this for fun or do you want to 
>>>>>> have that integrated with this JEP?
>>>>>>
>>>>>>> Thank you and best regards,
>>>>>>> Volker
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Sep 18, 2015 at 10:20 AM, Volker Simonis
>>>>>>> <volker.simonis at gmail.com <mailto:volker.simonis at gmail.com>> wrote:
>>>>>>>> For the top-level change I always get a strange error when 
>>>>>>>> importing it:
>>>>>>>>
>>>>>>>> patch failed, unable to continue (try -v)
>>>>>>>> patch failed, rejects left in working dir
>>>>>>>> errors during apply, please fix and refresh 
>>>>>>>> 8062493_jvmci_top_0911.v1.patch
>>>>>>>>
>>>>>>>> It is because of the strange path syntax of the last hunk in 
>>>>>>>> the patch file:
>>>>>>>>
>>>>>>>> --- old/./modules.xml 2015-09-16 15:11:14.000000000 -0700
>>>>>>>> +++ new/./modules.xml 2015-09-16 15:11:14.000000000 -0700
>>>>>>>> @@ -254,6 +254,14 @@
>>>>>>>>    <to>jdk.jfr</to>
>>>>>>>>  </export>
>>>>>>>>  <export>
>>>>>>>> +      <name>jdk.internal.jvmci.hotspot</name>
>>>>>>>> +      <to>jdk.jfr</to>
>>>>>>>> +    </export>
>>>>>>>> +    <export>
>>>>>>>> +      <name>jdk.internal.jvmci.hotspot.events</name>
>>>>>>>> +      <to>jdk.jfr</to>
>>>>>>>> +    </export>
>>>>>>>> +    <export>
>>>>>>>>    <name>sun.misc</name>
>>>>>>>>    <to>java.corba</to>
>>>>>>>>    <to>java.desktop</to>
>>>>>>>>
>>>>>>>>
>>>>>>>> Notice the strange syntax "old/./modules.xml" and 
>>>>>>>> "new/./modules.xml".
>>>>>>>> If I remove the redundant './' from the path, everything works 
>>>>>>>> fine.
>>>>>>>> For some reason only the diffs for modules.xml has this strange 
>>>>>>>> path
>>>>>>>> syntax in the patch.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Volker
>>>>>>>>
>>>>>>>>
>>>>>>>> On Thu, Sep 17, 2015 at 10:32 PM, Christian Thalinger
>>>>>>>> <christian.thalinger at oracle.com 
>>>>>>>> <mailto:christian.thalinger at oracle.com>> wrote:
>>>>>>>>> Since there are no objections I’m going to push this…
>>>>>>>>>
>>>>>>>>> http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/6a6766a8cbbf
>>>>>>>>>
>>>>>>>>>> On Sep 16, 2015, at 3:05 PM, Christian Thalinger 
>>>>>>>>>> <christian.thalinger at oracle.com> wrote:
>>>>>>>>>>
>>>>>>>>>> I would like to add this change:
>>>>>>>>>>
>>>>>>>>>> diff -r 2134e08cc132 src/share/vm/utilities/vmError.cpp
>>>>>>>>>> --- a/src/share/vm/utilities/vmError.cpp      Wed Sep 16 
>>>>>>>>>> 14:28:33 2015 -1000
>>>>>>>>>> +++ b/src/share/vm/utilities/vmError.cpp      Wed Sep 16 
>>>>>>>>>> 15:04:02 2015 -1000
>>>>>>>>>> @@ -517,6 +517,10 @@ void VMError::report(outputStream* st) {
>>>>>>>>>>                Abstract_VM_Version::vm_release(),
>>>>>>>>>>                Abstract_VM_Version::vm_info_string(),
>>>>>>>>>>                TieredCompilation ? ", tiered" : "",
>>>>>>>>>> +#if INCLUDE_JVMCI
>>>>>>>>>> +                   EnableJVMCI ? ", jvmci" : "",
>>>>>>>>>> +                   UseJVMCICompiler ? ", jvmci compiler" : "",
>>>>>>>>>> +#endif
>>>>>>>>>>                UseCompressedOops ? ", compressed oops" : "",
>>>>>>>>>>                gc_mode(),
>>>>>>>>>>                Abstract_VM_Version::vm_platform_string()
>>>>>>>>>>
>>>>>>>>>> It would be useful to see in the crash logs if this 
>>>>>>>>>> experimental feature was turned on.
>>>>>>>>>>
>>>>>>>>>>> On Sep 16, 2015, at 12:43 PM, Vladimir Kozlov 
>>>>>>>>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> I updated top and hotspot webrev with latest (build) changes.
>>>>>>>>>>>
>>>>>>>>>>> Vladimir
>>>>>>>>>>>
>>>>>>>>>>> On 9/16/15 2:28 PM, Christian Thalinger wrote:
>>>>>>>>>>>>> On Sep 16, 2015, at 6:52 AM, Christian Thalinger 
>>>>>>>>>>>>> <christian.thalinger at oracle.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie 
>>>>>>>>>>>>>> <magnus.ihse.bursie at oracle.com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 2015-09-14 09:24, Christian Thalinger wrote:
>>>>>>>>>>>>>>> The JEP itself can be found here:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8062493 
>>>>>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8062493>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Here are the webrevs:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 
>>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/>
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 
>>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The change has already undergone a few iterations of 
>>>>>>>>>>>>>>> internal review by different people of different teams. 
>>>>>>>>>>>>>>>  Most comments and suggestions were handled accordingly. 
>>>>>>>>>>>>>>>  Although there is one open item we agreed we will 
>>>>>>>>>>>>>>> address after the integration of JEP 243 and that work 
>>>>>>>>>>>>>>> is captured in RFE:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8134994 
>>>>>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8134994>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> SQE is still working on the tests and all test tasks can 
>>>>>>>>>>>>>>> be seen as sub-tasks of the JEP.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The integration will happen under the bug number:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8136421 
>>>>>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8136421>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Christian,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> (Adding build-dev since this review includes some major 
>>>>>>>>>>>>>> build changes.)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The makefile changes looks good in general to me. I have 
>>>>>>>>>>>>>> not reviewed the source code changes.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Some comments:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> * modules.xml:
>>>>>>>>>>>>>> Make sure you get sign-off from a Jigsaw developer for 
>>>>>>>>>>>>>> modifying this file.
>>>>>>>>>>>>> I’ve asked Alan to take a look.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> * hotspot/make/linux/makefiles/gcc.make:
>>>>>>>>>>>>>> Seems unfortunate to have to disable a new warning 
>>>>>>>>>>>>>> (undefined-bool-conversion) for newly incorporated code. 
>>>>>>>>>>>>>> Is it not possible to fix the code emitting this warning 
>>>>>>>>>>>>>> instead?
>>>>>>>>>>>>> We had this question before.  It’s not obvious because you 
>>>>>>>>>>>>> can’t see it in the regular diff views but this is under:
>>>>>>>>>>>>>
>>>>>>>>>>>>> ifeq ($(USE_CLANG), true)
>>>>>>>>>>>>>
>>>>>>>>>>>>>> * make/common/MakeBase.gmk and 
>>>>>>>>>>>>>> hotspot/make/gensrc/Gensrc-java.base.gmk:
>>>>>>>>>>>>>> I see a coming merge conflict. In jdk9/dev, there is now 
>>>>>>>>>>>>>> a new function that performs the same function as 
>>>>>>>>>>>>>> CreatePath, but it's named PathList. (It's a bit 
>>>>>>>>>>>>>> unfortunate that this code snippet has bounced around as 
>>>>>>>>>>>>>> patches without a definite name.) I assume you are going 
>>>>>>>>>>>>>> to push this through a hotspot forest. If the PathList 
>>>>>>>>>>>>>> patch reaches the hotspot repo before this, please remove 
>>>>>>>>>>>>>> the CreatePath from MakeBase, and change the calls to 
>>>>>>>>>>>>>> CreatePath to PathList instead. (I could only find such 
>>>>>>>>>>>>>> calls in hotspot/make/gensrc/Gensrc-java.base.gmk). If 
>>>>>>>>>>>>>> this patch goes in before that, we'll need to give a 
>>>>>>>>>>>>>> heads-up to the integrator about this conflict.
>>>>>>>>>>>>> Thanks for the heads up.
>>>>>>>>>>>> Erik sent me a patch to avoid merge conflicts.  I’ve 
>>>>>>>>>>>> integrated two changesets:
>>>>>>>>>>>>
>>>>>>>>>>>> http://hg.openjdk.java.net/graal/graal-jvmci-9/rev/ddedccc5c0ab 
>>>>>>>>>>>> <http://hg.openjdk.java.net/graal/graal-jvmci-9/rev/ddedccc5c0ab><http://hg.openjdk.java.net/graal/graal-jvmci-9/rev/ddedccc5c0ab 
>>>>>>>>>>>> <http://hg.openjdk.java.net/graal/graal-jvmci-9/rev/ddedccc5c0ab>>
>>>>>>>>>>>> http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/fee6b89199c9 
>>>>>>>>>>>> <http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/fee6b89199c9><http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/fee6b89199c9 
>>>>>>>>>>>> <http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/fee6b89199c9>>
>>>>>>>>>>>>
>>>>>>>>>>>>>> Another potential coming merge conflict relates to 
>>>>>>>>>>>>>> ListPathsSafely in Gensrc-java.base.gmk. There is 
>>>>>>>>>>>>>> currenlty a review out from Erik which modifies the API 
>>>>>>>>>>>>>> for ListPathsSafely. If/when it goes in, the call to 
>>>>>>>>>>>>>> ListPathsSafely in Gensrc-java.base.gmk will need to be 
>>>>>>>>>>>>>> modified (Erik can give advice on how). Depending on 
>>>>>>>>>>>>>> timing, this too might hit the integrator rather than 
>>>>>>>>>>>>>> your push.
>>>>>>>>>>>>> Ok, thanks.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> /Magnus
>

More information about the hotspot-dev mailing list