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

[Christian Thalinger]

> On Sep 18, 2015, at 12:14 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> I'm adopting Kim's '----' delimiters (only minimally).
> 
> ----
> 
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/cpu/sparc/vm/cppInterpreter_sparc.cpp.udiff.html
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/cpu/x86/vm/cppInterpreter_x86.cpp.udiff.html
> 
> Even though this doesn't build, you should change
> 
> *-  void InterpreterGenerator::lock_method(void) {*
> 
> 
> to CppInterpreterGenerator::lock_method(void) since you've already changed the line.

Done.

> 
> ----
> 
> eg. http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/cpu/x86/vm/interp_masm_x86.cpp.udiff.html
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/cpu/x86/vm/vm_version_x86.cpp.udiff.html
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/share/vm/compiler/compileBroker.cpp.udiff.html
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/share/vm/oops/methodData.hpp.udiff.html
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/share/vm/runtime/sharedRuntime.cpp.udiff.html
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/share/vm/runtime/thread.cpp.udiff.html
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/share/vm/runtime/thread.hpp.udiff.html
> 
> Can you go through and add more #endif // INCLUDE_JVMTI comments?  I hate raw #endif preprocessor directives.  They make the code really hard to read.  Above is a sample, but there are more.   If it's a few lines separated, it's not too bad but some of these are long patches of code inserted in the middle of functions.

I’ll try… 

> 
> ----
> 
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/cpu/x86/vm/sharedRuntime_x86_64.cpp.udiff.html
> 
> *+     map->set_callee_saved(YMMHI_STACK_OFFSET(  0), xmm0->as_VMReg()->next()->next()->next()->next());*
> 
> 
> What?  Are 4 next() calls really enough?   I'm glad I don't work on the compiler.  This is really odd looking.  Maybe there could be a function in VMReg->4th_next().
> 
> ----
> 
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/cpu/x86/vm/templateTable_x86.cpp.udiff.html
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/cpu/sparc/vm/templateTable_sparc.cpp.udiff.html
> 
> profiled_called_method could be JVMCI_ONLY().   Is there always a profile_called_method() before call_from_interpreter() ?  maybe the method should be called in there instead at the callers.   Or make it unconditional and have !JVMCI version not do anything.   Just to cut down on all this conditional code.

I’ve added NOT_JVMCI_RETURN and used that.

> 
> ----
> 
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/share/vm/classfile/*
> 
> Very nice refactoring!!  Thank you.
> 
> ----
> 
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/share/vm/code/relocInfo.hpp.udiff.html
> 
> Why is the copyright updated to 2014?  Do you want my sed script?

Maybe the last change done to that file was in 2014.  Not sure.

> 
> ----
> 
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/share/vm/oops/method.cpp.udiff.html
> 
> In most of the code WizardMode prints *more* information not less, but this prints less.  This change is going to be broken with Unified Logging.

I made that change.  It does not print less but in a more low-level way as it prints signatures the way they are stored in the constant pool (with L and /).  Wizard-style.

> 
> ----
> 
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/src/share/vm/runtime/thread.hpp.udiff.html
> 
> I thought you were only going to add 3 words to thread?  (If you reordered the fields, it would be).

I reordered the fields a bit.

> 
> Having to declare this many fields inline to any class suggests it should be refactored into a different class, and a pointer to that class in the global thread class.   I've made this comment before, I know.   It seems all the blue in this could be a new class.

We can make this change later if we want to.

> 
> That's all the comments I have.   These are minor.  I'm not going to look at the 42000 lines of new code again.  Thank you for making the suggested changes in the code preview.

This should address all your review comments:

http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/7e53ffed78bc <http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/7e53ffed78bc>

Thanks for the review.

> 
> Thanks,
> Coleen
> 
> On 9/14/15 3:24 AM, 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>
>> 
>