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

[Coleen Phillimore]

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.

----

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.

----

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.

----

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?

----

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.

----

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).

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.

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.

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>
>